fixed a few more format detection bugs

mojolicious · Apr 14, 2012 · 9645cf4 · 9645cf4
1 parent 9493698
commit 9645cf4
Show file tree

Hide file tree

Showing 5 changed files with 129 additions and 33 deletions.
diff --git a/Changes b/Changes
@@ -5,7 +5,7 @@ This file documents the revision history for Perl extension Mojolicious.
   - Improved all debug messages.
   - Improved documentation.
   - Improved tests.
-  - Fixed format detection bug in Mojolicious::Routes::Pattern.
+  - Fixed format detection bugs in Mojolicious::Routes::Pattern.
 
 2.80  2012-04-10
   - Added support for alternative MIME types to Mojolicious::Types.

diff --git a/lib/Mojolicious/Command/routes.pm b/lib/Mojolicious/Command/routes.pm
@@ -70,13 +70,10 @@ sub _draw {
 
     # Regex
     my $pattern = $node->[1]->pattern;
-    $pattern->match('/');
-    my $regex  = (regexp_pattern $pattern->regex)[0];
-    my $format = (regexp_pattern $pattern->format)[0];
-    my $req    = $pattern->reqs->{format};
-    $format = defined $req ? '' : "(?:$format)?" unless $req;
-    push @parts, $node->[1]->is_endpoint ? "$regex$format" : $regex
-      if $verbose;
+    $pattern->match('/', $node->[1]->is_endpoint);
+    my $regex = (regexp_pattern $pattern->regex)[0];
+    my $format = (regexp_pattern $pattern->format || '')[0];
+    push @parts, $format ? "$regex(?:$format)" : $regex if $verbose;
 
     # Route
     say join('  ', @parts);

diff --git a/lib/Mojolicious/Routes/Pattern.pm b/lib/Mojolicious/Routes/Pattern.pm
@@ -2,8 +2,7 @@ package Mojolicious::Routes::Pattern;
 use Mojo::Base -base;
 
 has [qw/defaults reqs/] => sub { {} };
-has format => sub {qr#\.([^/]+)$#};
-has [qw/pattern regex/];
+has [qw/format pattern regex/];
 has quote_end     => ')';
 has quote_start   => '(';
 has relaxed_start => '.';
@@ -15,10 +14,9 @@ has wildcard_start => '*';
 sub new { shift->SUPER::new->parse(@_) }
 
 sub match {
-  my ($self, $path) = @_;
-  my $result = $self->shape_match(\$path);
-  return $result if !$path || $path eq '/';
-  return;
+  my ($self, $path, $detect) = @_;
+  my $result = $self->shape_match(\$path, $detect);
+  return !$path || $path eq '/' ? $result : undef;
 }
 
 sub parse {
@@ -33,8 +31,7 @@ sub parse {
   $self->reqs($reqs);
 
   # Format in pattern
-  $reqs->{format} = quotemeta($self->{strict} = $1)
-    if $pattern =~ m#\.([^/\)]+)$#;
+  $self->{strict} = $1 if $pattern =~ m#\.([^/\)]+)$#;
 
   # Tokenize
   return $self->pattern($pattern)->_tokenize;
@@ -82,8 +79,8 @@ sub shape_match {
   my ($self, $pathref, $detect) = @_;
 
   # Compile on demand
-  my $regex;
-  $regex = $self->_compile unless $regex = $self->regex;
+  my $regex = $self->regex || $self->_compile;
+  my $format = $detect ? ($self->format || $self->_compile_format) : undef;
 
   # Match
   return unless my @captures = $$pathref =~ $regex;
@@ -98,11 +95,13 @@ sub shape_match {
   }
 
   # Format
-  $result->{format} ||= $self->{strict} if $detect && exists $self->{strict};
   my $req = $self->reqs->{format};
   return $result if !$detect || defined $req && !$req;
-  my $format = $self->format;
-  if ($$pathref =~ s|^/?$format||) { $result->{format} ||= $1 }
+  if (exists $self->{strict}) {
+    $result->{format} //= $self->{strict};
+    return $result;
+  }
+  if ($$pathref =~ s|^/?$format||) { $result->{format} = $1 }
   elsif ($req) { return if !$result->{format} }
 
   return $result;
@@ -111,15 +110,8 @@ sub shape_match {
 sub _compile {
   my $self = shift;
 
-  # Compile format regex
-  my $reqs = $self->reqs;
-  if (!exists $reqs->{format} || $reqs->{format}) {
-    my $format =
-      defined $reqs->{format} ? _compile_req($reqs->{format}) : '([^/]+)';
-    $self->format(qr#\.$format$#);
-  }
-
   # Compile tree to regex
+  my $reqs     = $self->reqs;
   my $block    = '';
   my $regex    = '';
   my $optional = 1;
@@ -181,6 +173,23 @@ sub _compile {
   return $regex;
 }
 
+sub _compile_format {
+  my $self = shift;
+
+  # Format in pattern
+  return if $self->{strict};
+
+  # Default regex
+  my $reqs = $self->reqs;
+  return $self->format(qr#\.([^/]+)$#)->format
+    if !exists $reqs->{format} && $reqs->{format};
+
+  # Compile custom regex
+  my $regex =
+    defined $reqs->{format} ? _compile_req($reqs->{format}) : '([^/]+)';
+  return $self->format(qr#\.$regex$#)->format;
+}
+
 # "Interesting... Oh no wait, the other thing, tedious."
 sub _compile_req {
   my $req = shift;
@@ -304,7 +313,7 @@ Default parameters.
   my $regex = $pattern->format;
   $pattern  = $pattern->format($regex);
 
-Compiled regex for format matching, defaults to C<\.([^/]+)$>.
+Compiled regex for format matching.
 
 =head2 C<pattern>
 
@@ -392,6 +401,7 @@ Construct a new pattern object.
 =head2 C<match>
 
   my $result = $pattern->match('/foo/bar');
+  my $result = $pattern->match('/foo/bar', $detect);
 
 Match pattern against a path.
 

diff --git a/t/mojolicious/pattern.t b/t/mojolicious/pattern.t
@@ -1,6 +1,6 @@
 use Mojo::Base -strict;
 
-use Test::More tests => 49;
+use Test::More tests => 74;
 
 # "People said I was dumb, but I proved them."
 use Mojo::ByteStream 'b';
@@ -9,7 +9,7 @@ use Mojolicious::Routes::Pattern;
 # Normal pattern with text, symbols and a default value
 my $pattern = Mojolicious::Routes::Pattern->new('/test/(controller)/:action');
 $pattern->defaults({action => 'index'});
-my $result = $pattern->match('/test/foo/bar');
+my $result = $pattern->match('/test/foo/bar', 1);
 is $result->{controller}, 'foo', 'right value';
 is $result->{action},     'bar', 'right value';
 $result = $pattern->match('/test/foo');
@@ -123,3 +123,52 @@ $value  = b('abc%20cba')->url_unescape->to_string;
 $result = $pattern->match("/$value");
 is $result->{test}, $value, 'right value';
 is $pattern->render({test => $value}), "/$value", 'right result';
+
+# Format detection
+$pattern = Mojolicious::Routes::Pattern->new('/test');
+$pattern->defaults({action => 'index'});
+ok !$pattern->regex,  'no regex';
+ok !$pattern->format, 'no format regex';
+$result = $pattern->match('/test.xml', 1);
+ok $pattern->regex,  'regex has been compiled on demand';
+ok $pattern->format, 'format regex has been compiled on demand';
+is $result->{action}, 'index', 'right value';
+is $result->{format}, 'xml',   'right value';
+$pattern = Mojolicious::Routes::Pattern->new('/test.json');
+$pattern->defaults({action => 'index'});
+ok !$pattern->regex,  'no regex';
+ok !$pattern->format, 'no format regex';
+$result = $pattern->match('/test.json');
+ok $pattern->regex, 'regex has been compiled on demand';
+ok !$pattern->format, 'no format regex';
+is $result->{action}, 'index', 'right value';
+ok !$result->{format}, 'no value';
+$result = $pattern->match('/test.json', 1);
+is $result->{action}, 'index', 'right value';
+is $result->{format}, 'json',  'right value';
+$result = $pattern->match('/test.xml');
+is $result, undef, 'no result';
+$result = $pattern->match('/test');
+is $result, undef, 'no result';
+
+# Formats without detection
+$pattern = Mojolicious::Routes::Pattern->new('/test');
+$pattern->defaults({action => 'index'});
+ok !$pattern->regex,  'no regex';
+ok !$pattern->format, 'no format regex';
+$result = $pattern->match('/test.xml');
+ok $pattern->regex, 'regex has been compiled on demand';
+ok !$pattern->format, 'no format regex';
+is $result, undef, 'no result';
+$result = $pattern->match('/test');
+is $result->{action}, 'index', 'right value';
+
+# Format detection disabled
+$pattern = Mojolicious::Routes::Pattern->new('/test');
+$pattern->reqs({format => 0});
+$pattern->defaults({action => 'index'});
+$result = $pattern->match('/test', 1);
+is $result->{action}, 'index', 'right value';
+ok !$result->{format}, 'no value';
+$result = $pattern->match('/test.xml', 1);
+is $result, undef, 'no result';
diff --git a/t/mojolicious/routes.t b/t/mojolicious/routes.t
@@ -1,6 +1,6 @@
 use Mojo::Base -strict;
 
-use Test::More tests => 365;
+use Test::More tests => 386;
 
 # "They're not very heavy, but you don't hear me not complaining."
 use Mojolicious::Routes;
@@ -194,6 +194,16 @@ $versioned->route('/1.0')->to(controller => 'bar')->route('/test')
 $versioned->route('/2.4')->to(controller => 'foo')->route('/test')
   ->to(action => 'bar');
 
+# /versioned/too/1.0
+my $too = $r->route('/versioned/too')->to('too#');
+$too->route('/1.0')->to('#foo');
+$too->route('/2.0', format => 0)->to('#bar');
+
+# /multi/foo.bar
+my $multi = $r->route('/multi');
+$multi->route('/foo.bar')->to('just#works');
+$multi->route('/bar.baz')->to('works#too', format => 'xml');
+
 # Make sure stash stays clean
 my $m = Mojolicious::Routes::Match->new(GET => '/clean')->match($r);
 is $m->stack->[0]->{clean},     1,     'right value';
@@ -749,3 +759,33 @@ $m = Mojolicious::Routes::Match->new(GET => '/versioned/3.4/test')->match($r);
 is $m->stack->[0], undef, 'no value';
 $m = Mojolicious::Routes::Match->new(GET => '/versioned/0.3/test')->match($r);
 is $m->stack->[0], undef, 'no value';
+
+# Route with version at the end
+$m = Mojolicious::Routes::Match->new(GET => '/versioned/too/1.0')->match($r);
+is $m->stack->[0]->{controller}, 'too', 'right value';
+is $m->stack->[0]->{action},     'foo', 'right value';
+is $m->stack->[0]->{format},     '0',   'right value';
+is $m->stack->[1], undef, 'no value';
+is $m->path_for, '/versioned/too/1.0', 'right path';
+$m = Mojolicious::Routes::Match->new(GET => '/versioned/too/2.0')->match($r);
+is $m->stack->[0]->{controller}, 'too', 'right value';
+is $m->stack->[0]->{action},     'bar', 'right value';
+is $m->stack->[0]->{format},     undef, 'no value';
+is $m->stack->[1], undef, 'no value';
+is $m->path_for, '/versioned/too/2.0', 'right path';
+
+# Multiple extensions
+$m = Mojolicious::Routes::Match->new(GET => '/multi/foo.bar')->match($r);
+is $m->stack->[0]->{controller}, 'just',  'right value';
+is $m->stack->[0]->{action},     'works', 'right value';
+is $m->stack->[0]->{format},     'bar',   'right value';
+is $m->stack->[1], undef, 'no value';
+is $m->path_for, '/multi/foo.bar', 'right path';
+$m = Mojolicious::Routes::Match->new(GET => '/multi/foo.bar.baz')->match($r);
+is $m->stack->[0], undef, 'no value';
+$m = Mojolicious::Routes::Match->new(GET => '/multi/bar.baz')->match($r);
+is $m->stack->[0]->{controller}, 'works', 'right value';
+is $m->stack->[0]->{action},     'too',   'right value';
+is $m->stack->[0]->{format},     'xml',   'right value';
+is $m->stack->[1], undef, 'no value';
+is $m->path_for, '/multi/bar.baz', 'right path';