Bug 24809 - OAI PMH can fail on fetching deleted records
Summary: OAI PMH can fail on fetching deleted records
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 12252
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-05 10:53 UTC by Nick Clemens
Modified: 2020-11-30 21:47 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10, 18.11.16


Attachments
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted (1.66 KB, patch)
2020-03-05 11:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted (1.71 KB, patch)
2020-03-08 21:49 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted (2.05 KB, patch)
2020-03-13 15:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted (2.14 KB, patch)
2020-03-20 10:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24809: (QA follow-up) Add prefix to prevent disambiguity (999 bytes, patch)
2020-03-20 10:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-03-05 10:53:47 UTC
When calling GetRecord if we pass a deleted biblionumber that has a deleted item we can end up thinking a record that is deleted is not deleted and so fail on getting the record

The problem is here:
 48             UNION
 49             SELECT timestamp from deleteditems
 50             WHERE biblionumber=?

If you delete an item from a record, then delete the record, the first query gets a timestamp and then attempts to get the record as if it is not deleted


To recreate:
1 - Edit/create a file in the koha home dir '/var/lib/koha/{instance}/OAI.yaml
2 - Set sysprefs:
  OAI-PMH => Enable
  OAI-PMH:ConfFile => /var/lib/koha/{instace}/OAI.yaml
3 - View a record with items and confirm the items show:
  http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:4
4 - Delete the items, then delete the record
5 - Reload the URL from step 3
6 - Internal Server Error:
    Empty String at /kohadevbox/koha/Koha/OAI/Server/Record.pm line 58.
 at /usr/share/perl/5.24/Carp.pm line 167
Comment 1 Nick Clemens 2020-03-05 11:02:00 UTC
Created attachment 100178 [details] [review]
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted

To test:
 1 - Edit/create a file in the koha home dir '/var/lib/koha/{instance}/OAI.yaml
 2 - Set sysprefs:
  OAI-PMH => Enable
  OAI-PMH:ConfFile => /var/lib/koha/{instace}/OAI.yaml
 3 - View a record with items and confirm the items show:
  http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:4
 4 - Delete the items, then delete the record
 5 - Reload the URL from step 3
 6 - Internal Server Error:
  Empty String at /kohadevbox/koha/Koha/OAI/Server/Record.pm line 58.
    at /usr/share/perl/5.24/Carp.pm line 167
 7 - Apply patch
 8 - restart_all
 9 - Repeat
10 - Reload URL from step 3
11 - Success
Comment 2 Michal Denar 2020-03-08 21:49:55 UTC
Created attachment 100317 [details] [review]
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted

To test:
 1 - Edit/create a file in the koha home dir '/var/lib/koha/{instance}/OAI.yaml
 2 - Set sysprefs:
  OAI-PMH => Enable
  OAI-PMH:ConfFile => /var/lib/koha/{instace}/OAI.yaml
 3 - View a record with items and confirm the items show:
  http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:4
 4 - Delete the items, then delete the record
 5 - Reload the URL from step 3
 6 - Internal Server Error:
  Empty String at /kohadevbox/koha/Koha/OAI/Server/Record.pm line 58.
    at /usr/share/perl/5.24/Carp.pm line 167
 7 - Apply patch
 8 - restart_all
 9 - Repeat
10 - Reload URL from step 3
11 - Success

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 3 Marcel de Rooy 2020-03-13 15:30:31 UTC
I am not able to reproduce this problem.
Probably a config issue. The commit message is incomplete?
Where do I insert that items should be included? I now only see the biblio record and have the impression that the yaml file is ignored.
Should I add the created yaml file in the prefs? etc. If I add     include_items: 1 and add the yaml file in the pref, I am seeing error: Dissemination as 'marcxml' is not supported.

Please provide more info.
Comment 4 Marcel de Rooy 2020-03-13 15:41:39 UTC
'/var/lib/koha/master/OAI.yaml' is empty or non-existent at /usr/lib/x86_64-linux-gnu/perl5/5.24/YAML/Syck.pm line 129.
Comment 5 Marcel de Rooy 2020-03-13 15:43:25 UTC
ok, copied something from bug 12252 (create a yaml file is not very helpful in this message here)
Comment 6 Marcel de Rooy 2020-03-13 15:44:48 UTC
ah reproduced finally:
Empty String at /usr/share/koha/Koha/OAI/Server/Record.pm line 58.
Comment 7 Marcel de Rooy 2020-03-13 15:52:51 UTC
Still not getting the point of the change completely:

        $sql .= "
            UNION
            SELECT deleteditems.timestamp FROM deleteditems JOIN biblio USING (biblionumber)
            WHERE biblionumber=?
            UNION
            SELECT timestamp from items
            WHERE biblionumber=?
        ";

Ambiguous now for biblionumber when adding the left join?
Why do you add the left join if you only want deleteditems ?

Please provide feedback.
Comment 8 Nick Clemens 2020-03-13 15:56:36 UTC
Created attachment 100687 [details] [review]
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted

To test:
 1 - Edit/create a file in the koha home dir '/var/lib/koha/{instance}/OAI.yaml' with content like below:
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 the systempreferences below to indicated values:
  OAI-PMH => Enable
  OAI-PMH:ConfFile => /var/lib/koha/{instace}/OAI.yaml
 3 - View a record with items and confirm the items show:
  http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:4
 4 - Delete the items, then delete the record
 5 - Reload the URL from step 3
 6 - Internal Server Error:
  Empty String at /kohadevbox/koha/Koha/OAI/Server/Record.pm line 58.
    at /usr/share/perl/5.24/Carp.pm line 167
 7 - Apply patch
 8 - restart_all
 9 - Repeat
10 - Reload URL from step 3
11 - Success

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 9 Nick Clemens 2020-03-13 16:14:34 UTC
(In reply to Marcel de Rooy from comment #7)
> Still not getting the point of the change completely:
> 
>         $sql .= "
>             UNION
>             SELECT deleteditems.timestamp FROM deleteditems JOIN biblio
> USING (biblionumber)
>             WHERE biblionumber=?
>             UNION
>             SELECT timestamp from items
>             WHERE biblionumber=?
>         ";

The goal of the query is the get the latest change for a record and return undef if record is deleted. We need to check deleted items because deleting an item will not update the biblioitems timestamp. We need to limit to deleted items where the bib is not deleted. 

> Ambiguous now for biblionumber when adding the left join?1
> Why do you add the left join if you only want deleteditems ?

Where is the LEFT JOIN?
Comment 10 Marcel de Rooy 2020-03-20 09:44:48 UTC
Revisiting
Comment 11 Marcel de Rooy 2020-03-20 10:28:11 UTC
(In reply to Nick Clemens from comment #9)
> Where is the LEFT JOIN?

Scratch that. It is an INNER join.
Comment 12 Marcel de Rooy 2020-03-20 10:34:06 UTC
Created attachment 101066 [details] [review]
Bug 24809: Get only timestamp of deleted items attached to existing bibs when check if bib deleted

To test:
 1 - Edit/create a file in the koha home dir '/var/lib/koha/{instance}/OAI.yaml' with content like below:
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 the systempreferences below to indicated values:
  OAI-PMH => Enable
  OAI-PMH:ConfFile => /var/lib/koha/{instace}/OAI.yaml
 3 - View a record with items and confirm the items show:
  http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:4
 4 - Delete the items, then delete the record
 5 - Reload the URL from step 3
 6 - Internal Server Error:
  Empty String at /kohadevbox/koha/Koha/OAI/Server/Record.pm line 58.
    at /usr/share/perl/5.24/Carp.pm line 167
 7 - Apply patch
 8 - restart_all
 9 - Repeat
10 - Reload URL from step 3
11 - Success

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2020-03-20 10:34:12 UTC
Created attachment 101067 [details] [review]
Bug 24809: (QA follow-up) Add prefix to prevent disambiguity

This seems not to be needed. Just at the safe side.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Martin Renvoize 2020-03-20 15:43:46 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 15 Joy Nelson 2020-03-31 22:20:25 UTC
Backported to 19.11.x branch for 19.11.05
Comment 16 Lucas Gass 2020-04-08 15:09:41 UTC
backported to 19.05.x for 19.05.10
Comment 17 Lucas Gass 2020-04-08 15:10:00 UTC
backported to 19.05.x for 19.05.10
Comment 18 Hayley Pelham 2020-04-14 03:11:24 UTC
Backported to 18.11.x for 18.11.16