Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set_cache should invalidate existing related_resultset #110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pdl
Copy link

@pdl pdl commented Nov 9, 2016

In DBIx::Class::ResultSet::related_resultset, if the current resultset has a cache of results, the rows in that cache are checked for related objects which can be used to populate the cache belonging to the related resultset which is going to be returned.

However, the related resultset objects themselves are also stored on $self in a cache ($self->{'related_resultsets'}), so that this process is skipped if the related resultset has already been created in a previous call to related_resultset.

This means that if the cache is set after a related resultset has been created, subsequent calls to related_resultset retrieve the existing resultset; because it was created before the cache existed, it either has no cache of its own, or has a cache which potentially contradicts the cache in set_cache.

There is no user-facing way to 'undo' the creation of a cached related resultset, nor is it obvious from the docs that this might be useful or necessary (not to mention why it should be). In fact, the docs don't even mention that related resultsets are cached.

For example: in this case $artist is an arrayref of one item:

$cd_rset->set_cache(\@cds);
my $artist_rset_after = $cd_rset->related_resultset('artist');
my $artist = $artist_rset_after->get_cache();

... whereas in this case $artist is undef:

my $artist_rset_before = $cd_rset->related_resultset('artist');
$cd_rset->set_cache(\@cds);
my $artist_rset_after = $cd_rset->related_resultset('artist');
my $artist = $artist_rset_after->get_cache();

I see two potential solutions:

  1. When $self->set_cache is called, clear $self->{'related_resultsets'}. New calls to related_resultset would return new resultsets with the correct cache. Resultsets previously created would be unchanged.

  2. When $self->set_cache is called, populate the cache for each resultset in $self->{'related_resultsets'}. This may be surprising if those resultsets have also previously populated a cache. Furthermore, what ought to happen if \@cds do not all have a populated cache in their related_resultset for a given relationship. Should the cache on the existing resultset be emptied, or should it be left alone?

I feel that option 1 is the cleanest solution.

This PR introduces:

  • A currently-failing test which demonstrates the issue
  • A one-line-plus-comments fix, following the first solution.

@ribasushi
Copy link
Collaborator

@pdl thanks for investigating this. I had something along these lines queued up to be able to properly implement the prototype in https://github.com/dbsrgits/dbix-class/tree/people/ilmari/delay-related-rs-creation-for-single-rels. I won't be able to look at this in depth (and combine it with my work) until the commotion on the ML is resolved (some time next month I'd think).

Stay tuned!

@pdl
Copy link
Author

pdl commented Nov 10, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants