]> git.donarmstrong.com Git - debbugs.git/commitdiff
Use ETags; return timestamp not is_valid
authorDon Armstrong <don@donarmstrong.com>
Wed, 10 Aug 2016 19:37:47 +0000 (12:37 -0700)
committerDon Armstrong <don@donarmstrong.com>
Wed, 10 Aug 2016 19:37:47 +0000 (12:37 -0700)
 - Add test for libravatar.cgi
 - retrieve_libravatar and cache_location now return a timestamp, not
   is_valid which can then be used to calculate the Etag
 - Use HTTP::Server::Simple::CGI::Environment

Debbugs/Libravatar.pm
cgi/libravatar.cgi
t/18_libravatar_cgi.t [new file with mode: 0644]
t/lib/DebbugsTest.pm

index 0dafaf27adbdee63fc7634dd243223d98be49151..0c849431e09a17e4174aab51da0382a73e4a96f6 100644 (file)
@@ -84,6 +84,7 @@ sub retrieve_libravatar{
         );
     my %param = @_;
     my $cache_location = $param{location};
+    my $timestamp;
     $cache_location =~ s/\.[^\.\/]+$//;
     # take out a lock on the cache location so that if another request
     # is made while we are serving this one, we don't do double work
@@ -94,9 +95,10 @@ sub retrieve_libravatar{
     } else {
         # figure out if the cache is now valid; if it is, return the
         # cache location
-        my ($temp_location, $is_valid) = cache_location(email => $param{email});
-        if ($is_valid) {
-            return $temp_location;
+       my $temp_location;
+        ($temp_location, $timestamp) = cache_location(email => $param{email});
+        if ($timestamp) {
+            return ($temp_location,$timestamp);
         }
     }
     require LWP::UserAgent;
@@ -166,7 +168,8 @@ sub retrieve_libravatar{
         return undef;
     }
     simple_unlockfile($fh,$lockfile);
-    return $cache_location.'.'.$dest_type;
+    $timestamp = (stat($cache_location.'.'.$dest_type))[9];
+    return ($cache_location.'.'.$dest_type,$timestamp);
 }
 
 sub blocked_libravatar {
@@ -185,9 +188,9 @@ sub blocked_libravatar {
     return $blocked;
 }
 
-# Returns ($path, $is_valid)
+# Returns ($path, $timestamp)
 # - For blocked images, $path will be undef
-# - If $is_valid is false (and $path is not undef), the image should
+# - If $timestamp is 0 (and $path is not undef), the image should
 #   be re-fetched.
 sub cache_location {
     my %param = @_;
@@ -204,8 +207,8 @@ sub cache_location {
     for my $ext ('.png', '.jpg', '') {
         my $path = $stem.$ext;
         if (-e $path) {
-            my $is_valid = (time - (stat(_))[9] < 60*60) ? 1 : 0;
-            return ($path, $is_valid);
+            my $timestamp = (time - (stat(_))[9] < 60*60) ? (stat(_))[9] : 0;
+            return ($path, $timestamp);
         }
     }
     return ($stem, 0);
@@ -258,21 +261,22 @@ sub handler {
         return Apache2::Const::DECLINED();
     }
     # figure out what the md5sum of the e-mail is.
-    my ($cache_location, $is_valid) = cache_location(email => $email);
+    my ($cache_location, $timestamp) = cache_location(email => $email);
     # if we've got it, and it's less than one hour old, return it.
-    if ($is_valid) {
+    if ($timestamp) {
         serve_cache_mod_perl($cache_location,$r);
         return Apache2::Const::DECLINED();
     }
-    $cache_location = retrieve_libravatar(location => $cache_location,
-                                          email => $email,
-                                         );
+    ($cache_location,$timestamp) =
+       retrieve_libravatar(location => $cache_location,
+                           email => $email,
+                          );
     if (not defined $cache_location) {
         # failure, serve the default image
-        serve_cache_mod_perl('',$r);
+        serve_cache_mod_perl('',$r,$timestamp);
         return Apache2::Const::DECLINED();
     } else {
-        serve_cache_mod_perl($cache_location,$r);
+        serve_cache_mod_perl($cache_location,$r,$timestamp);
         return Apache2::Const::DECLINED();
     }
 }
@@ -282,7 +286,7 @@ sub handler {
 our $magic;
 
 sub serve_cache_mod_perl {
-    my ($cache_location,$r) = @_;
+    my ($cache_location,$r,$timestamp) = @_;
     if (not defined $cache_location or not length $cache_location) {
         # serve the default image
         $cache_location = $config{libravatar_default_image};
index 33aa90a7d5b09c8a3d8296e77dbd93bf336983d0..81f9c73855a141dd46ed58158355d8f20c7e5926 100755 (executable)
@@ -13,6 +13,7 @@ use Libravatar::URL;
 
 use CGI::Simple;
 use Cwd qw(abs_path);
+use Digest::MD5 qw(md5_hex);
 
 my $q = CGI::Simple->new();
 
@@ -25,45 +26,64 @@ my %param =
                   );
 # if avatar is no, serve the empty png
 if ($param{avatar} ne 'yes' or not defined $param{email} or not length $param{email}) {
-    serve_cache('',$q);
+    serve_cache('',$q,0);
     exit 0;
 }
 
-my ($cache_location, $is_valid) = cache_location(email => lc($param{email}));
+my ($cache_location, $timestamp) = cache_location(email => lc($param{email}));
 # if we've got it, and it's less than one hour old, return it.
-if ($is_valid) {
-    serve_cache($cache_location,$q);
+if ($timestamp) {
+    serve_cache($cache_location,$q,$timestamp);
     exit 0;
 }
 # if we don't have it, get it, and store it in the cache
-$cache_location = retrieve_libravatar(location => $cache_location,
-                                      email => lc($param{email}),
-                                     );
+($cache_location,$timestamp) =
+    retrieve_libravatar(location => $cache_location,
+                       email => lc($param{email}),
+                      );
 if (not defined $cache_location) {
     # failure, serve the default image
-    serve_cache('',$q);
+    serve_cache('',$q,0);
     exit 0;
 } else {
-    serve_cache($cache_location,$q);
+    serve_cache($cache_location,$q,$timestamp);
     exit 0;
 }
 
 
 sub serve_cache {
-    my ($cache_location,$q) = @_;
+    my ($cache_location,$q,$timestamp) = @_;
     if (not defined $cache_location or not length $cache_location) {
         # serve the default image
         $cache_location = $config{libravatar_default_image};
+       if (not defined $timestamp or not $timestamp) {
+           $timestamp = (stat($cache_location))[9];
+       }
+    }
+    if (not defined $timestamp) {
+       # this probably means that the default image doesn't exist
+       print $q->header(status => 404);
+       print "404: Not found\n";
+       return;
+    }
+    my $etag = md5_hex($cache_location.$timestamp);
+    if (defined $q->http('if-none-match')
+       and $etag eq $q->http('if-none-match')) {
+       print $q->header(-status => 304);
+       print "304: Not modified\n";
+       return;
     }
     my $fh = IO::File->new($cache_location,'r') or
-        error($q,404, "Failed to open cached image $cache_location");
+       error($q,404, "Failed to open cached image $cache_location");
     my $m = File::LibMagic->new() or
-        error($q,500,'Unable to create File::LibMagic object');
+       error($q,500,'Unable to create File::LibMagic object');
     my $mime_string = $m->checktype_filename(abs_path($cache_location)) or
-        error($q,500,'Bad file; no mime known');
+       error($q,500,'Bad file; no mime known');
     print $q->header(-type => $mime_string,
-                     -expires => '+1d',
-                    );
+                    -expires => '+1d',
+                    -status => 200,
+                    -etag => $etag,
+                   );
     print <$fh>;
     close($fh);
 }
diff --git a/t/18_libravatar_cgi.t b/t/18_libravatar_cgi.t
new file mode 100644 (file)
index 0000000..4d20e47
--- /dev/null
@@ -0,0 +1,78 @@
+# -*- mode: cperl;-*-
+
+use warnings;
+use strict;
+
+use Test::More;
+# The test functions are placed here to make things easier
+use lib qw(t/lib);
+use DebbugsTest qw(:all);
+
+plan tests => 3;
+
+my $port = 11344;
+
+# HTTP::Server:::Simple defines a SIG{CHLD} handler that breaks system; undef it here.
+$SIG{CHLD} = sub {};
+our %config;
+eval {
+    %config = create_debbugs_configuration(debug => exists $ENV{DEBUG}?$ENV{DEBUG}:0);
+};
+if ($@) {
+     BAIL_OUT($@);
+ }
+$ENV{DEBBUGS_CONFIG_FILE}  = "$config{config_dir}/debbugs_config";
+END{
+     if ($ENV{DEBUG}) {
+         diag("spool_dir:   $config{spool_dir}\n");
+         diag("config_dir:   $config{config_dir}\n");
+         diag("sendmail_dir: $config{sendmail_dir}\n");
+     }
+}
+
+my $libravatar_cgi_handler = sub {
+    my $fh;
+    $ENV{DEBBUGS_CONFIG_FILE} = $config{config_dir}."/debbugs_config";
+    open($fh,'-|',-e './cgi/libravatar.cgi'? './cgi/libravatar.cgi'
+        : '../cgi/libravatar.cgi');
+    my $headers;
+    my $status = 200;
+    while (<$fh>) {
+       if (/^\s*$/ and $status) {
+           print "HTTP/1.1 $status OK\n";
+           print $headers;
+           $status = 0;
+           print $_;
+       } elsif ($status) {
+           $headers .= $_;
+           if (/^Status:\s*(\d+)/i) {
+               $status = $1;
+           }
+       } else {
+           print $_;
+       }
+    }
+};
+
+
+ok(DebbugsTest::HTTPServer::fork_and_create_webserver($libravatar_cgi_handler,$port),
+   'forked HTTP::Server::Simple successfully');
+
+use LWP::UserAgent;
+my $ua = LWP::UserAgent->new;
+$ua->agent("DebbugsTesting/0.1 ");
+
+# Create a request
+my $req = HTTP::Request->new(GET => "http://localhost:$port/?avatar=no");
+
+my $res = $ua->request($req);
+ok($res->is_success(),'cgi/libravatar.cgi returns success');
+my $etag = $res->header('Etag');
+
+$req = HTTP::Request->new(GET => "http://localhost:$port/?avatar=no",
+                         ['If-None-Match',$etag]);
+$res = $ua->request($req);
+ok($res->code() eq '304','If-None-Match set gives us 304 not modified');
+
+
+
index df280061512421f67fe3045652ab8810cc3c1567..603b6ecd35a904f7af49424c831bf6fd09326b5a 100644 (file)
@@ -82,6 +82,7 @@ sub create_debbugs_configuration {
 \$gSpoolDir='$spool_dir';
 \$gLibPath='@{[getcwd()]}/scripts';
 \$gTemplateDir='@{[getcwd()]}/templates';
+\$gWebDir='@{[getcwd()]}/html';
 \$gWebHost='localhost';
 1;
 END
@@ -194,7 +195,7 @@ sub send_message{
 
 {
      package DebbugsTest::HTTPServer;
-     use base qw(HTTP::Server::Simple::CGI);
+     use base qw(HTTP::Server::Simple::CGI HTTP::Server::Simple::CGI::Environment);
 
      our $child_pid = undef;
      our $webserver = undef;