Bug 10824 - OAI-PMH repository/server: Add handling of time in "from" argument
Summary: OAI-PMH repository/server: Add handling of time in "from" argument
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-05 04:55 UTC by David Cook
Modified: 2016-06-21 21:40 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this: 2013-10-30 00:00:00
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10824 - OAI-PMH repository/server not handling time in "from" argument (4.21 KB, patch)
2013-09-05 05:37 UTC, David Cook
Details | Diff | Splinter Review
Bug 10824 - OAI-PMH repository/server not handling time in "from" argument (4.21 KB, patch)
2013-09-29 23:44 UTC, David Cook
Details | Diff | Splinter Review
Bug 10824 - OAI-PMH repository/server not handling time in 'until' and 'from' (5.53 KB, patch)
2013-09-30 03:41 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10824 - OAI-PMH repository/server not handling time in 'until' and 'from' (5.63 KB, patch)
2015-05-18 00:29 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 10824 - OAI-PMH repository/server not handling time in 'until' and 'from' (5.68 KB, patch)
2015-05-22 14:29 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2013-09-05 04:55:12 UTC
Currently, if you try to use time in a "from" argument with the Koha OAI-PMH repository/server, you'll get unexpected results.

Case 1:
There are 3 records with the following timestamps:
2013-09-02 10:29:40
2013-09-05 13:44:33
2013-09-05 13:44:34

However, if you sent a request using "2013-09-02 8:29:40" or "2013-09-02T8:29:40Z" (the latter being the 'proper' way), you'll only retrieve the last 2 records of the 3 record set.

This is because we're using this code to check the "from" and "until" arguments:

"WHERE DATE(timestamp) >= ? AND DATE(timestamp) <= ?"

We need to strip DATE() from around the timestamps. 

If we just compare the normal timestamp, MySQL can handle properly compare the timestamp against the following strings:

"2013-09-02"
"2013-09-02 8:29:40"
"2013-09-02T8:29:40Z" (NB This one produces a warning (not an error) so it's less than ideal)

I'm proposing that we remove DATE() and include a regex to strip UTC designators T and Z.

This way, we'll always be passing in "2013-09-02" or "2013-09-02 8:29:40" which work perfectly.
Comment 1 David Cook 2013-09-05 05:37:47 UTC Comment hidden (obsolete)
Comment 2 I'm just a bot 2013-09-29 05:51:57 UTC
Patch applied cleanly, go forth and signoff
Comment 3 David Cook 2013-09-29 23:44:19 UTC Comment hidden (obsolete)
Comment 4 David Cook 2013-09-30 03:41:59 UTC Comment hidden (obsolete)
Comment 5 David Cook 2013-09-30 05:44:03 UTC
In hindsight, I suppose this could technically be called an enhancement, since using a granularity of YYYY-MM-DD instead of YYYY-MM-DDThh:mm:ssZ is valid.

Personally, I don't see why we don't use the finer granularity.
Comment 6 Srdjan Jankovic 2013-09-30 05:57:41 UTC
Not nitpicking or anything, just curious: why adding T...Z string just to strip it off? Wouldn't it be better to do something on the lines of

$arg = length($date_string) == 10
  ? ($date_string .= " $time_string") # 00:00:00 or 23:59:59
  : _strip_UTC_designators($date_string);
Comment 7 David Cook 2013-09-30 23:34:22 UTC
(In reply to Srdjan Jankovic from comment #6)
> Not nitpicking or anything, just curious: why adding T...Z string just to
> strip it off? Wouldn't it be better to do something on the lines of
> 
> $arg = length($date_string) == 10
>   ? ($date_string .= " $time_string") # 00:00:00 or 23:59:59
>   : _strip_UTC_designators($date_string);

Fair question.

I'm adding T...Z string, because it's only stripped off half the time. 

If you read through the code, you'll see that "from" and "until" properties retain the T and Z designators. It's only the "from_arg" and "until_arg" properties that have them stripped off.

The reasoning here is that the "from" and "until" properties are used to generate the actual resumption token, which requires the T and Z designators, while the "from_arg" and "until_arg" have them stripped off for matching in MySQL.

Does that make sense? We want the T and Z for the token, but no T and Z for the database.
Comment 8 I'm just a bot 2013-10-30 07:00:36 UTC
Patch applied cleanly, go forth and signoff
Comment 9 David Cook 2013-12-23 05:25:47 UTC
If this does get signed off and pushed, we'll probably want to update the OAI server config to include time in its granularity.
Comment 10 Bernardo Gonzalez Kriegel 2015-05-18 00:29:40 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2015-05-22 14:29:24 UTC
Created attachment 39416 [details] [review]
[PASSED QA] Bug 10824 - OAI-PMH repository/server not handling time in 'until' and 'from'

This patch removes the DATE() function from a query on timestamp, and adds a
sub that strips the UTC designators "T" and "Z" from incoming "from" and
"until" arguments in OAI-PMH requests so that they're more compliant with
MySQL (and probably other databases as well). This means that the date
and time for the 'from' and 'until' arguments will be matched correctly
in the database.

This patch also adds 'T00:00:00Z' to 'from' arguments and 'T23:59:59Z' to
until arguments, when only dates are provided via the OAI parameters.

The zero time isn't necessary, since MySQL treats '2013-09-30' as
'2013-09-30 00:00:00' by default. However, the near midnight time
is needed for 'until'. Otherwise, you'll never be able to retrieve
a record with a date/time matching the 'until' argument.

In summary, this patch adds handling for times as well as dates, which
is necessary so that Koha is closer to meeting the actual OAI-PMH spec.

TEST PLAN:

0) Note down a selection of timestamps from your biblio table

1) Enable your OAI-PMH server through the global system preferences
Web services tab.

2) Craft and submit a similar request to the following in your browser:
KOHAINSTANCE/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc&
from=2013-09-02T13:44:33Z&until=2013-09-05T13:44:33Z

Change the exact dates to accord with your timestamps, but keep the
YYYY-MM-DDTHH:MM:SSZ format.

3) Note the unexpected behaviour. A "from" argument with the timestamp
2013-09-02T13:44:33Z will show records from 2013-09-03 but not records
from 2013-09-02 even though the timestamp in the database will say
"2013-09-02 13:44:33".

Also note that records with a timestamp later than 13:44:33 will show
up for the day 2013-09-05, even though they shouldn't.

4) APPLY THE PATCH

5) Resubmit the links you tried above

6) Note that the applicable records now appear (or do not appear) in
accordance with the precise date/time ranges!

--

Developer Note: We could've not stripped the UTC designators and used
DATE() around the parameters in the SQL queries, but that would have
lost the whole purpose of using times in the "from" arguments, since
they would've been generalized to just the dates.

I think this is probably the best solution. Admittedly, creating
"form_arg" and "until_arg" hashrefs in the ResumptionToken object
might not be ideal, but I preferred that to copying the
_strip_UTC_designator subroutine into two other objects. Perhaps this
sub could go somewhere else and be imported into those other two objects
but this seemed to be the most sensible decision. I'm open to other
opinions though.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Works, find results with correct timestamp
No koha-qa errors

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2015-06-05 16:05:47 UTC
Enhancement pushed to master.

Nice one David!