Bug 24191 - Sorting doesn't use to_model
Summary: Sorting doesn't use to_model
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-09 14:23 UTC by Tomás Cohen Arazi
Modified: 2020-06-04 20:37 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, 19.11.01, 19.05.07, 18.11.13


Attachments
Bug 24191: Regression tests (2.51 KB, patch)
2019-12-09 18:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting (6.05 KB, patch)
2019-12-09 18:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24191: Make objects.search pass to_model to dbic_merge_sorting (1.40 KB, patch)
2019-12-09 18:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24191: Regression tests (2.57 KB, patch)
2019-12-09 19:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting (6.11 KB, patch)
2019-12-09 19:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24191: Make objects.search pass to_model to dbic_merge_sorting (1.45 KB, patch)
2019-12-09 19:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24191: Regression tests (2.63 KB, patch)
2019-12-10 08:07 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting (6.16 KB, patch)
2019-12-10 08:07 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 24191: Make objects.search pass to_model to dbic_merge_sorting (1.51 KB, patch)
2019-12-10 08:07 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-12-09 14:23:56 UTC
There are no tests for sorting the results from objects.search (there are unit test for the query building code only) and it is not working as expected, as objects.search is not using to_model to map the attribute names.

To reproduce:
- Perform the following with your favourite tool:
  GET http://kohadev-intra.myDNSname.org:8081/api/v1/cities?_order_by=+name
=> FAIL: You get a DBIC error about 'name' not being a valid column name
Comment 1 Tomás Cohen Arazi 2019-12-09 18:50:34 UTC
Created attachment 96114 [details] [review]
Bug 24191: Regression tests

This patch adds missing tests for calling objects.search with
non-existent column names for sorting, that should be mapped using
to_model for that matter.

Tests should fail as there's no current use of to_model for building the
order_by portion of the query.

Sponsored-by: ByWater Solutions
Comment 2 Tomás Cohen Arazi 2019-12-09 18:50:37 UTC
Created attachment 96115 [details] [review]
Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting

This patch adds a to_model parameter to dbic_merge_sorting so it is
passed when used (for example from objects.search). The to_model param
is passed along to the _build_order_atom method where it is finally
used.

In the process I wrote tests that reflected some problems in the current
code:
- Mojolicious automatically returns a scalar if a query parameter only
happens once on a request. The code expected an arrayref in every case.
- There's a design issue that forced me to use some hacky code in
_build_order_atom.

The first issue is dealth with, by using Scalar::Util::reftype as the
Perl docs recommend.

The second issue, I don't plan to clean it here, as there's ongoing work
on a Koha::Objects->search_from_api method that will obsolete this code
most probably (see bug 23893 for a better picture of where the mappings
will be living soon).

To test:
1. Apply this patch
2. Run:
  $ kshell
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!!
3. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 3 Tomás Cohen Arazi 2019-12-09 18:50:40 UTC
Created attachment 96116 [details] [review]
Bug 24191: Make objects.search pass to_model to dbic_merge_sorting

This patch makes objects.search pass the to_mode method reference so it
can be used to map API attributes to column names when building the
sorting portion of the query.

To test:
1. Apply the regression tests on this patchset
2. Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail!
3. Apply the rest of the patchset
4. Run:
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! changes to the methods work as expected!
5. Run:
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
6. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 4 Tomás Cohen Arazi 2019-12-09 19:11:43 UTC
@RM I set this to major as it is a blocker for serious uses of the API pagination and sorting. Please feel free to set it back to normal!
Comment 5 Owen Leonard 2019-12-09 19:28:36 UTC
Created attachment 96117 [details] [review]
Bug 24191: Regression tests

This patch adds missing tests for calling objects.search with
non-existent column names for sorting, that should be mapped using
to_model for that matter.

Tests should fail as there's no current use of to_model for building the
order_by portion of the query.

Sponsored-by: ByWater Solutions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Owen Leonard 2019-12-09 19:28:38 UTC
Created attachment 96118 [details] [review]
Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting

This patch adds a to_model parameter to dbic_merge_sorting so it is
passed when used (for example from objects.search). The to_model param
is passed along to the _build_order_atom method where it is finally
used.

In the process I wrote tests that reflected some problems in the current
code:
- Mojolicious automatically returns a scalar if a query parameter only
happens once on a request. The code expected an arrayref in every case.
- There's a design issue that forced me to use some hacky code in
_build_order_atom.

The first issue is dealth with, by using Scalar::Util::reftype as the
Perl docs recommend.

The second issue, I don't plan to clean it here, as there's ongoing work
on a Koha::Objects->search_from_api method that will obsolete this code
most probably (see bug 23893 for a better picture of where the mappings
will be living soon).

To test:
1. Apply this patch
2. Run:
  $ kshell
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!!
3. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Owen Leonard 2019-12-09 19:28:41 UTC
Created attachment 96119 [details] [review]
Bug 24191: Make objects.search pass to_model to dbic_merge_sorting

This patch makes objects.search pass the to_mode method reference so it
can be used to map API attributes to column names when building the
sorting portion of the query.

To test:
1. Apply the regression tests on this patchset
2. Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail!
3. Apply the rest of the patchset
4. Run:
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! changes to the methods work as expected!
5. Run:
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
6. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Josef Moravec 2019-12-10 08:07:48 UTC
Created attachment 96133 [details] [review]
Bug 24191: Regression tests

This patch adds missing tests for calling objects.search with
non-existent column names for sorting, that should be mapped using
to_model for that matter.

Tests should fail as there's no current use of to_model for building the
order_by portion of the query.

Sponsored-by: ByWater Solutions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Josef Moravec 2019-12-10 08:07:52 UTC
Created attachment 96134 [details] [review]
Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting

This patch adds a to_model parameter to dbic_merge_sorting so it is
passed when used (for example from objects.search). The to_model param
is passed along to the _build_order_atom method where it is finally
used.

In the process I wrote tests that reflected some problems in the current
code:
- Mojolicious automatically returns a scalar if a query parameter only
happens once on a request. The code expected an arrayref in every case.
- There's a design issue that forced me to use some hacky code in
_build_order_atom.

The first issue is dealth with, by using Scalar::Util::reftype as the
Perl docs recommend.

The second issue, I don't plan to clean it here, as there's ongoing work
on a Koha::Objects->search_from_api method that will obsolete this code
most probably (see bug 23893 for a better picture of where the mappings
will be living soon).

To test:
1. Apply this patch
2. Run:
  $ kshell
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!!
3. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2019-12-10 08:07:56 UTC
Created attachment 96135 [details] [review]
Bug 24191: Make objects.search pass to_model to dbic_merge_sorting

This patch makes objects.search pass the to_mode method reference so it
can be used to map API attributes to column names when building the
sorting portion of the query.

To test:
1. Apply the regression tests on this patchset
2. Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> FAIL: Tests fail!
3. Apply the rest of the patchset
4. Run:
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! changes to the methods work as expected!
5. Run:
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
6. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Martin Renvoize 2019-12-10 16:18:31 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2019-12-20 15:49:14 UTC
Pushed to 19.11.01
Comment 13 Lucas Gass 2020-01-06 20:54:57 UTC
backported to 19.05.x for 19.05.07
Comment 14 Hayley Mapley 2020-01-09 22:13:05 UTC
Backported to 18.11.x for 18.11.13