From: Gareth Kirwan Date: Sun, 2 Sep 2007 17:42:48 +0000 (+0000) Subject: Fix Chained dispatch for several first-come first-served bugs when comparing Args... X-Git-Tag: 5.7099_04~141 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Runtime.git;a=commitdiff_plain;h=6365b5279cf3bbecd61735b3b744809983a7bfdc;hp=4dca6c081be62b6542fda0a8098e8787dd5bcae2 Fix Chained dispatch for several first-come first-served bugs when comparing Args() to a chain or two separate chains. Added tests to that effect. --- diff --git a/lib/Catalyst/DispatchType/Chained.pm b/lib/Catalyst/DispatchType/Chained.pm index 4eec171..bee76f1 100644 --- a/lib/Catalyst/DispatchType/Chained.pm +++ b/lib/Catalyst/DispatchType/Chained.pm @@ -101,7 +101,8 @@ sub match { my @parts = split('/', $path); - my ($chain, $captures) = $self->recurse_match($c, '/', \@parts); + my ($chain, $captures, $parts) = $self->recurse_match($c, '/', \@parts); + push @{$c->req->args}, @$parts; return 0 unless $chain; @@ -126,6 +127,7 @@ sub recurse_match { my ( $self, $c, $parent, $path_parts ) = @_; my $children = $self->{children_of}{$parent}; return () unless $children; + my $best_action; my @captures; TRY: foreach my $try_part (sort { length($b) <=> length($a) } keys %$children) { @@ -152,22 +154,33 @@ sub recurse_match { push(@captures, splice(@parts, 0, $capture_attr->[0])); # try the remaining parts against children of this action - my ($actions, $captures) = $self->recurse_match( + my ($actions, $captures, $action_parts) = $self->recurse_match( $c, '/'.$action->reverse, \@parts ); - if ($actions) { - return [ $action, @$actions ], [ @captures, @$captures ]; - } - } else { + if ($actions && (!$best_action || $#$action_parts < $#{$best_action->{parts}})){ + $best_action = { + actions => [ $action, @$actions ], + captures=> [ @captures, @$captures ], + parts => $action_parts + }; + } + } + else { { local $c->req->{arguments} = [ @{$c->req->args}, @parts ]; next TRY_ACTION unless $action->match($c); } - push(@{$c->req->args}, @parts); - return [ $action ], [ ]; + if (!$best_action || $#parts < $#{$best_action->{parts}}){ + $best_action = { + actions => [ $action ], + captures=> [], + parts => \@parts + } + } } } } + return @$best_action{qw/actions captures parts /} if $best_action; return (); } diff --git a/t/lib/TestApp/Controller/Action/Chained.pm b/t/lib/TestApp/Controller/Action/Chained.pm index e89b8e0..61210dc 100644 --- a/t/lib/TestApp/Controller/Action/Chained.pm +++ b/t/lib/TestApp/Controller/Action/Chained.pm @@ -158,6 +158,23 @@ sub mult_nopp_id : Chained('mult_nopp_base') PathPart('') CaptureArgs(1) { } sub mult_nopp_idall : Chained('mult_nopp_id') PathPart('') Args(0) { } sub mult_nopp_idnew : Chained('mult_nopp_id') PathPart('new') Args(0) { } +# +# Test Choice between branches and early return logic +# Declaration order is important for $children->{$*}, since this is first match best. +# +sub cc_base : Chained('/') PathPart('chained/choose_capture') CaptureArgs(0) { } +sub cc_link : Chained('cc_base') PathPart('') CaptureArgs(0) { } +sub cc_anchor : Chained('cc_link') PathPart('anchor.html') Args(0) { } +sub cc_all : Chained('cc_base') PathPart('') Args() { } + +sub cc_a : Chained('cc_base') PathPart('') CaptureArgs(1) { } +sub cc_a_link : Chained('cc_a') PathPart('a') CaptureArgs(0) { } +sub cc_a_anchor : Chained('cc_a_link') PathPart('') Args() { } + +sub cc_b : Chained('cc_base') PathPart('b') CaptureArgs(0) { } +sub cc_b_link : Chained('cc_b') PathPart('') CaptureArgs(1) { } +sub cc_b_anchor : Chained('cc_b_link') PathPart('anchor.html') Args() { } + sub end :Private { my ($self, $c) = @_; return if $c->stash->{no_end}; diff --git a/t/live_component_controller_action_chained.t b/t/live_component_controller_action_chained.t index cb4f0dc..411542e 100644 --- a/t/live_component_controller_action_chained.t +++ b/t/live_component_controller_action_chained.t @@ -10,7 +10,7 @@ our $iters; BEGIN { $iters = $ENV{CAT_BENCH_ITERS} || 1; } -use Test::More tests => 109*$iters; +use Test::More tests => 118*$iters; use Catalyst::Test 'TestApp'; if ( $ENV{CAT_BENCHMARK} ) { @@ -766,4 +766,66 @@ sub run_tests { is( $response->content, '; ', 'Content OK' ); } + # + # Higher Args() hiding more specific CaptureArgs chains sections + # + { + my @expected = qw[ + TestApp::Controller::Action::Chained->begin + TestApp::Controller::Action::Chained->cc_base + TestApp::Controller::Action::Chained->cc_link + TestApp::Controller::Action::Chained->cc_anchor + TestApp::Controller::Action::Chained->end + ]; + + my $expected = join ', ', @expected; + + ok( my $response = request('http://localhost/chained/choose_capture/anchor.html'), + 'Choose between an early Args() and a later more ideal chain' ); + is( $response->header('X-Catalyst-Executed') => $expected, 'Executed actions'); + is( $response->content => '; ', 'Content OK' ); + } + + # + # Less specific chain not being seen correctly due to earlier looser capture + # + { + my @expected = qw[ + TestApp::Controller::Action::Chained->begin + TestApp::Controller::Action::Chained->cc_base + TestApp::Controller::Action::Chained->cc_b + TestApp::Controller::Action::Chained->cc_b_link + TestApp::Controller::Action::Chained->cc_b_anchor + TestApp::Controller::Action::Chained->end + ]; + + my $expected = join ', ', @expected; + + ok( my $response = request('http://localhost/chained/choose_capture/b/a/anchor.html'), + 'Choose between a more specific chain and an earlier looser one' ); + is( $response->header('X-Catalyst-Executed') => $expected, 'Executed actions'); + is( $response->content => 'a; ', 'Content OK' ); + } + + # + # Check we get the looser one when it's the correct match + # + { + my @expected = qw[ + TestApp::Controller::Action::Chained->begin + TestApp::Controller::Action::Chained->cc_base + TestApp::Controller::Action::Chained->cc_a + TestApp::Controller::Action::Chained->cc_a_link + TestApp::Controller::Action::Chained->cc_a_anchor + TestApp::Controller::Action::Chained->end + ]; + + my $expected = join ', ', @expected; + + ok( my $response = request('http://localhost/chained/choose_capture/a/a/anchor.html'), + 'Choose between a more specific chain and an earlier looser one' ); + is( $response->header('X-Catalyst-Executed') => $expected, 'Executed actions'); + is( $response->content => 'a; anchor.html', 'Content OK' ); + } + }