Browse Source

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 <jonathan.druart@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
16.11.x
Martin Renvoize 6 years ago
committed by Kyle M Hall
parent
commit
e08639129a
  1. 16
      Koha/REST/V1/Cities.pm
  2. 42
      api/v1/swagger/definitions/city.json
  3. 122
      api/v1/swagger/paths/cities.json
  4. 3
      api/v1/swagger/x-primitives.json
  5. 142
      t/db_dependent/api/v1/cities.t

16
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 {

42
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"]
}

122
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": {

3
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"],

142
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 {

Loading…
Cancel
Save