Use ETags; return timestamp not is_valid

[debbugs.git] / Debbugs / Libravatar.pm

diff --git a/Debbugs/Libravatar.pm b/Debbugs/Libravatar.pm
index ca9c035df350bd48c6e59b80d0c8316690c21143..0c849431e09a17e4174aab51da0382a73e4a96f6 100644 (file)
--- a/Debbugs/Libravatar.pm
+++ b/Debbugs/Libravatar.pm
@@ -31,7 +31,7 @@ None known.
 use warnings;
 use strict;
 use vars qw($VERSION $DEBUG %EXPORT_TAGS @EXPORT_OK @EXPORT);
-use base qw(Exporter);
+use Exporter qw(import);
 
 use Debbugs::Config qw(:config);
 use Debbugs::Common qw(:lock);
@@ -39,7 +39,6 @@ use Libravatar::URL;
 use CGI::Simple;
 use Debbugs::CGI qw(cgi_parameters);
 use Digest::MD5 qw(md5_hex);
-use LWP::UserAgent;
 use File::Temp qw(tempfile);
 use File::LibMagic;
 use Cwd qw(abs_path);
@@ -51,22 +50,13 @@ BEGIN{
      $DEBUG = 0 unless defined $DEBUG;
 
      @EXPORT = ();
-     %EXPORT_TAGS = (libravatar => [qw(cache_valid retrieve_libravatar cache_location)]
+     %EXPORT_TAGS = (libravatar => [qw(retrieve_libravatar cache_location)]
                    );
      @EXPORT_OK = ();
      Exporter::export_ok_tags(keys %EXPORT_TAGS);
      $EXPORT_TAGS{all} = [@EXPORT_OK];
 }
 
-sub cache_valid{
-    my ($cache_location) = @_;
-    if (-e $cache_location) {
-        if (time - (stat($cache_location))[9] < 60*60) {
-            return 1;
-        }
-    }
-    return 0;
-}
 
 =over
 
@@ -94,7 +84,8 @@ sub retrieve_libravatar{
         );
     my %param = @_;
     my $cache_location = $param{location};
-    $cache_location =~ s/\.[^\.]+$//;
+    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
     my ($fh,$lockfile,$errors) =
@@ -104,11 +95,14 @@ sub retrieve_libravatar{
     } else {
         # figure out if the cache is now valid; if it is, return the
         # cache location
-        my $temp_location = cache_location(email => $param{email});
-        if (cache_valid($temp_location)) {
-            return $temp_location;
+       my $temp_location;
+        ($temp_location, $timestamp) = cache_location(email => $param{email});
+        if ($timestamp) {
+            return ($temp_location,$timestamp);
         }
     }
+    require LWP::UserAgent;
+
     my $dest_type;
     eval {
         my $uri = libravatar_url(email => $param{email},
@@ -159,7 +153,7 @@ sub retrieve_libravatar{
                    $temp_fn,
                    $cache_location.'.'.$dest_type) == 0 or
                        die "convert file failed";
-            unlink($temp_fh);
+            unlink($temp_fn);
         };
         if ($@) {
             unlink($cache_location.'.'.$dest_type) if -e $cache_location.'.'.$dest_type;
@@ -174,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 {
@@ -193,9 +188,13 @@ sub blocked_libravatar {
     return $blocked;
 }
 
+# Returns ($path, $timestamp)
+# - For blocked images, $path will be undef
+# - If $timestamp is 0 (and $path is not undef), the image should
+#   be re-fetched.
 sub cache_location {
     my %param = @_;
-    my $md5sum;
+    my ($md5sum, $stem);
     if (exists $param{md5sum}) {
         $md5sum = $param{md5sum};
     }elsif (exists $param{email}) {
@@ -203,13 +202,16 @@ sub cache_location {
     } else {
         croak("cache_location must be called with one of md5sum or email");
     }
-    return undef if blocked_libravatar($param{email},$md5sum);
-    for my $ext (qw(.png .jpg)) {
-        if (-e $config{libravatar_cache_dir}.'/'.$md5sum.$ext) {
-            return $config{libravatar_cache_dir}.'/'.$md5sum.$ext;
+    return (undef, 0) if blocked_libravatar($param{email},$md5sum);
+    $stem = $config{libravatar_cache_dir}.'/'.$md5sum;
+    for my $ext ('.png', '.jpg', '') {
+        my $path = $stem.$ext;
+        if (-e $path) {
+            my $timestamp = (time - (stat(_))[9] < 60*60) ? (stat(_))[9] : 0;
+            return ($path, $timestamp);
         }
     }
-    return $config{libravatar_cache_dir}.'/'.$md5sum;
+    return ($stem, 0);
 }
 
 ## the following is mod_perl specific
@@ -259,21 +261,22 @@ sub handler {
         return Apache2::Const::DECLINED();
     }
     # figure out what the md5sum of the e-mail is.
-    my $cache_location = 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 (cache_valid($cache_location)) {
+    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();
     }
 }
@@ -283,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};