Bug 34828 - Add Koha::Biblio::Metadata::Extractor* classes
Summary: Add Koha::Biblio::Metadata::Extractor* classes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 35142
  Show dependency treegraph
 
Reported: 2023-09-19 12:35 UTC by Tomás Cohen Arazi
Modified: 2023-10-24 12:38 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development adds a new class for handling metadata extraction from the metadata registry in Koha. The way it is built provides a good framework for reorganizing the codebase around this area, as well as making it more streamlined to write tests.
Version(s) released in:
23.11.00


Attachments
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc (17.08 KB, patch)
2023-09-19 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc (17.08 KB, patch)
2023-10-05 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34828: Move to Koha::Biblio::Metadata::Extractor (8.79 KB, patch)
2023-10-05 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC (22.20 KB, patch)
2023-10-05 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor (2.86 KB, patch)
2023-10-05 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc (17.12 KB, patch)
2023-10-05 17:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 34828: Move to Koha::Biblio::Metadata::Extractor (8.82 KB, patch)
2023-10-05 17:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC (22.23 KB, patch)
2023-10-05 17:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor (2.90 KB, patch)
2023-10-05 17:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc (17.18 KB, patch)
2023-10-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34828: Move to Koha::Biblio::Metadata::Extractor (8.89 KB, patch)
2023-10-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC (22.30 KB, patch)
2023-10-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor (2.96 KB, patch)
2023-10-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34828: Remove useless variable (703 bytes, patch)
2023-10-10 11:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34828: Fix test (530 bytes, patch)
2023-10-10 12:16 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 Tomás Cohen Arazi 2023-09-19 12:35:05 UTC

    
Comment 1 Tomás Cohen Arazi 2023-09-19 18:49:28 UTC
Created attachment 155907 [details] [review]
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc

This patch introduces a new pattern for the different ->get_<thing>
methods we've been adding. The aim is that code will look more like:

my $metadata_extractor = Koha::MetadataExtractor->new;

while ( my $biblio = $biblios->next ) {
    my $record = $biblio->record;
    my $schema = $biblio->record_schema;

    $data->{$biblio->id}->{normalized_upc} =
$metadata_extractor->get_normalized_upc( { record => $record, schema =>
$schema } );
    $data->{$biblio->id}->{normalized_ean} =
$metadata_extractor->get_normalized_ean( { record => $record, schema =>
$schema } );
}

The key is that we are actually reusing the MARC::Record, and code for
each schema is organized cleanly so easier to maintain.

For the class names, I chose to add the 'MARC' name in the path, so we
don't need to refactor anything if we want to add support for another
serialization formats.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa -c 1
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 2 David Cook 2023-09-20 03:52:21 UTC
I'm intrigued by this one. It looks practical and high performance.

However, it also feels a bit unnatural. It's more natural to have something like $metadata->get_normalized_upc. 

--

Looking at your patches, I notice that Koha::MetadataExtractor::MARC::MARC21 doesn't have any internal state, so it doesn't necessarily need to require object instantiation. And the only internal state for Koha::MetadataExtractor is a list of stateless objects. 

So I have a suggestion for an alternative using static class methods. It would be just as practical and high performance, but it would be more concise. 

It would look like this: $biblio_metadata->extractor->get_normalized_upc({ record => $record })

I suppose that might look a little confusing, so you could do something like 'my $extractor = $biblio_metadata->get_extractor' and then use '$extractor->get_normalized_upc({ record => $record })'. 

The Koha::Biblio::Metadata object has the "schema" to-hand, so the "extractor" method would dynamically return the name of the schema-specific extractor class for that metadata record, and then "get_normalized_upc" would be called as a static class method like Koha::MetadataExtractor::MARC::MARC21->get_normalized_upc({ record => $record }).

It's very concise and has all the same benefits of your current implementation. 

I'll include a little proof-of-concept as a comment.
Comment 3 David Cook 2023-09-20 03:52:39 UTC Comment hidden (obsolete)
Comment 4 David Cook 2023-09-20 03:54:17 UTC
#!/usr/bin/perl

use strict;
use warnings;

my $biblio = Local::Biblio->new({type => 'Test'});
warn $biblio;
warn $biblio->extractor;
warn $biblio->extractor->awesome;

package Local::Biblio;

sub new {
    my ($class,$args) = @_;
    $args = {} unless $args;
    my $self = bless($args,$class);
    return $self;
}

sub extractor {
    my ($self) = @_;
    my $type = $self->{type};
    return "Local::Extractor::$type";
}

package Local::Extractor::Test;
sub awesome {
    my ($class,$args) = @_;
    return "$class == awesome";
}

1;
Comment 5 David Cook 2023-09-20 03:57:19 UTC
Another option would be to implement get_normalized_upc() in Koha::Biblio::Metadata and have it do an internal call to that "extractor" function. 

That could be useful for caching and storing internal state in Koha::Biblio::Metadata if we wanted.
Comment 6 Tomás Cohen Arazi 2023-09-20 11:33:35 UTC
David and all, I'm happy to have the ball roll and have all your ideas. I'm not attached to this particular code in any way.

I'm just generally not sure about the approach we have taken, and this is the best I have come up with.

opac-detail.pl has calls to:

    GetNormalizedEAN
    GetNormalizedISBN
    GetNormalizedOCLCNumber
    GetNormalizedUPC
    GetMarcControlnumber
    GetMarcISBN
    GetMarcISSN
    GetMarcSeries
    GetMarcSubjects
    GetMarcUrls

About object orientation: I think I just didn't want to type long names for each call (considering we might have mixes of schemas, even). I would agree my code could just be passed a Koha::Biblio::Metadata object instead, which *should* cache the calculated MARC::Record object in some way.

The ambivalence between Koha::Biblio and Koha::Biblio::Metadata has always smelled. It become evident when we were developing the /biblios endpoint. And the reason I filed bug 14645 8 years ago (LOL, just remembered).
Comment 7 Jonathan Druart 2023-09-20 15:01:11 UTC
What do we want to type/read?

I personally would like something like:

  my $extractor = Koha::Biblio::MetadataExtractor->new({biblio => $biblio});

Koha::MetadataExtractor retrieves the record and the schema from the biblio object.

then simply retrieve using:

  my $upc = $extractor->[get_]normalized_upc;

And we will see for authorities later, we don't need it for now.
Comment 8 Tomás Cohen Arazi 2023-09-20 15:12:12 UTC
(In reply to Jonathan Druart from comment #7)
> What do we want to type/read?
> 
> I personally would like something like:
> 
>   my $extractor = Koha::Biblio::MetadataExtractor->new({biblio => $biblio});
> 
> Koha::MetadataExtractor retrieves the record and the schema from the biblio
> object.
> 
> then simply retrieve using:
> 
>   my $upc = $extractor->[get_]normalized_upc;

I like that.
Comment 9 David Cook 2023-09-20 23:20:37 UTC
(In reply to Jonathan Druart from comment #7)
> What do we want to type/read?
> 
> I personally would like something like:
> 
>   my $extractor = Koha::Biblio::MetadataExtractor->new({biblio => $biblio});

Thinking about it again today and I suppose I just wonder about all the different objects we'll have to do similar things.

Koha::Biblio
Koha::Biblio::Metadata
Koha::Biblio::MetadataExtractor
Koha::MetadataRecord
Koha::RecordProcessor

I can't keep them all straight. I hadn't even heard of Koha::MetadataRecord until yesterday. 

Something like "get_normalized_upc" should conceptually be a method of Koha::Biblio or Koha::Biblio::Metadata. But it would be a bit annoying to have a lot of if/elsif/else statements in methods like "get_normalized_upc" based on a schema condition.

So I can see the value of Koha::Biblio::MetadataExtractor and having separate subclasses for different schemas, as it is a clean way of providing schema specific method overrides. 

But surely "hiding" it within Koha::Biblio or Koha::Biblio::Metadata would be better reading/writing for developers.
Comment 10 Jonathan Druart 2023-10-05 10:45:11 UTC
Created attachment 156565 [details] [review]
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc

This patch introduces a new pattern for the different ->get_<thing>
methods we've been adding. The aim is that code will look more like:

my $metadata_extractor = Koha::MetadataExtractor->new;

while ( my $biblio = $biblios->next ) {
    my $record = $biblio->record;
    my $schema = $biblio->record_schema;

    $data->{$biblio->id}->{normalized_upc} =
$metadata_extractor->get_normalized_upc( { record => $record, schema =>
$schema } );
    $data->{$biblio->id}->{normalized_ean} =
$metadata_extractor->get_normalized_ean( { record => $record, schema =>
$schema } );
}

The key is that we are actually reusing the MARC::Record, and code for
each schema is organized cleanly so easier to maintain.

For the class names, I chose to add the 'MARC' name in the path, so we
don't need to refactor anything if we want to add support for another
serialization formats.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa -c 1
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 11 Jonathan Druart 2023-10-05 10:45:14 UTC
Created attachment 156566 [details] [review]
Bug 34828: Move to Koha::Biblio::Metadata::Extractor

We are not directly linked with the other Koha::Metadata* packages.
Better to isolate this under Koha::Biblio::Metadata (at least for now).
Comment 12 Jonathan Druart 2023-10-05 10:45:17 UTC
Created attachment 156567 [details] [review]
Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC

and inherit from it

The new usage is now:

1. With a Koha::Biblio object
my $extractor = Koha::Biblio::Metadata::Extractor->new({biblio => $biblio});
$extractor->get_normalized_upc;

or

2. With a MARC::Record
my $extractor = Koha::Biblio::Metadata::Extractor->new({metadata=> $biblio->metadata->record});
$extractor->get_normalized_upc;

Note that there are "Inconsistent hierarchy during C3 merge of class"
warnings raised by the QA script. We could remove them by replacing the
'use' by 'require' in Koha::Biblio::Metadata::Extractor::MARC (in ->new)
but that's suboptimal.
Comment 13 Jonathan Druart 2023-10-05 10:45:20 UTC
Created attachment 156568 [details] [review]
Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor
Comment 14 Jonathan Druart 2023-10-05 10:47:56 UTC
Hey, what do you think of this change?

Note that I don't think we should add this new concept/module/abstraction/complexity if there is no plan to move it further (i.e. a goal/roadmap we share to replace what is C4::Koha and Koha::Biblio that is parsing MARC::Record to get data).

Also note that bug 23235 become even more relevant now...
Comment 15 Tomás Cohen Arazi 2023-10-05 16:18:25 UTC
(In reply to Jonathan Druart from comment #14)
> Hey, what do you think of this change?

I like it. That's how I thought we could use it.

> Note that I don't think we should add this new
> concept/module/abstraction/complexity if there is no plan to move it further
> (i.e. a goal/roadmap we share to replace what is C4::Koha and Koha::Biblio
> that is parsing MARC::Record to get data).

It is done, I don't think having 'MARC' in the package name hurts, as of UNIMARC and MARC21, the different packages make sense, though.

> Also note that bug 23235 become even more relevant now...

Yeah, probably. Setting to NSO?
Comment 16 David Nind 2023-10-05 17:22:53 UTC
Created attachment 156575 [details] [review]
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc

This patch introduces a new pattern for the different ->get_<thing>
methods we've been adding. The aim is that code will look more like:

my $metadata_extractor = Koha::MetadataExtractor->new;

while ( my $biblio = $biblios->next ) {
    my $record = $biblio->record;
    my $schema = $biblio->record_schema;

    $data->{$biblio->id}->{normalized_upc} =
$metadata_extractor->get_normalized_upc( { record => $record, schema =>
$schema } );
    $data->{$biblio->id}->{normalized_ean} =
$metadata_extractor->get_normalized_ean( { record => $record, schema =>
$schema } );
}

The key is that we are actually reusing the MARC::Record, and code for
each schema is organized cleanly so easier to maintain.

For the class names, I chose to add the 'MARC' name in the path, so we
don't need to refactor anything if we want to add support for another
serialization formats.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa -c 1
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 David Nind 2023-10-05 17:22:56 UTC
Created attachment 156576 [details] [review]
Bug 34828: Move to Koha::Biblio::Metadata::Extractor

We are not directly linked with the other Koha::Metadata* packages.
Better to isolate this under Koha::Biblio::Metadata (at least for now).

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2023-10-05 17:22:59 UTC
Created attachment 156577 [details] [review]
Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC

and inherit from it

The new usage is now:

1. With a Koha::Biblio object
my $extractor = Koha::Biblio::Metadata::Extractor->new({biblio => $biblio});
$extractor->get_normalized_upc;

or

2. With a MARC::Record
my $extractor = Koha::Biblio::Metadata::Extractor->new({metadata=> $biblio->metadata->record});
$extractor->get_normalized_upc;

Note that there are "Inconsistent hierarchy during C3 merge of class"
warnings raised by the QA script. We could remove them by replacing the
'use' by 'require' in Koha::Biblio::Metadata::Extractor::MARC (in ->new)
but that's suboptimal.

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2023-10-05 17:23:02 UTC
Created attachment 156578 [details] [review]
Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2023-10-05 17:25:00 UTC
I've signed off, as the test plan works.

However, I don't really understand the changes as I'm not a developer - please feel free to change it back to needs sign off if that is required.
Comment 21 Tomás Cohen Arazi 2023-10-09 15:02:05 UTC
Created attachment 156743 [details] [review]
Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc

This patch introduces a new pattern for the different ->get_<thing>
methods we've been adding. The aim is that code will look more like:

my $metadata_extractor = Koha::MetadataExtractor->new;

while ( my $biblio = $biblios->next ) {
    my $record = $biblio->record;
    my $schema = $biblio->record_schema;

    $data->{$biblio->id}->{normalized_upc} =
$metadata_extractor->get_normalized_upc( { record => $record, schema =>
$schema } );
    $data->{$biblio->id}->{normalized_ean} =
$metadata_extractor->get_normalized_ean( { record => $record, schema =>
$schema } );
}

The key is that we are actually reusing the MARC::Record, and code for
each schema is organized cleanly so easier to maintain.

For the class names, I chose to add the 'MARC' name in the path, so we
don't need to refactor anything if we want to add support for another
serialization formats.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa -c 1
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2023-10-09 15:02:08 UTC
Created attachment 156744 [details] [review]
Bug 34828: Move to Koha::Biblio::Metadata::Extractor

We are not directly linked with the other Koha::Metadata* packages.
Better to isolate this under Koha::Biblio::Metadata (at least for now).

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2023-10-09 15:02:11 UTC
Created attachment 156745 [details] [review]
Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC

and inherit from it

The new usage is now:

1. With a Koha::Biblio object
my $extractor = Koha::Biblio::Metadata::Extractor->new({biblio => $biblio});
$extractor->get_normalized_upc;

or

2. With a MARC::Record
my $extractor = Koha::Biblio::Metadata::Extractor->new({metadata=> $biblio->metadata->record});
$extractor->get_normalized_upc;

Note that there are "Inconsistent hierarchy during C3 merge of class"
warnings raised by the QA script. We could remove them by replacing the
'use' by 'require' in Koha::Biblio::Metadata::Extractor::MARC (in ->new)
but that's suboptimal.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2023-10-09 15:02:15 UTC
Created attachment 156746 [details] [review]
Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2023-10-09 15:03:07 UTC
Thanks Jonathan and David^2. I really feel we now have a better path for future refactoring efforts.
Comment 26 Tomás Cohen Arazi 2023-10-09 19:42:16 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 27 Fridolin Somers 2023-10-10 06:45:07 UTC
Looks great.
Maybe there will be a plugin hook for this.

Enhancement not pushed to 23.05.x
Comment 28 Tomás Cohen Arazi 2023-10-10 11:34:38 UTC
Created attachment 156789 [details] [review]
Bug 34828: Remove useless variable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Jonathan Druart 2023-10-10 12:16:28 UTC
Created attachment 156791 [details] [review]
Bug 34828: Fix test
Comment 30 Tomás Cohen Arazi 2023-10-10 12:23:56 UTC
Follow-ups pushed to master. Thanks!