From e08639129aec34d2228e08e65a0ef8ba008c6efc Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 12 Oct 2016 21:31:30 +0000 Subject: [PATCH] Bug 17428: [REST] best practice followup This followup alters a few area's to be aligned more closely with RESTfull best practices: * PUT should always be full objects, and not partial updates (use PATCH for partials) * Validate query parameters instead of blindly passing them to the model * Functional Change: Convert filter params from 'equality' to 'starts with' matching * Update tests to check for swagger validation errors instead of koha exceptions * Mark 'id' as readOnly so swagger may prevent, via validation, id changes. Signed-off-by: Jonathan Druart Signed-off-by: Tomas Cohen Arazi Signed-off-by: Kyle M Hall --- Koha/REST/V1/Cities.pm | 16 +-- api/v1/swagger/definitions/city.json | 42 ++++---- api/v1/swagger/paths/cities.json | 122 +++++++++++++---------- api/v1/swagger/x-primitives.json | 3 +- t/db_dependent/api/v1/cities.t | 142 +++++++++++++++++++++------ 5 files changed, 212 insertions(+), 113 deletions(-) diff --git a/Koha/REST/V1/Cities.pm b/Koha/REST/V1/Cities.pm index 1b04e10e9d..d9d2774f6b 100644 --- a/Koha/REST/V1/Cities.pm +++ b/Koha/REST/V1/Cities.pm @@ -29,10 +29,14 @@ sub list { my ( $c, $args, $cb ) = @_; my $cities; + my $filter; + + for my $filter_param ( keys $args ) { + $filter->{$filter_param} = { LIKE => $args->{$filter_param} . "%" }; + } return try { - $cities = - Koha::Cities->search( $c->req->query_params->to_hash )->unblessed; + $cities = Koha::Cities->search($filter)->unblessed; return $c->$cb( $cities, 200 ); } catch { @@ -60,7 +64,7 @@ sub get { sub add { my ( $c, $args, $cb ) = @_; - my $city = Koha::City->new( $c->req->json ); + my $city = Koha::City->new( $args->{body} ); return try { $city->store; @@ -84,10 +88,8 @@ sub update { return try { $city = Koha::Cities->find( $args->{cityid} ); - while ( my ( $k, $v ) = each %{ $c->req->json } ) { - $city->$k($v); - } - $city->store; + $city->set( $args->{body} ); + $city->store(); return $c->$cb( $city->unblessed, 200 ); } catch { diff --git a/api/v1/swagger/definitions/city.json b/api/v1/swagger/definitions/city.json index a1d7d40139..f322136fd9 100644 --- a/api/v1/swagger/definitions/city.json +++ b/api/v1/swagger/definitions/city.json @@ -1,24 +1,26 @@ { "type": "object", "properties": { - "cityid": { - "$ref": "../x-primitives.json#/cityid" - }, - "city_name": { - "description": "city name", - "type": "string" - }, - "city_state": { - "description": "city state", - "type": ["string", "null"] - }, - "city_zipcode": { - "description": "city zipcode", - "type": ["string", "null"] - }, - "city_country": { - "description": "city country", - "type": ["string", "null"] - } - } + "cityid": { + "$ref": "../x-primitives.json#/cityid" + }, + "city_name": { + "description": "city name", + "type": "string" + }, + "city_state": { + "description": "city state", + "type": ["string", "null"] + }, + "city_zipcode": { + "description": "city zipcode", + "type": ["string", "null"] + }, + "city_country": { + "description": "city country", + "type": ["string", "null"] + } + }, + "additionalProperties": false, + "required": ["city_name", "city_state", "city_zipcode", "city_country"] } diff --git a/api/v1/swagger/paths/cities.json b/api/v1/swagger/paths/cities.json index 9727bcd914..650d224019 100644 --- a/api/v1/swagger/paths/cities.json +++ b/api/v1/swagger/paths/cities.json @@ -7,6 +7,31 @@ "produces": [ "application/json" ], + "parameters": [{ + "name": "city_name", + "in": "query", + "description": "Case insensative 'starts-with' search on city name", + "required": false, + "type": "string" + }, { + "name": "city_state", + "in": "query", + "description": "Case insensative 'starts-with' search on city state", + "required": false, + "type": "string" + }, { + "name": "city_country", + "in": "query", + "description": "Case insensative 'starts_with' search on city country", + "required": false, + "type": "string" + }, { + "name": "city_zipcode", + "in": "query", + "description": "Case Insensative 'starts-with' search on zipcode", + "required": false, + "type": "string" + }], "responses": { "200": { "description": "A list of cities", @@ -24,10 +49,10 @@ } }, "500": { - "description": "Internal error", - "schema": { - "$ref": "../definitions.json#/error" - } + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } } } }, @@ -36,15 +61,14 @@ "operationId": "add", "tags": ["cities"], "parameters": [{ - "name": "body", - "in": "body", - "description": "A JSON object containing informations about the new hold", - "required": true, - "schema": { - "$ref": "../definitions.json#/city" - } + "name": "body", + "in": "body", + "description": "A JSON object containing informations about the new hold", + "required": true, + "schema": { + "$ref": "../definitions.json#/city" } - ], + }], "produces": [ "application/json" ], @@ -62,10 +86,10 @@ } }, "500": { - "description": "Internal error", - "schema": { - "$ref": "../definitions.json#/error" - } + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } } }, "x-koha-authorization": { @@ -80,11 +104,9 @@ "x-mojo-controller": "Koha::REST::V1::Cities", "operationId": "get", "tags": ["cities"], - "parameters": [ - { - "$ref": "../parameters.json#/cityidPathParam" - } - ], + "parameters": [{ + "$ref": "../parameters.json#/cityidPathParam" + }], "produces": [ "application/json" ], @@ -106,13 +128,12 @@ "schema": { "$ref": "../definitions.json#/error" } - } - , + }, "500": { - "description": "Internal error", - "schema": { - "$ref": "../definitions.json#/error" - } + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } } } }, @@ -120,20 +141,17 @@ "x-mojo-controller": "Koha::REST::V1::Cities", "operationId": "update", "tags": ["cities"], - "parameters": [ - { - "$ref": "../parameters.json#/cityidPathParam" - }, - { - "name": "body", - "in": "body", - "description": "A JSON object containing informations about the new hold", - "required": true, - "schema": { - "$ref": "../definitions.json#/city" - } + "parameters": [{ + "$ref": "../parameters.json#/cityidPathParam" + }, { + "name": "body", + "in": "body", + "description": "A JSON object containing informations about the new hold", + "required": true, + "schema": { + "$ref": "../definitions.json#/city" } - ], + }], "produces": [ "application/json" ], @@ -157,10 +175,10 @@ } }, "500": { - "description": "Internal error", - "schema": { - "$ref": "../definitions.json#/error" - } + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } } }, "x-koha-authorization": { @@ -173,11 +191,9 @@ "x-mojo-controller": "Koha::REST::V1::Cities", "operationId": "delete", "tags": ["cities"], - "parameters": [ - { - "$ref": "../parameters.json#/cityidPathParam" - } - ], + "parameters": [{ + "$ref": "../parameters.json#/cityidPathParam" + }], "produces": [ "application/json" ], @@ -201,10 +217,10 @@ } }, "500": { - "description": "Internal error", - "schema": { - "$ref": "../definitions.json#/error" - } + "description": "Internal error", + "schema": { + "$ref": "../definitions.json#/error" + } } }, "x-koha-authorization": { diff --git a/api/v1/swagger/x-primitives.json b/api/v1/swagger/x-primitives.json index f5ff2e939d..1b90b30c46 100644 --- a/api/v1/swagger/x-primitives.json +++ b/api/v1/swagger/x-primitives.json @@ -17,7 +17,8 @@ }, "cityid": { "type": "string", - "description": "internally assigned city identifier" + "description": "internally assigned city identifier", + "readOnly": true }, "email": { "type": ["string", "null"], diff --git a/t/db_dependent/api/v1/cities.t b/t/db_dependent/api/v1/cities.t index 0b1cd5764c..e6e13c6865 100644 --- a/t/db_dependent/api/v1/cities.t +++ b/t/db_dependent/api/v1/cities.t @@ -24,14 +24,13 @@ use Test::Warn; use t::lib::TestBuilder; use t::lib::Mocks; -use Data::Printer colored => 1; - use C4::Auth; use Koha::Cities; use Koha::Database; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; + # FIXME: sessionStorage defaults to mysql, but it seems to break transaction handling # this affects the other REST api tests t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' ); @@ -41,7 +40,7 @@ my $t = Test::Mojo->new('Koha::REST::V1'); subtest 'list() tests' => sub { - plan tests => 19; + plan tests => 18; $schema->storage->txn_begin; @@ -82,7 +81,8 @@ subtest 'list() tests' => sub { $t->ua->build_tx( GET => "/api/v1/cities?city_country=" . $city_country ); $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - $t->request_ok($tx)->status_is(200)->json_is( [ $city, $another_city ] ); + my $result = + $t->request_ok($tx)->status_is(200)->json_is( [ $city, $another_city ] ); $tx = $t->ua->build_tx( GET => "/api/v1/cities?city_name=" . $city->{city_name} ); @@ -90,15 +90,12 @@ subtest 'list() tests' => sub { $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx)->status_is(200)->json_is( [$city] ); + # Warn on unsupported query parameter $tx = $t->ua->build_tx( GET => '/api/v1/cities?city_blah=blah' ); $tx->req->cookies( { name => 'CGISESSID', value => $session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - - warning_like { - $t->request_ok($tx)->status_is(500) - ->json_like( '/error' => qr/Unknown column/ ); - } - qr/Unknown column/, 'Wrong parameters raise warnings'; + $t->request_ok($tx)->status_is(400) + ->json_is( [{ path => '/query/city_blah', message => 'Malformed query string'}] ); $schema->storage->txn_rollback; }; @@ -130,7 +127,7 @@ subtest 'get() tests' => sub { subtest 'add() tests' => sub { - plan tests => 10; + plan tests => 17; $schema->storage->txn_begin; @@ -152,38 +149,71 @@ subtest 'add() tests' => sub { $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx)->status_is(403); + # Authorized attempt to write invalid data + my $city_with_invalid_field = { + city_blah => "City Blah", + city_state => "City State", + city_zipcode => "City Zipcode", + city_country => "City Country" + }; + + $tx = $t->ua->build_tx( + POST => "/api/v1/cities/" => json => $city_with_invalid_field ); + $tx->req->cookies( + { name => 'CGISESSID', value => $authorized_session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + $t->request_ok($tx)->status_is(400)->json_is( + "/errors" => [ + { + message => "Properties not allowed: city_blah.", + path => "/body" + } + ] + ); + # Authorized attempt to write $tx = $t->ua->build_tx( POST => "/api/v1/cities/" => json => $city ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - $t->request_ok($tx)->status_is(200) + my $cityid = $t->request_ok($tx)->status_is(200) ->json_is( '/city_name' => $city->{city_name} ) ->json_is( '/city_state' => $city->{city_state} ) ->json_is( '/city_zipcode' => $city->{city_zipcode} ) - ->json_is( '/city_country' => $city->{city_country} ); + ->json_is( '/city_country' => $city->{city_country} ) + ->tx->res->json->{cityid}; - my $city_with_invalid_field = { - city_blah => "City Blah", - city_state => "City State", - city_zipcode => "City Zipcode", - city_country => "City Country" - }; + # Authorized attempt to create with null id + $city->{cityid} = undef; + $tx = $t->ua->build_tx( + POST => "/api/v1/cities/" => json => $city ); + $tx->req->cookies( + { name => 'CGISESSID', value => $authorized_session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + $t->request_ok($tx)->status_is(400)->json_has('/errors'); - # Authorized attempt to write invalid data + # Authorized attempt to create with existing id + $city->{cityid} = $cityid; $tx = $t->ua->build_tx( - POST => "/api/v1/cities/" => json => $city_with_invalid_field ); + POST => "/api/v1/cities/" => json => $city ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - $t->request_ok($tx)->status_is(500); + $t->request_ok($tx)->status_is(400)->json_is( + "/errors" => [ + { + message => "Read-only.", + path => "/body/cityid" + } + ] + ); $schema->storage->txn_rollback; }; subtest 'update() tests' => sub { - plan tests => 10; + plan tests => 15; $schema->storage->txn_begin; @@ -202,32 +232,80 @@ subtest 'update() tests' => sub { $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx)->status_is(403); + # Attempt partial update on a PUT + my $city_with_missing_field = { + city_name => 'New name', + city_state => 'New state', + city_country => 'New country' + }; + $tx = $t->ua->build_tx( - PUT => "/api/v1/cities/$city_id" => json => { city_name => 'New name' } - ); + PUT => "/api/v1/cities/$city_id" => json => $city_with_missing_field ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - $t->request_ok($tx)->status_is(200)->json_is( '/city_name' => 'New name' ); + $t->request_ok($tx)->status_is(400) + ->json_is( "/errors" => + [ { message => "Missing property.", path => "/body/city_zipcode" } ] + ); + + # Full object update on PUT + my $city_with_updated_field = { + city_name => "London", + city_state => "City State", + city_zipcode => "City Zipcode", + city_country => "City Country" + }; $tx = $t->ua->build_tx( - PUT => "/api/v1/cities/$city_id" => json => { city_blah => 'New blah' } - ); + PUT => "/api/v1/cities/$city_id" => json => $city_with_updated_field ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); - $t->request_ok($tx)->status_is(500) - ->json_is( '/error' => "No method city_blah for Koha::City" ); + $t->request_ok($tx)->status_is(200)->json_is( '/city_name' => 'London' ); + + # Authorized attempt to write invalid data + my $city_with_invalid_field = { + city_blah => "City Blah", + city_state => "City State", + city_zipcode => "City Zipcode", + city_country => "City Country" + }; + + $tx = $t->ua->build_tx( + PUT => "/api/v1/cities/$city_id" => json => $city_with_invalid_field ); + $tx->req->cookies( + { name => 'CGISESSID', value => $authorized_session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + $t->request_ok($tx)->status_is(400)->json_is( + "/errors" => [ + { + message => "Properties not allowed: city_blah.", + path => "/body" + } + ] + ); my $non_existent_id = $city_id + 1; - $tx = $t->ua->build_tx( PUT => "/api/v1/cities/$non_existent_id" => json => - { city_name => 'New name' } ); + $tx = + $t->ua->build_tx( PUT => "/api/v1/cities/$non_existent_id" => json => + $city_with_updated_field ); $tx->req->cookies( { name => 'CGISESSID', value => $authorized_session_id } ); $tx->req->env( { REMOTE_ADDR => $remote_address } ); $t->request_ok($tx)->status_is(404); $schema->storage->txn_rollback; + + # Wrong mathod (POST) + $city_with_updated_field->{cityid} = 2; + + $tx = $t->ua->build_tx( + POST => "/api/v1/cities/$city_id" => json => $city_with_updated_field ); + $tx->req->cookies( + { name => 'CGISESSID', value => $authorized_session_id } ); + $tx->req->env( { REMOTE_ADDR => $remote_address } ); + $t->request_ok($tx)->status_is(404); }; subtest 'delete() tests' => sub { -- 2.39.5