Bug 32478 - Remove Koha::Config::SysPref->find since bypasses cache
Summary: Remove Koha::Config::SysPref->find since bypasses cache
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 15494
Blocks: 33844
  Show dependency treegraph
 
Reported: 2022-12-15 18:34 UTC by David Gustafsson
Modified: 2023-12-28 20:47 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.01,22.11.08


Attachments
Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache (4.43 KB, patch)
2022-12-15 18:38 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32478: Fix handling of undef values in ItemsDeniedRenewal (2.66 KB, patch)
2023-05-04 15:53 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache (4.48 KB, patch)
2023-05-04 18:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32478: Fix handling of undef values in ItemsDeniedRenewal (2.71 KB, patch)
2023-05-04 18:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32478: (follow-up) UpdateItemLocationOnCheckin no longer needs flattening (2.33 KB, patch)
2023-05-04 18:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32478: (follow-up) Tidy code (4.52 KB, patch)
2023-05-04 18:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32478: Fix syspref NULL values in tests (1.73 KB, patch)
2023-05-12 14:54 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache (4.58 KB, patch)
2023-05-26 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32478: Fix handling of undef values in ItemsDeniedRenewal (2.80 KB, patch)
2023-05-26 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32478: (follow-up) UpdateItemLocationOnCheckin no longer needs flattening (2.42 KB, patch)
2023-05-26 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32478: (follow-up) Tidy code (4.61 KB, patch)
2023-05-26 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32478: Fix syspref NULL values in tests (1.82 KB, patch)
2023-05-26 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32478: (QA follow-up) Shebang and chmod for Koha/Config/SysPrefs.t (815 bytes, patch)
2023-05-26 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32478: (QA follow-up) Keep current hashref behavior (2.48 KB, patch)
2023-05-26 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2022-12-15 18:34:54 UTC
Using Koha::Config::SysPrefs->find('UpdateItemLocationOnCheckin')->get_yaml_pref_hash() to retrieve yaml sysprefs will bypass caching negating the very minor performance gain per call get_yaml_pref_hash was meant to achieve since it in some cases is being called repeatedly a very large number of times. Replacing it with C4::Context->yaml_preference('ItemsDeniedRenewal') will utilize the syspref cache and also not support a very small (not even valid) subset of YAML.
Comment 1 David Gustafsson 2022-12-15 18:38:32 UTC
Created attachment 144633 [details] [review]
Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache

get_yaml_pref_hash also allows invalid YAML and only parses a limited
subset so remove this method to avoid future issues.

To test):

Since tests already exists for C4::Context->yaml_preference and this
is a trivial change, do we really need a test plan for this?

Sponsored-by: Gothenburg University Library
Comment 2 Jonathan Druart 2023-05-04 12:43:29 UTC
I don't think the removal here is correct, the method does more than C4::Context->yaml_preference

Nick maybe?
Comment 3 Nick Clemens 2023-05-04 12:54:46 UTC
(In reply to Jonathan Druart from comment #2)
> I don't think the removal here is correct, the method does more than
> C4::Context->yaml_preference
> 
> Nick maybe?

perl -e 'use Koha::Config::SysPrefs; use C4::Context; my $sp = Koha::Config::SysPrefs->find("ItemsDeniedRenewal")->get_yaml_pref_hash; warn Data::Dumper::Dumper($sp); warn Data::Dumper::Dumper( C4::Context->yaml_preference("ItemsDeniedRenewal"));'

It doesn't convert from 'NULL' to undef - the expectation is that we can pass the return value into a DBIC search directly
Comment 4 Nick Clemens 2023-05-04 12:58:51 UTC
(In reply to Nick Clemens from comment #3)
> (In reply to Jonathan Druart from comment #2)
> > I don't think the removal here is correct, the method does more than
> > C4::Context->yaml_preference
> > 
> > Nick maybe?
> 
> perl -e 'use Koha::Config::SysPrefs; use C4::Context; my $sp =
> Koha::Config::SysPrefs->find("ItemsDeniedRenewal")->get_yaml_pref_hash; warn
> Data::Dumper::Dumper($sp); warn Data::Dumper::Dumper(
> C4::Context->yaml_preference("ItemsDeniedRenewal"));'
> 
> It doesn't convert from 'NULL' to undef - the expectation is that we can
> pass the return value into a DBIC search directly

Perhaps we removed the wrong copy here:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27154

One was an object, the other took a pref name, so would have used cache
Comment 5 David Gustafsson 2023-05-04 15:53:36 UTC
Created attachment 150685 [details] [review]
Bug 32478: Fix handling of undef values in ItemsDeniedRenewal
Comment 6 David Gustafsson 2023-05-04 16:00:47 UTC
(In reply to Nick Clemens from comment #4)
> (In reply to Nick Clemens from comment #3)
> > (In reply to Jonathan Druart from comment #2)
> > > I don't think the removal here is correct, the method does more than
> > > C4::Context->yaml_preference
> > > 
> > > Nick maybe?
> > 
> > perl -e 'use Koha::Config::SysPrefs; use C4::Context; my $sp =
> > Koha::Config::SysPrefs->find("ItemsDeniedRenewal")->get_yaml_pref_hash; warn
> > Data::Dumper::Dumper($sp); warn Data::Dumper::Dumper(
> > C4::Context->yaml_preference("ItemsDeniedRenewal"));'
> > 
> > It doesn't convert from 'NULL' to undef - the expectation is that we can
> > pass the return value into a DBIC search directly
> 
> Perhaps we removed the wrong copy here:
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27154
> 
> One was an object, the other took a pref name, so would have used cache

Yes, it would have used cache, but still a good thing to remove it as C4::Context->yaml_preference exists using a proper yaml-parser (and utilized cache). Thanks for pointing out the nuance of the special handling of NULL values. null (lowercase) is valid yaml for null values (converted to undef by the perl parser). So the problem can be solved by using that instead of NULL. Provided a patch to fix this.
Comment 7 Nick Clemens 2023-05-04 18:40:21 UTC
Created attachment 150687 [details] [review]
Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache

get_yaml_pref_hash also allows invalid YAML and only parses a limited
subset so remove this method to avoid future issues.

To test):

Since tests already exists for C4::Context->yaml_preference and this
is a trivial change, do we really need a test plan for this?

Sponsored-by: Gothenburg University Library
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2023-05-04 18:40:23 UTC
Created attachment 150688 [details] [review]
Bug 32478: Fix handling of undef values in ItemsDeniedRenewal

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2023-05-04 18:40:25 UTC
Created attachment 150689 [details] [review]
Bug 32478: (follow-up) UpdateItemLocationOnCheckin no longer needs flattening

This patch removes a line flattening the arrays generated by get_yaml_pref_hash
as it is no longer necessary

Conditionals are adjusted to avoid warnings in tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2023-05-04 18:40:28 UTC
Created attachment 150690 [details] [review]
Bug 32478: (follow-up) Tidy code

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Marcel de Rooy 2023-05-09 14:48:15 UTC
# Subtest: is_denied_renewal
    1..11
    ok 1 - Renewal allowed when no rules
    ok 2 - Renewal blocked when 1 rules (withdrawn)
    ok 3 - Renewal allowed when 1 rules not matched (withdrawn)
    ok 4 - Renewal blocked when 2 rules matched (withdrawn, itype)
    ok 5 - Renewal allowed when 2 rules not matched (withdrawn, itype)
    ok 6 - Renewal blocked when 3 rules matched (withdrawn, itype, location)
    ok 7 - Renewal allowed when 3 rules not matched (withdrawn, itype, location)
    not ok 8 - Renewal blocked for undef when NULL in pref
    #   Failed test 'Renewal blocked for undef when NULL in pref'
    #   at t/db_dependent/Koha/Item.t line 2157.
    #          got: '0'
    #     expected: '1'
    ok 9 - Renewal not blocked for undef when "" in pref
    ok 10 - Renewal not blocked for "" when NULL in pref
    ok 11 - Renewal blocked for empty string when "" in pref
    # Looks like you failed 1 test of 11.
not ok 28 - is_denied_renewal
Comment 12 Marcel de Rooy 2023-05-09 14:48:58 UTC
The replace NULL by null thing feels a bit odd btw..
Comment 13 David Gustafsson 2023-05-09 15:05:00 UTC
Ok! Suppose due to forgot to fix the tests that still uses the 'NULL' value, will do so. Why is it odd? null is valid json, NULL is not.
Comment 14 Marcel de Rooy 2023-05-10 12:50:20 UTC
(In reply to David Gustafsson from comment #13)
> Ok! Suppose due to forgot to fix the tests that still uses the 'NULL' value,
> will do so. Why is it odd? null is valid json, NULL is not.

Some things are hard to explain :)
The fact that you need to type null as a valid JSON expression in a textarea is not very appealing as to user interface. (Or even stricter, valid YAML.) But I could certainly live with it :) Just document it clearly.

And going bit out of scope, but why are we specifying a: [ b, c ] instead of the pure YAML approach with something like:
a:
- b
- c
We probably need some discussion/consensus about handling all such preferences.

The fact that is all kind of YAML now, does however make a difference. See next comment.
Comment 15 Marcel de Rooy 2023-05-10 12:51:07 UTC
We remove these lines completely now and just trust ->yaml_preference:
-    foreach my $line (@lines){
-        my ($field,$array) = split /:/, $line;
-        next if !$array;
-        $field =~ s/^\s*|\s*$//g;
-        $array =~ s/[ [\]\r]//g;
-        my @array = split /,/, $array;
-        @array = map { $_ eq '""' || $_ eq "''" ? '' : $_ } @array;
-        @array = map { $_ eq 'NULL' ? undef : $_ } @array;
-        $pref_as_hash->{$field} = \@array;
-    }

But this is not the same. If I pasted correct YAML previously like below, it would not work.
ccode  :
- null
- NULL
- undefined
- 2
- '3'
Now it does.

But perhaps more serious is the opposite, if I am entering some valid YAML array by 'forgetting' the first line:
- null
- NULL
- undefined
- 2
- '3'
I will get back an array, while the former routine returned an empty hash !
This will trigger an error like: Not a HASH reference at /usr/share/koha/Koha/Item.pm line 2071. => foreach my $field (keys %$denyingrules) {

This is a bit long way of saying: I still think that we may need an intermediate routine between pure YAML (yaml_preference) and what we expect for specific preferences?
Comment 16 David Gustafsson 2023-05-12 13:53:20 UTC
(In reply to Marcel de Rooy from comment #14)
> (In reply to David Gustafsson from comment #13)
> > Ok! Suppose due to forgot to fix the tests that still uses the 'NULL' value,
> > will do so. Why is it odd? null is valid json, NULL is not.
> 
> Some things are hard to explain :)
> The fact that you need to type null as a valid JSON expression in a textarea
> is not very appealing as to user interface. (Or even stricter, valid YAML.)
> But I could certainly live with it :) Just document it clearly.
> 
> And going bit out of scope, but why are we specifying a: [ b, c ] instead of
> the pure YAML approach with something like:
> a:
> - b
> - c
> We probably need some discussion/consensus about handling all such
> preferences.
> 
> The fact that is all kind of YAML now, does however make a difference. See
> next comment.

Yes, but previously you still had to type and invalid subset of YAML, which should be worse? That you can use null values is documented in the preference description, where the patch changes the mentions of NULL to lowercase. 

both a: [ b, c ] and
a:
 - b
 - c
is valid YAML. The syspref documentation doesn't explicitly say the format is YAML, but I don't really see any reason to inform the user about the alternative syntax.
Comment 17 David Gustafsson 2023-05-12 14:17:27 UTC
(In reply to Marcel de Rooy from comment #15)
> We remove these lines completely now and just trust ->yaml_preference:
> -    foreach my $line (@lines){
> -        my ($field,$array) = split /:/, $line;
> -        next if !$array;
> -        $field =~ s/^\s*|\s*$//g;
> -        $array =~ s/[ [\]\r]//g;
> -        my @array = split /,/, $array;
> -        @array = map { $_ eq '""' || $_ eq "''" ? '' : $_ } @array;
> -        @array = map { $_ eq 'NULL' ? undef : $_ } @array;
> -        $pref_as_hash->{$field} = \@array;
> -    }
> 
> But this is not the same. If I pasted correct YAML previously like below, it
> would not work.
> ccode  :
> - null
> - NULL
> - undefined
> - 2
> - '3'
> Now it does.
> 
> But perhaps more serious is the opposite, if I am entering some valid YAML
> array by 'forgetting' the first line:
> - null
> - NULL
> - undefined
> - 2
> - '3'
> I will get back an array, while the former routine returned an empty hash !
> This will trigger an error like: Not a HASH reference at
> /usr/share/koha/Koha/Item.pm line 2071. => foreach my $field (keys
> %$denyingrules) {
> 
> This is a bit long way of saying: I still think that we may need an
> intermediate routine between pure YAML (yaml_preference) and what we expect
> for specific preferences?

I see your point, but was it not already possible to make that code crash (or even worse things) by for example using key values that does not exist on the item object, or for example using "delete" as key :/

 sub is_denied_renewal {
        my ( $self ) = @_;                                                                          
                                      
        my $denyingrules = Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash();
        return 0 unless $denyingrules;                     
        foreach my $field (keys %$denyingrules) {
            my $val = $self->$field;

Validation would be nice, but feels like out of the scope for this issue. I will update the patch to check that an actual hash before iterating over the keys though, as this particular case was not a problem before. If I'm not missing something $self->$field; where $field is user input is a security vulnerability and a pretty bad one at that. Should use get_column instead! I guess should create a new issue for this, or if we handle it in this one?
Comment 18 David Gustafsson 2023-05-12 14:45:09 UTC
I'm leaning towards that it's better to leave the patch as it is with regards to the problem of crashing on non hash-values. The same problem applies for 'OpacHiddenItems', that is currently using C4::Context->yaml_preference and not get_yaml_pref_hash. So feel like this issue should be handled in another issue and solved properly instead of some band aid solution like validating everywhere the syspref is used instead of on input.
Comment 19 David Gustafsson 2023-05-12 14:54:07 UTC
Created attachment 151136 [details] [review]
Bug 32478: Fix syspref NULL values in tests
Comment 20 Marcel de Rooy 2023-05-26 09:49:02 UTC
sub is_denied_renewal

    foreach my $field (keys %$denyingrules) {
        my $val = $self->$field;

This is out of scope, but there is not even a check if it is a valid field.

The method Koha::Item->a is not covered by tests!
Comment 21 Marcel de Rooy 2023-05-26 10:07:19 UTC
Created attachment 151726 [details] [review]
Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache

get_yaml_pref_hash also allows invalid YAML and only parses a limited
subset so remove this method to avoid future issues.

To test):

Since tests already exists for C4::Context->yaml_preference and this
is a trivial change, do we really need a test plan for this?

Sponsored-by: Gothenburg University Library
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2023-05-26 10:07:22 UTC
Created attachment 151727 [details] [review]
Bug 32478: Fix handling of undef values in ItemsDeniedRenewal

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2023-05-26 10:07:25 UTC
Created attachment 151728 [details] [review]
Bug 32478: (follow-up) UpdateItemLocationOnCheckin no longer needs flattening

This patch removes a line flattening the arrays generated by get_yaml_pref_hash
as it is no longer necessary

Conditionals are adjusted to avoid warnings in tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2023-05-26 10:07:27 UTC
Created attachment 151729 [details] [review]
Bug 32478: (follow-up) Tidy code

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2023-05-26 10:07:30 UTC
Created attachment 151730 [details] [review]
Bug 32478: Fix syspref NULL values in tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2023-05-26 10:07:33 UTC
Created attachment 151731 [details] [review]
Bug 32478: (QA follow-up) Shebang and chmod for Koha/Config/SysPrefs.t

Test plan:
Run t/db_dependent/Koha/Config/SysPrefs.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2023-05-26 10:07:35 UTC
Created attachment 151732 [details] [review]
Bug 32478: (QA follow-up) Keep current hashref behavior

Prevent a crash on wrong contents for ItemsDeniedRenewal pref
as we did before.
Note: Could be a provisional measure (no band aid to repeat anywhere)
until we resolve this in preferences.pl.

Test plan:
Without this patch:
Change ItemsDeniedRenewal to 'nonsense'
Run perl -MKoha::Items -e'Koha::Items->find(X)->is_denied_renewal; print "OK\n"'
=> Replace X by a valid itemnumber
Crashes with: Can't use string ("nonsense") as a HASH ref ... No OK print.

Apply this patch
Run perl -MKoha::Items -e'Koha::Items->find(X)->is_denied_renewal; print "OK\n"'
=> Replace X by a valid itemnumber
Warns only with: Hashref expected for ItemsDeniedRenewal. You got OK.
Clear ItemsDeniedRenewal
Try again. No warning anymore.
Run t/Context.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2023-05-26 10:13:04 UTC
(In reply to David Gustafsson from comment #17)

> I see your point, but was it not already possible to make that code crash
> (or even worse things) by for example using key values that does not exist
> on the item object, or for example using "delete" as key :/

Hmm. You already said it. Read too quick. The delete example is even worse!
Will add a report.
Comment 29 David Gustafsson 2023-05-26 10:18:39 UTC
Ok, good! Yes, even though highly unlikely anyone would do that by mistake or malicious intent, but still serious issue. As wrote above get_column should be used here instead of dynamic method invokation.
Comment 30 Tomás Cohen Arazi 2023-06-07 18:34:02 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 31 Tomás Cohen Arazi 2023-06-09 19:06:02 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 32 Pedro Amorim 2023-07-18 09:23:42 UTC
Nice work everyone!

Pushed to 22.11.x for next release