Attachment 56557 Details for Bug 17428 – Bug 17428: Followup - More RESTfull handling of endpoints

[patch] Bug 17428: Followup - More RESTfull handling of endpoints

Bug-17428-Followup---More-RESTfull-handling-of-end.patch (text/plain), 15.11 KB, created by Martin Renvoize (ashimema) on 2016-10-14 15:31:44 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2016-10-14 15:31:44 UTC

Size: 15.11 KB

patch

obsolete

>From 59e4a385803de9442f75ad17e704720cbb5c2593 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: 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
>---
> 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

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