Bug 15108

Summary: OAI-PMH provider improvements
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: SearchingAssignee: Ere Maijala <ere.maijala>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: dcook, dhd.koha, ere.maijala, f.demians, jonathan.druart, josef.moravec, julian.maurice, katrin.fischer, kyle, m.de.rooy, mirko, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14939
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17493, 17778    
Bug Blocks: 15406, 19353, 21517    
Attachments: Bug 15108 - OAI-PMH provider improvements
Bug 15108 - OAI-PMH provider improvements. AtomicUpdate.
Bug 15108 - OAI-PMH provider improvements
OAI-PMH provider improvements
Bug 15108 - OAI-PMH provider improvements: - Fixed date handling to use UTC as specs require. - Added support for second precision in time stamps. - Added support for marc21 metadata prefix as recommended in the guidelines (synonym for marcxml). - L
Bug 15108 - OAI-PMH provider improvements:
Remove Koha::AtomicUpdater dependency
Bug 15108 - OAI-PMH provider improvements
Fix OAI Server tests
Fix OAI Server tests
Bug 15108 - OAI-PMH provider improvements
Remove Koha::AtomicUpdater dependency
Bug 15108: Fix OAI Server tests
Bug 15108: [QA Follow-up] Remove the sleep
Bug 15108 - OAI-PMH provider improvements
Remove Koha::AtomicUpdater dependency
Bug 15108: Fix OAI Server tests
Bug 15108: [QA Follow-up] Remove the sleep
Bug 15108: [QA Follow-up] Remove truncate
Bug 15108: [QA Follow-up] Remove the sleep
Bug 15108 - OAI-PMH provider improvements
Bug 15108: Remove Koha::AtomicUpdater dependency
Bug 15108: Fix OAI Server tests
Bug 15108: [QA Follow-up] Remove truncate
Bug 15108: [QA Follow-up] Remove the sleep
Bug 15108: [QA Follow-up] Changes from removing collection wrapper
Bug 15108: [QA Follow-up] Serious doubts on ResumptionToken::deleted
Bug 15108 - OAI-PMH provider improvements
[SIGNED-OFF] Bug 15108 - OAI-PMH provider improvements
Fixed tests to be more robust and also work with UNIMARC.
Bug 15108 - OAI-PMH provider improvements
Bug 15108 - OAI-PMH provider improvements
Bug 15108: OAI-PMH provider improvements
Bug 15108: (followup) OAI-PMH provider improvements
Bug 15108: (QA followup) Fix atomic update file

Description Olli-Antti Kivilahti 2015-11-02 15:50:06 UTC
- Fixed date handling to use UTC as specs require.
- Added support for deleted records.
- Added support for second precision in time stamps.
- Added support for marc21 metadata prefix as recommended in the guidelines (synonym for marcxml).
- ListIdentifiers and ListRecords always return records in biblionumber order.
- Improved performance of database queries especially for large collections.
Comment 1 Frédéric Demians 2015-11-02 16:44:16 UTC
Do you plan to work on this? Support for deleted record is already there in master.
Comment 2 Olli-Antti Kivilahti 2015-11-02 16:50:10 UTC
Hi! 

We got somebody from the national library to take a look at the OAI-PMH-implementation of Koha, and the Bugzilla entries didn't help a lot in deciphering what features have been implemented since 3.16.

I will provide the commit as soon as I receive the improved version of this patch.
Comment 3 Frédéric Demians 2015-11-02 17:25:05 UTC
> We got somebody from the national library to take a look at the
> OAI-PMH-implementation of Koha, and the Bugzilla entries didn't help a lot
> in deciphering what features have been implemented since 3.16.
> 11
> I will provide the commit as soon as I receive the improved version of this
> patch.

Please work on the top of bug 14939. Signing it would even been better. Having a modularized OAI server will help a lot in term of maintainability.
Comment 4 Olli-Antti Kivilahti 2015-11-03 09:04:50 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2015-11-03 09:05:05 UTC Comment hidden (obsolete)
Comment 6 Olli-Antti Kivilahti 2015-11-03 09:07:22 UTC
(In reply to Frédéric Demians from comment #3)
> > We got somebody from the national library to take a look at the
> > OAI-PMH-implementation of Koha, and the Bugzilla entries didn't help a lot
> > in deciphering what features have been implemented since 3.16.
> > 11
> > I will provide the commit as soon as I receive the improved version of this
> > patch.
> 
> Please work on the top of bug 14939. Signing it would even been better.
> Having a modularized OAI server will help a lot in term of maintainability.

I agree. Modularity is awesome.
Having complete test coverage is even better.
Comment 7 Olli-Antti Kivilahti 2015-11-03 09:07:37 UTC
Failing QA because of missing tests
Comment 8 Frédéric Demians 2015-11-03 09:31:25 UTC
> I agree. Modularity is awesome.
> Having complete test coverage is even better.

Now, there isn't any test at all now. So modularizing is a first step. If you want tests, add them. And don't forget UT for you own patch which modify a lot of OAI server code without guaranty it won't something somewhere.
Comment 9 Olli-Antti Kivilahti 2015-11-03 09:34:24 UTC
(In reply to Frédéric Demians from comment #8)
> > I agree. Modularity is awesome.
> > Having complete test coverage is even better.
> 
> Now, there isn't any test at all now. So modularizing is a first step. If
> you want tests, add them. And don't forget UT for you own patch which modify
> a lot of OAI server code without guaranty it won't something somewhere.

Just sharing the code here in hopes of it being helpful.
Comment 10 Olli-Antti Kivilahti 2015-11-03 09:40:16 UTC
And you should make tests before you refactor.
Comment 11 Frédéric Demians 2015-11-03 09:57:46 UTC
(In reply to Olli-Antti Kivilahti from comment #10)
> And you should make tests before you refactor.

You're welcome.

It's modularizing not refactoring. The existing classes are split from oai.pl into individual .pm files, one per class. It doesn't change a line of code. So coding a full UT for OAI server would be awesome, I can't disagree, but it isn't a requirement for this patch.
Comment 12 Jonathan Druart 2016-02-02 11:19:26 UTC
Olli, Could you provide a rebased patch?
It seems that bug 15406 would need the second patch.
Comment 13 Frédéric Demians 2016-02-02 11:34:06 UTC
(In reply to Jonathan Druart from comment #12)
> Olli, Could you provide a rebased patch?
> It seems that bug 15406 would need the second patch.

And this part of the 1st patch:

diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql
index e9d836c..c2eec84 100644
--- a/installer/data/mysql/kohastructure.sql
+++ b/installer/data/mysql/kohastructure.sql
@@ -209,6 +209,7 @@ CREATE TABLE `biblioitems` ( -- information related to bibliographic records in
   KEY `isbn` (`isbn`(255)),
   KEY `issn` (`issn`(255)),
   KEY `publishercode` (`publishercode`),
+  KEY `timestamp_bibno` (`timestamp`, `biblionumber`),
   CONSTRAINT `biblioitems_ibfk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
@@ -866,7 +867,8 @@ CREATE TABLE `deletedbiblioitems` ( -- information about bibliographic records t
   KEY `bibnoidx` (`biblionumber`),
   KEY `itemtype_idx` (`itemtype`),
   KEY `isbn` (`isbn`(255)),
-  KEY `publishercode` (`publishercode`)
+  KEY `publishercode` (`publishercode`),
+  KEY `timestamp_bibno` (`timestamp`, `biblionumber`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
Comment 14 Olli-Antti Kivilahti 2016-02-08 13:53:04 UTC
Hi!

We are upgrading our KohaSuomi branch (with 300 custom commits) built on top of Koha 3.16 to master this summer.
I heard that it is "impossible".

Looking forward to doing it with our soon-to-be continuous integration environment :)

When this is ready, this patch is also rebased.

Feel free to do it sooner.
Comment 15 Ere Maijala 2016-06-02 11:04:05 UTC Comment hidden (obsolete)
Comment 16 Ere Maijala 2016-06-02 11:05:28 UTC
It would be great if others could try this out and see if it works for them.
Comment 17 Katrin Fischer 2016-06-10 06:01:33 UTC
Hi Ere, status here is still Failed QA - so testers most likely won't see this. If you feel this is testable with current master, please switch to 'needs signoff'

As others have pointed out, we already have support for deleted records. Could you take another look and maybe update your commit message? It would also be good to have a test plan. Ideally all those changes should be splitted into separate bugs to make them easier to manage.
Comment 18 Frédéric Demians 2016-06-15 09:38:15 UTC
(In reply to Ere Maijala from comment #16)
> It would be great if others could try this out and see if it works for them.

It looks very good, Ere! How did you generate your patch? It seems you haven't do it with Git. You should...
Comment 19 Ere Maijala 2016-06-15 09:59:09 UTC
It's definitely done with a simple git diff command. Let me know if you need it done with specific parameters or something.
Comment 20 Katrin Fischer 2016-06-15 10:25:31 UTC
Hi Ere, it's missing the information about you as the author and such - that's usually the first lines. Take a look at git bz for our usual workflow: 
https://wiki.koha-community.org/wiki/Git_bz_configuration
If you don't want to get that far yet, take a look at the user.name  and user.email settings in git and the git format-patch command (https://wiki.koha-community.org/wiki/Version_Control_Using_Git)
Comment 21 Ere Maijala 2016-06-15 10:45:46 UTC Comment hidden (obsolete)
Comment 22 Ere Maijala 2016-06-15 10:46:56 UTC
Thanks again for pointing that out. I hope I've now managed to attach a patch properly.
Comment 23 Katrin Fischer 2016-06-15 11:45:35 UTC
Hi Ere, looking good! One more thing: please attach a current test plan/description of the changes - best to include it in the commit message of your patch already. And one more important thing: First thing in the subject line should always be "Bug XXXXX". Thx!
Comment 24 Ere Maijala 2016-06-15 12:05:22 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2016-06-15 12:07:39 UTC
Thx!
Comment 26 Ere Maijala 2016-06-15 12:08:13 UTC Comment hidden (obsolete)
Comment 27 Ere Maijala 2016-06-15 12:09:02 UTC
OKay, I've done my best. Sorry for the messy comments!
Comment 28 Frédéric Demians 2016-06-15 12:51:38 UTC
It seems that this patch depends on bug 14698.
Comment 29 Katrin Fischer 2016-06-15 12:55:59 UTC
Hi Frederic, that's ok - not really a dependency. The RM moves the sql file into updatedatabase on pushing the enh. This way we avoid constant rebasing and it's easier on the sanboxes.
Comment 30 Frédéric Demians 2016-06-15 14:44:29 UTC
(In reply to Katrin Fischer from comment #29)
> Hi Frederic, that's ok - not really a dependency. The RM moves the sql file
> into updatedatabase on pushing the enh. This way we avoid constant rebasing
> and it's easier on the sanboxes.

Ok. For testers (what I'm doing now), it requires tweaking the atomic .pl file in order to be able to update the DB. But I agree it's not that difficult...
Comment 31 Frédéric Demians 2016-06-15 15:19:29 UTC
> - Fixed date handling to use UTC as specs require.
> - Added support for second precision in time stamps.
> - Added support for marc21 metadata prefix as recommended in the guidelines
> (synonym for marcxml).
> - ListIdentifiers and ListRecords always return records in timestamp order
> (faster than biblionumber order with the new index).
> - Improved performance of database queries especially for large collections.

All good. I can see the SQL improvements, and immediate response time
improvement.

> - Unified functionality of ListRecords and ListIdentifiers to a common base
> class.

Good catch. It was stupid repeating convoluted code and queries in two
places.

> - If items are included in the records, their timestamps are taken into
> account everywhere so that whichever is the most recent (timestamp of
> biblioitem or any of its items) is considered the record's timestamp.

I approve this. It can be interesting to send back to the harvester items info
when items are modified in Koha. But it could also have a huge impact on the
quantity of records returned by incremental harvesting. Any circulation
transaction modify items.timestamp. So for libraries doing a lot of
circulation, harvester will have to harvest a lot of records just for the 'due
date' field or other circulation related fields. In some scenario, it may be
interesting to include items (include_items parameter) in order to have
homebranch and call number, but availability info is not required, so
resending the whole biblio+items when a transaction is done is not desirable.
I can't see any solution without adding a new 'datemodified' field to items
table, which would only by updated when non circulation-related info are
updated. 

Have you considered this point? Have you evaluated the impact of including
items with a Koha doing a lot of circulation?
Comment 32 Katrin Fischer 2016-06-15 15:29:10 UTC
Hm, I think in some cases it would be nice to include updates on available - I wouldn't rule that out. Maybe the decision to take the item change into account could be configurable?
Comment 33 Katrin Fischer 2016-06-15 15:36:09 UTC
(In reply to Frédéric Demians from comment #30)
> (In reply to Katrin Fischer from comment #29)
> > Hi Frederic, that's ok - not really a dependency. The RM moves the sql file
> > into updatedatabase on pushing the enh. This way we avoid constant rebasing
> > and it's easier on the sanboxes.
> 
> Ok. For testers (what I'm doing now), it requires tweaking the atomic .pl
> file in order to be able to update the DB. But I agree it's not that
> difficult...

Hm, if it's done right it should be enough to run updatedatabase.pl from commandline - it should run the atomic updates then automatically.
Comment 34 Ere Maijala 2016-06-16 06:56:06 UTC
(In reply to Frédéric Demians from comment #31)
> > - If items are included in the records, their timestamps are taken into
> > account everywhere so that whichever is the most recent (timestamp of
> > biblioitem or any of its items) is considered the record's timestamp.
> 
> I approve this. It can be interesting to send back to the harvester items
> info
> when items are modified in Koha. But it could also have a huge impact on the
> quantity of records returned by incremental harvesting. Any circulation
> transaction modify items.timestamp. So for libraries doing a lot of
> circulation, harvester will have to harvest a lot of records just for the
> 'due
> date' field or other circulation related fields. In some scenario, it may be

I did consider it but I left it out for now since from our point of view any change in the data that's included in a harvested record should make the record be reharvested since you can't know what data the harvester finds important.

That said, if you think there should be an option to control whether item timestamps are taken into account, I can add that. But you're right in that if you'd like to only ignore status changes caused by circulation actions, that's not possible without changes deeper in the system.

Additionally we need to take into account floating collections that would cause item's location to change on checkin. I'm really an outsider to Koha (mainly a VuFind developer), so I'm not sure how everything works in practice, so I apologize if what I say doesn't make sense or use the correct terminology.
Comment 35 Frédéric Demians 2016-06-16 11:06:00 UTC
(In reply to Katrin Fischer from comment #32)

> Hm, I think in some cases it would be nice to include updates on available -
> I wouldn't rule that out. Maybe the decision to take the item change into
> account could be configurable?

You're correct. IMO there are three cases:

(1) I don't want to send item info to harvesters. The item info are not needed
or are retrieved later using Koha webservices. This could be achieved with
include_items=0.

(2) I want to send all item info to harvester, ie descriptive info (branch,
call number, etc.) and circulation info (due date). This could be achieved
with include_items=1. With Ere patch, those info are resent to harvester each
time an item record is modified, or if a new item is added to an existing
biblio record. It wasn't the case until now, which is a bug from my
perspective.

(3) I want to send to harvester item info, but just 'descriptive' info. I want
that because I don't need availability info for example, or because it will be
retrieved later via webservices. With the OAI-PMH protocol, the availability
can't be provided to third party tools (say VuFind) in real time: webservices
are needed. But items.homebranch could be very useful in a metacatalog
populated via harvesting. And in this case, real time is not a requirement. So
this mixed mode could make sense for some. Especially to avoid bringing a Koha
server to its knee.

(In reply to Ere Maijala from comment #34)

> That said, if you think there should be an option to control whether item
> timestamps are taken into account, I can add that. But you're right in that
> if you'd like to only ignore status changes caused by circulation actions,
> that's not possible without changes deeper in the system.

I suspect that such an option would just add confusion. For scenario (3)
described above, a new patch should be needed.

But I was just curious whether you was able to measure the number of OAI-PMH
requests to Koha on a large catalog with a lot of circulation.

> Additionally we need to take into account floating collections that would
> cause item's location to change on checkin. I'm really an outsider to Koha
> (mainly a VuFind developer), so I'm not sure how everything works in
> practice, so I apologize if what I say doesn't make sense or use the correct
> terminology.

Floating collections modify item records, and so timestamp field. So you UNION
request will work.
Comment 36 Ere Maijala 2016-06-16 11:13:14 UTC
(In reply to Frédéric Demians from comment #35)
> > That said, if you think there should be an option to control whether item
> > timestamps are taken into account, I can add that. But you're right in that
> > if you'd like to only ignore status changes caused by circulation actions,
> > that's not possible without changes deeper in the system.
> 
> I suspect that such an option would just add confusion. For scenario (3)
> described above, a new patch should be needed.
> 
> But I was just curious whether you was able to measure the number of OAI-PMH
> requests to Koha on a large catalog with a lot of circulation.

I haven't had a chance to measure this in a live production system. It is estimated that for a large installation we may end up harvesting records in the scale of tens of thousand per day. However, we currently update the VuFind index every half an hour, so the amount of records that change per an update is not as large.

When considering scenario (3), please keep in mind that item addition and deletion are also important events. If you don't check item timestamps, you won't get e.g. the locations for any newly added items.
Comment 37 Katrin Fischer 2016-06-17 14:38:32 UTC
Hm, reading through the use cases Frederic wrote I wonder if it wouldn't work for a first step if we say that the item changes should only be taken into account if you have the add_items flag set. And then we could open a separate bug for an enhanced mode, where only some changes lead to 'republishing' the record via OAI.
Comment 38 Katrin Fischer 2016-06-17 16:39:49 UTC
include_items option ... is what I meant :)
Comment 39 Ere Maijala 2016-06-20 07:11:51 UTC
(In reply to Katrin Fischer from comment #37)

That makes sense to me.
Comment 40 Frédéric Demians 2016-08-01 17:03:23 UTC
Created attachment 53853 [details] [review]
Remove Koha::AtomicUpdater dependency

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Comment 41 Frédéric Demians 2016-08-01 17:04:29 UTC
Created attachment 53854 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Comment 42 Marcel de Rooy 2016-10-10 11:44:43 UTC
(In reply to Frédéric Demians from comment #8)
> > Having complete test coverage is even better.
> 
> Now, there isn't any test at all now. So modularizing is a first step. If
> you want tests, add them. And don't forget UT for you own patch which modify
> a lot of OAI server code without guaranty it won't something somewhere.

Frederic, the absence of unit tests in this patch set might be a reason for these patches getting stuck in the SO queue.
Comment 43 David Cook 2016-10-21 06:37:45 UTC
(In reply to Marcel de Rooy from comment #42)
> Frederic, the absence of unit tests in this patch set might be a reason for
> these patches getting stuck in the SO queue.

Jonathan just pointed me at this bug...

I've been mocking an OAI server very simply using Test::TCP and Net::Server::HTTP, so that I can test my OAI client.

The responses are all hard-coded at the moment, and it doesn't check the incoming requests besides a few verbs, but it's a start. It's 5:33pm on a Friday here, so I don't have time to flesh this out at the moment.

However, here's the bit of code I'm using:

my $server = Test::TCP->new(
    code => sub {
        my $port = shift;
        Net::Server::HTTP->run(
            log_level => 0,
            port => [$port],
            app => {
                '/' => sub {
                    my ($server) = @_;
                    my $request = $server->http_request_info;
                    my @uri_strings = ();
                    my @uri_string =  map { $_ if defined $_ } ($request->{request_protocol},"://",$request->{request_path},$request->{query_string});
                    my $uri_string = join("",@uri_string);
                    # my $uri_string = join($request->{request_protocol},"://",$request->{request_path},$request->{query_string});
                    my $uri = URI->new($uri_string);
                    my %query_parameters = $uri->query_form;
                    print "Content-type: text/xml\n\n";
                    if (%query_parameters){
                        if ($query_parameters{verb} eq "Identify"){
                            print "$identify_xml\n";
                        }
                        elsif ($query_parameters{verb} eq "GetRecord"){
                            print "$getrecord_xml\n";
                        }
                        elsif ($query_parameters{verb} eq "ListRecords"){
                            print "$listrecords_xml\n";
                        }
                    }
                    else {
                        print "$error_xml\n";
                    }
                },
            },
        );
    },
);

I imagine it should be fairly easy to test the Koha::OAI::Server modules by building off this pattern. 

Call $server->port to get the port to which the test server has bound, and you're able to connect to the server using localhost.
Comment 44 Frédéric Demians 2016-10-24 07:45:56 UTC
> I've been mocking an OAI server very simply using Test::TCP and
> Net::Server::HTTP, so that I can test my OAI client.

db_dependent/OAI/Server.t already mocks Koha OAI Server. But tests are to be extended...
Comment 45 Frédéric Demians 2016-10-24 16:37:43 UTC Comment hidden (obsolete)
Comment 46 Frédéric Demians 2016-10-25 16:37:46 UTC
Created attachment 56844 [details] [review]
Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.
Comment 47 Marcel de Rooy 2016-12-09 09:50:02 UTC
QA: Looking here now. Rebasing first.
Comment 48 Marcel de Rooy 2016-12-09 10:15:16 UTC
Created attachment 58058 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Comment 49 Marcel de Rooy 2016-12-09 10:15:21 UTC
Created attachment 58059 [details] [review]
Remove Koha::AtomicUpdater dependency

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Comment 50 Marcel de Rooy 2016-12-09 10:15:26 UTC
Created attachment 58060 [details] [review]
Bug 15108: Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.
Comment 51 Marcel de Rooy 2016-12-09 10:16:06 UTC
Just a rebase to begin with. Note that we had merge markers in one of those patches too.
Comment 52 Marcel de Rooy 2016-12-09 10:29:03 UTC
Created attachment 58061 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

If we order by biblionumber within the same timestamp, there is no
need to wait two seconds. Test is 20 seconds faster too. (Especially the
last 2 seconds were very annoying :-)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2016-12-09 10:48:20 UTC
Created attachment 58062 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Comment 54 Marcel de Rooy 2016-12-09 10:48:24 UTC
Created attachment 58063 [details] [review]
Remove Koha::AtomicUpdater dependency

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

EDIT:
Extension should be .perl [Otherwise it will not be executed at all.]
This file is just temporary, needs no license.
Followed the new skeleton file here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2016-12-09 10:48:28 UTC
Created attachment 58064 [details] [review]
Bug 15108: Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.
Comment 56 Marcel de Rooy 2016-12-09 10:48:33 UTC
Created attachment 58065 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

If we order by biblionumber within the same timestamp, there is no
need to wait two seconds. Test is 20 seconds faster too. (Especially the
last 2 seconds were very annoying :-)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2016-12-09 10:49:01 UTC
Only adjusted dbrev now.
Comment 58 Marcel de Rooy 2016-12-09 10:59:00 UTC
Hm I have serious doubts about my last fourth patch. Will the adjusted order by undo the performance gain from the new indexes?
Comment 59 Marcel de Rooy 2016-12-09 11:07:59 UTC
Comment on attachment 58065 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

Obsoleting it. We need to make the test more intelligent some other way.
Comment 60 Marcel de Rooy 2016-12-09 12:35:32 UTC
Truncate operations cause an implicit commit, and so cannot be rolled back. 
I discovered that now too in my test database..
Will adjust the statement :)
Comment 61 Marcel de Rooy 2016-12-09 13:17:49 UTC
Created attachment 58072 [details] [review]
Bug 15108: [QA Follow-up] Remove truncate

Truncate operations cause an implicit commit, and so cannot be rolled back.
So we need to replace it by a DELETE and adjust the code so that it
does not assume numbering biblionumbers from 1, etc.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2016-12-09 13:30:46 UTC
Created attachment 58080 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

Ordering by timestamp, biblionumber in ListBase.pm c/would be a
performance killer.
In order to speed up the test, we manipulate the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2016-12-09 13:32:28 UTC
QA: Unfortunately I cannot finish this QA session now. Will resume it later next week. Please wait :)
Comment 64 Marcel de Rooy 2016-12-16 07:13:53 UTC
QA: Resuming today.
Comment 65 Marcel de Rooy 2016-12-16 11:02:54 UTC
Created attachment 58245 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- ListIdentifiers and ListRecords always return records in timestamp
  order (faster than biblionumber order with the new index).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Comment 66 Marcel de Rooy 2016-12-16 11:02:58 UTC
Created attachment 58246 [details] [review]
Bug 15108: Remove Koha::AtomicUpdater dependency

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

EDIT:
Extension should be .perl [Otherwise it will not be executed at all.]
This file is just temporary, needs no license.
Followed the new skeleton file here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Marcel de Rooy 2016-12-16 11:03:03 UTC
Created attachment 58247 [details] [review]
Bug 15108: Fix OAI Server tests

Tests introduced by bug 17493 must be amended to accomodate the new
resumption tokens (with time) returned by OAI Server.
Comment 68 Marcel de Rooy 2016-12-16 11:03:08 UTC
Created attachment 58248 [details] [review]
Bug 15108: [QA Follow-up] Remove truncate

Truncate operations cause an implicit commit, and so cannot be rolled back.
So we need to replace it by a DELETE and adjust the code so that it
does not assume numbering biblionumbers from 1, etc.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 69 Marcel de Rooy 2016-12-16 11:03:12 UTC
Created attachment 58249 [details] [review]
Bug 15108: [QA Follow-up] Remove the sleep

Ordering by timestamp, biblionumber in ListBase.pm c/would be a
performance killer.
In order to speed up the test, we manipulate the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2016-12-16 11:03:17 UTC
Created attachment 58250 [details] [review]
Bug 15108: [QA Follow-up] Changes from removing collection wrapper

See bug 17744.
Comment 71 Marcel de Rooy 2016-12-16 11:03:21 UTC
Created attachment 58251 [details] [review]
Bug 15108: [QA Follow-up] Serious doubts on ResumptionToken::deleted

ListBase: Remove a comma construction by two statements.
ResumptionToken: Tidy sub deleted, added FIXME.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2016-12-16 11:08:35 UTC
The refactoring to ListBase looks good.
But although I spent hours on this patch set already, I am not yet convinced that we are ready to go.

Please respond to my doubts on the sub deleted (see commit message).
Please check this too:
Running aoi with
marcxml/50/1970-01-01T00:00:00Z/2016-12-16T23:59:59Z//1
starts with the first record (not expected !)
Running
marcxml/50/1970-01-01T00:00:00Z/2016-12-16T23:59:59Z//0
starts with record 51 (as expected)

If you propose further changes, please add a follow-up in order to keep sight of the changes.

Failed QA for now
Comment 73 Marcel de Rooy 2016-12-16 11:11:43 UTC
One further note (from QA tools):
FAIL   Koha/OAI/Server/ListIdentifiers.pm
FAIL   Koha/OAI/Server/ListRecords.pm
   FAIL   valid
                Can't locate package HTTP::OAI::PartialList for @HTTP::OAI::ListIdentifiers::ISA

This seems to be a problem in HTTP::OAI::ListIdentifiers ?
Comment 74 Frédéric Demians 2016-12-16 11:13:27 UTC
Thanks for your work Marcel.
Comment 75 Josef Moravec 2017-01-13 11:41:37 UTC
Needs rebase due to bug 17778
Comment 76 Ere Maijala 2017-02-14 13:50:09 UTC
I'm finally able to get back to this. Thanks everyone for the work you've done meanwhile. I'll try to rebase and address any issues soon.
Comment 77 Ere Maijala 2017-02-15 14:09:51 UTC
Created attachment 60260 [details] [review]
Bug 15108 - OAI-PMH provider improvements

This patch incorporates the extended test from bug 17493 and all other patches (if I didn't mess up anything!). It also fixes a record range display problem in OAI.xslt and reverts the order back to biblionumber to avoid a situation where a record update could have changed the order of records and cause records to be skipped.

This also fixes the QA problem with PartialList not found, removes the broken and unused deleted method from ResumptionToken, fixes tests and fixes resumptionToken cursor handling etc. compared to the previous patch. 

Here's the complete change list compared to master branch:

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.
Comment 78 Josef Moravec 2017-02-16 12:27:27 UTC
Created attachment 60331 [details] [review]
[SIGNED-OFF] Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.

- Incorporated extended tests from Bug 17493 and their tweaks from Bug 15108.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 79 Marcel de Rooy 2017-02-16 12:38:52 UTC
Ere: Did you move all tests here now ?
Comment 80 Ere Maijala 2017-02-16 13:19:55 UTC
Marcel: Yes, this patch should include all the tests from Bug 17493. If there were even more somewhere, then I've missed them.
Comment 81 Marcel de Rooy 2017-02-20 07:41:48 UTC
(In reply to Ere Maijala from comment #80)
> Marcel: Yes, this patch should include all the tests from Bug 17493. If
> there were even more somewhere, then I've missed them.

OK I closed 17493
Comment 82 Julian Maurice 2017-02-23 09:04:04 UTC
The tests fail for me:

1. If I have data in the issues table I get an error :

> DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key
> constraint fails (`koha`.`issues`, CONSTRAINT `issues_ib fk_2` FOREIGN KEY
> (`itemnumber`) REFERENCES `items` (`itemnumber`) ON UPDATE CASCADE) [for
> Statement "DELETE FROM biblio"] at t/db_dependent/OAI/Server.t line 62.
This can be easily fixed by deleting all issues before deleting all biblios

2. Biblio records are MARC21 (245$a for the title), so, as a UNIMARC user I get nothing in dc:title (tests "ListRecords oai_dc ...").

3. I don't know if this one is related to MARC21/UNIMARC but I also get nothing in dc:language (hashref expected) (tests "ListRecords oai_dc ...")

Result of prove here: http://paste.koha-community.org/325
Comment 83 Ere Maijala 2017-02-23 11:45:00 UTC
Marcel: Was there a reason for removing the deletion of issues in your patch in Bug 17493?

I suppose the issue with UNIMARC is that the OAI-PMH provider does take it into account while the test doesn't.

I'll see if I can fix these issues with a follow-up patch to the test.
Comment 84 Marcel de Rooy 2017-02-23 12:13:28 UTC
(In reply to Ere Maijala from comment #83)
> Marcel: Was there a reason for removing the deletion of issues in your patch
> in Bug 17493?
> 
> I suppose the issue with UNIMARC is that the OAI-PMH provider does take it
> into account while the test doesn't.
> 
> I'll see if I can fix these issues with a follow-up patch to the test.

Not sure anymore (seen too much code in the meantime).
Guess it was not needed..
Comment 85 Ere Maijala 2017-02-23 12:20:33 UTC
I suppose that would have been the case with a clean database. I'll revise the tests a bit to assume it's not clean (delete issues and oai_sets at least).
Comment 86 Ere Maijala 2017-02-23 12:51:43 UTC
Created attachment 60608 [details] [review]
Fixed tests to be more robust and also work with UNIMARC.

Added a patch to improve the server tests:
- delete issues before biblios
- delete oai_sets to avoid having them in the response
- create UNIMARC test records when UNIMARC is enabled
- include language code in the records to avoid failures caused by minor differences in how a missing language code is handled in OAI_DC transformation of different MARC flavors.
Comment 87 Olli-Antti Kivilahti 2017-03-06 11:36:04 UTC
Created attachment 60835 [details] [review]
Bug 15108 - OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.

- Incorporated extended tests from Bug 17493 and their tweaks from Bug 15108.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 88 Olli-Antti Kivilahti 2017-03-06 11:36:32 UTC
Created attachment 60836 [details] [review]
Bug 15108 - OAI-PMH provider improvements

Fixed OAI-PMH Server tests to delete any existing issues before deleting biblios, to delete oai_sets to avoid sets in the responses and to work with UNIMARC and NORMARC as well as MARC 21.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 89 Tomás Cohen Arazi 2017-05-05 14:51:54 UTC
*** Bug 15406 has been marked as a duplicate of this bug. ***
Comment 90 Tomás Cohen Arazi 2017-05-05 15:00:48 UTC
Created attachment 63172 [details] [review]
Bug 15108: OAI-PMH provider improvements

- Fixed date handling to use UTC as specs require.

- Added support for second precision in time stamps.

- Added support for marc21 metadata prefix as recommended in the
  guidelines (synonym for marcxml).

- Improved performance of database queries especially for large
  collections.

- Unified functionality of ListRecords and ListIdentifiers to a common
  base class.

- If items are included in the records, their timestamps are taken into
  account everywhere so that whichever is the most recent (timestamp of
  biblioitem or any of its items) is considered the record's timestamp.

- Fixed OAI.xslt to show correct record range.

- Incorporated extended tests from Bug 17493 and their tweaks from Bug 15108.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 91 Tomás Cohen Arazi 2017-05-05 15:01:11 UTC
Created attachment 63173 [details] [review]
Bug 15108: (followup) OAI-PMH provider improvements

Fixed OAI-PMH Server tests to delete any existing issues before deleting biblios,
to delete oai_sets to avoid sets in the responses and to work with UNIMARC and
NORMARC as well as MARC21.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 92 Tomás Cohen Arazi 2017-05-05 15:01:22 UTC
Created attachment 63174 [details] [review]
Bug 15108: (QA followup) Fix atomic update file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 93 Tomás Cohen Arazi 2017-05-05 15:05:24 UTC
This patchset is a really cool improvement to our OAI server implementation. The patches themselves are not organized as we are pushing for, but the overall result (covered by more tests) and the speed improvement (plus the small fixes) are a good point, and the fact that Frederic has been looking after this one (I talked him about it personally) gives me the confidence that this is a good addition for the 17.05 release.

Good job, Ere :-D
Comment 94 Kyle M Hall 2017-05-08 13:34:15 UTC
Pushed to master for 17.05, thanks Ere, Tomas!
Comment 95 Katrin Fischer 2017-05-14 09:26:23 UTC
I think part of this would well qualify as bug fixes, but overall it's a big patch and I am not confident pushing it to the stable version. What do people think?
Comment 96 Marcel de Rooy 2017-05-15 07:35:31 UTC
One of the OAI unit tests seems to have an issue with transactions.
Comment 97 Marcel de Rooy 2017-05-15 07:58:53 UTC
(In reply to Marcel de Rooy from comment #96)
> One of the OAI unit tests seems to have an issue with transactions.

No, it is Sets.t and it is not in this patch set.
Comment 98 Katrin Fischer 2017-07-16 12:17:23 UTC
This won't get ported back to 16.11.x as it is an enhancement.