From 38321bbc7a35f24d43bc45c89b57d6feb9ed8469 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 12 Oct 2016 21:31:30 +0000 Subject: [PATCH] Bug 174828: Followup - More RESTfull handling of endpoints * 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 * Update tests to check for swagger validation errors instead of koha exceptions https://bugs.koha-community.org/show_bug.cgi?id=17428 --- Koha/REST/V1/Cities.pm | 21 ++++-- api/v1/swagger/definitions/city.json | 42 ++++++------ api/v1/swagger/paths/cities.json | 122 ++++++++++++++++++++--------------- t/db_dependent/api/v1/cities.t | 86 ++++++++++++++++++------ 4 files changed, 170 insertions(+), 101 deletions(-) diff --git a/Koha/REST/V1/Cities.pm b/Koha/REST/V1/Cities.pm index 1b04e10..08eaac9 100644 --- a/Koha/REST/V1/Cities.pm +++ b/Koha/REST/V1/Cities.pm @@ -29,10 +29,19 @@ sub list { my ( $c, $args, $cb ) = @_; my $cities; + my $filter; + + $filter->{city_name} = { LIKE => $args->{city_name}."%" } + if defined( $args->{city_name} ); + $filter->{city_state} = { LIKE => $args->{city_state}."%" } + if defined( $args->{city_state} ); + $filter->{city_country} = { LIKE => $args->{city_country}."%" } + if defined( $args->{city_country} ); + $filter->{city_zipcode} = { LIKE => $args->{city_zipcode}."%" } + if defined( $args->{city_zipcode} ); 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 +69,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 +93,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 a1d7d40..f322136 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 9727bcd..650d224 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/t/db_dependent/api/v1/cities.t b/t/db_dependent/api/v1/cities.t index 0b1cd57..b5d20ff 100644 --- a/t/db_dependent/api/v1/cities.t +++ b/t/db_dependent/api/v1/cities.t @@ -32,6 +32,7 @@ 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 +42,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 +83,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 +92,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 +129,7 @@ subtest 'get() tests' => sub { subtest 'add() tests' => sub { - plan tests => 10; + plan tests => 11; $schema->storage->txn_begin; @@ -163,6 +162,7 @@ subtest 'add() tests' => sub { ->json_is( '/city_zipcode' => $city->{city_zipcode} ) ->json_is( '/city_country' => $city->{city_country} ); + # Authorized attempt to write invalid data my $city_with_invalid_field = { city_blah => "City Blah", city_state => "City State", @@ -170,20 +170,26 @@ subtest 'add() tests' => sub { city_country => "City Country" }; - # Authorized attempt to write invalid data $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(500); + $t->request_ok($tx)->status_is(400)->json_is( + "/errors" => [ + { + message => "Properties not allowed: city_blah.", + path => "/body" + } + ] + ); $schema->storage->txn_rollback; }; subtest 'update() tests' => sub { - plan tests => 10; + plan tests => 13; $schema->storage->txn_begin; @@ -202,26 +208,64 @@ 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 } ); -- 2.1.4