Bugzilla – Attachment 96108 Details for
Bug 20930
Validate and cache parsed YAML/JSON type system preferences
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20930: Dump and load system preferences of YAML/JSON type
Bug-20930-Dump-and-load-system-preferences-of-YAML.patch (text/plain), 15.69 KB, created by
Owen Leonard
on 2019-12-09 18:00:37 UTC
(
hide
)
Description:
Bug 20930: Dump and load system preferences of YAML/JSON type
Filename:
MIME Type:
Creator:
Owen Leonard
Created:
2019-12-09 18:00:37 UTC
Size:
15.69 KB
patch
obsolete
>From d50a0732608f59e9ad3239208c081921daf74468 Mon Sep 17 00:00:00 2001 >From: Lari Taskula <lari.taskula@jns.fi> >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 <oleonard@myacpl.org> >--- > 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 0fe571da2ac..c84106502cc 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 a7cc766351a..f759126e36b 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 14ae3152461..57859357a5c 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 cb7c94f5fcb..4fde0f23867 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 20930
:
76382
|
76383
|
76384
|
91861
|
91862
|
91863
|
94412
|
94413
|
96108
|
96109
|
96110
|
96111
|
96112
|
96817
|
96818
|
96819
|
96820
|
96821