Fix HttpService route precedence so Any("*") no longer shadows specific handlers

http/server/HttpService.cpp

@@ -2,9 +2,50 @@
 #include "HttpMiddleware.h"
 
 #include "hbase.h" // import hv_strendswith
+#include <algorithm>
+#include <vector>
 
 namespace hv {
 
+static bool is_restful_route(const std::string& path) {
+    return path.find("/:") != std::string::npos ||
+           path.find("/{") != std::string::npos;
+}
+
+static bool match_route_path(const std::string& key_path, const std::string& path, std::map<std::string, std::string>& params) {
+    const char *kp = key_path.c_str(), *ks, *vp = path.c_str(), *vs;
+    bool match = false;
+    while (*kp && *vp) {
+        if (kp[0] == '*') {
+            // wildcard *
+            match = hv_strendswith(vp, kp+1);
+            break;
+        } else if (*kp != *vp) {
+            match = false;
+            break;
+        } else if (kp[0] == '/' && (kp[1] == ':' || kp[1] == '{')) {
+            // RESTful /:field/
+            // RESTful /{field}/
+            kp += 2;
+            ks = kp;
+            while (*kp && *kp != '/') {++kp;}
+            vp += 1;
+            vs = vp;
+            while (*vp && *vp != '/') {++vp;}
+            int klen = kp - ks;
+            if (*(ks-1) == '{' && *(kp-1) == '}') {
+                --klen;
+            }
+            params[std::string(ks, klen)] = std::string(vs, vp-vs);
+            continue;
+        } else {
+            ++kp;
+            ++vp;
+        }
+    }
+    return match ? match : (*kp == '\0' && *vp == '\0');
+}
+
 void HttpService::AddRoute(const char* path, http_method method, const http_handler& handler) {
     std::shared_ptr<http_method_handlers> method_handlers = NULL;
     auto iter = pathHandlers.find(path);
@@ -67,64 +108,70 @@ int HttpService::GetRoute(HttpRequest* req, http_handler** handler) {
     while (*e && *e != '?') ++e;
 
     std::string path = std::string(s, e);
-    const char *kp, *ks, *vp, *vs;
-    bool match;
-    for (auto iter = pathHandlers.begin(); iter != pathHandlers.end(); ++iter) {
-        kp = iter->first.c_str();
-        vp = path.c_str();
-        match = false;
-        std::map<std::string, std::string> params;
-
-        while (*kp && *vp) {
-            if (kp[0] == '*') {
-                // wildcard *
-                match = hv_strendswith(vp, kp+1);
-                break;
-            } else if (*kp != *vp) {
-                match = false;
-                break;
-            } else if (kp[0] == '/' && (kp[1] == ':' || kp[1] == '{')) {
-                    // RESTful /:field/
-                    // RESTful /{field}/
-                    kp += 2;
-                    ks = kp;
-                    while (*kp && *kp != '/') {++kp;}
-                    vp += 1;
-                    vs = vp;
-                    while (*vp && *vp != '/') {++vp;}
-                    int klen = kp - ks;
-                    if (*(ks-1) == '{' && *(kp-1) == '}') {
-                        --klen;
-                    }
-                    params[std::string(ks, klen)] = std::string(vs, vp-vs);
-                    continue;
-            } else {
-                ++kp;
-                ++vp;
+    auto iter = pathHandlers.find(path);
+    if (iter != pathHandlers.end()) {
+        auto method_handlers = iter->second;
+        for (auto mh = method_handlers->begin(); mh != method_handlers->end(); ++mh) {
+            if (mh->method == req->method) {
+                if (handler) *handler = &mh->handler;
+                return 0;
             }
         }
+        if (handler) *handler = NULL;
+        return HTTP_STATUS_METHOD_NOT_ALLOWED;
+    }
 
-        match = match ? match : (*kp == '\0' && *vp == '\0');
+    std::vector<http_path_handlers::const_iterator> restful_iters;
+    std::vector<http_path_handlers::const_iterator> wildcard_iters;
+    for (auto it = pathHandlers.begin(); it != pathHandlers.end(); ++it) {
+        if (it->first.find('*') != std::string::npos) {
+            wildcard_iters.push_back(it);
+        } else if (is_restful_route(it->first)) {
+            restful_iters.push_back(it);
+        }
+    }
+    auto route_compare = [](const http_path_handlers::const_iterator& lhs, const http_path_handlers::const_iterator& rhs) {
+        if (lhs->first.size() != rhs->first.size()) {
+            return lhs->first.size() > rhs->first.size();
+        }
+        return lhs->first < rhs->first;
+    };
+    std::sort(restful_iters.begin(), restful_iters.end(), route_compare);
+    std::sort(wildcard_iters.begin(), wildcard_iters.end(), route_compare);
 
+    for (auto route_iter : restful_iters) {
+        std::map<std::string, std::string> params;
+        bool match = match_route_path(route_iter->first, path, params);
         if (match) {
-            auto method_handlers = iter->second;
-            for (auto iter = method_handlers->begin(); iter != method_handlers->end(); ++iter) {
-                if (iter->method == req->method) {
+            auto method_handlers = route_iter->second;
+            for (auto mh = method_handlers->begin(); mh != method_handlers->end(); ++mh) {
+                if (mh->method == req->method) {
                     for (auto& param : params) {
                         // RESTful /:field/ => req->query_params[field]
                         req->query_params[param.first] = param.second;
                     }
-                    if (handler) *handler = &iter->handler;
+                    if (handler) *handler = &mh->handler;
                     return 0;
                 }
             }
+        }
+    }
 
-            if (params.size() == 0) {
-                if (handler) *handler = NULL;
-                return HTTP_STATUS_METHOD_NOT_ALLOWED;
+    for (auto route_iter : wildcard_iters) {
+        std::map<std::string, std::string> params;
+        bool match = match_route_path(route_iter->first, path, params);
+        if (!match) continue;
+        auto method_handlers = route_iter->second;
+        for (auto mh = method_handlers->begin(); mh != method_handlers->end(); ++mh) {
+            if (mh->method == req->method) {
+                if (handler) *handler = &mh->handler;
+                return 0;
             }
         }
+        if (handler) *handler = NULL;
+        return HTTP_STATUS_METHOD_NOT_ALLOWED;
     }
+
     if (handler) *handler = NULL;
     return HTTP_STATUS_NOT_FOUND;
 }

unittest/CMakeLists.txt

@@ -72,6 +72,10 @@ add_executable(objectpool_test objectpool_test.cpp)
 target_include_directories(objectpool_test PRIVATE .. ../base ../cpputil)
 target_link_libraries(objectpool_test -lpthread)
 
+add_executable(http_service_route_test http_service_route_test.cpp)
+target_include_directories(http_service_route_test PRIVATE ..)
+target_link_libraries(http_service_route_test hv_static)
+
 # ------protocol------
 add_executable(nslookup nslookup_test.c ../protocol/dns.c ../base/hsocket.c ../base/htime.c)
 target_include_directories(nslookup PRIVATE .. ../base ../protocol)
@@ -111,6 +115,7 @@ add_custom_target(unittest DEPENDS
     synchronized_test
     threadpool_test
     objectpool_test
+    http_service_route_test
     nslookup
     ping
     ftp

unittest/http_service_route_test.cpp

@@ -0,0 +1,43 @@
+#include <stdio.h>
+
+#include "hv/HttpService.h"
+
+using namespace hv;
+
+static int specific_handler(HttpRequest* req, HttpResponse* resp) {
+    (void)req;
+    (void)resp;
+    return 200;
+}
+
+static int fallback_handler(HttpRequest* req, HttpResponse* resp) {
+    (void)req;
+    (void)resp;
+    return 418;
+}
+
+int main() {
+    HttpService service;
+    service.Any("*", fallback_handler);
+    service.GET("/ping", specific_handler);
+
+    http_handler* handler = NULL;
+    HttpResponse resp;
+    HttpRequest req;
+
+    req.path = "/ping";
+    req.method = HTTP_GET;
+    if (service.GetRoute(&req, &handler) != 0 || handler == NULL || handler->sync_handler(&req, &resp) != 200) {
+        fprintf(stderr, "specific route should win over '*' catch-all\n");
+        return -1;
+    }
+
+    req.path = "/not-found";
+    req.method = HTTP_GET;
+    if (service.GetRoute(&req, &handler) != 0 || handler == NULL || handler->sync_handler(&req, &resp) != 418) {
+        fprintf(stderr, "catch-all should match unknown path\n");
+        return -2;
+    }
+
+    return 0;
+}