From 26902fc30ec55f2a1d699ad42d4a916c0de33fcd 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. prove t/db_dependent/Context.t --- C4/Context.pm | 2 +- Koha/Config/SysPref.pm | 107 +++++++++++++++++++++++++++++++++++++++++++++- Koha/Exceptions/Config.pm | 5 +++ t/db_dependent/Context.t | 79 ++++++++++++++++++++++++++++++++++ 4 files changed, 190 insertions(+), 3 deletions(-) diff --git a/C4/Context.pm b/C4/Context.pm index 41bce3b..bed381a 100644 --- a/C4/Context.pm +++ b/C4/Context.pm @@ -533,7 +533,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 1e71b8c..bca2303 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; @@ -46,9 +48,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); + } - return $self->SUPER::store($self); + 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; } =head3 delete @@ -67,6 +77,99 @@ sub delete { return $deleted; } +=head3 value + +=cut + +sub value { + my $self = shift; + + my $value = $self->SUPER::value(@_); + return $value unless $value; + + $self->_load_datatypes($value); + + return $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 14ae315..5785935 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 cb7c94f..c31aa01 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,84 @@ 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 { + $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 { + $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'); -- 2.7.4