From 002f1112a9db83effcd1b7453127c61778f8fe04 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Wed, 13 Jun 2018 15:35:20 +0300 Subject: [PATCH] Bug 20930: Dump and load system preferences of YAML/JSON type Currently, C4::Context->preference('YAMLpref') returns a string that needs to be parsed by any code using the preference. My suggestion is to return a parsed version instead (a hash/array ref or a string) and using the syspref cache with the parsed value. Like this, we could: - centralize parsing of YAML system preferences - optimize use of system preference cache by caching the parsed version instead. Parse once and store in cache; no need to re-parse on each call! We can centralize this functionality into Koha::Config::SysPref->value() (used by C4::Context->preference). Use database column systempreferences.type with value of "YAML" to trigger this functionality in "value()"-method. To test: 1. Make sure memcached is available 2. Run tests with caching enabled, e.g.: MEMCACHED_SERVERS=localhost:11211 prove t/db_dependent/Context.t 3. Make sure all tests pass and none were skipped Signed-off-by: Owen Leonard Signed-off-by: Michal Denar --- C4/Context.pm | 2 +- Koha/Config/SysPref.pm | 105 ++++++++++++++++++++++++- Koha/Exceptions/Config.pm | 5 ++ t/db_dependent/Context.t | 190 +++++++++++++++++++++++++++++++++------------- 4 files changed, 248 insertions(+), 54 deletions(-) diff --git a/C4/Context.pm b/C4/Context.pm index 0fe571da2a..c84106502c 100644 --- a/C4/Context.pm +++ b/C4/Context.pm @@ -554,7 +554,7 @@ sub set_preference { } if ( $use_syspref_cache ) { - $syspref_cache->set_in_cache( "syspref_$variable", $value ); + $syspref_cache->set_in_cache( "syspref_$variable", $syspref->value ); } return $syspref; diff --git a/Koha/Config/SysPref.pm b/Koha/Config/SysPref.pm index a7cc766351..f759126e36 100644 --- a/Koha/Config/SysPref.pm +++ b/Koha/Config/SysPref.pm @@ -20,8 +20,10 @@ package Koha::Config::SysPref; use Modern::Perl; use Carp; +use YAML::Syck; use Koha::Database; +use Koha::Exceptions::Config; use C4::Log; @@ -73,9 +75,17 @@ sub store { my $action = $self->in_storage ? 'MODIFY' : 'ADD'; - C4::Log::logaction( 'SYSTEMPREFERENCE', $action, undef, $self->variable . ' | ' . $self->value ); + my $value = $self->value; + if (ref($value)) { + $value = $self->_dump_datatypes($value); + } + + C4::Log::logaction( 'SYSTEMPREFERENCE', $action, undef, $self->variable . ' | ' . $value ); + + $self->SUPER::store($self); + $self->set({ value => $self->_load_datatypes($value) }) unless ref($value); - return $self->SUPER::store($self); + return $self; } =head3 delete @@ -94,6 +104,97 @@ sub delete { return $deleted; } +=head3 value + +=cut + +sub value { + my $self = shift; + + my $value = $self->SUPER::value(@_); + return $value unless $value; + + return $self->_load_datatypes($value); +} + +=head3 _dump_datatypes + +=cut + +sub _dump_datatypes { + my ($self, $value) = @_; + + $value = $self->_dump_yaml_or_json($value); + + return $value; +} + +=head3 _load_datatypes + +=cut + +sub _load_datatypes { + my ($self, $value) = @_; + + $value = $self->_load_yaml_or_json($value); + + return $value; +} + +=head3 _load_yaml_or_json + +Loads system preference of YAML / JSON type. + +Throws Koha::Exceptions::Config::InvalidSyntax if not valid YAML / JSON. + +=cut + +sub _load_yaml_or_json { + my ($self, $value) = @_; + + my $type = defined $self->type ? lc($self->type) : ''; + return $value if $type ne 'yaml' and $type ne 'json'; + if (!ref($value)) { + eval { $value = Load($value); }; + } else { + return $value; + } + if ($@) { + Koha::Exceptions::Config::InvalidSyntax->throw( + error => $@, + value => $value, + ); + } + return $value; +} + +=head3 _dump_yaml_or_json + +Dumps system preference of YAML / JSON type. + +Throws Koha::Exceptions::Config::InvalidSyntax if not valid YAML / JSON. + +=cut + +sub _dump_yaml_or_json { + my ($self, $value) = @_; + + my $type = defined $self->type ? lc($self->type) : ''; + return $value if $type ne 'yaml' and $type ne 'json'; + if (ref($value)) { + eval { $value = Dump($value); }; + } else { + return $value; + } + if ($@) { + Koha::Exceptions::Config::InvalidSyntax->throw( + error => $@, + value => $value, + ); + } + return $value; +} + =head3 type =cut diff --git a/Koha/Exceptions/Config.pm b/Koha/Exceptions/Config.pm index 14ae315246..57859357a5 100644 --- a/Koha/Exceptions/Config.pm +++ b/Koha/Exceptions/Config.pm @@ -7,6 +7,11 @@ use Exception::Class ( 'Koha::Exceptions::Config' => { description => 'Something went wrong!', }, + 'Koha::Exceptions::Config::InvalidSyntax' => { + isa => 'Koha::Exceptions::Config', + description => 'The configuration value has invalid syntax', + fields => ['value'], + }, 'Koha::Exceptions::Config::MissingEntry' => { isa => 'Koha::Exceptions::Config', description => 'The required entry is missing in the configuration file' diff --git a/t/db_dependent/Context.t b/t/db_dependent/Context.t index cb7c94f5fc..4fde0f2386 100755 --- a/t/db_dependent/Context.t +++ b/t/db_dependent/Context.t @@ -8,6 +8,7 @@ use vars qw($debug $koha $dbh $config $ret); use t::lib::Mocks; use Koha::Database; +use Koha::Caches; BEGIN { $debug = $ENV{DEBUG} || 0; @@ -48,6 +49,88 @@ my $SillyPeference = C4::Context->preference('SillyPreference'); is($SillyPeference,'random','SillyPreference saved as specified'); C4::Context->clear_syspref_cache(); C4::Context->enable_syspref_cache(); + +subtest 'test system preference of type YAML or JSON' => sub { + my ($hashref, $arrayref, $string, $pref); + my $cache = Koha::Caches->get_instance('syspref'); + + subtest 'Invalid syntax' => sub { + my $notyaml = ': - asd'; + my $notjson = '{"test"": { "fail\' ]}'; + eval { + C4::Context->set_preference('YAMLpref', $notyaml, '', 'yaml', ''); + }; + is(ref($@), 'Koha::Exceptions::Config::InvalidSyntax', + 'Invalid YAML syntax'); + eval { + C4::Context->set_preference('YAMLpref', $notjson, '', 'json', ''); + }; + is(ref($@), 'Koha::Exceptions::Config::InvalidSyntax', + 'Invalid JSON syntax'); + C4::Context->set_preference('Normalpref', $notyaml, '', 'Free', ''); + is(C4::Context->preference('Normalpref'), $notyaml, 'Does not affect ' + .'preferences with type != json || yaml'); + }; + + subtest 'test YAML' => sub { + plan skip_all => 'Cache not active' unless $cache->is_cache_active(); + + $hashref = qq/--- + test: + test: 1 + /; + $arrayref = qq/--- + [much, elements, in, this, very, dogearray] + /; + $string = qq/--- + wat + /; + + # Check HASHref + C4::Context->set_preference('YAMLpref', $hashref, '', 'yaml', ''); + $pref = C4::Context->preference('YAMLpref'); + is($pref->{test}->{test}, 1, 'Got hashref'); + is_deeply($cache->get_from_cache("syspref_yamlpref"), $pref, 'is_deeply'); + + # Check ARRAYref + C4::Context->set_preference('YAMLpref', $arrayref, '', 'yaml', ''); + $pref = C4::Context->preference('YAMLpref'); + is($pref->[5], 'dogearray', 'Got arrayref'); + is_deeply($cache->get_from_cache("syspref_yamlpref"), $pref, 'is_deeply'); + + # Check string + C4::Context->set_preference('YAMLpref', $string, '', 'yaml', ''); + $pref = C4::Context->preference('YAMLpref'); + is($pref, 'wat', 'Got string'); + is_deeply($cache->get_from_cache("syspref_yamlpref"), $pref, 'is_deeply'); + }; + subtest 'test JSON' => sub { + plan skip_all => 'Cache not active' unless $cache->is_cache_active(); + + $hashref = qq/{"test": {"test": 1}}/; + $arrayref = qq/["much", "elements", "in", "this", "very", "dogearray"]/; + $string = qq/"wat"/; + + # Check HASHref + C4::Context->set_preference('JSONpref', $hashref, '', 'json', ''); + $pref = C4::Context->preference('JSONpref'); + is($pref->{test}->{test}, 1, 'Got hashref'); + is_deeply($cache->get_from_cache("syspref_jsonpref"), $pref, 'is_deeply'); + + # Check ARRAYref + C4::Context->set_preference('JSONpref', $arrayref, '', 'json', ''); + $pref = C4::Context->preference('JSONpref'); + is($pref->[5], 'dogearray', 'Got arrayref'); + is_deeply($cache->get_from_cache("syspref_jsonpref"), $pref, 'is_deeply'); + + # Check string + C4::Context->set_preference('JSONpref', $string, '', 'json', ''); + $pref = C4::Context->preference('JSONpref'); + is($pref, 'wat', 'Got string'); + is_deeply($cache->get_from_cache("syspref_jsonpref"), $pref, 'is_deeply'); + }; +}; + $dbh->rollback; ok($koha = C4::Context->new, 'C4::Context->new'); @@ -63,73 +146,78 @@ foreach (sort @keys) { . ((defined $koha->{$_}) ? "and is defined." : "but is not defined.") ); } -ok($config = $koha->{config}, 'Getting $koha->{config} '); -# Testing syspref caching -use Test::DBIx::Class; +subtest 'test system preference caching' => sub { + my $cache = Koha::Caches->get_instance('syspref'); + plan skip_all => 'Cache not active' unless $cache->is_cache_active(); -my $history; + ok($config = $koha->{config}, 'Getting $koha->{config} '); -my $schema = Koha::Database->new()->schema(); -$schema->storage->debug(1); -my $trace_read; -open my $trace, '>', \$trace_read or die "Can't open variable: $!"; -$schema->storage->debugfh( $trace ); + use Test::DBIx::Class; -C4::Context->set_preference('SillyPreference', 'thing1'); -my $silly_preference = Koha::Config::SysPrefs->find('SillyPreference'); -is( $silly_preference->variable, 'SillyPreference', 'set_preference should have kept the case sensitivity' ); + my $history; -my $pref = C4::Context->preference("SillyPreference"); -is(C4::Context->preference("SillyPreference"), 'thing1', "Retrieved syspref (value='thing1') successfully with default behavior"); -ok( $trace_read, 'Retrieved syspref from database'); -$trace_read = q{}; + my $schema = Koha::Database->new()->schema(); + $schema->storage->debug(1); + my $trace_read; + open my $trace, '>', \$trace_read or die "Can't open variable: $!"; + $schema->storage->debugfh( $trace ); -is(C4::Context->preference("SillyPreference"), 'thing1', "Retrieved syspref (value='thing1') successfully with default behavior"); -is( $trace_read , q{}, 'Did not retrieve syspref from database'); -$trace_read = q{}; + C4::Context->set_preference('SillyPreference', 'thing1'); + my $silly_preference = Koha::Config::SysPrefs->find('SillyPreference'); + is( $silly_preference->variable, 'SillyPreference', 'set_preference should have kept the case sensitivity' ); -C4::Context->disable_syspref_cache(); -$silly_preference->set( { value => 'thing2' } )->store(); -is(C4::Context->preference("SillyPreference"), 'thing2', "Retrieved syspref (value='thing2') successfully with disabled cache"); -ok($trace_read, 'Retrieved syspref from database'); -$trace_read = q{}; + my $pref = C4::Context->preference("SillyPreference"); + is(C4::Context->preference("SillyPreference"), 'thing1', "Retrieved syspref (value='thing1') successfully with default behavior"); + ok( $trace_read, 'Retrieved syspref from database'); + $trace_read = q{}; -$silly_preference->set( { value => 'thing3' } )->store(); -is(C4::Context->preference("SillyPreference"), 'thing3', "Retrieved syspref (value='thing3') successfully with disabled cache"); -ok($trace_read, 'Retrieved syspref from database'); -$trace_read = q{}; + is(C4::Context->preference("SillyPreference"), 'thing1', "Retrieved syspref (value='thing1') successfully with default behavior"); + is( $trace_read , q{}, 'Did not retrieve syspref from database'); + $trace_read = q{}; -C4::Context->enable_syspref_cache(); -is(C4::Context->preference("SillyPreference"), 'thing3', "Retrieved syspref (value='thing3') successfully from cache"); -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"); -ok($trace_read, 'Retrieved syspref from database'); -$trace_read = q{}; + C4::Context->disable_syspref_cache(); + $silly_preference->set( { value => 'thing2' } )->store(); + is(C4::Context->preference("SillyPreference"), 'thing2', "Retrieved syspref (value='thing2') successfully with disabled cache"); + ok($trace_read, 'Retrieved syspref from database'); + $trace_read = q{}; + + $silly_preference->set( { value => 'thing3' } )->store(); + is(C4::Context->preference("SillyPreference"), 'thing3', "Retrieved syspref (value='thing3') successfully with disabled cache"); + ok($trace_read, 'Retrieved syspref from database'); + $trace_read = q{}; -is(C4::Context->preference("SillyPreference"), 'thing4', "Retrieved syspref (value='thing4') successfully from cache"); -is( $trace_read, q{}, 'Did not retrieve syspref from database'); -$trace_read = q{}; + C4::Context->enable_syspref_cache(); + is(C4::Context->preference("SillyPreference"), 'thing3', "Retrieved syspref (value='thing3') successfully from cache"); + 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"); + ok($trace_read, 'Retrieved syspref from database'); + $trace_read = q{}; + + is(C4::Context->preference("SillyPreference"), 'thing4', "Retrieved syspref (value='thing4') successfully from cache"); + is( $trace_read, q{}, 'Did not retrieve syspref from database'); + $trace_read = q{}; + + $silly_preference->delete(); +}; my $oConnection = C4::Context->Zconn('biblioserver', 0); isnt($oConnection->option('async'), 1, "ZOOM connection is synchronous"); $oConnection = C4::Context->Zconn('biblioserver', 1); is($oConnection->option('async'), 1, "ZOOM connection is asynchronous"); -$silly_preference->delete(); - # AutoEmailOpacUser should be a YesNo pref C4::Context->set_preference('AutoEmailOpacUser', ''); my $yesno_pref = Koha::Config::SysPrefs->find('AutoEmailOpacUser'); -- 2.11.0