Bug 24432 - order_by broken for date columns
Summary: order_by broken for date columns
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Agustín Moyano
QA Contact:
URL:
Keywords:
Depends on: 23893
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-15 21:39 UTC by Agustín Moyano
Modified: 2020-05-06 23:15 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24432: Regresion test (1.99 KB, patch)
2020-01-15 22:54 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24432: Regresion test (2.11 KB, patch)
2020-01-15 23:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24432: add Koha::Objects->from_api_mapping (1.98 KB, patch)
2020-01-15 23:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24432: Use Koha::Objects->from_api_mapping instead of attributes_from_api to get the column name in Koha::REST::Plugins::Query->_build_order_atom (1.09 KB, patch)
2020-01-15 23:18 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24432: Regresion test (2.17 KB, patch)
2020-01-17 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24432: add Koha::Objects->from_api_mapping (2.03 KB, patch)
2020-01-17 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom (1.07 KB, patch)
2020-01-17 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24432: Regresion test (2.22 KB, patch)
2020-01-21 13:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24432: add Koha::Objects->from_api_mapping (2.09 KB, patch)
2020-01-21 13:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom (1.13 KB, patch)
2020-01-21 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2020-01-15 21:39:06 UTC
When you request an endpoint in the API with a date as orderby, it throws Koha::Exceptions::BadParameter.

Koha::REST::Plugin::Query in _build_order_atom, in order to get the model parameter for the order by, it requests 

{instance of Koha::Objects}->attributes_from_api({ <api parameter name> => 1 });

to get that hash translated, and use the resulting key as the model column to do the order by.

This works ok for all datatypes except date types, because it's trying to transate the value of "1" as a date, throwig the mentioned exception.

In my opinion, _build_order_atom should not use attributes_from_api, but should use the mapping in Koha::Object to get the column name.
Comment 1 Agustín Moyano 2020-01-15 22:54:25 UTC
Created attachment 97421 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.
Comment 2 Agustín Moyano 2020-01-15 23:18:35 UTC
Created attachment 97424 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.
Comment 3 Agustín Moyano 2020-01-15 23:18:38 UTC
Created attachment 97425 [details] [review]
Bug 24432: add Koha::Objects->from_api_mapping

This patch adds from_api_mapping to Koha::Objects, in order to be able to get the mapping from a result set.

To test:
1. apply this patch
2. prove t/db_dependent/Koha/Objects.t
3. sign off
Comment 4 Agustín Moyano 2020-01-15 23:18:42 UTC
Created attachment 97426 [details] [review]
Bug 24432: Use Koha::Objects->from_api_mapping instead of attributes_from_api to get the column name in Koha::REST::Plugins::Query->_build_order_atom

Use the mapping instead of building a fake hash only to get the key name.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t
3. sign off
Comment 5 Tomás Cohen Arazi 2020-01-17 14:31:42 UTC
Great catch!
Comment 6 Tomás Cohen Arazi 2020-01-17 14:37:03 UTC
Created attachment 97528 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2020-01-17 14:37:07 UTC
Created attachment 97529 [details] [review]
Bug 24432: add Koha::Objects->from_api_mapping

This patch adds from_api_mapping to Koha::Objects, in order to be able to get the mapping from a result set.

To test:
1. apply this patch
2. prove t/db_dependent/Koha/Objects.t
3. sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2020-01-17 14:37:10 UTC
Created attachment 97530 [details] [review]
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom

Use the mapping instead of building a fake hash only to get the key name.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t
3. sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Nick Clemens 2020-01-21 13:21:52 UTC
Created attachment 97660 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2020-01-21 13:21:56 UTC
Created attachment 97661 [details] [review]
Bug 24432: add Koha::Objects->from_api_mapping

This patch adds from_api_mapping to Koha::Objects, in order to be able to get the mapping from a result set.

To test:
1. apply this patch
2. prove t/db_dependent/Koha/Objects.t
3. sign off

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2020-01-21 13:22:00 UTC
Created attachment 97662 [details] [review]
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom

Use the mapping instead of building a fake hash only to get the key name.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t
3. sign off

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Martin Renvoize 2020-01-23 08:53:40 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Joy Nelson 2020-02-06 22:50:48 UTC
does not apply to 19.11.x
not backported