From ee14689d5151838d62b244b5cae721fac2109c2a Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 31 Dec 2019 10:23:44 -0300 Subject: [PATCH] Bug 24321: Make objects.search use mappings from Koha::Object(s) This patch simplifies the objects.search helper so it relies entirely on the result set object for the attribute mappings. The result is no more to_api or to_model mappings are passed. The controllers need to be cleaned up after this patch. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! 3. Sign off :-D Note: the original version of this helpers accepted arbitrary mappings and are now constrianed to real mappings on the Koha::Object level. As such, the number of tests got reduced. Signed-off-by: Josef Moravec Signed-off-by: Kyle M Hall Signed-off-by: Martin Renvoize --- Koha/REST/Plugin/Objects.pm | 27 ++-- t/db_dependent/Koha/REST/Plugin/Objects.t | 160 +++------------------- 2 files changed, 29 insertions(+), 158 deletions(-) diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm index 3d314a784e..e8f0616f94 100644 --- a/Koha/REST/Plugin/Objects.pm +++ b/Koha/REST/Plugin/Objects.pm @@ -29,17 +29,13 @@ Koha::REST::Plugin::Objects =head3 objects.search - my $patrons_set = Koha::Patrons->new; - my $patrons = $c->objects->search( $patrons_set, [\&to_model, \&to_api] ); + my $patrons_rs = Koha::Patrons->new; + my $patrons = $c->objects->search( $patrons_rs ); Performs a database search using given Koha::Objects object and query parameters. -It (optionally) applies the I<$to_model> function reference before building the -query itself, and (optionally) applies I<$to_api> to the result. Returns an arrayref of the hashrefs representing the resulting objects -for JSON rendering. - -Note: Make sure I<$to_model> and I<$to_api> don't autovivify keys. +for API rendering. =cut @@ -48,7 +44,7 @@ sub register { $app->helper( 'objects.search' => sub { - my ( $c, $objects_set, $to_model, $to_api ) = @_; + my ( $c, $result_set ) = @_; my $args = $c->validation->output; my $attributes = {}; @@ -61,7 +57,7 @@ sub register { { attributes => $attributes, params => $reserved_params, - to_model => $to_model + result_set => $result_set } ); @@ -77,13 +73,12 @@ sub register { if ( defined $filtered_params ) { # Apply the mapping function to the passed params - $filtered_params = $to_model->($filtered_params) - if defined $to_model; + $filtered_params = $result_set->attributes_from_api($filtered_params); $filtered_params = $c->build_query_params( $filtered_params, $reserved_params ); } # Perform search - my $objects = $objects_set->search( $filtered_params, $attributes ); + my $objects = $result_set->search( $filtered_params, $attributes ); if ($objects->is_paged) { $c->add_pagination_headers({ @@ -92,13 +87,7 @@ sub register { }); } - my @objects_list = map { - ( defined $to_api ) - ? $to_api->( $_->TO_JSON ) - : $_->TO_JSON - } $objects->as_list; - - return \@objects_list; + return $objects->to_api; } ); } diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t index a65d949fc6..c1a76de564 100644 --- a/t/db_dependent/Koha/REST/Plugin/Objects.t +++ b/t/db_dependent/Koha/REST/Plugin/Objects.t @@ -34,50 +34,6 @@ get '/cities' => sub { $c->render( status => 200, json => $cities ); }; -get '/cities_to_model' => sub { - my $c = shift; - $c->validation->output($c->req->params->to_hash); - my $cities_set = Koha::Cities->new; - my $cities = $c->objects->search( $cities_set, \&to_model ); - $c->render( status => 200, json => $cities ); -}; - -get '/cities_to_model_to_api' => sub { - my $c = shift; - $c->validation->output($c->req->params->to_hash); - my $cities_set = Koha::Cities->new; - my $cities = $c->objects->search( $cities_set, \&to_model, \&to_api ); - $c->render( status => 200, json => $cities ); -}; - -get '/cities_sorted' => sub { - my $c = shift; - $c->validation->output($c->req->params->to_hash); - my $cities_set = Koha::Cities->new; - my $cities = $c->objects->search( $cities_set, \&to_model, \&to_api ); - $c->render( status => 200, json => $cities ); -}; - -sub to_model { - my $params = shift; - - if ( exists $params->{nombre} ) { - $params->{city_name} = delete $params->{nombre}; - } - - return $params; -} - -sub to_api { - my $params = shift; - - if ( exists $params->{city_name} ) { - $params->{nombre} = delete $params->{city_name}; - } - - return $params; -} - # The tests use Test::More tests => 2; use Test::Mojo; @@ -92,7 +48,7 @@ my $builder = t::lib::TestBuilder->new; subtest 'objects.search helper' => sub { - plan tests => 90; + plan tests => 34; my $t = Test::Mojo->new; @@ -115,12 +71,12 @@ subtest 'objects.search helper' => sub { } }); - $t->get_ok('/cities?city_name=manuel&_per_page=1&_page=1') + $t->get_ok('/cities?name=manuel&_per_page=1&_page=1') ->status_is(200) ->header_like( 'Link' => qr/; rel="next",/ ) ->json_has('/0') ->json_hasnt('/1') - ->json_is('/0/city_name' => 'Manuel'); + ->json_is('/0/name' => 'Manuel'); $builder->build_object({ class => 'Koha::Cities', @@ -130,114 +86,40 @@ subtest 'objects.search helper' => sub { }); # _match=starts_with - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=starts_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela'); - - # _match=ends_with - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=ends_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Emanuel'); - - # _match=exact - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=exact') - ->status_is(200) - ->json_has('/0') - ->json_hasnt('/1') - ->json_is('/0/city_name' => 'Manuel'); - - # _match=contains - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=contains') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_has('/2') - ->json_hasnt('/3') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela') - ->json_is('/2/city_name' => 'Emanuel'); - - ## _to_model tests - # _match=starts_with - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=starts_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela'); - - # _match=ends_with - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=ends_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Emanuel'); - - # _match=exact - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=exact') - ->status_is(200) - ->json_has('/0') - ->json_hasnt('/1') - ->json_is('/0/city_name' => 'Manuel'); - - # _match=contains - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=contains') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_has('/2') - ->json_hasnt('/3') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela') - ->json_is('/2/city_name' => 'Emanuel'); - - ## _to_model && _to_api tests - # _match=starts_with - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=starts_with') + $t->get_ok('/cities?name=manuel&_per_page=3&_page=1&_match=starts_with') ->status_is(200) ->json_has('/0') ->json_has('/1') ->json_hasnt('/2') - ->json_is('/0/nombre' => 'Manuel') - ->json_is('/1/nombre' => 'Manuela'); + ->json_is('/0/name' => 'Manuel') + ->json_is('/1/name' => 'Manuela'); # _match=ends_with - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=ends_with') + $t->get_ok('/cities?name=manuel&_per_page=3&_page=1&_match=ends_with') ->status_is(200) ->json_has('/0') ->json_has('/1') ->json_hasnt('/2') - ->json_is('/0/nombre' => 'Manuel') - ->json_is('/1/nombre' => 'Emanuel'); + ->json_is('/0/name' => 'Manuel') + ->json_is('/1/name' => 'Emanuel'); # _match=exact - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=exact') + $t->get_ok('/cities?name=manuel&_per_page=3&_page=1&_match=exact') ->status_is(200) ->json_has('/0') ->json_hasnt('/1') - ->json_is('/0/nombre' => 'Manuel'); + ->json_is('/0/name' => 'Manuel'); # _match=contains - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=contains') + $t->get_ok('/cities?name=manuel&_per_page=3&_page=1&_match=contains') ->status_is(200) ->json_has('/0') ->json_has('/1') ->json_has('/2') ->json_hasnt('/3') - ->json_is('/0/nombre' => 'Manuel') - ->json_is('/1/nombre' => 'Manuela') - ->json_is('/2/nombre' => 'Emanuel'); + ->json_is('/0/name' => 'Manuel') + ->json_is('/1/name' => 'Manuela') + ->json_is('/2/name' => 'Emanuel'); $schema->storage->txn_rollback; }; @@ -256,21 +138,21 @@ subtest 'objects.search helper, sorting on mapped column' => sub { $builder->build_object({ class => 'Koha::Cities', value => { city_name => 'A', city_country => 'Argentina' } }); $builder->build_object({ class => 'Koha::Cities', value => { city_name => 'B', city_country => 'Argentina' } }); - $t->get_ok('/cities_sorted?_order_by=%2Bnombre&_order_by=+city_country') + $t->get_ok('/cities?_order_by=%2Bname&_order_by=+country') ->status_is(200) ->json_has('/0') ->json_has('/1') ->json_hasnt('/2') - ->json_is('/0/nombre' => 'A') - ->json_is('/1/nombre' => 'B'); + ->json_is('/0/name' => 'A') + ->json_is('/1/name' => 'B'); - $t->get_ok('/cities_sorted?_order_by=-nombre') + $t->get_ok('/cities?_order_by=-name') ->status_is(200) ->json_has('/0') ->json_has('/1') ->json_hasnt('/2') - ->json_is('/0/nombre' => 'B') - ->json_is('/1/nombre' => 'A'); + ->json_is('/0/name' => 'B') + ->json_is('/1/name' => 'A'); $schema->storage->txn_rollback; } -- 2.39.5