Attachment 144771 Details for Bug 32476 – Bug 32476: Add caching for relatively expensive patron methods

[patch] Bug 32476: Add caching for relatively expensive patron methods

Bug-32476-Add-caching-for-relatively-expensive-pat.patch (text/plain), 12.53 KB, created by David Gustafsson on 2022-12-21 16:13:34 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Gustafsson

Created: 2022-12-21 16:13:34 UTC

Size: 12.53 KB

patch

obsolete

>From e36f4b1b760c5b5d0579413703ef313d3b4b897b Mon Sep 17 00:00:00 2001
>From: David Gustafsson <david.gustafsson@ub.gu.se>
>Date: Thu, 29 Sep 2022 17:18:01 +0200
>Subject: [PATCH] Bug 32476: Add caching for relatively expensive patron
> methods
>
>To test:
>1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass
>
>Sponsored-by: Gothenburg University Library
>---
> C4/Circulation.pm             | 13 ++++---
> Koha/Object.pm                | 67 +++++++++++++++++++++++++++++++++++
> Koha/Patron.pm                | 52 ++++++++++++++++++++++-----
> t/db_dependent/Koha/Patrons.t | 56 +++++++++++++++++++++++++----
> 4 files changed, 167 insertions(+), 21 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 336ce9490a..07d5371577 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -841,7 +841,7 @@ sub CanBookBeIssued {
>         $issuingimpossible{DEBARRED} = 1;
>     }
> 
>-    if ( $patron->is_expired ) {
>+    if ( $patron->is_expired({ cache => 1 }) ) {
>         $issuingimpossible{EXPIRED} = 1;
>     }
> 
>@@ -927,7 +927,7 @@ sub CanBookBeIssued {
>         else {
>             $issuingimpossible{USERBLOCKEDWITHENDDATE} = $debarred_date;
>         }
>-    } elsif ( my $num_overdues = $patron->has_overdues ) {
>+    } elsif ( my $num_overdues = $patron->has_overdues({ cache => 1 }) ) {
>         ## patron has outstanding overdue loans
>         if ( C4::Context->preference("OverduesBlockCirc") eq 'block'){
>             $issuingimpossible{USERBLOCKEDOVERDUE} = $num_overdues;
>@@ -1737,7 +1737,6 @@ sub AddIssue {
>                 if ( $accumulate_charge > 0 ) {
>                     AddIssuingCharge( $issue, $accumulate_charge, 'RENT_DAILY' );
>                     $charge += $accumulate_charge;
>-                    $item_unblessed->{charge} = $charge;
>                 }
>             }
> 
>@@ -2542,7 +2541,7 @@ sub MarkIssueReturned {
>         # Remove any OVERDUES related debarment if the borrower has no overdues
>         if ( C4::Context->preference('AutoRemoveOverduesRestrictions')
>           && $patron->debarred
>-          && !$patron->has_overdues
>+          && !$patron->has_overdues({ cache => 1 })
>           && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) }
>         ) {
>             DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' });
>@@ -2898,7 +2897,7 @@ sub CanBookBeRenewed {
>         my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing');
>         $patron         = Koha::Patrons->find($borrowernumber); # FIXME Is this really useful?
>         my $restricted  = $patron->is_debarred;
>-        my $hasoverdues = $patron->has_overdues;
>+        my $hasoverdues = $patron->has_overdues({ cache => 1 });
> 
>         if ( $restricted and $restrictionblockrenewing ) {
>             return ( 0, 'restriction');
>@@ -3173,7 +3172,7 @@ sub AddRenewal {
>         # Remove any OVERDUES related debarment if the borrower has no overdues
>         if ( $patron
>           && $patron->is_debarred
>-          && ! $patron->has_overdues
>+          && ! $patron->has_overdues({ cache => 1 })
>           && @{ GetDebarments({ borrowernumber => $borrowernumber, type => 'OVERDUES' }) }
>         ) {
>             DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' });
>@@ -4385,7 +4384,7 @@ sub _CanBookBeAutoRenewed {
>         }
>     );
> 
>-    if ( $patron->category->effective_BlockExpiredPatronOpacActions and $patron->is_expired ) {
>+    if ( $patron->is_expired({ cache => 1 }) && $patron->category->effective_BlockExpiredPatronOpacActions ) {
>         return 'auto_account_expired';
>     }
> 
>diff --git a/Koha/Object.pm b/Koha/Object.pm
>index b4b5717c34..512b4fa3f6 100644
>--- a/Koha/Object.pm
>+++ b/Koha/Object.pm
>@@ -80,6 +80,7 @@ sub new {
>     }
> 
>     $self->{_messages} = [];
>+    $self->{_cache} = {};
> 
>     croak("No _type found! Koha::Object must be subclassed!")
>       unless $class->_type();
>@@ -88,6 +89,72 @@ sub new {
> 
> }
> 
>+=head3 Koha::Object->_cache_get();
>+
>+Koha::Object->_cache_get($cache_key);
>+
>+=cut
>+
>+sub _cache_get {
>+    my ($self, $key) = @_;
>+    return $self->{_cache}->{$key};
>+}
>+
>+=head3 Koha::Object->_cache_set();
>+
>+Koha::Object->_cache_set($cache_key, $value);
>+
>+=cut
>+
>+sub _cache_set {
>+    my ($self, $key, $value) = @_;
>+    $self->{_cache}->{$key} = $value;
>+}
>+
>+
>+=head3 Koha::Object->_cache_clear($cache_key);
>+
>+Koha::Object->_cache_clear($cache_key);
>+Koha::Object->_cache_clear();
>+
>+=cut
>+
>+sub _cache_clear {
>+    my ($self, $key) = @_;
>+    if (defined $key) {
>+        delete $self->{_cache}->{$key};
>+    }
>+    else {
>+        $self->{_cache} = {};
>+    }
>+}
>+
>+=head3 Koha::Object->_maybe_cached($cache_key);
>+
>+Koha::Object->_maybe_cached($use_cache, $cache_key, $getter);
>+
>+=cut
>+
>+sub _maybe_cached {
>+    my ($self, $use_cache, $cache_key, $getter) = @_;
>+    my $value;
>+
>+    if ($use_cache) {
>+        $value = $self->_cache_get($cache_key);
>+        return $value if defined $value;
>+    }
>+
>+    $value = $getter->();
>+    if ($use_cache) {
>+        $self->_cache_set($cache_key, $value);
>+    }
>+    else {
>+        $self->_cache_clear($cache_key);
>+    }
>+
>+    return $value;
>+}
>+
> =head3 Koha::Object->_new_from_dbic();
> 
> my $object = Koha::Object->_new_from_dbic($dbic_row);
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index 96a72a393a..8d8ad99821 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -762,20 +762,52 @@ sub is_debarred {
>     return;
> }
> 
>+=head3 dateexpiry
>+
>+Override Patron::dateexpiry to invalidate Patron::is_expired cache.
>+
>+=cut
>+
>+sub dateexpiry {
>+    my $self = shift @_;
>+    if (@_) {
>+        $self->_cache_clear('is_expired');
>+    }
>+    return $self->SUPER::dateexpiry(@_);
>+}
>+
>+=head3 set
>+
>+Override Patron::set to invalidate Patron::is_expired cache if dateexpiry is set.
>+
>+=cut
>+
>+sub set {
>+    my $self = shift @_;
>+    my ($properties) = @_;
>+    if (exists $properties->{dateexpiry}) {
>+        $self->_cache_clear('is_expired');
>+    }
>+    return $self->SUPER::set(@_);
>+}
>+
> =head3 is_expired
> 
> my $is_expired = $patron->is_expired;
>+my $is_expired = $patron->is_expired({ cache => 1});
> 
> Returns 1 if the patron is expired or 0;
> 
> =cut
> 
> sub is_expired {
>-    my ($self) = @_;
>-    return 0 unless $self->dateexpiry;
>-    return 0 if $self->dateexpiry =~ '^9999';
>-    return 1 if dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to => 'day' );
>-    return 0;
>+    my ($self, $params) = @_;
>+    $params //= {};
>+    return $self->_maybe_cached($params->{cache}, 'is_expired', sub {
>+        return $self->dateexpiry &&
>+            $self->dateexpiry !~ '^9999' &&
>+            dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to => 'day' ) ? 1 : 0;
>+    });
> }
> 
> =head3 password_expired
>@@ -973,15 +1005,19 @@ sub renew_account {
> =head3 has_overdues
> 
> my $has_overdues = $patron->has_overdues;
>+my $has_overdues = $patron->has_overdues({ cache => 1 });
> 
> Returns the number of patron's overdues
> 
> =cut
> 
> sub has_overdues {
>-    my ($self) = @_;
>-    my $dtf = Koha::Database->new->schema->storage->datetime_parser;
>-    return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count;
>+    my ($self, $params) = @_;
>+    $params //= {};
>+    return $self->_maybe_cached($params->{cache}, 'has_overdues', sub {
>+        my $dtf = Koha::Database->new->schema->storage->datetime_parser;
>+        return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count;
>+    });
> }
> 
> =head3 track_login
>diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
>index b1adccde3d..35a74c077b 100755
>--- a/t/db_dependent/Koha/Patrons.t
>+++ b/t/db_dependent/Koha/Patrons.t
>@@ -264,7 +264,7 @@ subtest 'siblings' => sub {
> };
> 
> subtest 'has_overdues' => sub {
>-    plan tests => 3;
>+    plan tests => 12;
> 
>     my $item_1 = $builder->build_sample_item;
>     my $retrieved_patron = Koha::Patrons->find( $new_patron_1->borrowernumber );
>@@ -273,26 +273,70 @@ subtest 'has_overdues' => sub {
>     my $tomorrow = DateTime->today( time_zone => C4::Context->tz() )->add( days => 1 );
>     my $issue = Koha::Checkout->new({ borrowernumber => $new_patron_1->id, itemnumber => $item_1->itemnumber, date_due => $tomorrow, branchcode => $library->{branchcode} })->store();
>     is( $retrieved_patron->has_overdues, 0, );
>+
>+    # Cache is set
>+    is( $retrieved_patron->has_overdues({ cache => 1}), 0, );
>+    is( $retrieved_patron->_cache_get('has_overdues'), 0, 'has_overdues cache has been set');
>+    # Cache is used
>+    is( $retrieved_patron->has_overdues({ cache => 1}), 0, );
>+
>     $issue->delete();
>     my $yesterday = DateTime->today(time_zone => C4::Context->tz())->add( days => -1 );
>     $issue = Koha::Checkout->new({ borrowernumber => $new_patron_1->id, itemnumber => $item_1->itemnumber, date_due => $yesterday, branchcode => $library->{branchcode} })->store();
>     $retrieved_patron = Koha::Patrons->find( $new_patron_1->borrowernumber );
>     is( $retrieved_patron->has_overdues, 1, );
>+    is( $retrieved_patron->_cache_get('has_overdues'),, undef, 'has_overdues cache has been invalidated' );
>+    # Cache is set
>+    is( $retrieved_patron->has_overdues({ cache => 1}), 1, );
>+    is ( $retrieved_patron->_cache_get('has_overdues'),, 1, 'has_overdues cache has been set' );
>+    # Cache is used
>+    is( $retrieved_patron->has_overdues({ cache => 1}), 1, );
>+
>     $issue->delete();
>+
>+    # Cache is used (with stale entry, flushing cache must be handled manually as automatic invalidation is
>+    # complex and probably not worth the effort)
>+    is( $retrieved_patron->has_overdues({ cache => 1}), 1, );
>+
>+    # Cache has been invalidated
>+    is( $retrieved_patron->has_overdues, 0, );
> };
> 
> subtest 'is_expired' => sub {
>-    plan tests => 4;
>+    plan tests => 14;
>     my $patron = $builder->build({ source => 'Borrower' });
>     $patron = Koha::Patrons->find( $patron->{borrowernumber} );
>     $patron->dateexpiry( undef )->store->discard_changes;
>-    is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is not set');
>+    is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is not set' );
>     $patron->dateexpiry( dt_from_string )->store->discard_changes;
>-    is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is today');
>+    is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is today' );
>     $patron->dateexpiry( dt_from_string->add( days => 1 ) )->store->discard_changes;
>-    is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is tomorrow');
>+    is( $patron->is_expired, 0, 'Patron should not be considered expired if dateexpiry is tomorrow' );
>     $patron->dateexpiry( dt_from_string->add( days => -1 ) )->store->discard_changes;
>-    is( $patron->is_expired, 1, 'Patron should be considered expired if dateexpiry is yesterday');
>+    is( $patron->is_expired, 1, 'Patron should be considered expired if dateexpiry is yesterday' );
>+
>+
>+    # Test is_expired cache
>+    $patron->dateexpiry( undef )->store->discard_changes;
>+    # Set cache
>+    is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, with caching enabled' );
>+    is( $patron->_cache_get('is_expired'), 0, 'is_expired cache has been set');
>+    is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, with caching enabled and cache hit' );
>+    $patron->dateexpiry( dt_from_string->add( days => -1 ) )->store->discard_changes;
>+    is( $patron->is_expired({ cache => 1 }), 1, 'Patron should be considered expired if dateexpiry is yesterday, cache has been invalidated' );
>+    is( $patron->_cache_get('is_expired'), 1, 'is_expired cache has been set');
>+    is( $patron->is_expired({ cache => 1 }), 1, 'Patron should be considered expired if dateexpiry is yesterday, on cache hit' );
>+
>+    $patron->{dateexpiry} = undef;
>+    # Checking cache is really utilized by setting dateexpiry property without using accessor method
>+    is( $patron->is_expired({ cache => 1 }), 1, 'Cache is used with stale value' );
>+
>+    $patron->set({ dateexpiry => undef })->store->discard_changes;
>+    is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, cache has been invalidated' );
>+    is( $patron->is_expired({ cache => 1 }), 0, 'Patron should not be considered expired if dateexpiry is not set, on cache hit' );
>+
>+    $patron->is_expired;
>+    is( $patron->_cache_get('is_expired'), undef , 'Cache has been invalidated after calling is_expired without caching enabled' );
> 
>     $patron->delete;
> };
>-- 
>2.35.1

Actions: View | Diff | Splinter Review

Attachments on bug 32476: 144626 | 144627 | 144744 | 144770 | 144771 | 145036 | 145037 | 147145 | 147148 | 150778 | 150779 | 157241 | 157258 | 157259 | 162537 | 162538 | 162539