From: John Napiorkowski <jjn1056@yahoo.com>
Date: Fri, 10 Apr 2015 17:22:06 +0000 (-0500)
Subject: tweaked how constraints work to narrow and tighten scope
X-Git-Tag: 5.90089_002~13
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Runtime.git;a=commitdiff_plain;h=d9f0a350554cca79adefd4e97b4982d431f8c914

tweaked how constraints work to narrow and tighten scope
---

diff --git a/Changes b/Changes
index e6049cb..416e2b7 100644
--- a/Changes
+++ b/Changes
@@ -9,6 +9,9 @@
     defined and associated with the Moose::TypeRegistry) you must now quote
     the type name.  This is to clearly disambiguate between Moose stringy types
     and imported types.
+  - Additional changes to type constraint detection to between determine when a
+    type constraint for reference types have a measured number of arguments or
+    not.  clarify restriction on reference type constraints.
 
 5.90089_001 - 2015-03-26
   - New development branch synched with 5.90085.
diff --git a/lib/Catalyst/Action.pm b/lib/Catalyst/Action.pm
index b5e75d4..0a38d75 100644
--- a/lib/Catalyst/Action.pm
+++ b/lib/Catalyst/Action.pm
@@ -83,24 +83,40 @@ has number_of_args_constraints => (
     my $self = shift;
     return unless $self->has_args_constraints;
 
-    my $total = 0;
-    foreach my $tc( @{$self->args_constraints}) {
-      if($tc->is_a_type_of('Ref')) {
-        if($tc->can('parameters') && $tc->has_parameters) {
-          my $total_params = scalar(@{ $tc->parameters||[] });
-          $total = $total + $total_params;
-        } else {
-          # Its a Reftype but we don't know the number of params it
-          # actually validates.
-          warn "Your type constraint '$tc' is a reference type but I cannot determine its number of parameters in action ${\$self->private_path}";
+    # If there is one constraint and its a ref, we need to decide
+    # if this number 'unknown' number or if the ref allows us to
+    # determine a length.
+
+    if(scalar @{$self->args_constraints} == 1) {
+      my $tc = $self->args_constraints->[0];
+      if(
+        $tc->can('is_strictly_a_type_of') &&
+        $tc->is_strictly_a_type_of('Tuple'))
+      {
+        my @parameters = @{ $tc->parameters||[]};
+        if( defined($parameters[-1]) and exists($parameters[-1]->{slurpy})) {
           return undef;
+        } else {
+          return my $total_params = scalar(@parameters);
         }
+      } elsif($tc->is_a_type_of('Ref')) {
+        return undef;
       } else {
-        $total++;
+        return 1; # Its a normal 1 arg type constraint.
       }
+    } else {
+      # We need to loop thru and error on ref types.  We don't allow a ref type
+      # in the middle.
+      my $total = 0;
+      foreach my $tc( @{$self->args_constraints}) {
+        if($tc->is_a_type_of('Ref')) {
+          die "$tc is a Ref type constraint.  You cannot mix Ref and non Ref type constraints in Args for action ${\$self->reverse}";
+        } else {
+          ++$total;
+        }
+      }
+      return $total;
     }
-
-    return $total;
   }
 
 has args_constraints => (
@@ -152,24 +168,40 @@ has number_of_captures_constraints => (
     my $self = shift;
     return unless $self->has_captures_constraints;
 
-    my $total = 0;
-    foreach my $tc( @{$self->captures_constraints}) {
-      if($tc->is_a_type_of('Ref')) {
-        if($tc->can('parameters') && $tc->has_parameters) {
-          my $total_params = scalar(@{ $tc->parameters||[] });
-          $total = $total + $total_params;
+    # If there is one constraint and its a ref, we need to decide
+    # if this number 'unknown' number or if the ref allows us to
+    # determine a length.
+
+    if(scalar @{$self->captures_constraints} == 1) {
+      my $tc = $self->captures_constraints->[0];
+      if(
+        $tc->can('is_strictly_a_type_of') &&
+        $tc->is_strictly_a_type_of('Tuple'))
+      {
+        my @parameters = @{ $tc->parameters||[]};
+        if( defined($parameters[-1]) and exists($parameters[-1]->{slurpy})) {
+          return undef;
         } else {
-          # Its a Reftype but we don't know the number of params it
-          # actually validates.  This is not currently permitted in
-          # a capture...
-          die "You cannot use CaptureArgs($tc) in ${\$self->reverse} because we cannot determined the number of its parameters";
+          return my $total_params = scalar(@parameters);
         }
+      } elsif($tc->is_a_type_of('Ref')) {
+        die "You cannot use CaptureArgs($tc) in ${\$self->reverse} because we cannot determined the number of its parameters";
       } else {
-        $total++;
+        return 1; # Its a normal 1 arg type constraint.
+      }
+    } else {
+      # We need to loop thru and error on ref types.  We don't allow a ref type
+      # in the middle.
+      my $total = 0;
+      foreach my $tc( @{$self->captures_constraints}) {
+        if($tc->is_a_type_of('Ref')) {
+          die "$tc is a Ref type constraint.  You cannot mix Ref and non Ref type constraints in CaptureArgs for action ${\$self->reverse}";
+        } else {
+          ++$total;
+        }
       }
+      return $total;
     }
-
-    return $total;
   }
 
 has captures_constraints => (
@@ -211,7 +243,7 @@ has captures_constraints => (
 
 sub resolve_type_constraint {
   my ($self, $name) = @_;
-  my @tc = eval "package ${\$self->class}; $name";
+  my @tc = eval "package ${\$self->class}; $name" or die "'$name' not a type constraint in ${\$self->private_path}";
   if($tc[0]) {
     return map { ref($_) ? $_ : Moose::Util::TypeConstraints::find_or_parse_type_constraint($_) } @tc;
   } else {
@@ -282,9 +314,6 @@ sub match_args {
     my ($self, $c, $args) = @_;
     my @args = @{$args||[]};
 
-    # If infinite args, we always match
-    return 1 if $self->normalized_arg_number == ~0;
-
     # There there are arg constraints, we must see to it that the constraints
     # check positive for each arg in the list.
     if($self->has_args_constraints) {
@@ -297,7 +326,21 @@ sub match_args {
           $self->args_constraints->[0]->is_a_type_of('ClassName')
         )
       ) {
-        return $self->args_constraints->[0]->check($args);
+        # Ok, the the type constraint is a ref type, which is allowed to have
+        # any number of args.  We need to check the arg length, if one is defined.
+        # If we had a ref type constraint that allowed us to determine the allowed
+        # number of args, we need to match that number.  Otherwise if there was an
+        # undetermined number (~0) then we allow all the args.  This is more of an
+        # Optimization since Tuple[Int, Int] would fail on 3,4,5 anyway, but this
+        # way we can avoid calling the constraint when the arg length is incorrect.
+        if(
+          $self->normalized_arg_number == ~0 ||
+          scalar( @args ) == $self->normalized_arg_number
+        ) {
+          return $self->args_constraints->[0]->check($args);
+        } else {
+          return 0;
+        }
         # Removing coercion stuff for the first go
         #if($self->args_constraints->[0]->coercion && $self->attributes->{Coerce}) {
         #  my $coerced = $self->args_constraints->[0]->coerce($c) || return 0;
@@ -315,6 +358,9 @@ sub match_args {
         return 1;
       }
     } else {
+      # If infinite args with no constraints, we always match
+      return 1 if $self->normalized_arg_number == ~0;
+
       # Otherwise, we just need to match the number of args.
       return scalar( @args ) == $self->normalized_arg_number;
     }
diff --git a/lib/Catalyst/Controller.pm b/lib/Catalyst/Controller.pm
index 820eaed..3db8f36 100644
--- a/lib/Catalyst/Controller.pm
+++ b/lib/Catalyst/Controller.pm
@@ -898,6 +898,15 @@ declared attributes you must quote them:
 
     sub my_moose_type :Local Args('Int') { ... }
 
+If you use 'reference' type constraints (such as ArrayRef[Int]) that have an unknown
+number of allowed matches, we set this the same way "Args" is.  Please keep in mind
+that actions with an undetermined number of args match at lower precidence than those
+with a fixed number.  You may use reference types such as Tuple from L<Types::Standard>
+that allows you to fix the number of allowed args.  For example Args(Tuple[Int,Int])
+would be determined to be two args (or really the same as Args(Int,Int).)  You may
+find this useful for creating custom subtypes with complex matching rules that you 
+wish to reuse over many actions.
+
 See L<Catalyst::RouteMatching> for more.
 
 =head2 Consumes('...')
diff --git a/lib/Catalyst/DispatchType/Chained.pm b/lib/Catalyst/DispatchType/Chained.pm
index 6705833..dc9d1a3 100644
--- a/lib/Catalyst/DispatchType/Chained.pm
+++ b/lib/Catalyst/DispatchType/Chained.pm
@@ -728,6 +728,10 @@ custom type constraints and import them into the controller namespace:
 
       sub int_priority_chain :Chained(chain_base) PathPart('') Args(Int) { }
 
+If you use a reference type constraint in CaptureArgs, it must be a type
+like Tuple in L<Types::Standard> that allows us to determine the number of
+args to match.  Otherwise this will raise an error during startup.
+
 See L<Catalyst::RouteMatching> for more.
 
 =item Args
@@ -748,6 +752,9 @@ Just as with C<:CaptureArgs>, the arguments get passed to the action in
 C<@_> after the context object. They can also be reached through
 C<$c-E<gt>request-E<gt>arguments>.
 
+You should see 'Args' in L<Catalyst::Controller> for more details on using
+type constraints in your Args declarations.
+
 =back
 
 =head2 Auto actions, dispatching and forwarding
diff --git a/t/arg_constraints.t b/t/arg_constraints.t
index 95b92b0..2a005b7 100644
--- a/t/arg_constraints.t
+++ b/t/arg_constraints.t
@@ -66,6 +66,7 @@ BEGIN {
 
   use Moose;
   use MooseX::MethodAttributes;
+  use Types::Standard qw/slurpy/;
   use MyApp::Types qw/Tuple Int Str StrMatch ArrayRef UserId User Heart/;
 
   extends 'Catalyst::Controller';
@@ -98,7 +99,7 @@ BEGIN {
   }
 
   sub many_ints :Local Args(ArrayRef[Int]) {
-    my ($self, $c, $int) = @_;
+    my ($self, $c, @ints) = @_;
     $c->res->body('many_ints');
   }
 
@@ -107,6 +108,11 @@ BEGIN {
     $c->res->body('tuple');
   }
 
+  sub slurpy_tuple :Local Args(Tuple[Str,Int, slurpy ArrayRef[Int]]) {
+    my ($self, $c, $str, $int) = @_;
+    $c->res->body('tuple');
+  }
+
   sub match :Local Args(StrMatch[qr{\d\d-\d\d-\d\d}]) {
     my ($self, $c, $int) = @_;
     $c->res->body('match');
@@ -300,6 +306,17 @@ SKIP: {
 }
 
 {
+  my $res = request '/tuple/aaa/111/111/111';
+  is $res->content, 'default';
+}
+
+{
+  my $res = request '/slurpy_tuple/aaa/111/111/111';
+  is $res->content, 'tuple';
+}
+
+
+{
   my $res = request '/many_ints/1/2/a';
   is $res->content, 'default';
 }