changelog | [dw-free] Cleanup duplicated "is this the right format" code

[commit: http://hg.dwscoalition.org/dw-free/rev/9477663e2e77]

http://bugs.dwscoalition.org/show_bug.cgi?id=2963

Refactoring to reduce duplication, and make it easier for future developers
who are writing pages.

Patch by
exor674.

Files modified:
cgi-bin/DW/Controller/CommentCount.pm
cgi-bin/DW/Controller/CutExpander.pm
cgi-bin/DW/Controller/Edges.pm
cgi-bin/DW/Controller/Latest/Mood.pm
cgi-bin/DW/Controller/Nav.pm
cgi-bin/DW/Routing.pm
cgi-bin/DW/Routing/CallInfo.pm
t/routing.t
--------------------------------------------------------------------------------
diff -r 1e9fafee8741 -r 9477663e2e77 cgi-bin/DW/Controller/CommentCount.pm
--- a/cgi-bin/DW/Controller/CommentCount.pm	Sat Aug 28 19:15:22 2010 +0800
+++ b/cgi-bin/DW/Controller/CommentCount.pm	Sat Aug 28 19:24:43 2010 +0800
@@ -23,7 +23,7 @@ use DW::Routing;
 use DW::Routing;
 use Image::Magick;
 
-DW::Routing->register_string("/tools/commentcount", \&commentcount_handler, app => 1);
+DW::Routing->register_string("/tools/commentcount", \&commentcount_handler, app => 1, format => 'png' );
 
 sub commentcount_handler {
     my $r = DW::Request->get;
@@ -50,7 +50,6 @@ sub commentcount_handler {
     $image->Read("label:$count");
 
     # return the image
-    $r->content_type("image/png");
     $r->print( $image->ImageToBlob( magick => "png" ) );
 
     return $r->OK;
diff -r 1e9fafee8741 -r 9477663e2e77 cgi-bin/DW/Controller/CutExpander.pm
--- a/cgi-bin/DW/Controller/CutExpander.pm	Sat Aug 28 19:15:22 2010 +0800
+++ b/cgi-bin/DW/Controller/CutExpander.pm	Sat Aug 28 19:24:43 2010 +0800
@@ -21,7 +21,7 @@ use DW::Routing;
 use DW::Routing;
 use JSON;
 
-DW::Routing->register_string( "/__rpc_cuttag", \&cutexpander_handler, app => 1, user => 1 );
+DW::Routing->register_string( "/__rpc_cuttag", \&cutexpander_handler, app => 1, user => 1, format => 'json' );
 
 my $formats = {
     'json' => [ "application/json; charset=utf-8", sub { $_[0]->print( objToJson( $_[1] ) ); } ],
diff -r 1e9fafee8741 -r 9477663e2e77 cgi-bin/DW/Controller/Edges.pm
--- a/cgi-bin/DW/Controller/Edges.pm	Sat Aug 28 19:15:22 2010 +0800
+++ b/cgi-bin/DW/Controller/Edges.pm	Sat Aug 28 19:24:43 2010 +0800
@@ -28,20 +28,14 @@ DW::Routing->register_string(  "/data/ed
 DW::Routing->register_string(  "/data/edges", \&edges_handler, user => 1, format => 'json' );
 
 my $formats = {
-    'json' => [ "application/json; charset=utf-8", sub { $_[0]->print( objToJson( $_[1] ) ); } ],
+    'json' => sub { $_[0]->print( objToJson( $_[1] ) ); },
 };
 
 sub edges_handler {
     my $opts = shift;
     my $r = DW::Request->get;
 
-    # allow them to pick what format they want the data in, but bail if they ask for one
-    # we don't understand
-    my $format = $formats->{$opts->format};
-    return $r->NOT_FOUND if ! $format;
-
-    # content type early on
-    $r->content_type( $format->[0] );
+    my $format = $formats->{ $opts->format };
 
     # Outputs an error message
     my $error_out = sub {
@@ -108,7 +102,7 @@ sub edges_handler {
         map { $_ => { name => $us->{$_}->user, type => $us->{$_}->journaltype } } keys %$us
     };
 
-    $format->[1]->( $r, $response );
+    $format->( $r, $response );
 
     return $r->OK;
 }
diff -r 1e9fafee8741 -r 9477663e2e77 cgi-bin/DW/Controller/Latest/Mood.pm
--- a/cgi-bin/DW/Controller/Latest/Mood.pm	Sat Aug 28 19:15:22 2010 +0800
+++ b/cgi-bin/DW/Controller/Latest/Mood.pm	Sat Aug 28 19:24:43 2010 +0800
@@ -22,7 +22,7 @@ use DW::Mood;
 use DW::Mood;
 use JSON;
 
-DW::Routing->register_string( "/latest/mood", \&mood_handler );
+DW::Routing->register_string( "/latest/mood", \&mood_handler, formats => [ 'html', 'json' ] );
 
 sub mood_handler {
     my $r = DW::Request->get;
@@ -38,9 +38,7 @@ sub mood_handler {
         },
     };
     
-    my $format = $formats->{ $_[0]->format || 'html' };
-
-    return $r->NOT_FOUND if ! $format;
+    my $format = $formats->{ $_[0]->format };
 
     my $moods = LJ::MemCache::get( "latest_moods" ) || [];
     my $out = {};
diff -r 1e9fafee8741 -r 9477663e2e77 cgi-bin/DW/Controller/Nav.pm
--- a/cgi-bin/DW/Controller/Nav.pm	Sat Aug 28 19:15:22 2010 +0800
+++ b/cgi-bin/DW/Controller/Nav.pm	Sat Aug 28 19:24:43 2010 +0800
@@ -26,7 +26,7 @@ use JSON;
 
 # Defines the URL for routing.  I could use register_string( '/nav' ... ) if I didn't want to capture arguments
 # This is an application page, not a user styled page, and the default format is HTML (ie, /nav gives /nav.html)
-DW::Routing->register_regex( qr!^/nav(?:/([a-z]*))?$!, \&nav_handler, app => 1 );
+DW::Routing->register_regex( qr!^/nav(?:/([a-z]*))?$!, \&nav_handler, app => 1, formats => [ 'html', 'json' ] );
 
 # handles menu nav pages
 sub nav_handler {
diff -r 1e9fafee8741 -r 9477663e2e77 cgi-bin/DW/Routing.pm
--- a/cgi-bin/DW/Routing.pm	Sat Aug 28 19:15:22 2010 +0800
+++ b/cgi-bin/DW/Routing.pm	Sat Aug 28 19:24:43 2010 +0800
@@ -35,6 +35,7 @@ my $default_content_types = {
     'html' => "text/html; charset=utf-8",
     'json' => "application/json; charset=utf-8",
     'plain' => "text/plain; charset=utf-8",
+    'png' => "image/png",
 };
 
 LJ::ModuleLoader->require_subclasses( "DW::Controller" )
@@ -159,6 +160,10 @@ sub _call_hash {
     $opts->prepare_for_call;
 
     my $format = $opts->format;
+    # check for format validity
+    return $r->NOT_FOUND unless $opts->format_valid;
+
+    # apply default content type if it exists
     $r->content_type( $default_content_types->{$format} )
         if $default_content_types->{$format};
 
@@ -204,7 +209,6 @@ sub _call_hash {
 
 sub _static_helper {
     my $r = DW::Request->get;
-    return $r->NOT_FOUND unless $_[0]->format eq 'html';
     return  DW::Template->render_template( $_[0]->args );
 }
 
@@ -220,17 +224,7 @@ Static page helper.
 
 =item filename - template filename
 
-=item Opts:
-
-=over
-
-=item ssl - If this sub should run for ssl.
-
-=item app - 1 if app
-
-=item user - 1 if user
-
-=back
+=item Opts ( see register_string )
 
 =back
 
@@ -255,13 +249,17 @@ sub register_static {
 
 =over
 
+=item args - passed verbatim to sub.
+
 =item ssl - If this sub should run for ssl.
-
-=item args - passed verbatim to sub.
 
 =item app - 1 if app
 
 =item user - 1 if user
+
+=item format - What format should be used, defaults to HTML
+
+=item formats - An array of possible formats, or 1 to allow everything.
 
 =back
 
@@ -272,15 +270,9 @@ sub register_string {
 sub register_string {
     my ( $class, $string, $sub, %opts ) = @_;
 
-    $opts{app} = 1 if ! defined $opts{app} && ! $opts{user};
-    my $hash = {
-        args   => $opts{args},
+    my $hash = _apply_defaults( \%opts, {
         sub    => $sub,
-        ssl    => $opts{ssl} || 0,
-        app    => $opts{app} || 0,
-        user   => $opts{user} || 0,
-        format => $opts{format} || 'html',
-    };
+    });
     $string_choices{'app'  . $string} = $hash if $hash->{app};
     $string_choices{'ssl'  . $string} = $hash if $hash->{ssl};
     $string_choices{'user' . $string} = $hash if $hash->{user};
@@ -294,19 +286,7 @@ sub register_string {
 
 =item sub - sub
 
-=item Opts:
-
-=over
-
-=item ssl - If this sub should run for ssl.
-
-=item args - passed verbatim to sub.
-
-=item app - 1 if app
-
-=item user - 1 if user
-
-=back
+=item Opts ( see register_string )
 
 =back
 
@@ -315,19 +295,35 @@ sub register_regex {
 sub register_regex {
     my ( $class, $regex, $sub, %opts ) = @_;
 
-    $opts{app} = 1 if ! defined $opts{app} && !$opts{user};
-    my $hash = {
+    my $hash = _apply_defaults( \%opts, {
         regex  => $regex,
-        args   => $opts{args},
         sub    => $sub,
-        ssl    => $opts{ssl} || 0,
-        app    => $opts{app} || 0,
-        user   => $opts{user} || 0,
-        format => $opts{format} || 'html',
-    };
+    });
     push @{$regex_choices{app}}, $hash if $hash->{app};
     push @{$regex_choices{ssl}}, $hash if $hash->{ssl};
     push @{$regex_choices{user}}, $hash if $hash->{user};
+}
+
+# internal method, intentionally no POD
+# applies default for opts and hash
+
+sub _apply_defaults {
+    my ( $opts, $hash ) = @_;
+
+    $hash ||= 0;
+    $opts->{app} = 1 if ! defined $opts->{app} && !$opts->{user};
+    $hash->{args} = $opts->{args};
+    $hash->{ssl} = $opts->{ssl} || 0;
+    $hash->{app} = $opts->{app} || 0;
+    $hash->{user} = $opts->{user} || 0;
+    $hash->{format} = $opts->{format} || 'html';
+
+    my $formats = $opts->{formats} || [ $hash->{format} ];
+    $formats = { map { ( $_, 1 ) } @$formats } if ( ref($formats) eq 'ARRAY' );
+
+    $hash->{formats} = $formats;
+
+    return $hash;
 }
 
 =head1 AUTHOR
diff -r 1e9fafee8741 -r 9477663e2e77 cgi-bin/DW/Routing/CallInfo.pm
--- a/cgi-bin/DW/Routing/CallInfo.pm	Sat Aug 28 19:15:22 2010 +0800
+++ b/cgi-bin/DW/Routing/CallInfo.pm	Sat Aug 28 19:24:43 2010 +0800
@@ -102,6 +102,18 @@ Return the format.
 
 sub format { return $_[0]->{format}; }
 
+=head2 C<< $self->format_valid >>
+
+Returns if the format is valid for this CallInfo
+
+=cut
+
+sub format_valid {
+    my $formats = $_[0]->{__hash}->{formats};
+    return 1 if $formats == 1;
+    return $formats->{$_[0]->format} || 0;
+}
+
 =head2 C<< $self->role >>
 
 Current mode: 'app' or 'user' or 'ssl'
diff -r 1e9fafee8741 -r 9477663e2e77 t/routing.t
--- a/t/routing.t	Sat Aug 28 19:15:22 2010 +0800
+++ b/t/routing.t	Sat Aug 28 19:24:43 2010 +0800
@@ -1,6 +1,6 @@
 # -*-perl-*-
 use strict;
-use Test::More tests => 174;
+use Test::More tests => 186;
 use lib "$ENV{LJHOME}/cgi-bin";
 
 # don't let DW::Routing load DW::Controller subclasses
@@ -19,7 +19,7 @@ handle_request( "foo", "/foo.format", 0,
 handle_request( "foo", "/foo.format", 0, 0 ); # 1 test
 # 2
 
-DW::Routing->register_string( "/test/app", \&handler, app => 1, args => "it_worked_app" );
+DW::Routing->register_string( "/test/app", \&handler, app => 1, args => "it_worked_app", formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/test app (app)" , "/test/app", 1, "it_worked_app" ); # 3 tests
@@ -33,7 +33,7 @@ handle_request( "/test app (user)", "/te
 handle_request( "/test app (user)", "/test/app.format", 0, "it_worked_app", username => 'test' ); # 1 test
 # 12
 
-DW::Routing->register_string( "/test/ssl", \&handler, ssl => 1, app => 0, args => "it_worked_ssl" );
+DW::Routing->register_string( "/test/ssl", \&handler, ssl => 1, app => 0, args => "it_worked_ssl", formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/test ssl (app)" , "/test/ssl", 0, "it_worked_ssl" ); # 1 tests
@@ -47,7 +47,7 @@ handle_request( "/test ssl (user)", "/te
 handle_request( "/test ssl (user)", "/test/ssl.format", 0, "it_worked_ssl", username => 'test' ); # 3 tests
 # 22
 
-DW::Routing->register_string( "/test/user", \&handler, user => 1, args => "it_worked_user" );
+DW::Routing->register_string( "/test/user", \&handler, user => 1, args => "it_worked_user", formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/test user (app)" , "/test/user", 0, "it_worked_user" ); # 1 tests
@@ -61,9 +61,9 @@ handle_request( "/test user (user)", "/t
 handle_request( "/test user (user)", "/test/user.format", 1, "it_worked_user", username => 'test' ); # 3 tests
 # 32
 
-DW::Routing->register_string( "/test", \&handler, app => 1, args => "it_worked_app" );
-DW::Routing->register_string( "/test", \&handler, ssl => 1, app => 0, args => "it_worked_ssl" );
-DW::Routing->register_string( "/test", \&handler, user => 1, args => "it_worked_user" );
+DW::Routing->register_string( "/test", \&handler, app => 1, args => "it_worked_app", formats => [ 'html', 'format' ] );
+DW::Routing->register_string( "/test", \&handler, ssl => 1, app => 0, args => "it_worked_ssl", formats => [ 'html', 'format' ] );
+DW::Routing->register_string( "/test", \&handler, user => 1, args => "it_worked_user", formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/test multi (app)" , "/test", 1, "it_worked_app" ); # 3 tests
@@ -77,7 +77,7 @@ handle_request( "/test multi (user)", "/
 handle_request( "/test multi (user)", "/test.format", 1, "it_worked_user", username => 'test' ); # 3 tests
 # 50
 
-DW::Routing->register_string( "/test/all", \&handler, app => 1, user => 1, ssl => 1, format => 'json', args => "it_worked_multi" );
+DW::Routing->register_string( "/test/all", \&handler, app => 1, user => 1, ssl => 1, format => 'json', args => "it_worked_multi", formats => [ 'json', 'format' ] );
 
 $expected_format = 'json';
 handle_request( "/test all (app)" , "/test/all", 1, "it_worked_multi"); # 3 tests
@@ -91,7 +91,7 @@ handle_request( "/test all (user)", "/te
 handle_request( "/test all (user)", "/test/all.format", 1, "it_worked_multi", username => 'test' ); # 3 tests
 # 68
 
-DW::Routing->register_regex( qr !^/r/app(/.+)$!, \&regex_handler, app => 1, args => ["/test", "it_worked_app"] );
+DW::Routing->register_regex( qr !^/r/app(/.+)$!, \&regex_handler, app => 1, args => ["/test", "it_worked_app"], formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/r/app (app)" , "/r/app/test", 1, "it_worked_app" ); # 3 tests
@@ -105,7 +105,7 @@ handle_request( "/r/app (user)", "/r/app
 handle_request( "/r/app (user)", "/r/app/test.format", 0, "it_worked_app", username => 'test' ); # 1 test
 # 79
 
-DW::Routing->register_regex( qr !^/r/ssl(/.+)$!, \&regex_handler, ssl => 1, app => 0, args => ["/test", "it_worked_ssl"] );
+DW::Routing->register_regex( qr !^/r/ssl(/.+)$!, \&regex_handler, ssl => 1, app => 0, args => ["/test", "it_worked_ssl"], formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/r/ssl (app)" , "/r/ssl/test", 0, "it_worked_ssl" ); # 1 tests
@@ -119,7 +119,7 @@ handle_request( "/r/ssl (user)", "/r/ssl
 handle_request( "/r/ssl (user)", "/r/ssl/test.format", 0, "it_worked_ssl", username => 'test' ); # 1 test
 # 92
 
-DW::Routing->register_regex( qr !^/r/user(/.+)$!, \&regex_handler, user => 1, args => ["/test", "it_worked_user"] );
+DW::Routing->register_regex( qr !^/r/user(/.+)$!, \&regex_handler, user => 1, args => ["/test", "it_worked_user"], formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/r/user (app)" , "/r/user/test", 0, "it_worked_user" ); # 1 tests
@@ -133,9 +133,9 @@ handle_request( "/r/user (user)", "/r/us
 handle_request( "/r/user (user)", "/r/user/test.format", 1, "it_worked_user", username => 'test' ); # 3 tests
 # 104
 
-DW::Routing->register_regex( qr !^/r/multi(/.+)$!, \&regex_handler, app => 1, args => ["/test", "it_worked_app"] );
-DW::Routing->register_regex( qr !^/r/multi(/.+)$!, \&regex_handler, ssl => 1, app => 0, args => ["/test", "it_worked_ssl"] );
-DW::Routing->register_regex( qr !^/r/multi(/.+)$!, \&regex_handler, user => 1, args => ["/test", "it_worked_user"] );
+DW::Routing->register_regex( qr !^/r/multi(/.+)$!, \&regex_handler, app => 1, args => ["/test", "it_worked_app"], formats => [ 'html', 'format' ] );
+DW::Routing->register_regex( qr !^/r/multi(/.+)$!, \&regex_handler, ssl => 1, app => 0, args => ["/test", "it_worked_ssl"], formats => [ 'html', 'format' ] );
+DW::Routing->register_regex( qr !^/r/multi(/.+)$!, \&regex_handler, user => 1, args => ["/test", "it_worked_user"], formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/r/multi (app)" , "/r/multi/test", 1, "it_worked_app" ); # 3 tests
@@ -149,7 +149,7 @@ handle_request( "/r/multi (user)", "/r/m
 handle_request( "/r/multi (user)", "/r/multi/test.format", 1, "it_worked_user", username => 'test' ); # 3 tests
 # 128
 
-DW::Routing->register_regex( qr !^/r/all(/.+)$!, \&regex_handler, app => 1, user => 1, ssl => 1, format => 'json', args => ["/test", "it_worked_all"] );
+DW::Routing->register_regex( qr !^/r/all(/.+)$!, \&regex_handler, app => 1, user => 1, ssl => 1, format => 'json', args => ["/test", "it_worked_all"], formats => [ 'json', 'format' ] );
 
 $expected_format = 'json';
 handle_request( "/r/all (app)" , "/r/all/test", 1, "it_worked_all" ); # 3 tests
@@ -163,7 +163,7 @@ handle_request( "/r/all (user)", "/r/all
 handle_request( "/r/all (user)", "/r/all/test.format", 1, "it_worked_all", username => 'test' ); # 3 tests
 # 152
 
-DW::Routing->register_string( "/test/app_implicit", \&handler, args => "it_worked_app" );
+DW::Routing->register_string( "/test/app_implicit", \&handler, args => "it_worked_app", formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/test appapp_implicit (app)" , "/test/app_implicit", 1, "it_worked_app" ); # 3 tests
@@ -177,7 +177,7 @@ handle_request( "/test appapp_implicit (
 handle_request( "/test appapp_implicit (user)", "/test/app_implicit.format", 0, "it_worked_app", username => 'test' ); # 1 test
 # 162
 
-DW::Routing->register_regex( qr !^/r/app_implicit(/.+)$!, \&regex_handler, args => ["/test", "it_worked_app"] );
+DW::Routing->register_regex( qr !^/r/app_implicit(/.+)$!, \&regex_handler, args => ["/test", "it_worked_app"], formats => [ 'html', 'format' ] );
 
 $expected_format = 'html';
 handle_request( "/r/app_implicit (app)" , "/r/app_implicit/test", 1, "it_worked_app" ); # 3 tests
@@ -190,6 +190,32 @@ handle_request( "/r/app_implicit (ssl)" 
 handle_request( "/r/app_implicit (ssl)" , "/r/app_implicit/test.format", 0, "it_worked_app", ssl => 1 ); # 1 test
 handle_request( "/r/app_implicit (user)", "/r/app_implicit/test.format", 0, "it_worked_app", username => 'test' ); # 1 test
 # 174
+
+# test improper formats
+$expected_format = 'json';
+handle_request( "/test app (app) invalid" , "/test/app.json", 1, undef, expected_error => 404 ); # 1 test
+handle_request( "/r/all (app) invalid" , "/r/all/test.html", 1, undef, expected_error => 404 ); # 1 test
+# 176
+
+# test implied formats
+DW::Routing->register_string( "/test/app/implied_format", \&handler, app => 1, args => "it_worked_app_if" );
+
+$expected_format = 'html';
+handle_request( "/test app implied_format (app)" , "/test/app/implied_format", 1, "it_worked_app_if" ); # 3 tests
+
+$expected_format = 'json';
+handle_request( "/test app implied_format (app) invalid" , "/test/app/implied_format.json", 1, undef, expected_error => 404 ); # 1 test
+# 180
+
+# test all formats
+DW::Routing->register_string( "/test/app/all_format", \&handler, app => 1, args => "it_worked_app_af", formats => 1 );
+
+$expected_format = 'html';
+handle_request( "/test app implied_format (app)" , "/test/app/all_format", 1, "it_worked_app_af" ); # 3 tests
+
+$expected_format = 'json';
+handle_request( "/test app implied_format (app)" , "/test/app/all_format.json", 1, "it_worked_app_af" ); # 3 test
+# 186
 
 use Data::Dumper;
 sub handle_request {
@@ -210,7 +236,8 @@ sub handle_request {
         return 1;
     }
 
-    is( $ret, $r->OK, "$name: wrong return" );
+    my $expected_ret = $opts{expected_error} || $r->OK;
+    is( $ret, $expected_ret, "$name: wrong return" );
     if ( $ret != $r->OK ) {
         return 0;
     }
--------------------------------------------------------------------------------