Bug 22407 - OMNIBUS: Use DBIC relations to fetch related object rather than searching for the object
Summary: OMNIBUS: Use DBIC relations to fetch related object rather than searching for...
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords: rel_20_11_target
Depends on: 22683 22685 25951 22684 22686 22700 22701
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-25 15:22 UTC by Nick Clemens
Modified: 2020-07-20 17:48 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-02-25 15:22:51 UTC
Example, instead of this:
return Koha::Patrons->find( $self->guarantorid() );
We should do this:
return Koha::Patron->_new_from_dbic($self->_result->guarantor);

This allows us to use prefetch and perform a single query as opposed to multiple queries
Comment 1 Jonathan Druart 2019-02-26 17:46:52 UTC
Yes, and no.

You can only do that if you know the patron has a guarantor, which is not always true.

You must do:
sub guarantor {
    my ( $self ) = @_;
    my $guarantor_rs = $self->_result->guarantorid;
    return unless $guarantor_rs;
    return Koha::Patron->_new_from_dbic($guarantor_rs);
}

I am definitely in favor of that, we should have a guideline about it IMO.
Comment 2 Tomás Cohen Arazi 2019-02-26 17:51:22 UTC
(In reply to Jonathan Druart from comment #1)
> Yes, and no.
> 
> You can only do that if you know the patron has a guarantor, which is not
> always true.
> 
> You must do:
> sub guarantor {
>     my ( $self ) = @_;
>     my $guarantor_rs = $self->_result->guarantorid;
>     return unless $guarantor_rs;
>     return Koha::Patron->_new_from_dbic($guarantor_rs);
> }
> 
> I am definitely in favor of that, we should have a guideline about it IMO.

The pattern should cover the one-to-many use case as well. And yes, the idea is to propose a coding guideline!
Comment 3 Jonathan Druart 2019-02-26 18:09:23 UTC
We have already everything in our code:

Koha::Item->biblio
 93 sub biblio {
 94     my ( $self ) = @_;
 95     my $biblio_rs = $self->_result->biblio;
 96     return Koha::Biblio->_new_from_dbic( $biblio_rs );
 97 }

Koha::Item->checkout
121 sub checkout {
122     my ( $self ) = @_;
123     my $checkout_rs = $self->_result->issue;
124     return unless $checkout_rs;
125     return Koha::Checkout->_new_from_dbic( $checkout_rs );
126 }

Koha::Item->current_holds
271 sub current_holds {
272     my ( $self ) = @_;
273     my $attributes = { order_by => 'priority' };
274     my $dtf = Koha::Database->new->schema->storage->datetime_parser;
275     my $params = {
276         itemnumber => $self->itemnumber,
277         suspend => 0,
278         -or => [
279             reservedate => { '<=' => $dtf->format_date(dt_from_string) },
280             waitingdate => { '!=' => undef },
281         ],
282     };
283     my $hold_rs = $self->_result->reserves->search( $params, $attributes );
284     return Koha::Holds->_new_from_dbic($hold_rs);
285 }
Comment 4 Katrin Fischer 2019-04-04 07:35:54 UTC
Do you still want to propose a coding guideline here, maybe for next dev meeting?
Comment 5 Kyle M Hall 2019-04-04 10:55:42 UTC
(In reply to Katrin Fischer from comment #4)
> Do you still want to propose a coding guideline here, maybe for next dev
> meeting?

I believe we should.
Comment 6 Katrin Fischer 2019-04-08 11:33:06 UTC
I have added it to the next meetings agenda, but as I am no expert for this topic it would be nice if someone could write up a proposal here.
Comment 7 Martin Renvoize 2019-04-10 14:40:11 UTC
DRAFT:

Within PERL15 under Koha::Object

Koha::Object relationship accessors should be written to allow 'prefetch'

For example one should use:

sub guarantor {
    my $self = shift;
    my $guarantor_rs = $self->_result->guarantorid;
    return unless $guarantor_rs;
    return Koha::Patron->_new_from_dbic($gaurantor_rs);
}

One should not use:

sub guarantor {
    my $self = shift;
    return Koha::Patrons->find( $self->guarantorid() );
}
Comment 8 Martin Renvoize 2019-04-11 09:23:43 UTC
I've started this off.. but there's a huge number of classes to go through :(
Comment 9 Martin Renvoize 2019-04-13 06:04:55 UTC
This has me wondering.. we're often renaming relationships within Koha::Objects (understandably as often our db derived ones are horrible because of poor initial db design).  Should this guideline also introduce a requirement to match the relation name in the dbic class up to the relation name in the Koha::Object?  It wouldn't be hard to add relations with such names and would lead to cleaner, easier to guess, prefetch strings.
Comment 10 Tomás Cohen Arazi 2019-04-13 12:43:00 UTC
(In reply to Martin Renvoize from comment #9)
> This has me wondering.. we're often renaming relationships within
> Koha::Objects (understandably as often our db derived ones are horrible
> because of poor initial db design).  Should this guideline also introduce a
> requirement to match the relation name in the dbic class up to the relation
> name in the Koha::Object?  It wouldn't be hard to add relations with such
> names and would lead to cleaner, easier to guess, prefetch strings.

I'm all for renaming relations like you say. I've tried myself to (a) migrate some methods into matching this guideline and (b) use it in a real life scenario and can say the only uncomfortable step was (b), due to the relation naming issues. See bug 22696.
Comment 11 Martin Renvoize 2019-04-13 13:47:17 UTC
We could also use 'skip_relationships' in our dbicdump script to prevent outputting superfluous relationships given we can't/won't use them until we've manually written code in the corresponding Koha::Object class.. this way we would end up being much more explicit about what relationships were building (and as such reduce the load time/run time memory use form building massive dbic object full of unreachable expensive relationship accessors.

That.. or we could start using the dbic recommended way of doing db work and go from schema to database rather than the other way around 😉 and ditch the script entirely.
Comment 12 Tomás Cohen Arazi 2019-04-16 13:08:08 UTC
(In reply to Martin Renvoize from comment #11)
> We could also use 'skip_relationships' in our dbicdump script to prevent
> outputting superfluous relationships given we can't/won't use them until
> we've manually written code in the corresponding Koha::Object class.. this
> way we would end up being much more explicit about what relationships were
> building (and as such reduce the load time/run time memory use form building
> massive dbic object full of unreachable expensive relationship accessors.

I agree with this, but it would require  us first finish setting our custom relationships. At least the ones we use.

> That.. or we could start using the dbic recommended way of doing db work and
> go from schema to database rather than the other way around 😉 and ditch the
> script entirely.

That's not gonna happen unless someone takes care of it, and it is not a simple task, changing the community workflow and upgrade scripts.
Comment 13 David Cook 2020-07-09 00:18:28 UTC
Honest question:

Why do this:
sub guarantor {
    my $self = shift;
    my $guarantor_rs = $self->_result->guarantorid;
    return unless $guarantor_rs;
    return Koha::Patron->_new_from_dbic($gaurantor_rs);
}

Instead of this:
sub guarantor {
    my $self = shift;
    my $guarantor;
    my $guarantor_rs = $self->_result->guarantorid;
    if ($guarantor_rs){
        $guarantor = Koha::Patron->_new_from_dbic($gaurantor_rs);
    }
    return $guarantor;
}

I admit that the latter has 3 more lines than the former, but the latter is way easier to read, easier to debug, and easier to add error-handling.

Actually, that latter should probably double-check the output of Koha::Patron->_new_from_dbic() before returning.

I've noticed a lot of Koha code directly returning the output of a method call, and it drives me crazy. We should do more data validation. It'll mean fewer bugs and easier maintenance.
Comment 14 Julian Maurice 2020-07-09 07:10:58 UTC
(In reply to David Cook from comment #13)
> I've noticed a lot of Koha code directly returning the output of a method
> call, and it drives me crazy. We should do more data validation. It'll mean
> fewer bugs and easier maintenance.

What is there to check in this particular situation ? _new_from_dbic either dies or return a blessed object. If it dies then Koha::Patron::guarantor should die too, if it returns a blessed object, then there is nothing to check.
Comment 15 David Cook 2020-07-09 23:57:54 UTC
(In reply to Julian Maurice from comment #14)
> What is there to check in this particular situation ? _new_from_dbic either
> dies or return a blessed object. If it dies then Koha::Patron::guarantor
> should die too, if it returns a blessed object, then there is nothing to
> check.

That's an interesting perspective. 

If it dies, it's probably going to bring down that entire Plack worker. That seems problematic to me. We could try to trap the error in the caller, but in practice that is rare in Koha. That being said, even if it doesn't die, we need to handle a non-fatal exception too. Maybe it is better to die in the grand scheme of things, as it'll more easily show code bugs and bad data.

If it doesn't die, what happens if you want to add a check to that object? Maybe you only want to return the guarantor under certain conditions. It probably makes sense to add that check here rather than in every caller of this method. To do that, you'd have to refactor to use the syntax I described anyway. Or maybe you're doing some debugging because the guarantor being returned isn't correct. You can't add any logging in this code unless you refactor to the syntax I described.
Comment 16 Julian Maurice 2020-07-10 08:21:57 UTC
(In reply to David Cook from comment #15)
> If it doesn't die, what happens if you want to add a check to that object?
> Maybe you only want to return the guarantor under certain conditions. It
> probably makes sense to add that check here rather than in every caller of
> this method. To do that, you'd have to refactor to use the syntax I
> described anyway. Or maybe you're doing some debugging because the guarantor
> being returned isn't correct. You can't add any logging in this code unless
> you refactor to the syntax I described.

In my opinion "maybe we will want to add something here" is not a good reason to refactor code. If that "maybe" ever comes true, then you can refactor the code while making the change. No need to do it now.

Note that I also tend to prefer your version, but I'm happy with both.
If it ain't broke don't fix it ;)
Comment 17 David Cook 2020-07-16 03:34:44 UTC
(In reply to Julian Maurice from comment #16)
> In my opinion "maybe we will want to add something here" is not a good
> reason to refactor code. If that "maybe" ever comes true, then you can
> refactor the code while making the change. No need to do it now.

Given the number of legacy projects I've maintained over the years, we'll just have to agree to disagree on that one ;).

> Note that I also tend to prefer your version, but I'm happy with both.
> If it ain't broke don't fix it ;)

True enough. There are better hills to die on heh.
Comment 18 Tomás Cohen Arazi 2020-07-20 17:48:09 UTC
Koha::Object is trapping DBIC exceptions and converting them into Koha::Exception* and we are shifting into using them everywhere. They get properly propagated if not catch locally, so I guess there will be places in which we could catch exceptions. But those would be specific cases.