Bug 19234 - Add query parameters handling helpers
Summary: Add query parameters handling helpers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 19196
Blocks: 18731 19369 19370 19410
  Show dependency treegraph
 
Reported: 2017-09-01 14:48 UTC by Tomás Cohen Arazi
Modified: 2018-12-03 20:03 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19234: Unit tests for query parameters handling helpers (2.98 KB, patch)
2017-09-01 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: Add query parameters handling helpers (3.79 KB, patch)
2017-09-01 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: (followup) Make Query plugin available to endpoints (670 bytes, patch)
2017-09-01 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: Unit tests for query parameters handling helpers (3.03 KB, patch)
2017-09-08 14:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: Add query parameters handling helpers (3.84 KB, patch)
2017-09-08 14:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: (followup) Make Query plugin available to endpoints (719 bytes, patch)
2017-09-08 14:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: Unit tests for query parameters handling helpers (3.21 KB, patch)
2017-09-26 14:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: Add query parameters handling helpers (3.64 KB, patch)
2017-09-26 14:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: (followup) Make Query plugin available to endpoints (718 bytes, patch)
2017-09-26 14:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: Unit tests for query parameters handling helpers (3.44 KB, patch)
2017-09-26 15:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: Add query parameters handling helpers (3.64 KB, patch)
2017-09-26 15:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: (followup) Make Query plugin available to endpoints (718 bytes, patch)
2017-09-26 15:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19234: Unit tests for query parameters handling helpers (3.50 KB, patch)
2017-09-26 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: Add query parameters handling helpers (3.69 KB, patch)
2017-09-26 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: (followup) Make Query plugin available to endpoints (775 bytes, patch)
2017-09-26 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: Unit tests for query parameters handling helpers (3.55 KB, patch)
2017-10-07 12:46 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 19234: Add query parameters handling helpers (3.74 KB, patch)
2017-10-07 12:46 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 19234: (followup) Make Query plugin available to endpoints (825 bytes, patch)
2017-10-07 12:46 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 19234: Unit tests for query parameters handling helpers (3.60 KB, patch)
2017-11-27 11:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: Add query parameters handling helpers (3.80 KB, patch)
2017-11-27 11:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19234: (followup) Make Query plugin available to endpoints (882 bytes, patch)
2017-11-27 11:27 UTC, Kyle M Hall
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 2017-09-01 14:48:44 UTC
In order to ease endpoint writing, we need a common (and fully tested) way of dealing with two things:
- Extract pagination controlling params from the query params and create the right pagination setup for Koha::Objects
- Generate the right DBIC query for the passed params
Comment 1 Tomás Cohen Arazi 2017-09-01 15:02:32 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-09-01 15:02:38 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-09-01 15:02:44 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2017-09-08 14:13:41 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2017-09-08 14:13:47 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2017-09-08 14:13:51 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2017-09-21 13:45:25 UTC
Comment on attachment 66991 [details] [review]
Bug 19234: Unit tests for query parameters handling helpers

Review of attachment 66991 [details] [review]:
-----------------------------------------------------------------

::: t/Koha/REST/Plugin/Query.t
@@ +68,5 @@
> +
> +    $t->get_ok('/query')
> +      ->status_is( 200 )
> +      ->json_is( '/attributes' => { page => 2, rows => 3 } )
> +      ->json_is( '/filter' => { firstname => { like => 'Manuel%' }, surname => { like => 'Cohen Arazi%' } } );

Is it by design that this 'like' is actually a 'starts with' rather than a 'contains' search?
Comment 8 Martin Renvoize 2017-09-21 13:52:22 UTC
This somehow feels like a step too far in trying to generalise a mapping between api query parameters and database fields.. perhaps I'm wrong though and we're aiming to try to keep that link strongly bound.. perhaps some examples of how you're expecting to use this outside of just the pagination merging.

On the pagination side.. I think I'd probably add that slice of code to ::Plugin::Pagination (to keep it with the pagination stuff) and name it something along the lines of 'dbic_merge_pagination_attributes'?
Comment 9 Martin Renvoize 2017-09-21 14:20:07 UTC
OR.. perhaps you need some sort of object (be that Koha:: or Schema::Result) introspection to match up fields to query parameters in there?
Comment 10 Martin Renvoize 2017-09-21 14:31:57 UTC
Comment on attachment 66992 [details] [review]
Bug 19234: Add query parameters handling helpers

Review of attachment 66992 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/Plugin/Query.pm
@@ +68,5 @@
> +    my ( $page, $per_page ) = @_;
> +
> +    my $attributes = {
> +        rows => $per_page,
> +        page => $page

As a general rule when doing paging one should also always explicitly set an order_by parameter to ensure the resultset is always sorted consistently between paged queries.
Comment 11 Tomás Cohen Arazi 2017-09-22 21:03:54 UTC
order_by is missing. This needs to be fixed.
Comment 12 Tomás Cohen Arazi 2017-09-22 21:05:28 UTC
(In reply to Martin Renvoize from comment #7)
> Comment on attachment 66991 [details] [review] [review]
> Bug 19234: Unit tests for query parameters handling helpers
> 
> Review of attachment 66991 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: t/Koha/REST/Plugin/Query.t
> @@ +68,5 @@
> > +
> > +    $t->get_ok('/query')
> > +      ->status_is( 200 )
> > +      ->json_is( '/attributes' => { page => 2, rows => 3 } )
> > +      ->json_is( '/filter' => { firstname => { like => 'Manuel%' }, surname => { like => 'Cohen Arazi%' } } );
> 
> Is it by design that this 'like' is actually a 'starts with' rather than a
> 'contains' search?

I was just keeping the current behaviour. If we added more options (contains, left truncation, etc) we should still keep right truncation as default to avoid unneeded behaviour changes.
Comment 13 Tomás Cohen Arazi 2017-09-22 21:45:44 UTC
(In reply to Martin Renvoize from comment #8)
> This somehow feels like a step too far in trying to generalise a mapping
> between api query parameters and database fields.. perhaps I'm wrong though
> and we're aiming to try to keep that link strongly bound.. perhaps some
> examples of how you're expecting to use this outside of just the pagination
> merging.

This helper is only intended to extract reserved params from the query (so a single point for having this written, and testing it, thus becoming a standard on the codebase too). It is up to the controller methods to pass the resulting $filter as-is or chew it a bit more. But I agree the naming of the method, and the sample usage is misleading.

> On the pagination side.. I think I'd probably add that slice of code to
> ::Plugin::Pagination (to keep it with the pagination stuff) and name it
> something along the lines of 'dbic_merge_pagination_attributes'?

Adding to my response above, I think this should just extract the reserved params, and return the two hashrefs it returns now, without mentioning attributes and filters. And yes, building the DBIC 'pagination filter' should be left to the Pagination helper.
Comment 14 Tomás Cohen Arazi 2017-09-26 14:49:24 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2017-09-26 14:49:34 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2017-09-26 14:49:43 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2017-09-26 14:57:23 UTC
Kyle, Martin, I have simplified this plugin, making it only extract reserved params from the query, and providing a 'cleaned up' attributes hash.

The commit message should be clear on how this is supposed to be used.

New helpers could be added in order to:
- Merge pagination params into $filter (for DBIC's ->search)
- Generate DBI'c search query based on _match and the received params.
- Handle order_by, this is important because we could add some reflection to Koha::Objects (or DBIC in order to have them say what columns can be used for sorting)

Please give this a try ASAP so I move on on top of this.
Thanks in advance!
Comment 18 Tomás Cohen Arazi 2017-09-26 15:10:07 UTC
Created attachment 67363 [details] [review]
Bug 19234: Unit tests for query parameters handling helpers

This patch adds unit tests for the new query parameters handling Mojo plugin.

Sponsored-by: Camden County
Sponsored-by: ByWater Solutions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2017-09-26 15:10:14 UTC
Created attachment 67364 [details] [review]
Bug 19234: Add query parameters handling helpers

This patch introduces a Mojolicious plugin to be used on the REST api.
It adds a helper method:

generate_dbic_query
===================

When used, it generates what's needed to perform a search on
DBIC/Koha::Objects like this:

    my $params  = $c->validation->output;
    my ($filtered_params, $reserved_params) = $c->extract_reserved_params($params);

    my $filter = do_smth($filtered_params, $reserved_params);
    my $attributes = do_smth_reserved($reserved_params);

    my $patrons = Koha::Patrons->search( $filter, $attributes );

It introduces reserved param names:
- _match
- _order_by
- _page
- _per_page

They are reserved for later usage (pagination, matching algorithm on building DB queries)

All the plugin's behaviour is tested.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater solutions
Sponsored-by: Camden County

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2017-09-26 15:10:19 UTC
Created attachment 67365 [details] [review]
Bug 19234: (followup) Make Query plugin available to endpoints

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2017-09-26 18:15:45 UTC
Created attachment 67374 [details] [review]
Bug 19234: Unit tests for query parameters handling helpers

This patch adds unit tests for the new query parameters handling Mojo plugin.

Sponsored-by: Camden County
Sponsored-by: ByWater Solutions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2017-09-26 18:15:53 UTC
Created attachment 67375 [details] [review]
Bug 19234: Add query parameters handling helpers

This patch introduces a Mojolicious plugin to be used on the REST api.
It adds a helper method:

generate_dbic_query
===================

When used, it generates what's needed to perform a search on
DBIC/Koha::Objects like this:

    my $params  = $c->validation->output;
    my ($filtered_params, $reserved_params) = $c->extract_reserved_params($params);

    my $filter = do_smth($filtered_params, $reserved_params);
    my $attributes = do_smth_reserved($reserved_params);

    my $patrons = Koha::Patrons->search( $filter, $attributes );

It introduces reserved param names:
- _match
- _order_by
- _page
- _per_page

They are reserved for later usage (pagination, matching algorithm on building DB queries)

All the plugin's behaviour is tested.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater solutions
Sponsored-by: Camden County

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2017-09-26 18:15:55 UTC
Created attachment 67377 [details] [review]
Bug 19234: (followup) Make Query plugin available to endpoints

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Lari Taskula 2017-10-07 12:46:27 UTC
Created attachment 67785 [details] [review]
Bug 19234: Unit tests for query parameters handling helpers

This patch adds unit tests for the new query parameters handling Mojo plugin.

Sponsored-by: Camden County
Sponsored-by: ByWater Solutions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 25 Lari Taskula 2017-10-07 12:46:41 UTC
Created attachment 67786 [details] [review]
Bug 19234: Add query parameters handling helpers

This patch introduces a Mojolicious plugin to be used on the REST api.
It adds a helper method:

generate_dbic_query
===================

When used, it generates what's needed to perform a search on
DBIC/Koha::Objects like this:

    my $params  = $c->validation->output;
    my ($filtered_params, $reserved_params) = $c->extract_reserved_params($params);

    my $filter = do_smth($filtered_params, $reserved_params);
    my $attributes = do_smth_reserved($reserved_params);

    my $patrons = Koha::Patrons->search( $filter, $attributes );

It introduces reserved param names:
- _match
- _order_by
- _page
- _per_page

They are reserved for later usage (pagination, matching algorithm on building DB queries)

All the plugin's behaviour is tested.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater solutions
Sponsored-by: Camden County

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 26 Lari Taskula 2017-10-07 12:46:54 UTC
Created attachment 67787 [details] [review]
Bug 19234: (followup) Make Query plugin available to endpoints

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 27 Julian Maurice 2017-11-16 13:27:27 UTC
Maybe I'm missing the big picture here, but I don't understand why this plugin is needed.
All it does is separate query parameters so one can pass 'reserved' parameters to others mojo helpers (at least that's how it's used in bug 19410), but helpers already have access to those parameters through their first parameter ($c), right ?
And not all helpers will have a use for all reserved parameters, so passing all those parameters to all helpers is useless, they can retrieve what they need on their own.

Please explain :)
Comment 28 Tomás Cohen Arazi 2017-11-16 13:39:01 UTC
(In reply to Julian Maurice from comment #27)
> Maybe I'm missing the big picture here, but I don't understand why this
> plugin is needed.
> All it does is separate query parameters so one can pass 'reserved'
> parameters to others mojo helpers (at least that's how it's used in bug
> 19410), but helpers already have access to those parameters through their
> first parameter ($c), right ?
> And not all helpers will have a use for all reserved parameters, so passing
> all those parameters to all helpers is useless, they can retrieve what they
> need on their own.
> 
> Please explain :)

The idea behind this was:
- To make sure some things are reserved and make it explicit. Have the list in a single place, make it grow if needed.
- Make parameters extraction algorithm testable, so no need to add thousands of tests for each endpoint... and ...
- Code less: we don't need to write useless repeating loops extracting parameters and separating the reserved ones on each controller method.

Koha::Objects->search_for_api is a clear example of how this would improve productivity when writing endpoints. If you look at how a controller looks like using it when it is just a wrapper or Koha::Objects-derived classes, it becomes clear:

sub list {

    my $c = ...

    my $objects = Koha::Blah->search_for_api($c);
    return $c->render( openapi => $objects, status => 200 );

}

In this tiny example, pagination, sorting and query building choice are solved already using this helpers.


And those who require more flexibility (business endpoints, that are not just wrappers to Koha::Object(s) derived classes) can just skip using them and do more interesting stuff.
Comment 29 Kyle M Hall 2017-11-27 11:27:13 UTC
Created attachment 69360 [details] [review]
Bug 19234: Unit tests for query parameters handling helpers

This patch adds unit tests for the new query parameters handling Mojo plugin.

Sponsored-by: Camden County
Sponsored-by: ByWater Solutions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2017-11-27 11:27:20 UTC
Created attachment 69361 [details] [review]
Bug 19234: Add query parameters handling helpers

This patch introduces a Mojolicious plugin to be used on the REST api.
It adds a helper method:

generate_dbic_query
===================

When used, it generates what's needed to perform a search on
DBIC/Koha::Objects like this:

    my $params  = $c->validation->output;
    my ($filtered_params, $reserved_params) = $c->extract_reserved_params($params);

    my $filter = do_smth($filtered_params, $reserved_params);
    my $attributes = do_smth_reserved($reserved_params);

    my $patrons = Koha::Patrons->search( $filter, $attributes );

It introduces reserved param names:
- _match
- _order_by
- _page
- _per_page

They are reserved for later usage (pagination, matching algorithm on building DB queries)

All the plugin's behaviour is tested.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater solutions
Sponsored-by: Camden County

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2017-11-27 11:27:23 UTC
Created attachment 69362 [details] [review]
Bug 19234: (followup) Make Query plugin available to endpoints

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Jonathan Druart 2017-12-11 20:47:26 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 33 Nick Clemens (kidclamp) 2017-12-20 11:40:09 UTC
Awesome work all! Enhancement, skipping for 17.11