Make "*" method actually catch all methods, tweak priority order a bit

uNetworking · Dec 19, 2023 · 97e8099 · 97e8099
1 parent 85c6f0f
commit 97e8099
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 22 deletions.
diff --git a/src/HttpContext.h b/src/HttpContext.h
@@ -426,7 +426,7 @@ struct HttpContext {
         /* Todo: This is ugly, fix */
         std::vector<std::string> methods;
         if (method == "*") {
-            methods = httpContextData->currentRouter->upperCasedMethods;
+            methods = {"*"};
         } else {
             methods = {method};
         }

diff --git a/src/HttpRouter.h b/src/HttpRouter.h
@@ -35,8 +35,7 @@ namespace uWS {
 
 template <class USERDATA>
 struct HttpRouter {
-    /* These are public for now */
-    std::vector<std::string> upperCasedMethods = {"GET", "POST", "HEAD", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH"};
+    static constexpr std::string_view ANY_METHOD_TOKEN = "*";
     static const uint32_t HIGH_PRIORITY = 0xd0000000, MEDIUM_PRIORITY = 0xe0000000, LOW_PRIORITY = 0xf0000000;
 
 private:
@@ -46,9 +45,6 @@ struct HttpRouter {
     /* Handler ids are 32-bit */
     static const uint32_t HANDLER_MASK = 0x0fffffff;
 
-    /* Methods and their respective priority */
-    std::map<std::string, int> priority;
-
     /* List of handlers */
     std::vector<MoveOnlyFunction<bool(HttpRouter *)>> handlers;
 
@@ -245,10 +241,8 @@ struct HttpRouter {
 
 public:
     HttpRouter() {
-        int p = 0;
-        for (std::string &method : upperCasedMethods) {
-            priority[method] = p++;
-        }
+        /* Always have ANY route */
+        getNode(&root, std::string(ANY_METHOD_TOKEN.data(), ANY_METHOD_TOKEN.length()), false);
     }
 
     std::pair<int, std::string_view *> getParameters() {
@@ -269,12 +263,16 @@ struct HttpRouter {
         for (auto &p : root.children) {
             if (p->name == method) {
                 /* Then route the url */
-                return executeHandlers(p.get(), 0, userData);
+                if (executeHandlers(p.get(), 0, userData)) {
+                    return true;
+                } else {
+                    break;
+                }
             }
         }
 
-        /* We did not find any handler for this method and url */
-        return false;
+        /* Always test any route last */
+        return executeHandlers(root.children.back().get(), 0, userData);
     }
 
     /* Adds the corresponding entires in matching tree and handler list */
@@ -299,6 +297,22 @@ struct HttpRouter {
             std::cerr << "Error: Internal routing error" << std::endl;
             std::abort();
         }
+
+        /* ANY method must be last, GET must be first */
+        std::sort(root.children.begin(), root.children.end(), [](const auto &a, const auto &b) {
+            /* Assuming the list of methods is unique, non-repeating */
+            if (a->name == "GET") {
+                return true;
+            } else if (b->name == "GET") {
+                return false;
+            } else if (a->name == ANY_METHOD_TOKEN) {
+                return false;
+            } else if (b->name == ANY_METHOD_TOKEN) {
+                return true;
+            } else {
+                return a->name < b->name;
+            }
+        });
     }
 
     bool cullNode(Node *parent, Node *node, uint32_t handler) {

diff --git a/tests/HttpRouter.cpp b/tests/HttpRouter.cpp
@@ -8,7 +8,7 @@ void testMethodPriority() {
     uWS::HttpRouter<int> r;
     std::string result;
 
-    r.add(r.upperCasedMethods, "/static/route", [&result](auto *) {
+    r.add({"*"}, "/static/route", [&result](auto *) {
         std::cout << "ANY static route" << std::endl;
         result += "AS";
         return true;
@@ -26,9 +26,9 @@ void testMethodPriority() {
         return true;
     });
 
-    assert(r.route("nonsense", "/static/route") == false);
+    assert(r.route("nonsense", "/static/route") == true);
     assert(r.route("GET", "/static") == false);
-    assert(result == "");
+    assert(result == "AS");
 
     /* Should end up directly in ANY handler */
     result.clear();
@@ -51,7 +51,7 @@ void testPatternPriority() {
     uWS::HttpRouter<int> r;
     std::string result;
 
-    r.add(r.upperCasedMethods, "/a/b/c", [&result](auto *) {
+    r.add({"*"}, "/a/b/c", [&result](auto *) {
         std::cout << "ANY static route" << std::endl;
         result += "AS";
         return false;
@@ -81,18 +81,18 @@ void testPatternPriority() {
         return false;
     });
 
-    r.add(r.upperCasedMethods, "/a/:b/c", [&result](auto *) {
+    r.add({"*"}, "/a/:b/c", [&result](auto *) {
         std::cout << "ANY parameter route" << std::endl;
         result += "AP";
         return false;
     }, r.LOW_PRIORITY);
 
     assert(r.route("POST", "/a/b/c") == false);
-    assert(result == "ASPPAP");
+    assert(result == "PPASAP");
 
     result.clear();
     assert(r.route("GET", "/a/b/c") == false);
-    assert(result == "GSASGPAPGW");
+    assert(result == "GSGPGWASAP");
 }
 
 void testUpgrade() {
@@ -224,7 +224,7 @@ void testBugReports() {
         }, r.MEDIUM_PRIORITY);
 
         /* ANY on /* */
-        r.add(r.upperCasedMethods, "/*", [&result](auto *) {
+        r.add({"*"}, "/*", [&result](auto *) {
             result += "AW";
             return false;
         }, r.LOW_PRIORITY);
@@ -256,7 +256,7 @@ void testBugReports() {
         }, r.MEDIUM_PRIORITY);
 
         /* ANY on /* */
-        r.add(r.upperCasedMethods, "/*", [&result](auto *) {
+        r.add({"*"}, "/*", [&result](auto *) {
             result += "AW";
             return false;
         }, r.LOW_PRIORITY);