Fix issue #25

Instead of first matching against the route and then checking the allowed methods, the dispatch now happens in reverse: Only the routes for a particular method are considered initially. If there is no route for the specified method, the URI will be matched against all other methods as well in order to determine the allowed methods list (or conclude that no route exists at all). The performance impact of this change is twofold: On the one hand fewer matches are required in case a route is found (because only the method we're actually interested is tested). On the other hand there is more logic and potentially also more matches necessary in cases where a method-not-allowed or not-found status occurs.
nikic · Nov 26, 2014 · 5b16723 · 5b16723
1 parent 88ac288
commit 5b16723
Show file tree

Hide file tree

Showing 11 changed files with 155 additions and 199 deletions.
diff --git a/src/DataGenerator/CharCountBased.php b/src/DataGenerator/CharCountBased.php
@@ -14,20 +14,15 @@ protected function processChunk($regexToRoutesMap) {
         $suffixLen = 0;
         $suffix = '';
         $count = count($regexToRoutesMap);
-        foreach ($regexToRoutesMap as $regex => $routes) {
+        foreach ($regexToRoutesMap as $regex => $route) {
             $suffixLen++;
             $suffix .= "\t";
 
-            foreach ($routes as $route) {
-                $routeMap[$suffix][$route->httpMethod] = [
-                    $route->handler, $route->variables
-                ];
-            }
-
             $regexes[] = '(?:' . $regex . '/(\t{' . $suffixLen . '})\t{' . ($count - $suffixLen) . '})';
+            $routeMap[$suffix] = [$route->handler, $route->variables];
         }
 
         $regex = '~^(?|' . implode('|', $regexes) . ')$~';
         return ['regex' => $regex, 'suffix' => '/' . $suffix, 'routeMap' => $routeMap];
     }
-}
+}
diff --git a/src/DataGenerator/GroupCountBased.php b/src/DataGenerator/GroupCountBased.php
@@ -11,16 +11,12 @@ protected function processChunk($regexToRoutesMap) {
         $routeMap = [];
         $regexes = [];
         $numGroups = 0;
-        foreach ($regexToRoutesMap as $regex => $routes) {
-            $numVariables = count(reset($routes)->variables);
+        foreach ($regexToRoutesMap as $regex => $route) {
+            $numVariables = count($route->variables);
             $numGroups = max($numGroups, $numVariables);
 
             $regexes[] = $regex . str_repeat('()', $numGroups - $numVariables);
-
-            foreach ($routes as $route) {
-                $routeMap[$numGroups + 1][$route->httpMethod]
-                    = [$route->handler, $route->variables];
-            }
+            $routeMap[$numGroups + 1] = [$route->handler, $route->variables];
 
             ++$numGroups;
         }

diff --git a/src/DataGenerator/GroupPosBased.php b/src/DataGenerator/GroupPosBased.php
@@ -11,14 +11,11 @@ protected function processChunk($regexToRoutesMap) {
         $routeMap = [];
         $regexes = [];
         $offset = 1;
-        foreach ($regexToRoutesMap as $regex => $routes) {
-            foreach ($routes as $route) {
-                $routeMap[$offset][$route->httpMethod]
-                    = [$route->handler, $route->variables];
-            }
-
+        foreach ($regexToRoutesMap as $regex => $route) {
             $regexes[] = $regex;
-            $offset += count(reset($routes)->variables);
+            $routeMap[$offset] = [$route->handler, $route->variables];
+
+            $offset += count($route->variables);
         }
 
         $regex = '~^(?:' . implode('|', $regexes) . ')$~';

diff --git a/src/DataGenerator/MarkBased.php b/src/DataGenerator/MarkBased.php
@@ -11,13 +11,9 @@ protected function processChunk($regexToRoutesMap) {
         $routeMap = [];
         $regexes = [];
         $markName = 'a';
-        foreach ($regexToRoutesMap as $regex => $routes) {
+        foreach ($regexToRoutesMap as $regex => $route) {
             $regexes[] = $regex . '(*MARK:' . $markName . ')';
-
-            foreach ($routes as $route) {
-                $routeMap[$markName][$route->httpMethod]
-                    = [$route->handler, $route->variables];
-            }
+            $routeMap[$markName] = [$route->handler, $route->variables];
 
             ++$markName;
         }

diff --git a/src/DataGenerator/RegexBasedAbstract.php b/src/DataGenerator/RegexBasedAbstract.php
@@ -8,7 +8,7 @@
 
 abstract class RegexBasedAbstract implements DataGenerator {
     protected $staticRoutes = [];
-    protected $regexToRoutesMap = [];
+    protected $methodToRegexToRoutesMap = [];
 
     protected abstract function getApproxChunkSize();
     protected abstract function processChunk($regexToRoutesMap);
@@ -22,17 +22,21 @@ public function addRoute($httpMethod, $routeData, $handler) {
     }
 
     public function getData() {
-        if (empty($this->regexToRoutesMap)) {
+        if (empty($this->methodToRegexToRoutesMap)) {
             return [$this->staticRoutes, []];
         }
 
         return [$this->staticRoutes, $this->generateVariableRouteData()];
     }
 
     private function generateVariableRouteData() {
-        $chunkSize = $this->computeChunkSize(count($this->regexToRoutesMap));
-        $chunks = array_chunk($this->regexToRoutesMap, $chunkSize, true);
-        return array_map([$this, 'processChunk'], $chunks);
+        $data = [];
+        foreach ($this->methodToRegexToRoutesMap as $method => $regexToRoutesMap) {
+            $chunkSize = $this->computeChunkSize(count($regexToRoutesMap));
+            $chunks = array_chunk($regexToRoutesMap, $chunkSize, true);
+            $data[$method] =  array_map([$this, 'processChunk'], $chunks);
+        }
+        return $data;
     }
 
     private function computeChunkSize($count) {
@@ -54,15 +58,14 @@ private function addStaticRoute($httpMethod, $routeData, $handler) {
             ));
         }
 
-        foreach ($this->regexToRoutesMap as $routes) {
-            if (!isset($routes[$httpMethod])) continue;
-
-            $route = $routes[$httpMethod];
-            if ($route->matches($routeStr)) {
-                throw new BadRouteException(sprintf(
-                    'Static route "%s" is shadowed by previously defined variable route "%s" for method "%s"',
-                    $routeStr, $route->regex, $httpMethod
-                ));
+        if (isset($this->methodToRegexToRoutesMap[$httpMethod])) {
+            foreach ($this->methodToRegexToRoutesMap[$httpMethod] as $route) {
+                if ($route->matches($routeStr)) {
+                    throw new BadRouteException(sprintf(
+                        'Static route "%s" is shadowed by previously defined variable route "%s" for method "%s"',
+                        $routeStr, $route->regex, $httpMethod
+                    ));
+                }
             }
         }
 
@@ -72,14 +75,14 @@ private function addStaticRoute($httpMethod, $routeData, $handler) {
     private function addVariableRoute($httpMethod, $routeData, $handler) {
         list($regex, $variables) = $this->buildRegexForRoute($routeData);
 
-        if (isset($this->regexToRoutesMap[$regex][$httpMethod])) {
+        if (isset($this->methodToRegexToRoutesMap[$httpMethod][$regex])) {
             throw new BadRouteException(sprintf(
                 'Cannot register two routes matching "%s" for method "%s"',
                 $regex, $httpMethod
             ));
         }
 
-        $this->regexToRoutesMap[$regex][$httpMethod] = new Route(
+        $this->methodToRegexToRoutesMap[$httpMethod][$regex] = new Route(
             $httpMethod, $handler, $regex, $variables
         );
     }
@@ -107,4 +110,4 @@ private function buildRegexForRoute($routeData) {
 
         return [$regex, $variables];
     }
-}
+}
diff --git a/src/Dispatcher/CharCountBased.php b/src/Dispatcher/CharCountBased.php
@@ -2,60 +2,24 @@
 
 namespace FastRoute\Dispatcher;
 
-use FastRoute\Dispatcher;
-
-class CharCountBased implements Dispatcher {
-    private $staticRouteMap;
-    private $variableRouteData;
-
+class CharCountBased extends RegexBasedAbstract {
     public function __construct($data) {
         list($this->staticRouteMap, $this->variableRouteData) = $data;
     }
 
-    public function dispatch($httpMethod, $uri) {
-        if (isset($this->staticRouteMap[$uri])) {
-            return $this->dispatchStaticRoute($httpMethod, $uri);
-        } else {
-            return $this->dispatchVariableRoute($httpMethod, $uri);
-        }
-    }
-
-    private function dispatchStaticRoute($httpMethod, $uri) {
-        $routes = $this->staticRouteMap[$uri];
-
-        if (isset($routes[$httpMethod])) {
-            return [self::FOUND, $routes[$httpMethod], []];
-        } elseif ($httpMethod === 'HEAD' && isset($routes['GET'])) {
-            return [self::FOUND, $routes['GET'], []];
-        } else {
-            return [self::METHOD_NOT_ALLOWED, array_keys($routes)];
-        }
-    }
-
-    private function dispatchVariableRoute($httpMethod, $uri) {
-        foreach ($this->variableRouteData as $data) {
+    protected function dispatchVariableRoute($routeData, $uri) {
+        foreach ($routeData as $data) {
             if (!preg_match($data['regex'], $uri . $data['suffix'], $matches)) {
                 continue;
             }
 
-            $routes = $data['routeMap'][end($matches)];
-
-            if (false === isset($routes[$httpMethod])) {
-                if ($httpMethod === 'HEAD' && isset($routes['GET'])) {
-                    $httpMethod = 'GET';
-                } else {
-                    return [self::METHOD_NOT_ALLOWED, array_keys($routes)];
-                }
-            }
-
-            list($handler, $varNames) = $routes[$httpMethod];
+            list($handler, $varNames) = $data['routeMap'][end($matches)];
 
             $vars = [];
             $i = 0;
             foreach ($varNames as $varName) {
                 $vars[$varName] = $matches[++$i];
             }
-
             return [self::FOUND, $handler, $vars];
         }
 

diff --git a/src/Dispatcher/GroupCountBased.php b/src/Dispatcher/GroupCountBased.php
@@ -2,52 +2,18 @@
 
 namespace FastRoute\Dispatcher;
 
-use FastRoute\Dispatcher;
-
-class GroupCountBased implements Dispatcher {
-    private $staticRouteMap;
-    private $variableRouteData;
-
+class GroupCountBased extends RegexBasedAbstract {
     public function __construct($data) {
         list($this->staticRouteMap, $this->variableRouteData) = $data;
     }
 
-    public function dispatch($httpMethod, $uri) {
-        if (isset($this->staticRouteMap[$uri])) {
-            return $this->dispatchStaticRoute($httpMethod, $uri);
-        } else {
-            return $this->dispatchVariableRoute($httpMethod, $uri);
-        }
-    }
-
-    private function dispatchStaticRoute($httpMethod, $uri) {
-        $routes = $this->staticRouteMap[$uri];
-
-        if (isset($routes[$httpMethod])) {
-            return [self::FOUND, $routes[$httpMethod], []];
-        } elseif ($httpMethod === 'HEAD' && isset($routes['GET'])) {
-            return [self::FOUND, $routes['GET'], []];
-        } else {
-            return [self::METHOD_NOT_ALLOWED, array_keys($routes)];
-        }
-    }
-
-    private function dispatchVariableRoute($httpMethod, $uri) {
-        foreach ($this->variableRouteData as $data) {
+    protected function dispatchVariableRoute($routeData, $uri) {
+        foreach ($routeData as $data) {
             if (!preg_match($data['regex'], $uri, $matches)) {
                 continue;
             }
 
-            $routes = $data['routeMap'][count($matches)];
-            if (!isset($routes[$httpMethod])) {
-                if ($httpMethod === 'HEAD' && isset($routes['GET'])) {
-                    $httpMethod = 'GET';
-                } else {
-                    return [self::METHOD_NOT_ALLOWED, array_keys($routes)];
-                }
-            }
-
-            list($handler, $varNames) = $routes[$httpMethod];
+            list($handler, $varNames) = $data['routeMap'][count($matches)];
 
             $vars = [];
             $i = 0;

diff --git a/src/Dispatcher/GroupPosBased.php b/src/Dispatcher/GroupPosBased.php
@@ -2,55 +2,21 @@
 
 namespace FastRoute\Dispatcher;
 
-use FastRoute\Dispatcher;
-
-class GroupPosBased implements Dispatcher {
-    private $staticRouteMap;
-    private $variableRouteData;
-
+class GroupPosBased extends RegexBasedAbstract {
     public function __construct($data) {
         list($this->staticRouteMap, $this->variableRouteData) = $data;
     }
 
-    public function dispatch($httpMethod, $uri) {
-        if (isset($this->staticRouteMap[$uri])) {
-            return $this->dispatchStaticRoute($httpMethod, $uri);
-        } else {
-            return $this->dispatchVariableRoute($httpMethod, $uri);
-        }
-    }
-
-    private function dispatchStaticRoute($httpMethod, $uri) {
-        $routes = $this->staticRouteMap[$uri];
-
-        if (isset($routes[$httpMethod])) {
-            return [self::FOUND, $routes[$httpMethod], []];
-        } elseif ($httpMethod === 'HEAD' && isset($routes['GET'])) {
-            return [self::FOUND, $routes['GET'], []];
-        } else {
-            return [self::METHOD_NOT_ALLOWED, array_keys($routes)];
-        }
-    }
-
-    private function dispatchVariableRoute($httpMethod, $uri) {
-        foreach ($this->variableRouteData as $data) {
+    protected function dispatchVariableRoute($routeData, $uri) {
+        foreach ($routeData as $data) {
             if (!preg_match($data['regex'], $uri, $matches)) {
                 continue;
             }
 
             // find first non-empty match
             for ($i = 1; '' === $matches[$i]; ++$i);
 
-            $routes = $data['routeMap'][$i];
-            if (!isset($routes[$httpMethod])) {
-                if ($httpMethod === 'HEAD' && isset($routes['GET'])) {
-                    $httpMethod = 'GET';
-                } else {
-                    return [self::METHOD_NOT_ALLOWED, array_keys($routes)];
-                }
-            }
-
-            list($handler, $varNames) = $routes[$httpMethod];
+            list($handler, $varNames) = $data['routeMap'][$i];
 
             $vars = [];
             foreach ($varNames as $varName) {