Bug 29135 - OAI should not include biblionumbers from deleteditems when determining deletedbiblios
Summary: OAI should not include biblionumbers from deleteditems when determining delet...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Ere Maijala
QA Contact: Testopia
URL:
Keywords:
Depends on: 27584
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-29 16:26 UTC by Nick Clemens
Modified: 2021-11-20 00:10 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05


Attachments
Bug 29135: Fix handling of deleted items in OAI-PMH provider (20.46 KB, patch)
2021-09-30 19:53 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 29135: Fix handling of deleted items in OAI-PMH provider (22.28 KB, patch)
2021-10-01 07:41 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 29135: Fix handling of deleted items in OAI-PMH provider (22.28 KB, patch)
2021-10-01 09:16 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 29135: Fix handling of deleted items in OAI-PMH provider (22.28 KB, patch)
2021-10-04 10:15 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 29135: Fix handling of deleted items in OAI-PMH provider (22.32 KB, patch)
2021-10-05 03:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 29135: Fix handling of deleted items in OAI-PMH provider (22.38 KB, patch)
2021-10-08 12:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29135: Fix handling of deleted items in OAI-PMH provider (23.20 KB, patch)
2021-10-11 06:17 UTC, Ere Maijala
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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