Bug 21105

Summary: oai.pl returns invalid earliestDatestamp
Product: Koha Reporter: Pablo AB <pablo.bianchi>
Component: Web servicesAssignee: Rudolf Byker <rudolfbyker>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: normal    
Priority: P5 - low CC: david, dcook, fridolin.somers, jonathan.druart, kyle, rudolfbyker, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17785
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29135
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the date format in OAI-PMH for Identify.earliestDatestamp so that it uses "YYYY-MM-DDThh:mm:ssZ" and is in UTC, instead of the SQL formsat "YYYY-MM-DD hh:mm:ss" currently used. For OAI-PMH all date and time values must be in the format "YYYY-MM-DDThh:mm:ssZ" and in UTC.
Version(s) released in:
21.11.00,21.05.05
Bug Depends on:    
Bug Blocks: 21102    
Attachments: Proposed patch
New patch. Fixed quote problem and tested.
Bug 21105 - Screenshots before and after patch applied
Screenshot of difference in raw XML
Bug 21105: oai.pl returns invalid earliestDatestamp
Bug 21105: oai.pl returns invalid earliestDatestamp
Bug 21105: Regression tests
Bug 21105: (QA follow-up) Silence warning because of tests setup
Bug 21105: (QA follow-up) Avoid MySQL-ism
Bug 21105: (follow-up) Regression tests
Bug 21105: Fix test if sample data missing
Bug 21105: Fix test if sample data missing
Bug 21105: Fix test if sample data missing

Description Pablo AB 2018-07-23 19:42:32 UTC
From: https://www.openarchives.org/OAI/openarchivesprotocol.html#DatestampsResponses
Dates should be YYYY-MM-DDThh:mm:ssZ
curl -s "baseURL?verb=Identify" | xmllint --format - | grep Date
responseDate -> 2018-07-23T18:34:15Z : OK
earliestDatestamp -> 2010-09-30 16:04:37 : FAIL

Originally deteced with official validator:
https://www.openarchives.org/Register/ValidateSite

Related: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17785#c13
Comment 1 Rudolf Byker 2018-10-08 15:22:05 UTC
This makes a HUGE difference for anyone not in a UTC timezone. I think this should be higher priority...
Comment 2 Rudolf Byker 2021-04-14 21:51:16 UTC
Currently, the code to get the earliestDatestamp looks like this:

earliestDatestamp   => _get_earliest_datestamp() || '0001-01-01T00:00:00Z',

and

sub _get_earliest_datestamp {
    my $dbh = C4::Context->dbh;
    my ( $earliest ) = $dbh->selectrow_array("SELECT MIN(timestamp) AS earliest FROM biblio" );
    return $earliest
}

There are two problems here:

1. From the MySQL docs: "MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back from UTC to the current time zone for retrieval." Similarly for MariaDB: "If a column uses the TIMESTAMP data type, then any inserted values are converted from the session's time zone to Coordinated Universal Time (UTC) when stored, and converted back to the session's time zone when retrieved." But we MUST get this as a UTC value according to the OAI-PMH spec (see http://www.openarchives.org/OAI/openarchivesprotocol.html#Identify and http://www.openarchives.org/OAI/openarchivesprotocol.html#Dates )

2. It's formatted in the default MySQL way, which is YYYY-MM-DD hh:mm:ss (local time) rather than YYYY-MM-DDThh:mm:ssZ (UTC).

I don't really know Perl, but I can suggest a SQL query that should fix the problem:

SELECT DATE_FORMAT(CONVERT_TZ(MIN(timestamp), 'SYSTEM', '+00:00'), '%Y-%m-%dT%H:%i:%SZ') AS earliest FROM biblio;

Example results:

Before:

+---------------------+
| earliest            |
+---------------------+
| 2012-11-03 13:41:32 |
+---------------------+

After:

+----------------------+
| earliest             |
+----------------------+
| 2012-11-03T11:41:32Z |
+----------------------+
Comment 3 David Cook 2021-04-14 23:37:49 UTC
Rudolf: It's just a formatting issue. If you examine your data, you'll see that the timestamp reported by oai.pl is in UTC.

This is thanks to some cleverness in "Koha/OAI/Server/Repository.pm" that sets the the time zone to UTC in the MySQL client session. (Thanks Ere :D)

So the following should be sufficient:

SELECT DATE_FORMAT(MIN(timestamp),'%Y-%m-%dT%H:%i:%SZ')  AS earliest FROM biblio;
Comment 4 Rudolf Byker 2021-04-15 07:57:43 UTC
Created attachment 119604 [details] [review]
Proposed patch
Comment 5 David Cook 2021-04-16 00:42:01 UTC
Thanks for writing the patch, Rudolf. 

Have you tested it out? I haven't tried it myself yet, but at a glance it contains a syntax error. You've used single quotes around the SQL, but the SQL contains single quotes, so it will bust.

Take a look at https://perldoc.perl.org/perlop#Quote-and-Quote-like-Operators. You could replace the outer single quotes with something like q{}.
Comment 6 Rudolf Byker 2021-04-16 06:22:57 UTC
Created attachment 119692 [details] [review]
New patch. Fixed quote problem and tested.

Sorry, I did not test the previous patch properly. Here is a new one. I tested it this time. It fixes the problem in the XML response, but it stills seems to be formatted as YYYY-MM-DD hh:mm:ss instead of YYYY-MM-DDThh:mm:ssZ in the browser. Not sure if that's a (separate) bug or a feature.
Comment 7 Katrin Fischer 2021-04-17 13:16:07 UTC
Setting back to Needs Sign-off as the comment from David has been taken care of in the new patch.
Comment 8 David Cook 2021-04-19 23:38:13 UTC
(In reply to Rudolf Byker from comment #6)
> Created attachment 119692 [details] [review] [review]
> New patch. Fixed quote problem and tested.
> 
> Sorry, I did not test the previous patch properly. Here is a new one. I
> tested it this time. It fixes the problem in the XML response, but it stills
> seems to be formatted as YYYY-MM-DD hh:mm:ss instead of YYYY-MM-DDThh:mm:ssZ
> in the browser. Not sure if that's a (separate) bug or a feature.

I think that it would be reasonable to handle it here.

Take a look at ./koha-tmpl/opac-tmpl/xslt/OAI.xslt. You'll see that the characters T and Z are being stripped out of the datestamps.
Comment 9 Rudolf Byker 2021-04-20 10:08:04 UTC
Do you mean that they are only stripped out when displayed in the browser? In that case, all is well.
Comment 10 David Cook 2021-04-20 23:38:44 UTC
(In reply to Rudolf Byker from comment #9)
> Do you mean that they are only stripped out when displayed in the browser?
> In that case, all is well.

Yes, the XSLT is sent in the browser response, and the browser applies the XSLT to the XML to generate a HTML document. That XSLT strips out the 'T' and the 'Z'.
Comment 11 Rudolf Byker 2021-06-01 19:45:15 UTC
Hi there. Who needs to sign off on this issue? Is there anything else I can do to help speed up the process?
Comment 12 Katrin Fischer 2021-06-01 21:16:37 UTC
(In reply to Rudolf Byker from comment #11)
> Hi there. Who needs to sign off on this issue? Is there anything else I can
> do to help speed up the process?

Hi Rudolf, at this stage, anyone can sign off - there is not much you can do, but maybe talk to people who might be interested in this patch got gain a sign off. Sometimes an email to the mailing list might help.
Comment 13 David Nind 2021-06-06 22:39:23 UTC
Hi Rudolf.

Could you write up a test plan so that I could test? This should be a step by step list of things to do, for example:

Test plan:
1. [Steps to replicate the issue]
2. Apply the patch
3. [Steps that indicate the issue is fixed, such as repeating steps in 1 and the result expected]

Here is a good example:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28408#c3

And also: https://wiki.koha-community.org/wiki/Commit_messages#Test_plan

Bearing in mind that those testing (like me) may not be developers or familiar with this area of Koha.

David Nind
Comment 14 Rudolf Byker 2021-10-13 13:41:47 UTC
Sorry for the late reply. The test plan is in the commit message. Here it is again:

1) Get a Koha instance with OAI-PMH enabled.
2) Visit /cgi-bin/koha/oai.pl?verb=Identify and observe earliestDate in wrong format "YYYY-MM-DD hh:mm:ss"
3) Apply patch
4) Visit /cgi-bin/koha/oai.pl?verb=Identify and observe earliestDate in correct format "YYYY-MM-DDThh:mm:ssZ"
5) Sign off
Comment 15 David Nind 2021-10-13 20:16:45 UTC
Created attachment 126236 [details]
Bug 21105 - Screenshots before and after patch applied

Thanks for the test plan!

I noticed no difference in the display before and after the patch was applied.
Comment 16 Rudolf Byker 2021-10-13 21:48:23 UTC
Created attachment 126240 [details]
Screenshot of difference in raw XML

That's because of the CSS. I should have been more specific: Disable CSS, or click on "View Page Source". Or use this validator, which also shows the raw XML, but pretty-printed: https://validator.oaipmh.com/ I attached my screenshot showing the difference in the raw XML.
Comment 17 David Nind 2021-10-13 22:33:37 UTC
Created attachment 126241 [details] [review]
Bug 21105: oai.pl returns invalid earliestDatestamp

For OAI-PMH, all date and time values must be in the format
"YYYY-MM-DDThh:mm:ssZ" and in UTC. Currently,
Identify.earliestDatestamp uses the SQL format "YYYY-MM-DD hh:mm:ss".
This patch fixes that.

To test:
1) Get a Koha instance with OAI-PMH enabled.
2) Visit /cgi-bin/koha/oai.pl?verb=Identify, view source for the page
   and observe that earliestDate is in the wrong format
   "YYYY-MM-DD hh:mm:ss"
3) Apply patch
4) Visit /cgi-bin/koha/oai.pl?verb=Identify, view the source for the page
   and observe that earliestDate is now in the correct format
   "YYYY-MM-DDThh:mm:ssZ"
5) Sign off

Sponsored-by: Reformational Study Centre

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2021-10-13 22:35:05 UTC
(In reply to Rudolf Byker from comment #16)
 
> That's because of the CSS. I should have been more specific: Disable CSS, or
> click on "View Page Source". Or use this validator, which also shows the raw
> XML, but pretty-printed: https://validator.oaipmh.com/ I attached my
> screenshot showing the difference in the raw XML.

Thanks Rudolf! Have signed off (have updated the patch to incorporate that).
Comment 19 Tomás Cohen Arazi 2021-11-11 23:05:11 UTC
Looking here. This requires regression tests, I'm writing them and will come back shortly.
Comment 20 Tomás Cohen Arazi 2021-11-12 14:37:37 UTC
Created attachment 127586 [details] [review]
Bug 21105: oai.pl returns invalid earliestDatestamp

For OAI-PMH, all date and time values must be in the format
"YYYY-MM-DDThh:mm:ssZ" and in UTC. Currently,
Identify.earliestDatestamp uses the SQL format "YYYY-MM-DD hh:mm:ss".
This patch fixes that.

To test:
1) Get a Koha instance with OAI-PMH enabled.
2) Visit /cgi-bin/koha/oai.pl?verb=Identify, view source for the page
   and observe that earliestDate is in the wrong format
   "YYYY-MM-DD hh:mm:ss"
3) Apply patch
4) Visit /cgi-bin/koha/oai.pl?verb=Identify, view the source for the page
   and observe that earliestDate is now in the correct format
   "YYYY-MM-DDThh:mm:ssZ"
5) Sign off

Sponsored-by: Reformational Study Centre

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2021-11-12 14:37:41 UTC
Created attachment 127587 [details] [review]
Bug 21105: Regression tests

This patch introduces a regression test for the fixed behavior.
I also changed a oneliner regex we had for stripping query parameters
because it was causing a warning with the tests base URL
(http://localhost) because it was generating an empty baseURL.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/OAI/Server.t
=> FAIL: Tests fail, the response is incorrect!
3. Apply the bug's patch
4. Repeat 2
=> SUCCESS: Tests pass! Good response!
Comment 22 Tomás Cohen Arazi 2021-11-12 14:37:46 UTC
Created attachment 127588 [details] [review]
Bug 21105: (QA follow-up) Silence warning because of tests setup

The tests are using 'http://localhost' and the regex is setting $baseURL
to empty in this case. It works on the oai.pl front-end.

This patch changes the regex.

To test:
1. Run:
   $ kshell
  k$ t/db_dependent/OAI/Server.t
=> FAIL: Boo! Warning!
2. Apply this patch
3. Repeat 2
=> SUCCESS: Oh, cool. No warning :-D
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2021-11-12 14:37:55 UTC
Created attachment 127589 [details] [review]
Bug 21105: (QA follow-up) Avoid MySQL-ism

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Jonathan Druart 2021-11-15 10:44:56 UTC
Created attachment 127628 [details] [review]
Bug 21105: (follow-up) Regression tests

Mock KohaAdminEmailAddress in case it differs from the default value
Comment 25 Jonathan Druart 2021-11-15 11:39:10 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 26 Jonathan Druart 2021-11-15 13:34:57 UTC
Created attachment 127644 [details] [review]
Bug 21105: Fix test if sample data missing

#   Failed test 'Identify - earliestDatestamp in the right format'
    #   at t/db_dependent/OAI/Server.t line 182.
    # Compared $data->{"Identify"}{"earliestDatestamp"}
    #    got : '2021-11-15T13:16:09Z'
    # expect : '2014-05-07T13:36:23Z'
Comment 27 Jonathan Druart 2021-11-15 13:35:49 UTC
(In reply to Jonathan Druart from comment #26)
> Created attachment 127644 [details] [review] [review]
> Bug 21105: Fix test if sample data missing
> 
> #   Failed test 'Identify - earliestDatestamp in the right format'
>     #   at t/db_dependent/OAI/Server.t line 182.
>     # Compared $data->{"Identify"}{"earliestDatestamp"}
>     #    got : '2021-11-15T13:16:09Z'
>     # expect : '2014-05-07T13:36:23Z'

Aren't we then missing the point of the test with this patch?
Comment 28 Jonathan Druart 2021-11-15 13:53:07 UTC
Created attachment 127650 [details] [review]
Bug 21105: Fix test if sample data missing

#   Failed test 'Identify - earliestDatestamp in the right format'
    #   at t/db_dependent/OAI/Server.t line 182.
    # Compared $data->{"Identify"}{"earliestDatestamp"}
    #    got : '2021-11-15T13:16:09Z'
    # expect : '2014-05-07T13:36:23Z'
Comment 29 Tomás Cohen Arazi 2021-11-15 14:00:06 UTC
Created attachment 127653 [details] [review]
Bug 21105: Fix test if sample data missing

    #   Failed test 'Identify - earliestDatestamp in the right format'
    #   at t/db_dependent/OAI/Server.t line 182.
    # Compared $data->{"Identify"}{"earliestDatestamp"}
    #    got : '2021-11-15T13:16:09Z'
    # expect : '2014-05-07T13:36:23Z'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Jonathan Druart 2021-11-15 14:07:22 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> Created attachment 127653 [details] [review] [review]
> Bug 21105: Fix test if sample data missing
> 
>     #   Failed test 'Identify - earliestDatestamp in the right format'
>     #   at t/db_dependent/OAI/Server.t line 182.
>     # Compared $data->{"Identify"}{"earliestDatestamp"}
>     #    got : '2021-11-15T13:16:09Z'
>     # expect : '2014-05-07T13:36:23Z'
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master.
Comment 31 Kyle M Hall 2021-11-19 16:35:12 UTC
Pushed to 21.05.x for 21.05.05
Comment 32 Fridolin Somers 2021-11-20 00:10:01 UTC
Does not apply on 20.11.x, fails on t/db_dependent/OAI/Server.t because of missing Bug 29135