Attachment 49022 Details for Bug 11998 – Bug 11998: Use Koha::Cache to cache sysprefs

[patch] Bug 11998: Use Koha::Cache to cache sysprefs

Bug-11998-Use-KohaCache-to-cache-sysprefs.patch (text/plain), 16.41 KB, created by Tomás Cohen Arazi (tcohen) on 2016-03-11 14:00:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2016-03-11 14:00:00 UTC

Size: 16.41 KB

patch

obsolete

>From 05fb967156361238f93dc094158e8ff9b59547c6 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Thu, 3 Mar 2016 16:54:30 +0000
>Subject: [PATCH] Bug 11998: Use Koha::Cache to cache sysprefs
>
>At the moment, the sysprefs are only cache in the thread memory
>executing the processus
>When using Plack, that means we need to clear the syspref cache on each
>page.
>To avoid that, we can use Koha::Cache to cache the sysprefs correctly.
>
>A big part of the authorship of this patch goes to Robin Sheat.
>
>Test plan:
>1/ Add/Update/Delete local use prefs
>2/ Update pref values and confirm that the changes are correctly taken
>into account
>
>Signed-off-by: Chris <chrisc@catalyst.net.nz>
>
>Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
>Tested with plack with syspref cache enabled, there is some time between setting the syspref and applying it, but it takes just one reload of page, it shouldn't be problem, should it?
>Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
>---
> C4/Context.pm                | 89 +++++++++++++++++++++++++++++++-------------
> Koha/Config/SysPref.pm       | 32 ++++++++++++++++
> admin/preferences.pl         |  1 -
> admin/systempreferences.pl   | 61 +++++++-----------------------
> misc/admin/koha-preferences  |  1 -
> svc/config/systempreferences |  2 -
> t/db_dependent/Context.t     | 11 +++++-
> t/db_dependent/sysprefs.t    | 15 +++++++-
> 8 files changed, 132 insertions(+), 80 deletions(-)
>
>diff --git a/C4/Context.pm b/C4/Context.pm
>index 226ba6a..5b27e28 100644
>--- a/C4/Context.pm
>+++ b/C4/Context.pm
>@@ -100,6 +100,7 @@ BEGIN {
> use Encode;
> use ZOOM;
> use XML::Simple;
>+use Koha::Cache;
> use POSIX ();
> use DateTime::TimeZone;
> use Module::Load::Conditional qw(can_load);
>@@ -108,6 +109,7 @@ use Carp;
> use C4::Boolean;
> use C4::Debug;
> use Koha;
>+use Koha::Config::SysPref;
> use Koha::Config::SysPrefs;
> 
> =head1 NAME
>@@ -502,19 +504,18 @@ with this method.
> 
> =cut
> 
>-# FIXME: running this under mod_perl will require a means of
>-# flushing the caching mechanism.
>-
>-my %sysprefs;
>+my $syspref_cache = Koha::Cache->get_instance();
> my $use_syspref_cache = 1;
>-
> sub preference {
>     my $self = shift;
>     my $var  = shift;    # The system preference to return
> 
>-    if ($use_syspref_cache && exists $sysprefs{lc $var}) {
>-        return $sysprefs{lc $var};
>-    }
>+    $var = lc $var;
>+
>+    my $cached_var = $use_syspref_cache
>+        ? $syspref_cache->get_from_cache("syspref_$var")
>+        : undef;
>+    return $cached_var if defined $cached_var;
> 
>     my $dbh  = C4::Context->dbh or return 0;
> 
>@@ -527,7 +528,7 @@ sub preference {
>         $value = $syspref ? $syspref->value() : undef;
>     }
> 
>-    $sysprefs{lc $var} = $value;
>+    $syspref_cache->set_in_cache("syspref_$var", $value) if $use_syspref_cache;
>     return $value;
> }
> 
>@@ -550,6 +551,8 @@ default behavior.
> sub enable_syspref_cache {
>     my ($self) = @_;
>     $use_syspref_cache = 1;
>+    # We need to clear the cache to have it up-to-date
>+    $self->clear_syspref_cache();
> }
> 
> =head2 disable_syspref_cache
>@@ -578,43 +581,79 @@ will not be seen by this process.
> =cut
> 
> sub clear_syspref_cache {
>-    %sysprefs = ();
>+    $syspref_cache->flush_all if $use_syspref_cache;
> }
> 
> =head2 set_preference
> 
>-  C4::Context->set_preference( $variable, $value );
>+  C4::Context->set_preference( $variable, $value, [ $explanation, $type, $options ] );
> 
> This updates a preference's value both in the systempreferences table and in
>-the sysprefs cache.
>+the sysprefs cache. If the optional parameters are provided, then the query
>+becomes a create. It won't update the parameters (except value) for an existing
>+preference.
> 
> =cut
> 
> sub set_preference {
>-    my $self = shift;
>-    my $var = lc(shift);
>-    my $value = shift;
>+    my ( $self, $variable, $value, $explanation, $type, $options ) = @_;
> 
>-    my $syspref = Koha::Config::SysPrefs->find( $var );
>-    my $type = $syspref ? $syspref->type() : undef;
>+    $variable = lc $variable;
> 
>-    $value = 0 if ( $type && $type eq 'YesNo' && $value eq '' );
>+    my $syspref = Koha::Config::SysPrefs->find($variable);
>+    $type =
>+        $type    ? $type
>+      : $syspref ? $syspref->type
>+      :            undef;
> 
>     # force explicit protocol on OPACBaseURL
>-    if ($var eq 'opacbaseurl' && substr($value,0,4) !~ /http/) {
>+    if ( $variable eq 'opacbaseurl' && substr( $value, 0, 4 ) !~ /http/ ) {
>         $value = 'http://' . $value;
>     }
> 
>     if ($syspref) {
>-        $syspref = $syspref->set( { value => $value } )->store();
>-    }
>-    else {
>-        $syspref = Koha::Config::SysPref->new( { variable => $var, value => $value } )->store();
>+        $syspref->set(
>+            {   ( defined $value ? ( value       => $value )       : () ),
>+                ( $explanation   ? ( explanation => $explanation ) : () ),
>+                ( $type          ? ( type        => $type )        : () ),
>+                ( $options       ? ( options     => $options )     : () ),
>+            }
>+        )->store;
>+    } else {
>+        $syspref = Koha::Config::SysPref->new(
>+            {   variable    => $variable,
>+                value       => $value,
>+                explanation => $explanation || undef,
>+                type        => $type,
>+                options     => $options || undef,
>+            }
>+        )->store();
>     }
> 
>-    if ($syspref) {
>-        $sysprefs{$var} = $value;
>+    $syspref_cache->set_in_cache( "syspref_$variable", $value )
>+      if $use_syspref_cache;
>+
>+    return $syspref;
>+}
>+
>+=head2 delete_preference
>+
>+    C4::Context->delete_preference( $variable );
>+
>+This deletes a system preference from the database. Returns a true value on
>+success. Failure means there was an issue with the database, not that there
>+was no syspref of the name.
>+
>+=cut
>+
>+sub delete_preference {
>+    my ( $self, $var ) = @_;
>+
>+    if ( Koha::Config::SysPrefs->find( $var )->delete ) {
>+        $syspref_cache->clear_from_cache("syspref_$var") if $use_syspref_cache;
>+        return 1;
>     }
>+    return 0;
> }
> 
> =head2 Zconn
>diff --git a/Koha/Config/SysPref.pm b/Koha/Config/SysPref.pm
>index e1850b5..1e71b8c 100644
>--- a/Koha/Config/SysPref.pm
>+++ b/Koha/Config/SysPref.pm
>@@ -23,6 +23,8 @@ use Carp;
> 
> use Koha::Database;
> 
>+use C4::Log;
>+
> use base qw(Koha::Object);
> 
> =head1 NAME
>@@ -35,6 +37,36 @@ Koha::Config::SysPref - Koha System Preference Object class
> 
> =cut
> 
>+=head3 store
>+
>+=cut
>+
>+sub store {
>+    my ($self) = @_;
>+
>+    my $action = $self->in_storage ? 'MODIFY' : 'ADD';
>+
>+    C4::Log::logaction( 'SYSTEMPREFERENCE', $action, undef, $self->variable . ' | ' . $self->value );
>+
>+    return $self->SUPER::store($self);
>+}
>+
>+=head3 delete
>+
>+=cut
>+
>+sub delete {
>+    my ($self) = @_;
>+
>+    my $variable = $self->variable;
>+    my $value    = $self->value;
>+    my $deleted  = $self->SUPER::delete($self);
>+
>+    C4::Log::logaction( 'SYSTEMPREFERENCE', 'DELETE', undef, " $variable | $value" );
>+
>+    return $deleted;
>+}
>+
> =head3 type
> 
> =cut
>diff --git a/admin/preferences.pl b/admin/preferences.pl
>index 24bffce..ef2b643 100755
>--- a/admin/preferences.pl
>+++ b/admin/preferences.pl
>@@ -314,7 +314,6 @@ if ( $op eq 'save' ) {
>             my $value = join( ',', $input->param( $param ) );
> 
>             C4::Context->set_preference( $pref, $value );
>-            logaction( 'SYSTEMPREFERENCE', 'MODIFY', undef, $pref . " | " . $value );
>         }
>     }
> 
>diff --git a/admin/systempreferences.pl b/admin/systempreferences.pl
>index 01c0300..67e0638 100755
>--- a/admin/systempreferences.pl
>+++ b/admin/systempreferences.pl
>@@ -50,7 +50,6 @@ use C4::Context;
> use C4::Koha;
> use C4::Languages qw(getTranslatedLanguages);
> use C4::ClassSource;
>-use C4::Log;
> use C4::Output;
> use YAML::Syck qw( Dump LoadFile );
> 
>@@ -276,24 +275,8 @@ if ( $op eq 'update_and_reedit' ) {
>             );    # we show only the TMPL_VAR names $op
>         }
>     }
>-    my $dbh   = C4::Context->dbh;
>-    my $query = "select * from systempreferences where variable=?";
>-    my $sth   = $dbh->prepare($query);
>-    $sth->execute( $input->param('variable') );
>-    if ( $sth->rows ) {
>-        unless ( C4::Context->config('demo') ) {
>-            my $sth = $dbh->prepare("update systempreferences set value=?,explanation=?,type=?,options=? where variable=?");
>-            $sth->execute( $value, $input->param('explanation'), $input->param('variable'), $input->param('preftype'), $input->param('prefoptions') );
>-            logaction( 'SYSTEMPREFERENCE', 'MODIFY', undef, $input->param('variable') . " | " . $value );
>-        }
>-    } else {
>-        unless ( C4::Context->config('demo') ) {
>-            my $sth = $dbh->prepare("insert into systempreferences (variable,value,explanation) values (?,?,?,?,?)");
>-            $sth->execute( $input->param('variable'), $input->param('value'), $input->param('explanation'), $input->param('preftype'), $input->param('prefoptions') );
>-            logaction( 'SYSTEMPREFERENCE', 'ADD', undef, $input->param('variable') . " | " . $input->param('value') );
>-        }
>-    }
>-
>+    my $variable = $input->param('variable');
>+    C4::Context->set_preference($variable, $value) unless C4::Context->config('demo');
> }
> 
> ################## ADD_FORM ##################################
>@@ -322,13 +305,14 @@ if ( $op eq 'add_form' ) {
> ################## ADD_VALIDATE ##################################
>     # called by add_form, used to insert/modify data in DB
> } elsif ( $op eq 'add_validate' ) {
>-    my $dbh = C4::Context->dbh;
>-    my $sth = $dbh->prepare("select * from systempreferences where variable=?");
>-    $sth->execute( $input->param('variable') );
>-
>     # to handle multiple values
>     my $value;
> 
>+    my $variable = $input->param('variable');
>+    my $expl     = $input->param('explanation');
>+    my $type     = $input->param('preftype');
>+    my $options  = $input->param('prefoptions');
>+
>     # handle multiple value strings (separated by ',')
>     my $params = $input->Vars;
>     if ( defined $params->{'value'} ) {
>@@ -345,49 +329,30 @@ if ( $op eq 'add_form' ) {
>         }
>     }
> 
>-    if ( $input->param('preftype') eq 'Upload' ) {
>+    if ( $type eq 'Upload' ) {
>         my $lgtfh = $input->upload('value');
>         $value = join '', <$lgtfh>;
>         $value = encode_base64($value);
>     }
> 
>-    if ( $sth->rows ) {
>-        unless ( C4::Context->config('demo') ) {
>-            my $sth = $dbh->prepare("update systempreferences set value=?,explanation=?,type=?,options=? where variable=?");
>-            $sth->execute( $value, $input->param('explanation'), $input->param('preftype'), $input->param('prefoptions'), $input->param('variable') );
>-            logaction( 'SYSTEMPREFERENCE', 'MODIFY', undef, $input->param('variable') . " | " . $value );
>-        }
>-    } else {
>-        unless ( C4::Context->config('demo') ) {
>-            my $sth = $dbh->prepare("insert into systempreferences (variable,value,explanation,type,options) values (?,?,?,?,?)");
>-            $sth->execute( $input->param('variable'), $value, $input->param('explanation'), $input->param('preftype'), $input->param('prefoptions') );
>-            logaction( 'SYSTEMPREFERENCE', 'ADD', undef, $input->param('variable') . " | " . $value );
>-        }
>-    }
>+    C4::Context->set_preference( $variable, $value, $expl, $type, $options )
>+        unless C4::Context->config('demo');
>     print $input->redirect("/cgi-bin/koha/admin/systempreferences.pl?tab=");
>     exit;
> ################## DELETE_CONFIRM ##################################
>     # called by default form, used to confirm deletion of data in DB
> } elsif ( $op eq 'delete_confirm' ) {
>-    my $dbh = C4::Context->dbh;
>-    my $sth = $dbh->prepare("select variable,value,explanation,type,options from systempreferences where variable=?");
>-    $sth->execute($searchfield);
>-    my $data = $sth->fetchrow_hashref;
>+    my $value = C4::Context->preference($searchfield);
>     $template->param(
>         searchfield => $searchfield,
>-        Tvalue      => $data->{'value'},
>+        Tvalue      => $value,
>     );
> 
>     # END $OP eq DELETE_CONFIRM
> ################## DELETE_CONFIRMED ##################################
>     # called by delete_confirm, used to effectively confirm deletion of data in DB
> } elsif ( $op eq 'delete_confirmed' ) {
>-    my $dbh = C4::Context->dbh;
>-    my $sth = $dbh->prepare("delete from systempreferences where variable=?");
>-    $sth->execute($searchfield);
>-    my $logstring = $searchfield . " | " . $Tvalue;
>-    logaction( 'SYSTEMPREFERENCE', 'DELETE', undef, $logstring );
>-
>+    C4::Context->delete_preference($searchfield);
>     # END $OP eq DELETE_CONFIRMED
> ################## DEFAULT ##################################
> } else {    # DEFAULT
>diff --git a/misc/admin/koha-preferences b/misc/admin/koha-preferences
>index 05b1595..eb23f87 100755
>--- a/misc/admin/koha-preferences
>+++ b/misc/admin/koha-preferences
>@@ -82,7 +82,6 @@ sub _set_preference {
>     _debug( "Setting $preference to $value" );
> 
>     C4::Context->set_preference( $preference, $value );
>-    logaction( 'SYSTEMPREFERENCE', 'MODIFY', undef, $preference . " | " . $value );
> }
> 
> sub GetPreferences {
>diff --git a/svc/config/systempreferences b/svc/config/systempreferences
>index 161a22e..021db3c 100755
>--- a/svc/config/systempreferences
>+++ b/svc/config/systempreferences
>@@ -64,7 +64,6 @@ sub set_preference {
>     unless ( C4::Context->config('demo') ) {
>         my $value = join( ',', $query->param( 'value' ) );
>         C4::Context->set_preference( $preference, $value );
>-        logaction( 'SYSTEMPREFERENCE', 'MODIFY', undef, $preference . " | " . $value );
>     }
> 
>     C4::Service->return_success( $response );
>@@ -98,7 +97,6 @@ sub set_preferences {
>             my $value = join( ',', $query->param( $param ) );
> 
>             C4::Context->set_preference( $pref, $value );
>-            logaction( 'SYSTEMPREFERENCE', 'MODIFY', undef, $pref . " | " . $value );
>         }
>     }
> 
>diff --git a/t/db_dependent/Context.t b/t/db_dependent/Context.t
>index 319a299..b5a050f 100755
>--- a/t/db_dependent/Context.t
>+++ b/t/db_dependent/Context.t
>@@ -24,7 +24,6 @@ BEGIN {
> ok($dbh = C4::Context->dbh(), 'Getting dbh from C4::Context');
> 
> $dbh->begin_work;
>-C4::Context->disable_syspref_cache();
> C4::Context->set_preference('OPACBaseURL','junk');
> C4::Context->clear_syspref_cache();
> my $OPACBaseURL = C4::Context->preference('OPACBaseURL');
>@@ -99,9 +98,17 @@ $trace_read = q{};
> 
> C4::Context->enable_syspref_cache();
> is(C4::Context->preference("SillyPreference"), 'thing3', "Retrieved syspref (value='thing3') successfully from cache");
>-is( $trace_read, q{}, 'Did not retrieve syspref from database');
>+isnt( $trace_read, q{}, 'The pref should be retrieved from the database if the cache has been enabled');
> $trace_read = q{};
> 
>+# FIXME This was added by Robin and does not pass anymore
>+# I don't understand why we should expect thing1 while thing3 is in the cache and in the DB
>+#$dbh->{mock_clear_history} = 1;
>+## This gives us the value that was cached on the first call, when the cache was active.
>+#is(C4::Context->preference("SillyPreference"), 'thing1', "Retrieved syspref (value='thing1') successfully from cache");
>+#$history = $dbh->{mock_all_history};
>+#is(scalar(@{$history}), 0, 'Did not retrieve syspref from database');
>+
> $silly_preference->set( { value => 'thing4' } )->store();
> C4::Context->clear_syspref_cache();
> is(C4::Context->preference("SillyPreference"), 'thing4', "Retrieved syspref (value='thing4') successfully after clearing cache");
>diff --git a/t/db_dependent/sysprefs.t b/t/db_dependent/sysprefs.t
>index c678d24..340e89a 100755
>--- a/t/db_dependent/sysprefs.t
>+++ b/t/db_dependent/sysprefs.t
>@@ -19,7 +19,7 @@
> # along with Koha; if not, see <http://www.gnu.org/licenses>.
> 
> use Modern::Perl;
>-use Test::More tests => 5;
>+use Test::More tests => 8;
> use C4::Context;
> 
> # Start transaction
>@@ -48,3 +48,16 @@ is( C4::Context->preference('IDoNotExist'), undef, 'Get a non-existent system pr
> 
> C4::Context->set_preference( 'IDoNotExist', 'NonExistent' );
> is( C4::Context->preference('IDoNotExist'), 'NonExistent', 'Test creation of non-existent system preference' );
>+
>+C4::Context->set_preference('testpreference', 'abc');
>+C4::Context->delete_preference('testpreference');
>+is(C4::Context->preference('testpreference'), undef, 'deleting preferences');
>+
>+C4::Context->set_preference('testpreference', 'def');
>+# Delete from the database, it should still be in cache
>+$dbh->do("DELETE FROM systempreferences WHERE variable='testpreference'");
>+is(C4::Context->preference('testpreference'), 'def', 'caching preferences');
>+C4::Context->clear_syspref_cache();
>+is(C4::Context->preference('testpreference'), undef, 'clearing preference cache');
>+
>+$dbh->rollback;
>-- 
>2.7.0

Actions: View | Diff | Splinter Review

Attachments on bug 11998: 26575 | 26621 | 26714 | 26716 | 26717 | 27382 | 31801 | 31802 | 32870 | 32871 | 32872 | 32873 | 32874 | 33183 | 33447 | 33670 | 33671 | 33685 | 33724 | 33725 | 33746 | 33829 | 33940 | 36523 | 36524 | 36525 | 36526 | 36527 | 36528 | 36529 | 36530 | 36531 | 36532 | 36533 | 36534 | 36571 | 36583 | 36584 | 36585 | 36586 | 36587 | 36588 | 36589 | 36590 | 36591 | 36592 | 36593 | 36594 | 36595 | 48640 | 48652 | 48655 | 48687 | 48688 | 48727 | 48758 | 48928 | 48929 | 48930 | 48931 | 48932 | 48933 | 48934 | 48935 | 48936 | 48961 | 49022 | 49023 | 49024 | 49025 | 49026 | 49027 | 49028 | 49029 | 49030 | 49100 | 49101 | 49102 | 49103 | 49104 | 49105 | 49106 | 49107 | 49108