Bug 13719

Kyle, what would "Store list of objects" mean? I'm sorry but it is too generic for my idiomatic limitations!

(In reply to Tomás Cohen Arazi from comment #2)
> Kyle, what would "Store list of objects" mean? I'm sorry but it is too
> generic for my idiomatic limitations!

Sorry, I was just posting the code to get it out there. I will be writing up a better explanation and a test plan shortly!

We can both simplify and improve the functioning of Koha::Objects by removing out reliance on DBIC for set iteration ( first(), next(), reset(), etc ). The problem is that DBIC destroys and refetches results every time reset() is called. For example, take the following code:

my $borrowers = Koha::Borrowers->search({ firstname => 'Kyle' }).
my $kyle1 = $borrowers->next();
$borrowers->reset();
my $kyle2 = $borrowers->next();

In this case, we would expect $kyle1 and $kyle2 to refer to the exact same object in memory, but they do *not*. This is simply a limitation of DBIx::Class.

However, by handling this issue ourselves, we not only solve the problem, but I believe we also reduce the complexity of our code.

This is all accomplished without changing the external behavior of the Koha::Objects module.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Borrowers.t

Couldn't this be a memory hog?

Comment on attachment 36022 [details] [review]
Bug 13719 - Make Koha::Objects store list of resultant objects on an as needed basis

Review of attachment 36022 [details] [review]:
-----------------------------------------------------------------

Is there some default behaviour that I'm unaware of in terms of attempting to access something past the boundary of a list?

::: Koha/Objects.pm
@@ +140,4 @@
>  sub next {
>      my ( $self ) = @_;
>  
> +    my $object = $self->_objects()->[ $self->{_iterator}++ ];

No boundary check?!

On a positive note, the test plan worked, and a "time prove -v t/db_dependent/Borrowers.t" seemed to run a little faster on default data with the patch.

Created attachment 45862 [details] [review]
[SIGNED-OFF] Bug 13719: Make Koha::Objects store list of resultant objects on an as needed basis

We can both simplify and improve the functioning of Koha::Objects by
removing out reliance on DBIC for set iteration ( first(), next(),
reset(), etc ). The problem is that DBIC destroys and refetches
results every time reset() is called. For example, take the following
code:

my $borrowers = Koha::Borrowers->search({ firstname => 'Kyle' }).
my $kyle1 = $borrowers->next();
$borrowers->reset();
my $kyle2 = $borrowers->next();

In this case, we would expect $kyle1 and $kyle2 to refer to the exact
same object in memory, but they do *not*. This is simply a limitation of
DBIx::Class.

However, by handling this issue ourselves, we not only solve the
problem, but I believe we also reduce the complexity of our code.

This is all accomplished without changing the external behavior of the
Koha::Objects module.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Borrowers.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass, no errors

From the doc
http://search.cpan.org/~ribasushi/DBIx-Class-0.082820/lib/DBIx/Class/ResultSet.pm#reset

"Implicitly resets the storage cursor, so a subsequent "next" will trigger another query."

It is not a limitation or a bug, but a feature.
I am not sure it's a good idea to make our methods behave differently than the DBIx::Class ones.

CCing Martin to get another feedback.

Totally agree with Jonathan here, I don't understand the point of this at all.. the idea of reset is to allow one to re-execute the query in the case that the data in the database may have changed between resultset usage.

If you want to make it more likely $kyle1 == $kyle2 then add an order_by into the search. (but of course, this won't ensure unless of course you're ordering by id and you've not changed the original object at in the database)

I've not looked into the code yet, but just on the description I'm not really sure what problem it's attempting to resolve.

OK, So, you're always calling ->all on he dbic resultset, even when you may only actually want the ->first result :(. Bad mojo, this is a big memory leak in my opinion.

Yes calling ->all is a good idea when you know you want the full set (or you know the full set will always be sufficiently small that your not worried about the memory overhead, but calling it for all requests is just asking for trouble in my opinion.

We should be encouraging thought behind these calls and good practice. Not coding around other coders deficiencies.. that's what QA is for, spotting such performance mistakes during QA time.

All in all, this gets a definitive thumbs down from me

(In reply to Martin Renvoize from comment #13)
> OK, So, you're always calling ->all on he dbic resultset, even when you may
> only actually want the ->first result :(. Bad mojo, this is a big memory
> leak in my opinion.
> 
> Yes calling ->all is a good idea when you know you want the full set (or you
> know the full set will always be sufficiently small that your not worried
> about the memory overhead, but calling it for all requests is just asking
> for trouble in my opinion.
> 
> We should be encouraging thought behind these calls and good practice. Not
> coding around other coders deficiencies.. that's what QA is for, spotting
> such performance mistakes during QA time.
> 
> All in all, this gets a definitive thumbs down from me

Good point. Let's just close this it out, it seemed like a good idea at first, but I think you are correct.