Bug 29135

Summary: OAI should not include biblionumbers from deleteditems when determining deletedbiblios
Product: Koha Reporter: Nick Clemens <nick>
Component: Architecture, internals, and plumbingAssignee: Ere Maijala <ere.maijala>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: david, dcook, ere.maijala, fridolin.somers, jonathan.druart, kyle
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21105
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30143
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05
Bug Depends on: 27584    
Bug Blocks: 30143    
Attachments: Bug 29135: Fix handling of deleted items in OAI-PMH provider
Bug 29135: Fix handling of deleted items in OAI-PMH provider
Bug 29135: Fix handling of deleted items in OAI-PMH provider
Bug 29135: Fix handling of deleted items in OAI-PMH provider
Bug 29135: Fix handling of deleted items in OAI-PMH provider
Bug 29135: Fix handling of deleted items in OAI-PMH provider
Bug 29135: Fix handling of deleted items in OAI-PMH provider

Description Nick Clemens 2021-09-29 16:26:01 UTC
When including items in OAI we add the biblionumbers of deleted items into the deleted list

This is from this line:
 93         if ( $include_items )  {
 94             $sql = "($sql) UNION (SELECT DISTINCT(biblionumber) FROM deleteditems $criteria)";
 95             push @bind_params, @part_bind_params;


This is wrong because deletion of a single item does not indicate a record is deleted

To recreate:
1 - Create /var/lib/koha/kohadev/OAI.yaml with:
format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1

2 - Set  OAI-PMH:ConfFile to '/var/lib/koha/kohadev/OAI.yaml'
3 - Delete an item from a record
4 - View the page: http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
5 - Note the record is now listed as deleted
Comment 1 David Cook 2021-09-29 23:34:09 UTC
Good catch.

I think there's some additional relevant lines lower down in Koha/OAI/Server/ListBase.pm as well when the max timestamp is fetched. 

$sql = "
    SELECT MAX(timestamp)
    FROM (
        SELECT timestamp FROM deletedbiblio_metadata WHERE biblionumber = ?
        UNION
        SELECT timestamp FROM deleteditems WHERE biblionumber = ?
    ) bis
";

I mean it should be impossible to deleted an item after the bib has been deleted. But this SQL would return a timestamp where there is a deleted item but not a deleted bib.
Comment 2 David Cook 2021-09-29 23:39:03 UTC
(In reply to David Cook from comment #1)
> Good catch.
> 
> I think there's some additional relevant lines lower down in
> Koha/OAI/Server/ListBase.pm as well when the max timestamp is fetched. 
> 

Koha/OAI/Server/GetRecord.pm has that same weird logic, although it would just be a performance degradation rather than a wrong classification...
Comment 3 Ere Maijala 2021-09-30 07:40:07 UTC
Oh no. It seems that I made a terrible mistake here. However, it's not that simple. The timestamp of a deleted item is still relevant for a live biblio. The latest timestamp of any change, be that change or deletion, must be used as the timestamp since everything affects what's in the returned record. So it's not as simple as removing the statements for non-deleted records.

I'll take this with a high priority.
Comment 4 Ere Maijala 2021-09-30 18:13:30 UTC
And yes, you're both right about the deleted records. I had missed the most logical shortcut that a deleted biblio can't have items deleted later. I think I have a fix which mostly just makes things more simple, but I'm still working on adding tests to cover all cases.
Comment 5 Ere Maijala 2021-09-30 19:53:56 UTC
Created attachment 125570 [details] [review]
Bug 29135: Fix handling of deleted items in OAI-PMH provider

Test plan:
1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0
2. Run prove -v t/db_dependent/OAI/
Comment 6 Ere Maijala 2021-09-30 19:55:35 UTC
This should fix the issues. I haven't had a chance to manually test much yet, but at least the tests added failed miserably without the changes.
Comment 7 Jonathan Druart 2021-10-01 07:39:01 UTC
Thanks a lot, Ere, for your reactivity!
Comment 8 Ere Maijala 2021-10-01 07:41:48 UTC
Created attachment 125576 [details] [review]
Bug 29135: Fix handling of deleted items in OAI-PMH provider

Test plan:
1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0
2. Run prove -v t/db_dependent/OAI/
Comment 9 Ere Maijala 2021-10-01 07:44:59 UTC
Jonathan, no problem, it was my bad anyway.. I've added a couple more tests. I'll run a harvesting job to verify.
Comment 10 Ere Maijala 2021-10-01 09:16:34 UTC
Created attachment 125591 [details] [review]
Bug 29135: Fix handling of deleted items in OAI-PMH provider

Test plan:
1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0
2. Run prove -v t/db_dependent/OAI/
Comment 11 Ere Maijala 2021-10-01 12:55:38 UTC
Test harvesting completed successfully and record counts match.
Comment 12 David Nind 2021-10-01 18:28:01 UTC
For me the tests fail:

not ok 33 - Tests for timestamp handling
#   Failed test 'Tests for timestamp handling'
#   at t/db_dependent/OAI/Server.t line 849.
# Looks like you failed 1 test of 33.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/33 subtests 


Testing notes (koha-testing-docker started with ku):
- Enable OAI-PMH
- Before the patch is replied there is an incomplete item record listed at the end of the metadata section for the item deleted, after the patch nothing is shown for the deleted item, for example:

 <datafield ind2="" tag="952" ind1="">
      <subfield code="0">0</subfield>
      <subfield code="1">0</subfield>
      <subfield code="4">0</subfield>
      <subfield code="6">_</subfield>
      <subfield code="7">0</subfield>
      <subfield code="8">REF</subfield>
      <subfield code="9">313</subfield>
      <subfield code="a">FPL</subfield>
      <subfield code="b">FPL</subfield>
      <subfield code="c">GEN</subfield>
      <subfield code="d">2014-09-04</subfield>
      <subfield code="p">39999000006117</subfield>
      <subfield code="r">2014-09-04</subfield>
      <subfield code="w">2014-09-04</subfield>
      <subfield code="y">BK</subfield>
Comment 13 Ere Maijala 2021-10-04 10:15:01 UTC
Created attachment 125688 [details] [review]
Bug 29135: Fix handling of deleted items in OAI-PMH provider

Test plan:
1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0
2. Run prove -v t/db_dependent/OAI/
Comment 14 Ere Maijala 2021-10-04 10:17:56 UTC
David, 

Oops, silly me. I somehow thought it was clever that I made the timestamps start from an hour ago. This was fine until you created a dev box and ran the tests in an hour.. Should be fixed now.
Comment 15 David Nind 2021-10-05 03:22:38 UTC
8-)
Comment 16 David Nind 2021-10-05 03:23:37 UTC
Created attachment 125718 [details] [review]
Bug 29135: Fix handling of deleted items in OAI-PMH provider

Test plan:
1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0
2. Run prove -v t/db_dependent/OAI/

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Nick Clemens 2021-10-08 12:34:31 UTC
Created attachment 125955 [details] [review]
Bug 29135: Fix handling of deleted items in OAI-PMH provider

Test plan:
1. Make sure this scenario now works properly: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135#c0
2. Run prove -v t/db_dependent/OAI/

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Jonathan Druart 2021-10-08 14:58:39 UTC
Can we get a proper commit message please?
Comment 19 Ere Maijala 2021-10-11 06:17:48 UTC
Created attachment 126029 [details] [review]
Bug 29135: Fix handling of deleted items in OAI-PMH provider

When the OAI-PMH provider was configured to include item information
in the returned records, deleting an item would cause the record to
be reported as deleted. The provider also did some useless checks when
determining the timestamp of a deleted record. These checks only had
a performance impact.

To test:
1) Create /var/lib/koha/kohadev/OAI.yaml with:

format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1

2) Set preference OAI-PMH to Enable
3) Set preference OAI-PMH:ConfFile to /var/lib/koha/kohadev/OAI.yaml
4) Delete an item from a record
5) View the page: http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
6) Note the record is now listed as deleted
7) Run prove -v t/db_dependent/OAI/

Sponsored-by: The National Library of Finland

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Ere Maijala 2021-10-11 06:18:55 UTC
Commit message revised.
Comment 21 Jonathan Druart 2021-10-12 09:19:04 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 22 Kyle M Hall 2021-10-22 13:42:32 UTC
Pushed to 21.05.x for 21.05.05
Comment 23 Fridolin Somers 2021-10-23 01:34:19 UTC
Depends on Bug 27584 not in 20.11.x