[Catalyst-commits] r13675 - in trunk/Catalyst-Plugin-PageCache: . lib/Catalyst/Plugin t

[timbunce at dev.catalyst.perl.org]

Author: timbunce
Date: 2010-11-01 18:43:50 +0000 (Mon, 01 Nov 2010)
New Revision: 13675

Modified:
   trunk/Catalyst-Plugin-PageCache/Changes
   trunk/Catalyst-Plugin-PageCache/Makefile.PL
   trunk/Catalyst-Plugin-PageCache/lib/Catalyst/Plugin/PageCache.pm
   trunk/Catalyst-Plugin-PageCache/t/08params.t
Log:
Use SHA1 to create cache key to limit length and charset. RT#62343.
Treat the order of repeated URL parameter values as significant.


Modified: trunk/Catalyst-Plugin-PageCache/Changes
===================================================================
--- trunk/Catalyst-Plugin-PageCache/Changes	2010-11-01 15:23:57 UTC (rev 13674)
+++ trunk/Catalyst-Plugin-PageCache/Changes	2010-11-01 18:43:50 UTC (rev 13675)
@@ -1,16 +1,18 @@
 Revision history for Perl extension Catalyst::Plugin::PageCache
 
 0.23    XXX
+        - XXX Test 15busy_lock.t fails as the moment.
         - Updated tests to use Catalyst::Plugin::Cache::Store::Memory instead
           of the deprecated ::FileCache (Report and patch from Rod Taylor) RT#53304.
         - Fixed t/04critic.t to not fail if Test::Perl::Critic is not installed.
-        - XXX Test 15busy_lock.t fails as the moment.
         - Updated test app code to avoid deprecated constructs.
         - Only serve GET and HEAD requests (instead of all except POST). RT#53307.
         - Allow key_maker to be the name of a method to be called on $c. RT#53529.
         - Assorted performance optimizations.
         - Add cache_dispatch_hook and cache_finalize_hook to config. RT#53503.
         - Refactored page cache storage logic, inspired by RT#53303.
+        - Use SHA1 to create cache key to limit length and charset. RT#62343.
+        - Treat the order of repeated URL parameter values as significant.
 
 0.22    2009-06-25 10:38:00
         - Update to use MRO::Compat

Modified: trunk/Catalyst-Plugin-PageCache/Makefile.PL
===================================================================
--- trunk/Catalyst-Plugin-PageCache/Makefile.PL	2010-11-01 15:23:57 UTC (rev 13674)
+++ trunk/Catalyst-Plugin-PageCache/Makefile.PL	2010-11-01 18:43:50 UTC (rev 13675)
@@ -6,6 +6,7 @@
 requires 'Catalyst::Runtime' => '0';
 requires 'Test::More';
 requires 'MRO::Compat' => '0.10';
+requires 'Digest::SHA1' => '0';
 
 auto_install;
 resources repository => 'http://dev.catalyst.perl.org/repos/Catalyst/trunk/Catalyst-Plugin-PageCache/';

Modified: trunk/Catalyst-Plugin-PageCache/lib/Catalyst/Plugin/PageCache.pm
===================================================================
--- trunk/Catalyst-Plugin-PageCache/lib/Catalyst/Plugin/PageCache.pm	2010-11-01 15:23:57 UTC (rev 13674)
+++ trunk/Catalyst-Plugin-PageCache/lib/Catalyst/Plugin/PageCache.pm	2010-11-01 18:43:50 UTC (rev 13675)
@@ -3,6 +3,7 @@
 use strict;
 use base qw/Class::Accessor::Fast/;
 use MRO::Compat;
+use Digest::SHA1 ();
 
 our $VERSION = '0.21';
 
@@ -319,6 +320,7 @@
 
     my $key = $c->_get_page_cache_key;
     my $config_PageCache = $c->config->{'Plugin::PageCache'};
+    my $now = time();
 
     my $headers = $c->res->headers;
 
@@ -422,11 +424,12 @@
 sub _get_page_cache_key {
     my $c = shift;
     
-    # We can't rely on the params after the user's code has run, so
-    # use the key created during the initial dispatch phase
+    # We can't rely on the params after the user's code has run,
+    # so we cache the key created during the initial dispatch phase
+    # and reuse it at finalize time.
     return $c->_page_cache_key if ( $c->_page_cache_key );
 
-    # override key if required
+    # override key if required, else use uri path
     my $keymaker = $c->config->{'Plugin::PageCache'}->{key_maker};
     my $key = $keymaker ? $keymaker->($c) : "/" . $c->req->path;
     
@@ -435,22 +438,31 @@
         $key = ':' . $c->language . ':' . $key;
     }
 
-    if ( scalar $c->req->param ) {
-        my @params;
-        foreach my $arg ( sort keys %{ $c->req->params } ) {
-            if ( ref $c->req->params->{$arg} ) {
-                my $list = $c->req->params->{$arg};
-                push @params, map { "$arg=" . $_  } sort @{$list};
-            }
-            else {
-                push @params, "$arg=" . $c->req->params->{$arg};
-            }
-        }
-        $key .= '?' . join( '&', @params );
+    # some caches have limits on the max key length (eg for memcached it's 250
+    # minus the namespace length) so if the key is a non-trvial length then
+    # replace the tail with a sha1. (We only replace the tail because it's
+    # useful to be able to see the leading portion of the path in the debug logs)
+    if (length($key) > 100) {
+        substr($key, 100) = Digest::SHA1::sha1_hex($key); # 40 bytes
     }
+    # Check for spaces as it's cheap insurance, just in case, for memcached
+    $key =~ s/\s/~/g;
+
+    # we always sha1 the parameters/query as they're typically long
+    # and it also avoids issues like spaces in keys for memcached
+    my $params_key;
+    my $parameters = $c->req->parameters;
+    if (%$parameters) {
+        local $Storable::canonical = 1;
+        $params_key = Storable::nfreeze($parameters);
+    }
     elsif ( my $query = $c->req->uri->query ) {
-        $key .= '?' . $query;
+        $params_key = $query;
     }
+    # use sha1 to avoid problems with over-long params or params
+    # containing values that can't be used as a key (eg spaces for memcached)
+    $key .= '?' . Digest::SHA1::sha1_hex($params_key) # 40 bytes
+        if $params_key;
 
     $c->_page_cache_key( $key );
 

Modified: trunk/Catalyst-Plugin-PageCache/t/08params.t
===================================================================
--- trunk/Catalyst-Plugin-PageCache/t/08params.t	2010-11-01 15:23:57 UTC (rev 13674)
+++ trunk/Catalyst-Plugin-PageCache/t/08params.t	2010-11-01 18:43:50 UTC (rev 13675)
@@ -30,12 +30,12 @@
 ok( $res = request('http://localhost/cache/count?c=3&b=2&a=1'), 'request ok' );
 is( $res->content, 1, 'count is still 1 from cache' );
 
-# test duplicate params
+# test multi-value params
 ok( $res = request('http://localhost/cache/count?a=1&a=2&a=3&b=4&c=5'), 'request ok' );
 is( $res->content, 2, 'count is 2' );
-
-# page will be served from cache even though params are different order
+# page NOT served from cache because multi-values for 'a' are in a different order
+# and we treat that as significant
 ok( $res = request('http://localhost/cache/count?b=4&c=5&a=2&a=3&a=1'), 'request ok' );
-is( $res->content, 2, 'count is still 2 from cache' );
+is( $res->content, 3, 'order of repeated param is significant' );