Fix Chained dispatch for several first-come first-served bugs when comparing Args...

Added tests to that effect.

diff --git a/lib/Catalyst/DispatchType/Chained.pm b/lib/Catalyst/DispatchType/Chained.pm
index 4eec171..bee76f1 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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' );
+       }
+
 }