28585 – Cannot search on date fields

Bug 28585 - Cannot search on date fields

Summary: Cannot search on date fields

Status:	CLOSED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	REST API (show other bugs)
Version:	unspecified
Hardware:	All All

Importance:	P5 - low major
Assignee:	Martin Renvoize (ashimema)
QA Contact:	Kyle M Hall (khall)

URL:
Keywords:

Depends on:
Blocks:

Reported:	2021-06-17 11:27 UTC by Hannamari Heiniluoma
Modified:	2022-06-06 20:27 UTC (History)
CC List:	5 users (show)

See Also:	28632 29157
Change sponsored?:	---
Patch complexity:	---
Documentation contact:
Documentation submission:
Text to go in the release notes:	This patch fixes the date handling for query parsing from the API. We use dt_from_string to convert out RFC3339 formatted date strings to DateTime objects with an associated timezone and then user the native datetime formatted provided by the SQL connection library to convert to an appropriately formated date time string.
Version(s) released in:	21.11.00,21.05.05
Circulation function:

Attachments
Bug 28585: Fix search on date field (928 bytes, patch) 2021-06-24 10:47 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 28585: Regression tests (2.41 KB, patch) 2021-06-25 14:53 UTC, Tomás Cohen Arazi (tcohen)	Details \| Diff \| Splinter Review
Bug 28585: Use the datetime_parser for handling API dates (1.92 KB, patch) 2021-06-25 14:53 UTC, Tomás Cohen Arazi (tcohen)	Details \| Diff \| Splinter Review
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters (2.25 KB, patch) 2021-06-25 14:53 UTC, Tomás Cohen Arazi (tcohen)	Details \| Diff \| Splinter Review
Bug 28585: Regression tests (2.46 KB, patch) 2021-06-25 23:32 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 28585: Use the datetime_parser for handling API dates (1.97 KB, patch) 2021-06-25 23:32 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters (2.30 KB, patch) 2021-06-25 23:32 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 28585: Regression tests (2.53 KB, patch) 2021-07-23 13:50 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
Bug 28585: Use the datetime_parser for handling API dates (2.04 KB, patch) 2021-07-23 13:51 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters (2.37 KB, patch) 2021-07-23 13:51 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
Bug 28585: Regression tests (2.57 KB, patch) 2021-10-21 15:18 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 28585: Use the datetime_parser for handling API dates (2.21 KB, patch) 2021-10-21 15:18 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters (2.42 KB, patch) 2021-10-21 15:18 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 28585: (QA follow-up) Fix object.t (2.95 KB, patch) 2021-10-21 15:19 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 28585: (QA follow-up) Add explicit tests for setting date/date-time attributes (3.92 KB, patch) 2021-10-21 18:36 UTC, Tomás Cohen Arazi (tcohen)	Details \| Diff \| Splinter Review
Show Obsolete (10) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Hannamari Heiniluoma 2021-06-17 11:27:56 UTC

Querying patrons by their date_of_birth returns an empty response, even though there are patrons with that date of birth in the database. Might be a bug?

Comment 1 Katrin Fischer 2021-06-19 11:39:36 UTC

Hi Hanna, could you include a sample for your query? Which date format was used?

Comment 2 Katrin Fischer 2021-06-19 11:39:58 UTC

(In reply to Katrin Fischer from comment #1)
> Hi Hanna, could you include a sample for your query? Which date format was
> used?

Sorry, Hannamari!

Comment 3 Hannamari Heiniluoma 2021-06-21 05:26:08 UTC

Hi!

My query was /patrons/?date_of_birth=2000-01-01. That returns status code 200 and an empty response []. If I change the date format, eg. 
/patrons/?date_of_birth=2000/01/01 (or any other format I can think of), I get 500 and 
{
    "error": "Something went wrong, check Koha logs for details."
}

Comment 4 Jonathan Druart 2021-06-24 10:44:10 UTC

DBIC is receiving
    dateofbirth   {                            
        like   "%1980-04-24T00:00:00%"                                                  
    }

Comment 5 Jonathan Druart 2021-06-24 10:47:34 UTC

Created attachment 122381 [details] [review]
Bug 28585: Fix search on date field

We should not rfc3339 format the date if it's not a datetime

Comment 6 Jonathan Druart 2021-06-24 10:48:00 UTC

Tomas, looks like this could be a fix. Do you see something else?

Comment 7 Tomás Cohen Arazi (tcohen) 2021-06-25 13:00:14 UTC

(In reply to Jonathan Druart from comment #6)
> Tomas, looks like this could be a fix. Do you see something else?

I wrote regression tests and this doesn't work, because it fails for date-time parameters. I'll take care.

Comment 8 Tomás Cohen Arazi (tcohen) 2021-06-25 14:53:21 UTC

Created attachment 122444 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Comment 9 Tomás Cohen Arazi (tcohen) 2021-06-25 14:53:25 UTC

Created attachment 122445 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Comment 10 Tomás Cohen Arazi (tcohen) 2021-06-25 14:53:29 UTC

Created attachment 122446 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Comment 11 Tomás Cohen Arazi (tcohen) 2021-06-25 15:17:03 UTC

I made the q= tests a follow-up, because I'm not sure bug 28480 will be backported to affected stable branches, and so ease backporting this important fix.

Comment 12 David Nind 2021-06-25 23:32:35 UTC

Created attachment 122457 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Comment 13 David Nind 2021-06-25 23:32:39 UTC

Created attachment 122458 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Comment 14 David Nind 2021-06-25 23:32:43 UTC

Created attachment 122459 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Comment 15 Martin Renvoize (ashimema) 2021-07-12 15:24:48 UTC

I think this will need some of the handling from Bug 24850 to work properly across timezones.

Comment 16 Kyle M Hall (khall) 2021-07-23 13:50:41 UTC

Created attachment 123115 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 17 Kyle M Hall (khall) 2021-07-23 13:51:00 UTC

Created attachment 123116 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 18 Kyle M Hall (khall) 2021-07-23 13:51:04 UTC

Created attachment 123117 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 19 Jonathan Druart 2021-07-26 08:43:19 UTC

Please add something for the release notes as well as on the wiki page.

Comment 20 Jonathan Druart 2021-07-26 09:24:05 UTC

Pushed to master for 21.11, thanks to everybody involved!

Comment 21 Jonathan Druart 2021-07-26 10:41:59 UTC

t/db_dependent/Koha/Object.t .. 9/21 
    #   Failed test 'Given a string, a timestamp field is converted into a DateTime object'
    #   at t/db_dependent/Koha/Object.t line 514.
    #          got: ''
    #     expected: 'DateTime'

    #   Failed test 'Given a string, a datetime field is converted into a DateTime object'
    #   at t/db_dependent/Koha/Object.t line 528.
    #          got: ''
    #     expected: 'DateTime'

    #   Failed test 'Given a string, a date field is converted into a DateTime object'
    #   at t/db_dependent/Koha/Object.t line 542.
    #          got: ''
    #     expected: 'DateTime'
    # Looks like you failed 3 tests of 12.

#   Failed test 'attributes_from_api() tests'
#   at t/db_dependent/Koha/Object.t line 576.


Tests added by 
  commit badbd7d85913749e33d9bdba397dc79836080719
  Bug 23893: Catch dt_from_string exceptions                                                         


Looks like we should remove the tests, do you have something else to suggest?

Comment 22 Jonathan Druart 2021-07-26 10:43:50 UTC

Can you also remind me why we are not using Koha::DateUtils instead of $dtf?
Is that because of bug 24850? If so, shouldn't we add a comment about replacing the line when 24850 will be pushed?

Comment 23 Jonathan Druart 2021-07-30 05:23:11 UTC

Patches reverted.

Comment 24 Martin Renvoize (ashimema) 2021-10-21 15:18:47 UTC

Created attachment 126668 [details] [review]
Bug 28585: Regression tests

This patch adds tests for filtering GET calls with date/date-time
parameters. Tests fail because the feature is not working

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Comment 25 Martin Renvoize (ashimema) 2021-10-21 15:18:51 UTC

Created attachment 126669 [details] [review]
Bug 28585: Use the datetime_parser for handling API dates

This patchset takes the GET /patrons route as a sample usage for filtering
on date and date-time (including timestamp) fields.

It then makes Koha::Object->attributes_from_api use the DB storage
datetime parser for format the parameters correctly.

To test:

1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: It doesn't find the patron when filtering by date
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works now!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Comment 26 Martin Renvoize (ashimema) 2021-10-21 15:18:55 UTC

Created attachment 126670 [details] [review]
Bug 28585: (follow-up) Tests for q= supporting date/date-time parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Comment 27 Martin Renvoize (ashimema) 2021-10-21 15:19:00 UTC

Created attachment 126671 [details] [review]
Bug 28585: (QA follow-up) Fix object.t

Object.t was still testing for the return of DateTime objects from
attributes_from_api. I checked all calls to attrbutes_from_api for
reliance of DateTime objects and confirmed they all get passed into
search calls and thus are better served as SQL formatted strings.

I then converted the test to check that the rfc3339 formatted dates
passed in were converted to strings appropriate for feeding into SQL
where statements instead.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Comment 28 Martin Renvoize (ashimema) 2021-10-21 15:22:04 UTC

(In reply to Jonathan Druart from comment #22)
> Can you also remind me why we are not using Koha::DateUtils instead of $dtf?
> Is that because of bug 24850? If so, shouldn't we add a comment about
> replacing the line when 24850 will be pushed?

It's pretty standard practice to use the DateTime parser returned by DBI for the connected database to format DateTime objects for SQL... I'm not even sure what function does the same in DateUtils?

Personally, I've used the dtf method all over the codebase.. it's what I reach for first when inputting a query.

Comment 29 Jonathan Druart 2021-10-21 15:42:54 UTC

FTR, discussed on IRC.

Seeing set_from_api calling attributes_from_api I hadn't notice we were doing something else than insert/update (and DBIC accepts DT object).

Here the method deals with search as well and so needs to format.

Comment 30 Jonathan Druart 2021-10-21 15:45:27 UTC

Are we certain we have a test for the "set a date using the REST API" case?

Comment 31 Martin Renvoize (ashimema) 2021-10-21 15:46:42 UTC

(In reply to Jonathan Druart from comment #30)
> Are we certain we have a test for the "set a date using the REST API" case?

I can't find one.. I'll look at writing one (or begging Tomas to write one) tomorrow.. brain is dead for today already ;)

Comment 32 Tomás Cohen Arazi (tcohen) 2021-10-21 15:54:24 UTC

Any PUT test on patrons is testing that implicitly. But it wouldn't hurt to add some explicit tests.

I'll take care in a bit.

Comment 33 Tomás Cohen Arazi (tcohen) 2021-10-21 18:36:06 UTC

Created attachment 126679 [details] [review]
Bug 28585: (QA follow-up) Add explicit tests for setting date/date-time attributes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Comment 34 Tomás Cohen Arazi (tcohen) 2021-10-21 18:43:34 UTC

I noticed I wrote this comment too fast, and maybe someone reading this back would benefit from a more detailed explanation, sorry for that :-D

(In reply to Tomás Cohen Arazi from comment #32)
> Any PUT test on patrons is testing that implicitly.

^^ This tried to mean that:

```
  my $json = $builder->build_object({ class => 'Koha::Patrons' })->to_api;
```

will have any date/date-time attribute filled with a random date/date-time (that's what TestBuilder does, and to_api converts to the correct format), which the POST and PUT tests already test when they do things like this:

```
   is_deeply($got, $newpatron, 'Returned patron from update matches expected');
```

Anyhow, we better have explicit tests for special things we want to highlight. Good catch, RM.

Comment 35 Martin Renvoize (ashimema) 2021-10-21 19:09:30 UTC

Ah, of course!  Nice explanation Tomas, I totally missed that deeply compare..

Glad to had an explicit test though too, just ensure we catch if should we ever change testbuilder in some way that reduces that implicit coverage.

Nice one

Comment 36 Jonathan Druart 2021-10-25 13:58:35 UTC

Pushed to master for 21.11, thanks to everybody involved!

Comment 37 Kyle M Hall (khall) 2021-10-29 12:35:08 UTC

Pushed to 21.05.x for 21.05.05

Comment 38 Fridolin Somers 2021-11-10 23:33:27 UTC

I tried to apply on 20.11.x but tests fail.
Can you try to rebase please ?