Bug 22700 - Make biblio_metadata prefetchable from Koha::Biblio
Summary: Make biblio_metadata prefetchable from Koha::Biblio
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 22407 23235 22696
  Show dependency treegraph
 
Reported: 2019-04-12 16:40 UTC by Tomás Cohen Arazi
Modified: 2019-06-28 18:18 UTC (History)
5 users (show)

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


Attachments
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio (1.02 KB, patch)
2019-04-12 16:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio (1.02 KB, patch)
2019-04-12 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio (1.07 KB, patch)
2019-04-14 10:35 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio (1.78 KB, patch)
2019-04-19 14:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio (1.85 KB, patch)
2019-04-23 14:34 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-04-12 16:40:19 UTC

    
Comment 1 Tomás Cohen Arazi 2019-04-12 16:47:35 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2019-04-12 18:19:26 UTC
Created attachment 87931 [details] [review]
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio

This patch makes $biblio->metadata be prefetchable as the Coding
guidelines require.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: No regressions!
- Sign off :-D
Comment 3 Michal Denar 2019-04-14 10:35:50 UTC
Created attachment 87943 [details] [review]
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio

This patch makes $biblio->metadata be prefetchable as the Coding
guidelines require.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: No regressions!
- Sign off :-D

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 4 Martin Renvoize 2019-04-16 11:32:39 UTC
Comment on attachment 87943 [details] [review]
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio

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

This doesn't feel entirely correct yet I'm afraid.. though I'm not close enough to the biblio_metadatas table to entirely understand whether we expect it to every be a more than one-to-one relationship with the biblio's table.

::: Koha/Biblio.pm
@@ +74,4 @@
>  sub metadata {
>      my ( $self ) = @_;
>  
> +    my $metadata_rs = $self->_result->biblio_metadatas->single;

Surprised this doesn't throw a warning?

https://metacpan.org/pod/DBIx::Class::ResultSet#single implies that calling 'single' on a 'has_many' (when prefetched) should throw one.

Also, as discussed in bug 22407 it would be nice to rename the underlying dbic relationship to match the Koha::Object relation accessor (we could also code said relationship to explicitly return a single related row rather than the possibility of a ResultSet)
Comment 5 Tomás Cohen Arazi 2019-04-16 13:10:38 UTC
(In reply to Martin Renvoize from comment #4)
> Comment on attachment 87943 [details] [review] [review]
> Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio
> 
> Review of attachment 87943 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This doesn't feel entirely correct yet I'm afraid.. though I'm not close
> enough to the biblio_metadatas table to entirely understand whether we
> expect it to every be a more than one-to-one relationship with the biblio's
> table.

It is a one-to-one relationship.

> ::: Koha/Biblio.pm
> @@ +74,4 @@
> >  sub metadata {
> >      my ( $self ) = @_;
> >  
> > +    my $metadata_rs = $self->_result->biblio_metadatas->single;
> 
> Surprised this doesn't throw a warning?
>
>
> https://metacpan.org/pod/DBIx::Class::ResultSet#single implies that calling
> 'single' on a 'has_many' (when prefetched) should throw one.

For a warning it requires more than one result. Something the schema allows, but the code doesn't.

> Also, as discussed in bug 22407 it would be nice to rename the underlying
> dbic relationship to match the Koha::Object relation accessor (we could also
> code said relationship to explicitly return a single related row rather than
> the possibility of a ResultSet)

I agree with this, but I wouldn't consider this a blocker for this one. I will give it a try, though.
Comment 6 Tomás Cohen Arazi 2019-04-19 14:15:47 UTC
Created attachment 88355 [details] [review]
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio

This patch makes $biblio->metadata be prefetchable as the Coding
guidelines require.

A new has_one relationship is added in the Biblio schema, named as the
accessor to also convey to proposed guidelines for readability.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: No regressions!
- Sign off :-D

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 7 Martin Renvoize 2019-04-23 14:34:56 UTC
Created attachment 88492 [details] [review]
Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio

This patch makes $biblio->metadata be prefetchable as the Coding
guidelines require.

A new has_one relationship is added in the Biblio schema, named as the
accessor to also convey to proposed guidelines for readability.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: No regressions!
- Sign off :-D

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2019-04-23 14:35:55 UTC
I like this version :).

Passing QA.
Comment 9 Nick Clemens 2019-04-24 11:48:30 UTC
Awesome work all!

Pushed to master for 19.05
Comment 10 Jonathan Druart 2019-04-24 12:48:50 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> (In reply to Martin Renvoize from comment #4)
> > Comment on attachment 87943 [details] [review] [review] [review]
> > Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio
> > 
> > Review of attachment 87943 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > This doesn't feel entirely correct yet I'm afraid.. though I'm not close
> > enough to the biblio_metadatas table to entirely understand whether we
> > expect it to every be a more than one-to-one relationship with the biblio's
> > table.
> 
> It is a one-to-one relationship.

Hum, really?
If it was, we would have not create a new table for that.
We could have different representation of the record (MARC, MARCXML, etc.), for different MARC profiles.
IMO this is completely wrong, but I may have missed a previous discussion.
Comment 11 Jonathan Druart 2019-04-24 12:51:12 UTC
Also note that Koha::Biblio->metadata was wrong prior to this patch...
Comment 12 Katrin Fischer 2019-04-24 12:53:41 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Tomás Cohen Arazi from comment #5)
> > (In reply to Martin Renvoize from comment #4)
> > > Comment on attachment 87943 [details] [review] [review] [review] [review]
> > > Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio
> > > 
> > > Review of attachment 87943 [details] [review] [review] [review] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This doesn't feel entirely correct yet I'm afraid.. though I'm not close
> > > enough to the biblio_metadatas table to entirely understand whether we
> > > expect it to every be a more than one-to-one relationship with the biblio's
> > > table.
> > 
> > It is a one-to-one relationship.
> 
> Hum, really?
> If it was, we would have not create a new table for that.
> We could have different representation of the record (MARC, MARCXML, etc.),
> for different MARC profiles.
> IMO this is completely wrong, but I may have missed a previous discussion.

It is 1:1 right now - splitting it off biblioitems was for performance reasons, not database design IIRC:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17196
Comment 13 Tomás Cohen Arazi 2019-04-24 12:56:33 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Tomás Cohen Arazi from comment #5)
> > (In reply to Martin Renvoize from comment #4)
> > > Comment on attachment 87943 [details] [review] [review] [review] [review]
> > > Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio
> > > 
> > > Review of attachment 87943 [details] [review] [review] [review] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This doesn't feel entirely correct yet I'm afraid.. though I'm not close
> > > enough to the biblio_metadatas table to entirely understand whether we
> > > expect it to every be a more than one-to-one relationship with the biblio's
> > > table.
> > 
> > It is a one-to-one relationship.
> 
> Hum, really?
> If it was, we would have not create a new table for that.
> We could have different representation of the record (MARC, MARCXML, etc.),
> for different MARC profiles.
> IMO this is completely wrong, but I may have missed a previous discussion.

What would you say is the source of truth if there were more than one representation? I'm not against some vaccine of different representativa, just saying many places assume THE metadata is the source for populating other columns and our design doesn't consider a way to determine which representation is the one to be used as basis.
As far as I can tell, many times it's been said that the table was splitted to save space in join operations where the metadata wasn't really needed.
Comment 14 Katrin Fischer 2019-04-24 12:59:45 UTC
I think Tomas has a good point - having multiple representations of the same record could get messy fast - we haven't worked out a concept for that yet. What we had discussed I think was storing data in other formats than MARC - so still 1:1. We do not have code for that yet I think (display, indexing, mapping...).
Comment 15 Jonathan Druart 2019-04-24 20:02:08 UTC
(In reply to Katrin Fischer from comment #14)
> What we had discussed I think was storing data in other formats than MARC -
> so still 1:1

No, it would become 1:n then.

What about renaming the method in Koha::S::R::Biblio to ->marc_xml_metadata (adding the conditions in it) and Koha::Biblio->metadata to ->marc_xml (make it returns the MARX XML directly)?
Comment 16 Katrin Fischer 2019-04-24 20:03:43 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Katrin Fischer from comment #14)
> > What we had discussed I think was storing data in other formats than MARC -
> > so still 1:1
> 
> No, it would become 1:n then.

I meant still only 1 metadata entry - either MARC or Dublincore or... not having both or multiple formats. It would allow to load data without converting to MARC.
Comment 17 Martin Renvoize 2019-04-24 20:25:09 UTC
I do sort of agree that 'metadata' feels like a bit a generic term if in reality it's always marc..

As an aside, do we have any inflators setup with our dbic schemata?
Comment 18 Jonathan Druart 2019-04-24 20:44:42 UTC
(In reply to Martin Renvoize from comment #17)
> As an aside, do we have any inflators setup with our dbic schemata?

Like:
  Koha/ItemTypes.pm:    $Koha::Schema::Result::Itemtype::LANGUAGE = $language;
?
Comment 19 Martin Renvoize 2019-04-26 14:57:29 UTC
Enhancement will not be backported to 18.11.x series.
Comment 20 Martin Renvoize 2019-06-05 12:08:05 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Martin Renvoize from comment #17)
> > As an aside, do we have any inflators setup with our dbic schemata?
> 
> Like:
>   Koha/ItemTypes.pm:    $Koha::Schema::Result::Itemtype::LANGUAGE =
> $language;
> ?

No, I meant like DBIC::InflateColumn::DateTime.. or in this case we could easily have DBIC::::InflateColumn::MARC and similar
Comment 21 Jonathan Druart 2019-06-08 17:57:18 UTC
(In reply to Jonathan Druart from comment #15)
> What about renaming the method in Koha::S::R::Biblio to ->marc_xml_metadata
> (adding the conditions in it) and Koha::Biblio->metadata to ->marc_xml (make
> it returns the MARX XML directly)?

Should we go with that for now?
Comment 22 Martin Renvoize 2019-06-13 16:31:12 UTC
I think that makes sense to me.. but in a new bug pretty please so we don't taint the release notes (and so I don't completely lose the change by forgetting about this bug as it's RESOLVED FIXED already ;)
Comment 23 Jonathan Druart 2019-06-28 18:18:00 UTC
(In reply to Martin Renvoize from comment #22)
> I think that makes sense to me.. but in a new bug pretty please so we don't
> taint the release notes (and so I don't completely lose the change by
> forgetting about this bug as it's RESOLVED FIXED already ;)

I have opened bug 23235.