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
Created attachment 66752 [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
Created attachment 66753 [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 ($filter, $attributes) = $c->generate_dbic_query($params); my $patrons = Koha::Patrons->search( $filter, $attributes ); It introduces a reserved word _exact_match that controls the generated $filter: - Default behaviour: right truncation on the query params - _exact_match=1: no truncation added. 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
Created attachment 66754 [details] [review] Bug 19234: (followup) Make Query plugin available to endpoints
Created attachment 66991 [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>
Created attachment 66992 [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 ($filter, $attributes) = $c->generate_dbic_query($params); my $patrons = Koha::Patrons->search( $filter, $attributes ); It introduces a reserved word _exact_match that controls the generated $filter: - Default behaviour: right truncation on the query params - _exact_match=1: no truncation added. 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>
Created attachment 66993 [details] [review] Bug 19234: (followup) Make Query plugin available to endpoints Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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?
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'?
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 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.
order_by is missing. This needs to be fixed.
(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.
(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.
Created attachment 67358 [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>
Created attachment 67359 [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>
Created attachment 67360 [details] [review] Bug 19234: (followup) Make Query plugin available to endpoints Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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!
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>
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>
Created attachment 67365 [details] [review] Bug 19234: (followup) Make Query plugin available to endpoints Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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>
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>
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>
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>
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>
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 :)
(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.
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>
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>
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>
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! Enhancement, skipping for 17.11