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
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
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
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
@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!
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>
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>
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>
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>
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>
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>
Nice work everyone! Pushed to master for 20.05
Pushed to 19.11.01
backported to 19.05.x for 19.05.07
Backported to 18.11.x for 18.11.13