Bug 19410 - Add a helper function for generating object searches for the API
Summary: Add a helper function for generating object searches for the API
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 19196 19234 19278 19369 19370
Blocks: 16330 18731 19686
  Show dependency treegraph
 
Reported: 2017-10-04 18:49 UTC by Tomás Cohen Arazi
Modified: 2017-12-20 11:43 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19410: Add Koha::Objects->search_for_api (2.71 KB, patch)
2017-10-04 18:59 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Add Koha::Objects->search_for_api (3.90 KB, patch)
2017-10-06 15:55 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Unit tests (2.93 KB, patch)
2017-10-06 15:55 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Add Koha::Objects->search_for_api (4.14 KB, patch)
2017-10-06 17:27 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Unit tests (2.92 KB, patch)
2017-10-06 17:27 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: (follow-up) Fix typo in POD (650 bytes, patch)
2017-10-07 12:56 UTC, Lari Taskula
Details | Splinter Review
Bug 19410: (follow-up) Fix typo in POD (650 bytes, patch)
2017-10-07 13:06 UTC, Lari Taskula
Details | Splinter Review
Bug 19410: Move search_for_api into a Mojo helper (6.98 KB, patch)
2017-11-21 14:37 UTC, Julian Maurice
Details | Splinter Review
Bug 19410: Add Koha::Objects->search_for_api (4.21 KB, patch)
2017-11-21 19:20 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Unit tests (2.98 KB, patch)
2017-11-21 19:20 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: (follow-up) Fix typo in POD (712 bytes, patch)
2017-11-21 19:20 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Move search_for_api into a Mojo helper (7.04 KB, patch)
2017-11-21 19:20 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: (QA followup) Move build_query_params_from_api into a helper (13.76 KB, patch)
2017-11-21 19:20 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Add Koha::Objects->search_for_api (4.26 KB, patch)
2017-11-21 19:31 UTC, Julian Maurice
Details | Splinter Review
Bug 19410: Unit tests (3.04 KB, patch)
2017-11-21 19:31 UTC, Julian Maurice
Details | Splinter Review
Bug 19410: (follow-up) Fix typo in POD (772 bytes, patch)
2017-11-21 19:31 UTC, Julian Maurice
Details | Splinter Review
Bug 19410: Move search_for_api into a Mojo helper (7.10 KB, patch)
2017-11-21 19:32 UTC, Julian Maurice
Details | Splinter Review
Bug 19410: (QA followup) Move build_query_params_from_api into a helper (13.82 KB, patch)
2017-11-21 19:32 UTC, Julian Maurice
Details | Splinter Review
Bug 19410: Add Koha::Objects->search_for_api (4.31 KB, patch)
2017-11-22 15:30 UTC, Lari Taskula
Details | Splinter Review
Bug 19410: Unit tests (3.08 KB, patch)
2017-11-22 15:30 UTC, Lari Taskula
Details | Splinter Review
Bug 19410: (follow-up) Fix typo in POD (816 bytes, patch)
2017-11-22 15:30 UTC, Lari Taskula
Details | Splinter Review
Bug 19410: Move search_for_api into a Mojo helper (7.12 KB, patch)
2017-11-22 15:30 UTC, Lari Taskula
Details | Splinter Review
Bug 19410: (QA followup) Move build_query_params_from_api into a helper (13.84 KB, patch)
2017-11-22 15:31 UTC, Lari Taskula
Details | Splinter Review
Bug 19410: Add Koha::Objects->search_for_api (4.31 KB, patch)
2017-11-27 14:48 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Unit tests (3.09 KB, patch)
2017-11-27 14:48 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: (follow-up) Fix typo in POD (823 bytes, patch)
2017-11-27 14:49 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Move search_for_api into a Mojo helper (7.15 KB, patch)
2017-11-27 14:49 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: (QA followup) Move build_query_params_from_api into a helper (13.75 KB, patch)
2017-11-27 14:49 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: Add Koha::Objects->search_for_api (4.36 KB, patch)
2017-11-27 18:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19410: Unit tests (3.14 KB, patch)
2017-11-27 18:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19410: (follow-up) Fix typo in POD (873 bytes, patch)
2017-11-27 18:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19410: Move search_for_api into a Mojo helper (7.18 KB, patch)
2017-11-27 18:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19410: (QA followup) Move build_query_params_from_api into a helper (13.77 KB, patch)
2017-11-27 18:46 UTC, Kyle M Hall
Details | Splinter Review
Bug 19410: (followup) Add reserved params definitions (1.92 KB, patch)
2017-12-05 18:43 UTC, Tomás Cohen Arazi
Details | Splinter Review
Bug 19410: (follow-up) Move build_query_params_from_api into a helper (582 bytes, patch)
2017-12-11 20:25 UTC, Jonathan Druart
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2017-10-04 18:49:13 UTC
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)
Comment 1 Tomás Cohen Arazi 2017-10-04 18:59:13 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-10-06 15:55:49 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-10-06 15:55:54 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2017-10-06 17:27:49 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2017-10-06 17:27:55 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2017-10-07 12:56:55 UTC Comment hidden (obsolete)
Comment 7 Lari Taskula 2017-10-07 13:06:57 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2017-11-16 14:27:10 UTC
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 ?
Comment 9 Tomás Cohen Arazi 2017-11-21 13:23:37 UTC
(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
Comment 10 Julian Maurice 2017-11-21 14:37:49 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2017-11-21 14:39:36 UTC
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 ...
Comment 12 Tomás Cohen Arazi 2017-11-21 19:20:16 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2017-11-21 19:20:22 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2017-11-21 19:20:29 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2017-11-21 19:20:35 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2017-11-21 19:20:41 UTC Comment hidden (obsolete)
Comment 17 Julian Maurice 2017-11-21 19:31:51 UTC Comment hidden (obsolete)
Comment 18 Julian Maurice 2017-11-21 19:31:56 UTC Comment hidden (obsolete)
Comment 19 Julian Maurice 2017-11-21 19:31:59 UTC Comment hidden (obsolete)
Comment 20 Julian Maurice 2017-11-21 19:32:03 UTC Comment hidden (obsolete)
Comment 21 Julian Maurice 2017-11-21 19:32:08 UTC Comment hidden (obsolete)
Comment 22 Lari Taskula 2017-11-22 15:30:17 UTC Comment hidden (obsolete)
Comment 23 Lari Taskula 2017-11-22 15:30:32 UTC Comment hidden (obsolete)
Comment 24 Lari Taskula 2017-11-22 15:30:42 UTC Comment hidden (obsolete)
Comment 25 Lari Taskula 2017-11-22 15:30:53 UTC Comment hidden (obsolete)
Comment 26 Lari Taskula 2017-11-22 15:31:05 UTC Comment hidden (obsolete)
Comment 27 Tomás Cohen Arazi 2017-11-27 14:48:48 UTC
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>
Comment 28 Tomás Cohen Arazi 2017-11-27 14:48:56 UTC
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>
Comment 29 Tomás Cohen Arazi 2017-11-27 14:49:02 UTC
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>
Comment 30 Tomás Cohen Arazi 2017-11-27 14:49:11 UTC
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>
Comment 31 Tomás Cohen Arazi 2017-11-27 14:49:17 UTC
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>
Comment 32 Kyle M Hall 2017-11-27 18:46:11 UTC
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>
Comment 33 Kyle M Hall 2017-11-27 18:46:19 UTC
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>
Comment 34 Kyle M Hall 2017-11-27 18:46:22 UTC
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>
Comment 35 Kyle M Hall 2017-11-27 18:46:25 UTC
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>
Comment 36 Kyle M Hall 2017-11-27 18:46:28 UTC
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>
Comment 37 Kyle M Hall 2017-11-27 18:52:40 UTC
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!
Comment 38 Tomás Cohen Arazi 2017-12-05 18:43:13 UTC
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>
Comment 39 Jonathan Druart 2017-12-11 20:25:36 UTC
Created attachment 69703 [details] [review]
Bug 19410: (follow-up) Move build_query_params_from_api into a helper
Comment 40 Jonathan Druart 2017-12-11 20:47:48 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 41 Nick Clemens 2017-12-20 11:43:12 UTC
Awesome work all! Enhancement, not backporting for 17.11.01