We should extend Koha::Objects so searching can deal with squery parameters, specially handling pagination, results sorting, matching criteria, etc. This could be easily implemented using: - Bug 19196 (Pagination handling helpers) - Bug 19278 (Make REST api default page size configurable) - Bug 19234 (Query params handling helpers) - Bug 19369 (Query pagination to SQL::Abstract) - Bug 19370 (Query order by to SQL::Abstract)
Created attachment 67613 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks!
Created attachment 67731 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks!
Created attachment 67732 [details] [review] Bug 19410: Unit tests
Created attachment 67750 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks!
Created attachment 67751 [details] [review] Bug 19410: Unit tests
Created attachment 67795 [details] [review] Bug 19410: (follow-up) Fix typo in POD
Created attachment 67798 [details] [review] Bug 19410: (follow-up) Fix typo in POD
It feels like search_for_api would fit better as a Mojo helper, given that it mostly call other helpers : $vendors = $c->search('Koha::Acquisition::Booksellers'); What do you think ?
(In reply to Julian Maurice from comment #8) > It feels like search_for_api would fit better as a Mojo helper, given that > it mostly call other helpers : > > $vendors = $c->search('Koha::Acquisition::Booksellers'); > > What do you think ? my $plural_object = Koha::Acquisition::Booksellers->new; my $vendors = $c->object_search( $plural_object ); ? I'm not sure how to call $plural_object :-P
Created attachment 69248 [details] [review] Bug 19410: Move search_for_api into a Mojo helper
The last patch is a proposal for moving search_for_api out of Koha::Objects. Please tell me what you think. (In reply to Tomás Cohen Arazi from comment #9) > I'm not sure how to call $plural_object :-P I don't know either, I called it $objects_set ...
Created attachment 69253 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69254 [details] [review] Bug 19410: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69255 [details] [review] Bug 19410: (follow-up) Fix typo in POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69256 [details] [review] Bug 19410: Move search_for_api into a Mojo helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69257 [details] [review] Bug 19410: (QA followup) Move build_query_params_from_api into a helper This patch creates the 'build_query_params' helper, instead of the original function in Koha::Objects. Unit tests are removed for Koha::Objects::_build_query_params_from_api and written for the helper plugin. The objects.search helper gets a call to build_query_params added. Tests for it updated to match this behaviour change. To test: - Apply this patches - Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t \ t/db_dependent/Koha/Objects.t \ t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69258 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 69259 [details] [review] Bug 19410: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 69260 [details] [review] Bug 19410: (follow-up) Fix typo in POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 69261 [details] [review] Bug 19410: Move search_for_api into a Mojo helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 69262 [details] [review] Bug 19410: (QA followup) Move build_query_params_from_api into a helper This patch creates the 'build_query_params' helper, instead of the original function in Koha::Objects. Unit tests are removed for Koha::Objects::_build_query_params_from_api and written for the helper plugin. The objects.search helper gets a call to build_query_params added. Tests for it updated to match this behaviour change. To test: - Apply this patches - Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t \ t/db_dependent/Koha/Objects.t \ t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 69282 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69283 [details] [review] Bug 19410: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69284 [details] [review] Bug 19410: (follow-up) Fix typo in POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69286 [details] [review] Bug 19410: Move search_for_api into a Mojo helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69287 [details] [review] Bug 19410: (QA followup) Move build_query_params_from_api into a helper This patch creates the 'build_query_params' helper, instead of the original function in Koha::Objects. Unit tests are removed for Koha::Objects::_build_query_params_from_api and written for the helper plugin. The objects.search helper gets a call to build_query_params added. Tests for it updated to match this behaviour change. To test: - Apply this patches - Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t \ t/db_dependent/Koha/Objects.t \ t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69385 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69386 [details] [review] Bug 19410: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69387 [details] [review] Bug 19410: (follow-up) Fix typo in POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69388 [details] [review] Bug 19410: Move search_for_api into a Mojo helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69389 [details] [review] Bug 19410: (QA followup) Move build_query_params_from_api into a helper This patch creates the 'build_query_params' helper, instead of the original function in Koha::Objects. Unit tests are removed for Koha::Objects::_build_query_params_from_api and written for the helper plugin. The objects.search helper gets a call to build_query_params added. Tests for it updated to match this behaviour change. To test: - Apply this patches - Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t \ t/db_dependent/Koha/Objects.t \ t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Created attachment 69397 [details] [review] Bug 19410: Add Koha::Objects->search_for_api Following the discussion about the best way to make things simpler for developing the REST api, I quote Lari's email: "As many other endpoint will have the exact same usage, by looking at your example, I would prefer to avoid writing parameter / pagination / sorting / header handling for each list operation in our API controllers. Do you think it's possible to centralize all of this e.g. by passing $c into a customized search sub? Perhaps in Koha::Objects? so instead we could have something like (ignore my bad choice of naming)...: sub list_vendors { my $c = shift->openapi->valid_input or return; my $args = $c->validation->output; my $vendors; return try { $vendors = Koha::Acquisition::Booksellers->api_list_search($c); return $c->render(status => 200, openapi => $vendors); } catch { ... } }" We all agreed we neeed something like that. Here's a possible implementation. I take advantage of the previously written Mojo helpers, that are fully covered by tests. I submit this early so anyone can take a look and gather ideas to make it even better. I'm already using it (effectively) for the /acquisitions/orders endpoint I'm writing on bug 18731. Thanks! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 69398 [details] [review] Bug 19410: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 69399 [details] [review] Bug 19410: (follow-up) Fix typo in POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 69400 [details] [review] Bug 19410: Move search_for_api into a Mojo helper Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 69401 [details] [review] Bug 19410: (QA followup) Move build_query_params_from_api into a helper This patch creates the 'build_query_params' helper, instead of the original function in Koha::Objects. Unit tests are removed for Koha::Objects::_build_query_params_from_api and written for the helper plugin. The objects.search helper gets a call to build_query_params added. Tests for it updated to match this behaviour change. To test: - Apply this patches - Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t \ t/db_dependent/Koha/Objects.t \ t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I think rather than calling _to_model in each Controller, we should move that call to Koha::REST::Plugin::Objects, something like diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm index eb3f951..163e364 100644 --- a/Koha/REST/Plugin/Objects.pm +++ b/Koha/REST/Plugin/Objects.pm @@ -45,7 +45,7 @@ sub register { 'objects.search' => sub { my ( $c, $objects_set ) = @_; - my $args = $c->validation->output; + my $args = $objects_set->_to_model( $c->validation->output ); my $attributes = {}; and change the extract to my ($params, $reserved_params) = $c->extract_reserved_params( $c->validation->output ); or something like that so the above 'patch' doesn't break it. We could add _to_model to a parent class of the modules in Koha::REST::V1 that essentially does nothing, and then each child class could override it with any customizations if necessary. That would simplify the code, and allow customization easily, if it is needed. If it isn't the class just won't have a _to_model method. This is probably better to implement in a followup bug report. That patches themselves appear to work great!
Created attachment 69545 [details] [review] Bug 19410: (followup) Add reserved params definitions This patch re-adds some parameters I left out during some rebasing tasks and ended up on a separate patchset (bug 18731). The introduced parameters definitions are only used on endpoint definitions that implement (at least) pagination. No need to test them here but easier adding them here than on a patch implementing a new enpoint, which would become a dependency for other endpoints. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69703 [details] [review] Bug 19410: (follow-up) Move build_query_params_from_api into a helper
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! Enhancement, not backporting for 17.11.01