Attachment 56810 Details for Bug 17428 – Bug 17428: [REST] best practice followup

[patch] Bug 17428: [REST] best practice followup

Bug-17428-REST-best-practice-followup.patch (text/plain), 18.07 KB, created by Tomás Cohen Arazi (tcohen) on 2016-10-24 16:08:37 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2016-10-24 16:08:37 UTC

Size: 18.07 KB

patch

obsolete

>From bd595bc9512c483df656ecf60f075b2702718383 Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>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 <jonathan.druart@biblibre.com>
>Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
>---
> 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 1b04e10..d9d2774 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 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/api/v1/swagger/x-primitives.json b/api/v1/swagger/x-primitives.json
>index f5ff2e9..1b90b30 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 0b1cd57..e6e13c6 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.7.4

Actions: View | Diff | Splinter Review

Attachments on bug 17428: 56189 | 56190 | 56191 | 56226 | 56227 | 56228 | 56261 | 56314 | 56501 | 56544 | 56545 | 56548 | 56553 | 56554 | 56555 | 56556 | 56557 | 56558 | 56559 | 56724 | 56741 | 56800 | 56801 | 56802 | 56803 | 56804 | 56807 | 56808 | 56809 | 56810 | 56811 | 56812