From 35f8b65ae74a28bee9e337bb42be47834027f4ea Mon Sep 17 00:00:00 2001 From: kobaken Date: Sun, 12 Nov 2023 23:48:17 +0900 Subject: [PATCH 1/2] Stabilize routing --- cpanfile | 2 +- lib/Kossy.pm | 42 +++++++++++++++------------- t/000_kossy/routing.t | 51 ++++++++++++++++++++++++++++++++++ t/100_bugs/003_random_routes.t | 30 ++++++++++++++++++++ 4 files changed, 105 insertions(+), 20 deletions(-) create mode 100644 t/000_kossy/routing.t create mode 100644 t/100_bugs/003_random_routes.t diff --git a/cpanfile b/cpanfile index f403354..6524624 100644 --- a/cpanfile +++ b/cpanfile @@ -4,7 +4,7 @@ requires 'Scalar::Util'; requires 'Plack' => '1.0029'; requires 'Plack::Middleware::Static'; requires 'Plack::Middleware::ReverseProxy'; -requires 'Router::Boom' => '1.01'; +requires 'Router::Boom' => '1.03'; requires 'Cwd'; requires 'File::Basename'; requires 'Text::Xslate' => '3.2.4'; diff --git a/lib/Kossy.pm b/lib/Kossy.pm index 0f5daf6..8654e4b 100644 --- a/lib/Kossy.pm +++ b/lib/Kossy.pm @@ -13,7 +13,7 @@ use JSON qw//; use Scalar::Util qw//; use Try::Tiny; use Encode; -use Router::Boom; +use Router::Boom::Method; use Class::Accessor::Lite ( new => 0, rw => [qw/root_dir/] @@ -74,9 +74,6 @@ sub psgi { sub build_app { my $self = shift; - #router - my $router = Router::Boom->new; - $router->add($_ => $self->_router->{$_} ) for keys %{$self->_router}; my $security_header_local = $SECURITY_HEADER; my %match_cache; @@ -110,17 +107,17 @@ sub build_app { return @{$match_cache{$cache_key}}; } my $path_info = Encode::decode_utf8( $env->{PATH_INFO}, Encode::FB_CROAK | Encode::LEAVE_SRC ); - my @match = $router->match($path_info); - if ( !@match ) { - $c->halt(404); + my ($dest, $captured, $method_not_allowed) = $self->_router->match($method, $path_info); + if ($method_not_allowed) { + $c->halt(405); } - if ( !exists $match[0]->{$method}) { - $c->halt(405); + if (!$dest) { + $c->halt(404); } - $match_cache{$cache_key} = [$match[0]->{$method},$match[1]] - if ! scalar keys %{$match[1]}; - return ($match[0]->{$method},$match[1]); + + $match_cache{$cache_key} = [$dest, $captured]; + return ($dest, $captured); } catch { if ( ref $_ && ref $_ eq 'Kossy::Exception' ) { die $_; #rethrow @@ -223,7 +220,7 @@ sub _router { my $klass = shift; my $class = ref $klass ? ref $klass : $klass; if ( !$_ROUTER->{$class} ) { - $_ROUTER->{$class} = {}; + $_ROUTER->{$class} = Router::Boom::Method->new; } $_ROUTER->{$class}; } @@ -236,12 +233,13 @@ sub _connect { $code = $filter; $filter = []; } - for my $method ( @$methods ) { - $class->_router->{$pattern}->{$method} = { - __action__ => $code, - __filter__ => $filter - }; + unless ( (ref $code ||'') eq 'CODE') { + Carp::croak "code must be CODE reference"; } + $class->_router->add($methods, $pattern, { + __action__ => $code, + __filter__ => $filter + }); } sub get { @@ -421,7 +419,13 @@ per-request object, herds request and response =item args:HashRef -Router::Simple->match result +path parameters captured by Router::Boom + + get '/user/:id' => sub { + my ($self, $c) = @_; + my $id = $c->args->{id}; + ... + }; =item halt(status_code, message) diff --git a/t/000_kossy/routing.t b/t/000_kossy/routing.t new file mode 100644 index 0000000..e707106 --- /dev/null +++ b/t/000_kossy/routing.t @@ -0,0 +1,51 @@ +use strict; +use warnings; + +use Test::More; +use Plack::Test; +use HTTP::Request::Common qw(GET POST PUT DELETE); + +{ + package SampleApp; + use Kossy; + + get '/' => sub { $_[1]->halt_text(200, 'GET /') }; + get '/user/me' => sub { $_[1]->halt_text(200, 'GET /user/me') }; + get '/user/:name' => sub { $_[1]->halt_text(200, 'GET /user/:name') }; + get '/user/:name/likes' => sub { $_[1]->halt_text(200, 'GET /user/:name/likes') }; + post '/user/:name/likes' => sub { $_[1]->halt_text(200, 'POST /user/:name/likes') }; + router 'DELETE', '/user/:name/likes' => sub { $_[1]->halt_text(200, 'DELETE /user/:name/likes') }; + + get '/entry/:id' => sub { $_[1]->halt_text(200, 'GET /entry/:id') }; + router 'PUT', '/entry/:id' => sub { $_[1]->halt_text(200, 'PUT /entry/:id') }; + get '/entry/foo' => sub { $_[1]->halt_text(200, 'GET /entry/foo') }; + + get '/org/{org_id:[0-9]+}' => sub { $_[1]->halt_text(200, 'GET /org/{org_id}') }; + get '/org/foo' => sub { $_[1]->halt_text(200, 'GET /org/foo') }; +} + +my $app = SampleApp->psgi; + +test_psgi $app, sub { + my $cb = shift; + + is $cb->(GET "/")->content, 'GET /'; + is $cb->(GET "/user/me")->content, 'GET /user/me'; + is $cb->(GET "/user/foo")->content, 'GET /user/:name'; + is $cb->(GET "/user/foo/likes")->content, 'GET /user/:name/likes'; + is $cb->(POST "/user/foo/likes")->content, 'POST /user/:name/likes'; + is $cb->(DELETE "/user/foo/likes")->content, 'DELETE /user/:name/likes'; + + is $cb->(GET "/entry/bar")->content, 'GET /entry/:id'; + is $cb->(PUT "/entry/bar")->content, 'PUT /entry/:id'; + is $cb->(GET "/entry/foo")->content, 'GET /entry/:id', 'Not match /entry/foo'; + + is $cb->(GET "/org/123")->content, 'GET /org/{org_id}'; + is $cb->(GET "/org/foo")->content, 'GET /org/foo'; + is $cb->(GET "/org/bar")->code, 404, 'Not match since `bar` is not number'; + + is $cb->(GET "/not_found")->code, 404, 'Not found path'; + is $cb->(POST "/")->code, 405, 'Method not allowed'; +}; + +done_testing; diff --git a/t/100_bugs/003_random_routes.t b/t/100_bugs/003_random_routes.t new file mode 100644 index 0000000..525c300 --- /dev/null +++ b/t/100_bugs/003_random_routes.t @@ -0,0 +1,30 @@ +use strict; +use warnings; + +use Test::More; +use Kossy; +use Plack::Test; +use HTTP::Request::Common; + +get '/api/me' => sub { + my ($self, $c) = @_; + $c->halt_text(200, '/api/me'); +}; + +get '/api/:name' => sub { + my ($self, $c) = @_; + $c->halt_text(200, '/api/:name'); +}; + +my $app = __PACKAGE__->psgi; + +test_psgi $app, sub { + my $cb = shift; + + # Matching routing was unstable due to random Hash keys. + # This change gives priority to the previously defined routing + my $res = $cb->(GET "/api/me"); + is $res->content, '/api/me'; +}; + +done_testing; From ed9a6ed3ab815459195fbbc94f85c8393c1a65aa Mon Sep 17 00:00:00 2001 From: kobaken Date: Mon, 13 Nov 2023 00:09:18 +0900 Subject: [PATCH 2/2] add test cases to t/000_kossy/routing.t --- lib/Kossy.pm | 2 +- t/000_kossy/routing.t | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/Kossy.pm b/lib/Kossy.pm index 8654e4b..b7abb90 100644 --- a/lib/Kossy.pm +++ b/lib/Kossy.pm @@ -234,7 +234,7 @@ sub _connect { $filter = []; } unless ( (ref $code ||'') eq 'CODE') { - Carp::croak "code must be CODE reference"; + Carp::croak "\$code argument must be a CODE reference"; } $class->_router->add($methods, $pattern, { __action__ => $code, diff --git a/t/000_kossy/routing.t b/t/000_kossy/routing.t index e707106..e2c60a9 100644 --- a/t/000_kossy/routing.t +++ b/t/000_kossy/routing.t @@ -48,4 +48,44 @@ test_psgi $app, sub { is $cb->(POST "/")->code, 405, 'Method not allowed'; }; +subtest 'filter' => sub { + + { + package FilterApp; + use Kossy; + + filter 'set_foo' => sub { + my $app = shift; + sub { + my ( $self, $c ) = @_; + $c->stash->{foo} = 'foo'; + $app->($self,$c); + } + }; + + get '/' => ['set_foo'] => sub { + my ($self, $c) = @_; + $c->halt_text(200, 'GET / and stash.foo is ' . $c->stash->{foo}) + }; + } + + my $app = FilterApp->psgi; + + test_psgi $app, sub { + my $cb = shift; + is $cb->(GET "/")->content, 'GET / and stash.foo is foo'; + }; +}; + +subtest 'exception' => sub { + { + package ExceptionApp; + use Kossy; + use Test::More; + + eval { get '/' => 'foo' }; + like $@, qr/must be a CODE reference/; + } +}; + done_testing;