Bug 32894 - Objects cache methods' result without invalidation
Summary: Objects cache methods' result without invalidation
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: rel_23_11_candidate
Depends on:
Blocks: 34603 34609
  Show dependency treegraph
 
Reported: 2023-02-07 06:26 UTC by Jonathan Druart
Modified: 2023-12-28 20:47 UTC (History)
6 users (show)

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


Attachments
Bug 32894: Remove wrong caching from Koha:: methods - simple (10.59 KB, patch)
2023-02-28 16:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Koha::Item->last_returned_by (2.73 KB, patch)
2023-02-28 16:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Koha::Biblio->biblioitem (837 bytes, patch)
2023-02-28 16:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Fix test (3.05 KB, patch)
2023-02-28 16:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Remove wrong caching from Koha:: methods - simple (10.64 KB, patch)
2023-04-21 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32894: Koha::Item->last_returned_by (2.79 KB, patch)
2023-04-21 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32894: Koha::Biblio->biblioitem (901 bytes, patch)
2023-04-21 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32894: Fix test (3.11 KB, patch)
2023-04-21 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32894: Remove incorrect caching from bundle_items (1.57 KB, patch)
2023-04-21 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32894: (QA follow-up) Fix typo (807 bytes, patch)
2023-04-21 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32894: (follow-up) Koha::Item->last_returned_by (2.03 KB, patch)
2023-05-09 09:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Make ->sharee looks like others (765 bytes, patch)
2023-05-09 09:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Remove wrong caching from Koha:: methods - simple (10.69 KB, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: Koha::Item->last_returned_by (2.84 KB, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: Koha::Biblio->biblioitem (957 bytes, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: Fix test (3.17 KB, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: Remove incorrect caching from bundle_items (1.63 KB, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: (QA follow-up) Fix typo (863 bytes, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: (follow-up) Koha::Item->last_returned_by (2.08 KB, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: Make ->sharee looks like others (819 bytes, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: (QA follow-up) Fix tests (1.42 KB, patch)
2023-05-09 11:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32894: Remove wrong caching from Koha:: methods - simple (10.71 KB, patch)
2023-06-07 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Koha::Item->last_returned_by (2.83 KB, patch)
2023-06-07 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Koha::Biblio->biblioitem (959 bytes, patch)
2023-06-07 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Fix test (3.17 KB, patch)
2023-06-07 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Remove incorrect caching from bundle_items (1.62 KB, patch)
2023-06-07 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: (QA follow-up) Fix typo (865 bytes, patch)
2023-06-07 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: (follow-up) Koha::Item->last_returned_by (2.09 KB, patch)
2023-06-07 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: Make ->sharee looks like others (821 bytes, patch)
2023-06-07 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32894: (QA follow-up) Fix tests (1.42 KB, patch)
2023-06-07 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-02-07 06:26:30 UTC
In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified.

For instance, in Koha/ArticleRequest.pm

sub biblio {
    my ($self) = @_; 

    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );

    return $self->{_biblio};
}

This pattern exists in several places.

It can lead to confusion and incorrect results, such as:

use Koha::ArticleRequests;
my $ar = Koha::ArticleRequest->new({
    borrowernumber => 42,
    biblionumber => 42,
})->store;
say $ar->biblio->biblionumber;               # Display 42, correct
$ar->set({ biblionumber => 24 })->store;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->discard_changes;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->delete;

We should remove those caching and rely on DBIC/DBMS caching mechanism instead.
Comment 1 Katrin Fischer 2023-02-26 11:08:39 UTC
No Discussion here, is the status correct? :)
Comment 2 Jonathan Druart 2023-02-28 07:54:01 UTC
(In reply to Katrin Fischer from comment #1)
> No Discussion here, is the status correct? :)

Yes, it's still expecting feedback from other devs.
Comment 3 Martin Renvoize 2023-02-28 08:17:20 UTC
I actually thought we'd alr Ady made he decision to not cache like this. I've not been actively removing such caches but I have generally tried to avoid adding new ones for a while.

There are some rare case where this style of caching does make sense to me.. though I I couldn't point you at one off the top of my head... The prevent public embed fetching stuff in the API perhaps?

Either way, do we think it's time to start retrospectively cleaning up the existing cases?

It could make a good hackfest project to bike them all together.
Comment 4 Jonathan Druart 2023-02-28 16:18:37 UTC
Created attachment 147499 [details] [review]
Bug 32894: Remove wrong caching from Koha:: methods - simple

In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified.

For instance, in Koha/ArticleRequest.pm

sub biblio {
    my ($self) = @_;

    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );

    return $self->{_biblio};
}

This pattern exists in several places.

It can lead to confusion and incorrect results, such as:

use Koha::ArticleRequests;
my $ar = Koha::ArticleRequest->new({
    borrowernumber => 42,
    biblionumber => 42,
})->store;
say $ar->biblio->biblionumber;               # Display 42, correct
$ar->set({ biblionumber => 24 })->store;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->discard_changes;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->delete;

We should remove those caching and rely on DBIC/DBMS caching mechanism instead.

This patch is adjusting the trivial occurrences
Comment 5 Jonathan Druart 2023-02-28 16:18:40 UTC
Created attachment 147500 [details] [review]
Bug 32894: Koha::Item->last_returned_by
Comment 6 Jonathan Druart 2023-02-28 16:18:42 UTC
Created attachment 147502 [details] [review]
Bug 32894: Koha::Biblio->biblioitem

Can we do better here?
Comment 7 Jonathan Druart 2023-02-28 16:18:45 UTC
Created attachment 147503 [details] [review]
Bug 32894: Fix test
Comment 8 Jonathan Druart 2023-02-28 16:19:38 UTC
Those patches deal with most of the cases.

What about Koha::Item::_bundle_items_cached and Koha::Objects?

I would not touch Koha::Objects behaviour here, maybe on a follow-up bug.
Comment 9 Jonathan Druart 2023-03-07 11:56:55 UTC
Martin, could you have a look at Koha::Item::_bundle_items_cached?
Comment 10 Martin Renvoize 2023-04-20 10:50:16 UTC
Yes, sorry.. I've just not had a moment to breath yet.. This is certainly still near the top of my list.
Comment 11 Martin Renvoize 2023-04-21 10:28:39 UTC
Created attachment 150016 [details] [review]
Bug 32894: Remove wrong caching from Koha:: methods - simple

In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified.

For instance, in Koha/ArticleRequest.pm

sub biblio {
    my ($self) = @_;

    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );

    return $self->{_biblio};
}

This pattern exists in several places.

It can lead to confusion and incorrect results, such as:

use Koha::ArticleRequests;
my $ar = Koha::ArticleRequest->new({
    borrowernumber => 42,
    biblionumber => 42,
})->store;
say $ar->biblio->biblionumber;               # Display 42, correct
$ar->set({ biblionumber => 24 })->store;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->discard_changes;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->delete;

We should remove those caching and rely on DBIC/DBMS caching mechanism instead.

This patch is adjusting the trivial occurrences

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2023-04-21 10:28:42 UTC
Created attachment 150017 [details] [review]
Bug 32894: Koha::Item->last_returned_by

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2023-04-21 10:28:44 UTC
Created attachment 150018 [details] [review]
Bug 32894: Koha::Biblio->biblioitem

Can we do better here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2023-04-21 10:28:47 UTC
Created attachment 150019 [details] [review]
Bug 32894: Fix test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2023-04-21 10:28:49 UTC
Created attachment 150020 [details] [review]
Bug 32894: Remove incorrect caching from bundle_items

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2023-04-21 10:28:52 UTC
Created attachment 150021 [details] [review]
Bug 32894: (QA follow-up) Fix typo

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2023-04-21 10:30:07 UTC
Added a tiny typo fix and also a patch for item_bundles.

I agree.. lets to Koha::Objects as it's own bug.

Signing off
Comment 18 Nick Clemens (kidclamp) 2023-04-28 18:36:32 UTC
Something is not right with Koha::Item::last_returned_by 

It returns a patron object, but the relationship is to ItemsLastBorrower and the call to set it doesn't work

prove -v t/db_dependent/Circulation/StoreLastBorrower.t


Also Koha/Virtualshelfshare.pm :
everywhere else we assign resultset to rs, then create new - it isn't wrong, just inconsistent
Comment 19 Jonathan Druart 2023-05-09 09:04:13 UTC
Created attachment 150827 [details] [review]
Bug 32894: (follow-up) Koha::Item->last_returned_by
Comment 20 Jonathan Druart 2023-05-09 09:04:15 UTC
Created attachment 150828 [details] [review]
Bug 32894: Make ->sharee looks like others
Comment 21 Jonathan Druart 2023-05-09 09:04:57 UTC
(In reply to Nick Clemens from comment #18)
> Something is not right with Koha::Item::last_returned_by 
> 
> It returns a patron object, but the relationship is to ItemsLastBorrower and
> the call to set it doesn't work
> 
> prove -v t/db_dependent/Circulation/StoreLastBorrower.t

Oops! Thanks!

> Also Koha/Virtualshelfshare.pm :
> everywhere else we assign resultset to rs, then create new - it isn't wrong,
> just inconsistent

Done.
Comment 22 Nick Clemens (kidclamp) 2023-05-09 11:51:04 UTC
Created attachment 150847 [details] [review]
Bug 32894: Remove wrong caching from Koha:: methods - simple

In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified.

For instance, in Koha/ArticleRequest.pm

sub biblio {
    my ($self) = @_;

    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );

    return $self->{_biblio};
}

This pattern exists in several places.

It can lead to confusion and incorrect results, such as:

use Koha::ArticleRequests;
my $ar = Koha::ArticleRequest->new({
    borrowernumber => 42,
    biblionumber => 42,
})->store;
say $ar->biblio->biblionumber;               # Display 42, correct
$ar->set({ biblionumber => 24 })->store;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->discard_changes;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->delete;

We should remove those caching and rely on DBIC/DBMS caching mechanism instead.

This patch is adjusting the trivial occurrences

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens (kidclamp) 2023-05-09 11:51:06 UTC
Created attachment 150848 [details] [review]
Bug 32894: Koha::Item->last_returned_by

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens (kidclamp) 2023-05-09 11:51:08 UTC
Created attachment 150849 [details] [review]
Bug 32894: Koha::Biblio->biblioitem

Can we do better here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens (kidclamp) 2023-05-09 11:51:10 UTC
Created attachment 150850 [details] [review]
Bug 32894: Fix test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Nick Clemens (kidclamp) 2023-05-09 11:51:12 UTC
Created attachment 150851 [details] [review]
Bug 32894: Remove incorrect caching from bundle_items

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens (kidclamp) 2023-05-09 11:51:15 UTC
Created attachment 150852 [details] [review]
Bug 32894: (QA follow-up) Fix typo

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens (kidclamp) 2023-05-09 11:51:17 UTC
Created attachment 150853 [details] [review]
Bug 32894: (follow-up) Koha::Item->last_returned_by

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens (kidclamp) 2023-05-09 11:51:19 UTC
Created attachment 150854 [details] [review]
Bug 32894: Make ->sharee looks like others

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens (kidclamp) 2023-05-09 11:51:21 UTC
Created attachment 150855 [details] [review]
Bug 32894: (QA follow-up) Fix tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Nick Clemens (kidclamp) 2023-05-09 11:52:04 UTC
Looks good, t/db_dependent all pass now
Comment 32 Kyle M Hall 2023-05-09 12:19:21 UTC
I do not think this is a great idea long term. This is moving backwards when it comes to efficiency and will introduce far more database calls. It seems like it might be better to fix the code that is making a bad assumption. The short term fix is to use discard_changes when needed. The long term fix is to use Koha objects from the perl scripts and pass them to subroutines so we have no need to re-fetch data from the database.

Have you identified any bugs being caused by this, or it just a possibility as you've shown?
Comment 33 Jonathan Druart 2023-05-10 04:26:04 UTC
(In reply to Kyle M Hall from comment #32)
> I do not think this is a great idea long term. This is moving backwards when
> it comes to efficiency and will introduce far more database calls. It seems
> like it might be better to fix the code that is making a bad assumption. The
> short term fix is to use discard_changes when needed. The long term fix is
> to use Koha objects from the perl scripts and pass them to subroutines so we
> have no need to re-fetch data from the database.
> 
> Have you identified any bugs being caused by this, or it just a possibility
> as you've shown?

This is a bad pattern, we should get rid of it and prevent it from propagating over the codebase.

The correct way to fix the refetch is patch like bug 32496 and bug 31735.

There is also bug 32476 that can be helpful to cache *calculated* values when specific performance problems are identified.

Here we are caching only 1 DB call that is already cached by an underlying mechanism anyway (DBMS or DBIC).

> It seems like it might be better to fix the code that is making a bad assumption.

No need to make bad assumption to get incorrect results, see comment 0.
Comment 34 Tomás Cohen Arazi 2023-05-16 18:54:40 UTC
Can be backported. Let's push early next cycle.
Comment 35 Kyle M Hall 2023-05-18 14:14:51 UTC
reset_all && multitime -n 10 prove t/db_dependent/Koha

Just Master:
1: prove t/db_dependent/Koha
            Mean        Std.Dev.    Min         Median      Max
real        129.754     3.631       121.324     130.872     134.770
user        107.146     2.759       100.859     108.025     111.129
sys         17.488      0.720       15.979      17.469      18.793

Master + 32890:
1: prove t/db_dependent/Koha
            Mean        Std.Dev.    Min         Median      Max
real        128.499     4.333       123.117     127.620     139.403
user        106.268     3.456       102.384     105.515     114.984
sys         17.237      0.992       15.540      17.337      19.357

I'm no statistician so take from this what you will. I think it's generally positive. Lower average with the patches but a higher standard deviation.
Comment 36 Tomás Cohen Arazi 2023-06-06 12:52:22 UTC
Please rebase.
Comment 37 Jonathan Druart 2023-06-07 10:03:56 UTC
Created attachment 152086 [details] [review]
Bug 32894: Remove wrong caching from Koha:: methods - simple

In some of our Koha:: objects we have methods that cache their result and return it in subsequent calls. However there is no invalidation of the cache if the object is modified.

For instance, in Koha/ArticleRequest.pm

sub biblio {
    my ($self) = @_;

    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );

    return $self->{_biblio};
}

This pattern exists in several places.

It can lead to confusion and incorrect results, such as:

use Koha::ArticleRequests;
my $ar = Koha::ArticleRequest->new({
    borrowernumber => 42,
    biblionumber => 42,
})->store;
say $ar->biblio->biblionumber;               # Display 42, correct
$ar->set({ biblionumber => 24 })->store;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->discard_changes;
say $ar->biblio->biblionumber;               # Display 42, wrong
$ar->delete;

We should remove those caching and rely on DBIC/DBMS caching mechanism instead.

This patch is adjusting the trivial occurrences

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 Jonathan Druart 2023-06-07 10:03:59 UTC
Created attachment 152087 [details] [review]
Bug 32894: Koha::Item->last_returned_by

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Jonathan Druart 2023-06-07 10:04:02 UTC
Created attachment 152088 [details] [review]
Bug 32894: Koha::Biblio->biblioitem

Can we do better here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Jonathan Druart 2023-06-07 10:04:06 UTC
Created attachment 152089 [details] [review]
Bug 32894: Fix test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Jonathan Druart 2023-06-07 10:04:10 UTC
Created attachment 152090 [details] [review]
Bug 32894: Remove incorrect caching from bundle_items

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 Jonathan Druart 2023-06-07 10:04:15 UTC
Created attachment 152091 [details] [review]
Bug 32894: (QA follow-up) Fix typo

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Jonathan Druart 2023-06-07 10:04:19 UTC
Created attachment 152092 [details] [review]
Bug 32894: (follow-up) Koha::Item->last_returned_by

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Jonathan Druart 2023-06-07 10:04:23 UTC
Created attachment 152093 [details] [review]
Bug 32894: Make ->sharee looks like others

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 45 Jonathan Druart 2023-06-07 10:04:27 UTC
Created attachment 152094 [details] [review]
Bug 32894: (QA follow-up) Fix tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 46 Tomás Cohen Arazi 2023-06-07 19:49:32 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 47 Martin Renvoize 2023-07-12 07:01:41 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 48 Matt Blenkinsop 2023-07-12 09:10:39 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 49 Jonathan Druart 2023-08-30 12:35:23 UTC
Stop backporting this.