Add MethodNotAllowedException::getAllowedMethods()

hhvm · Nov 22, 2019 · c71cb58 · c71cb58
1 parent d02bd48
commit c71cb58
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 66 deletions.
diff --git a/src/http-exceptions/MethodNotAllowedException.php b/src/http-exceptions/MethodNotAllowedException.php
@@ -11,4 +11,16 @@
 namespace Facebook\HackRouter;
 
 class MethodNotAllowedException extends HttpException {
+  public function __construct(
+    protected keyset<HttpMethod> $allowed,
+    string $message = '',
+    int $code = 0,
+    ?\Exception $previous = null,
+  ) {
+    parent::__construct($message, $code, $previous);
+  }
+
+  public function getAllowedMethods(): keyset<HttpMethod> {
+    return $this->allowed;
+  }
 }
diff --git a/src/router/BaseRouter.php b/src/router/BaseRouter.php
@@ -10,7 +10,7 @@
 
 namespace Facebook\HackRouter;
 
-use namespace HH\Lib\Dict;
+use namespace HH\Lib\{C, Dict};
 use function Facebook\AutoloadMap\Generated\is_dev;
 
 abstract class BaseRouter<+TResponder> {
@@ -27,22 +27,20 @@ final public function routeMethodAndPath(
       $data = Dict\map($data, $value ==> \urldecode($value));
       return tuple($responder, new ImmMap($data));
     } catch (NotFoundException $e) {
-      foreach (HttpMethod::getValues() as $next) {
-        if ($next === $method) {
-          continue;
-        }
-        try {
-          list($responder, $data) = $resolver->resolve($next, $path);
-          if ($method === HttpMethod::HEAD && $next === HttpMethod::GET) {
-            $data = Dict\map($data, $value ==> \urldecode($value));
-            return tuple($responder, new ImmMap($data));
-          }
-          throw new MethodNotAllowedException();
-        } catch (NotFoundException $_) {
-          continue;
-        }
+      $allowed = $this->getAllowedMethods($path);
+      if (C\is_empty($allowed)) {
+        throw $e;
       }
-      throw $e;
+
+      if (
+        $method === HttpMethod::HEAD && $allowed === keyset[HttpMethod::GET]
+      ) {
+        list($responder, $data) = $resolver->resolve(HttpMethod::GET, $path);
+        $data = Dict\map($data, $value ==> \urldecode($value));
+        return tuple($responder, new ImmMap($data));
+      }
+
+      throw new MethodNotAllowedException($allowed);
     }
   }
 
@@ -51,11 +49,29 @@ final public function routeRequest(
   ): (TResponder, ImmMap<string, string>) {
     $method = HttpMethod::coerce($request->getMethod());
     if ($method === null) {
-      throw new MethodNotAllowedException();
+      throw new MethodNotAllowedException(
+        $this->getAllowedMethods($request->getUri()->getPath()),
+      );
     }
+
     return $this->routeMethodAndPath($method, $request->getUri()->getPath());
   }
 
+  private function getAllowedMethods(string $path): keyset<HttpMethod> {
+    $resolver = $this->getResolver();
+    $allowed = keyset[];
+    foreach (HttpMethod::getValues() as $method) {
+      try {
+        list($_responder, $_data) = $resolver->resolve($method, $path);
+        $allowed[] = $method;
+      } catch (NotFoundException $_) {
+        continue;
+      }
+    }
+
+    return $allowed;
+  }
+
   private ?IResolver<TResponder> $resolver = null;
 
   protected function getResolver(): IResolver<TResponder> {
@@ -76,9 +92,8 @@ protected function getResolver(): IResolver<TResponder> {
     if ($routes === null) {
       $routes = Dict\map(
         $this->getRoutes(),
-        $method_routes ==> PrefixMatching\PrefixMap::fromFlatMap(
-          dict($method_routes),
-        ),
+        $method_routes ==>
+          PrefixMatching\PrefixMap::fromFlatMap(dict($method_routes)),
       );
 
       if (!is_dev()) {

diff --git a/tests/RouterTest.php b/tests/RouterTest.php
@@ -18,24 +18,23 @@
 use type Facebook\Experimental\Http\Message\HTTPMethod;
 
 final class RouterTest extends \Facebook\HackTest\HackTest {
-  const keyset<string>
-    MAP = keyset[
-      '/foo',
-      '/foo/',
-      '/foo/bar',
-      '/foo/bar/{baz}',
-      '/foo/{bar}',
-      '/foo/{bar}/baz',
-      '/foo/{bar}{baz:.+}',
-      '/food/{noms}',
-      '/bar/{herp:\\d+}',
-      '/bar/{herp}',
-      '/unique/{foo}/bar',
-      '/optional_suffix_[foo]',
-      '/optional_suffix[/]',
-      '/optional_suffixes/[herp[/derp]]',
-      '/manual/en/{LegacyID}.php',
-    ];
+  const keyset<string> MAP = keyset[
+    '/foo',
+    '/foo/',
+    '/foo/bar',
+    '/foo/bar/{baz}',
+    '/foo/{bar}',
+    '/foo/{bar}/baz',
+    '/foo/{bar}{baz:.+}',
+    '/food/{noms}',
+    '/bar/{herp:\\d+}',
+    '/bar/{herp}',
+    '/unique/{foo}/bar',
+    '/optional_suffix_[foo]',
+    '/optional_suffix[/]',
+    '/optional_suffixes/[herp[/derp]]',
+    '/manual/en/{LegacyID}.php',
+  ];
 
   public function expectedMatches(
   ): varray<(string, string, dict<string, string>)> {
@@ -124,35 +123,45 @@ public function expectedMatchesWithResolvers(
   <<DataProvider('getAllResolvers')>>
   public function testMethodNotAllowedResponses(
     string $_name,
-    (function(dict<HttpMethod, dict<string, string>>): IResolver<string>)
-      $factory,
+    (function(
+      dict<HttpMethod, dict<string, string>>,
+    ): IResolver<string>) $factory,
   ): void {
     $map = dict[
       HttpMethod::GET => dict[
-        'getonly' => 'getonly',
+        '/get' => 'get',
       ],
       HttpMethod::HEAD => dict[
-        'headonly' => 'headonly',
+        '/head' => 'head',
       ],
       HttpMethod::POST => dict[
-        'postonly' => 'postonly',
+        '/post' => 'post',
       ],
     ];
 
     $router = $this->getRouter()->setResolver($factory($map));
 
-    list($responder, $_data) =
-      $router->routeMethodAndPath(HttpMethod::HEAD, 'getonly');
-    expect($responder)->toBeSame('getonly');
-    expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, 'headonly'))->toThrow(
-      MethodNotAllowedException::class,
-    );
-    expect(() ==> $router->routeMethodAndPath(HttpMethod::HEAD, 'postonly'))->toThrow(
-      MethodNotAllowedException::class,
-    );
-    expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, 'postonly'))->toThrow(
-      MethodNotAllowedException::class,
+    // HEAD -> GET ( re-routing )
+    list($responder, $_data) = $router->routeMethodAndPath(
+      HttpMethod::HEAD,
+      '/get',
     );
+    expect($responder)->toBeSame('get');
+
+    // GET -> HEAD
+    $e = expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, '/head'))
+      ->toThrow(MethodNotAllowedException::class);
+    expect($e->getAllowedMethods())->toBeSame(keyset[HttpMethod::HEAD]);
+
+    // HEAD -> POST
+    $e = expect(() ==> $router->routeMethodAndPath(HttpMethod::HEAD, '/post'))
+      ->toThrow(MethodNotAllowedException::class);
+    expect($e->getAllowedMethods())->toBeSame(keyset[HttpMethod::POST]);
+
+    // GET -> POST
+    $e = expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, '/post'))
+      ->toThrow(MethodNotAllowedException::class);
+    expect($e->getAllowedMethods())->toEqual(keyset[HttpMethod::POST]);
   }
 
   <<DataProvider('expectedMatches')>>
@@ -161,8 +170,8 @@ public function testMatchesPattern(
     string $expected_responder,
     dict<string, string> $expected_data,
   ): void {
-    list($actual_responder, $actual_data) =
-      $this->getRouter()->routeMethodAndPath(HttpMethod::GET, $in);
+    list($actual_responder, $actual_data) = $this->getRouter()
+      ->routeMethodAndPath(HttpMethod::GET, $in);
     expect($actual_responder)->toBeSame($expected_responder);
     expect(dict($actual_data))->toBeSame($expected_data);
   }
@@ -199,8 +208,10 @@ public function testRequestResponseInterfacesSupport(
     dict<string, string> $_expected_data,
   ): void {
     $router = $this->getRouter();
-    list($direct_responder, $direct_data) =
-      $router->routeMethodAndPath(HttpMethod::GET, $path);
+    list($direct_responder, $direct_data) = $router->routeMethodAndPath(
+      HttpMethod::GET,
+      $path,
+    );
 
     expect($path[0])->toBeSame('/');
 
@@ -217,21 +228,20 @@ public function testRequestResponseInterfacesSupport(
   <<DataProvider('getAllResolvers')>>
   public function testNotFound(
     string $_resolver_name,
-    (function(dict<HttpMethod, dict<string, string>>): IResolver<string>)
-      $factory,
+    (function(
+      dict<HttpMethod, dict<string, string>>,
+    ): IResolver<string>) $factory,
   ): void {
     $router = $this->getRouter()->setResolver($factory(dict[]));
-    expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, '/__404'))->toThrow(
-      NotFoundException::class,
-    );
+    expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, '/__404'))
+      ->toThrow(NotFoundException::class);
 
     $router = $this->getRouter()
       ->setResolver($factory(dict[
         HttpMethod::GET => dict['/foo' => '/foo'],
       ]));
-    expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, '/__404'))->toThrow(
-      NotFoundException::class,
-    );
+    expect(() ==> $router->routeMethodAndPath(HttpMethod::GET, '/__404'))
+      ->toThrow(NotFoundException::class);
   }
 
   public function testMethodNotAllowed(): void {