|
Description
Tomás Cohen Arazi (tcohen)
2016-10-11 13:43:12 UTC
Created attachment 56189 [details] [review] Bug 17428: [REST] Cities endpoint unit tests This patch introduces unit tests for the cities endpoint. Created attachment 56190 [details] [review] Bug 17428: [REST] Cities swagger specification This patch adds the swagger definitions for the /cities endpoint Created attachment 56191 [details] [review] Bug 17428: [REST] Koha::REST::V1::Cities introduced This patch introduces full CRUD for Koha::Cit(ies|y) classes through the REST api. To test, point your browser to /api/v1/cities to use HTTPRequester/Postman (or the like). And of course, run the unit tests: - Apply the patches - Update your minified swagger file: $ cd kohaclone/ $ misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json \ -d api/v1/swagger/swagger.min.json - Run: $ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests should return green, and no warnings. - Sign off :-D I'm getting raptor not found when pointing my browser at it! (In reply to Claire Gravely from comment #4) > I'm getting raptor not found when pointing my browser at it! ignore me! Created attachment 56226 [details] [review] Bug 17428: [REST] Cities endpoint unit tests This patch introduces unit tests for the cities endpoint. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56227 [details] [review] Bug 17428: [REST] Cities swagger specification This patch adds the swagger definitions for the /cities endpoint Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56228 [details] [review] Bug 17428: [REST] Koha::REST::V1::Cities introduced This patch introduces full CRUD for Koha::Cit(ies|y) classes through the REST api. To test, point your browser to /api/v1/cities to use HTTPRequester/Postman (or the like). And of course, run the unit tests: - Apply the patches - Update your minified swagger file: $ cd kohaclone/ $ misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json \ -d api/v1/swagger/swagger.min.json - Run: $ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests should return green, and no warnings. - Sign off :-D Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56261 [details] [review] Bug 17428: [REST] Koha::REST::V1::Cities introduced This patch introduces full CRUD for Koha::Cit(ies|y) classes through the REST api. To test, point your browser to /api/v1/cities to use HTTPRequester/Postman (or the like). And of course, run the unit tests: - Apply the patches - Update your minified swagger file: $ cd kohaclone/ $ misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json \ -d api/v1/swagger/swagger.min.json - Run: $ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests should return green, and no warnings. - Sign off :-D Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56314 [details] [review] 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 I have added a followup with some minor alterations to encourage Restful best practice a little more. The biggest change is to always expect full object for PUT requests. We have some options to discus should we wish to implement partial updates, but I believe that should be left for it's own bug. Just as a quick overview, we have two options for partial updates: 1) Individual endpoints (hopefully an automated process to create them could be developed) for each field in the object. 2) Defining a HTTP PATCH method for each overall object endpoint using somthing along the lines of JSON-Patch (http://jsonpatch.com/) or JSONT (JSON Transforms) for delivering patch instructions Comment on attachment 56314 [details] [review] Bug 174828: Followup - More RESTfull handling of endpoints Review of attachment 56314 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/api/v1/cities.t @@ +96,5 @@ > $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 } ); > + $t->request_ok($tx)->status_is(200) > + ->json_is( [ $city, $another_city, $city_with_another_country ] ); I'd like to make sure this is the expected behaviour. It will make debug less easier. Could you find resources/best practices about that please? POST with id return html with "<head><title>Server error</title></head>" DBD::mysql::st execute failed: Duplicate entry '1' for key 'PRIMARY' Not sure it's expected ;) POST with non-existing id allows me to choose the id. (In reply to Jonathan Druart from comment #12) > Comment on attachment 56314 [details] [review] [review] > Bug 174828: Followup - More RESTfull handling of endpoints > > Review of attachment 56314 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: t/db_dependent/api/v1/cities.t > @@ +96,5 @@ > > $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 } ); > > + $t->request_ok($tx)->status_is(200) > > + ->json_is( [ $city, $another_city, $city_with_another_country ] ); > > I'd like to make sure this is the expected behaviour. It will make debug > less easier. > Could you find resources/best practices about that please? From an api consumers stand point I totally agree it's nicer to receive an error as apposed to silently throwing away the incorrect parameters. I'm going to implement a standard error response for such a case in it's own bug and make this patch depend upon it. Created attachment 56501 [details] [review] 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 Created attachment 56544 [details] [review] Bug 17428: Just some refactoring The idea is to make the code more readable moving the code to its own subroutine Created attachment 56545 [details] [review] Bug 17428: Move the params check after the authentication check If the user is not authorised to call this route, we would prefer to raise a 403 instead of 400 Note that we wanted to submit tests for this change but the city code does not let use do that (we are allowed to list/show cities even without any permissions). The patrons.t is not complete enought and the holds.t tests do not pass... Tomas plans to submit tests but we reach the end of the hackfest ;) Created attachment 56548 [details] [review] Bug 17428: Remove debug statement Created attachment 56553 [details] [review] Bug 174828: QA Followup - Add readOnly marker to cityid This should prevent api consumers from specifying an id on PUT or POST requests and thus restrict them from creating new cities with id's of their choosing, or worse updating cities via the POST method Created attachment 56554 [details] [review] Bug 17428: [REST] Cities endpoint unit tests This patch introduces unit tests for the cities endpoint. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56555 [details] [review] Bug 17428: [REST] Cities swagger specification This patch adds the swagger definitions for the /cities endpoint Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56556 [details] [review] Bug 17428: [REST] Koha::REST::V1::Cities introduced This patch introduces full CRUD for Koha::Cit(ies|y) classes through the REST api. To test, point your browser to /api/v1/cities to use HTTPRequester/Postman (or the like). And of course, run the unit tests: - Apply the patches - Update your minified swagger file: $ cd kohaclone/ $ misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json \ -d api/v1/swagger/swagger.min.json - Run: $ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests should return green, and no warnings. - Sign off :-D Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56557 [details] [review] 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 Created attachment 56558 [details] [review] Bug 17428: Remove debug statement Created attachment 56559 [details] [review] Bug 17428: QA Followup - Add readOnly marker to cityid This should prevent api consumers from specifying an id on PUT or POST requests and thus restrict them from creating new cities with id's of their choosing, or worse updating cities via the POST method Please note, the last followup include a currently failing test case.. The failure is in the upstream library (::Swagger2), and I intend to attempt to fix it there. In short, the readOnly marker doesn't appear to handle the difference between existence and definedness correctly yet. Created attachment 56724 [details] [review] QA Followup - Fix failing test Failing test case is due to random array order, so the best we can do is check for the presence of the errors key. Final followup, I was wrong in my last comment.. Swagger Validator is in fact doing the right thing it's just that we can't easily test against it. We are given an array or validation errors. Those errors are in the order in which the hash structure is validated.. so due to Perl randomisation the error order varies and thus we cannot easily test the actual error message when there is more than one. I have amended the test to simply check for the presence of the /errors key which should suffice. Created attachment 56741 [details] [review] 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. Only the final [REST best practice Followup] needs signing off, would love to get some movement on this again before feature freeze ;) Comment on attachment 56555 [details] [review] Bug 17428: [REST] Cities swagger specification Review of attachment 56555 [details] [review]: ----------------------------------------------------------------- Minor query regarding returned serialised bodies ::: api/v1/swagger/definitions/city.json @@ +3,5 @@ > + "properties": { > + "cityid": { > + "$ref": "../x-primitives.json#/cityid" > + }, > + "city_name": { Does this not break our own rule of following the UI term.. I feel this should simply be 'name' and not expose the db field names to the end user (the api consumer in this case). Please try to separate indentation fixes and other changes. Created attachment 56800 [details] [review] Bug 17428: [REST] Cities endpoint unit tests This patch introduces unit tests for the cities endpoint. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56801 [details] [review] Bug 17428: [REST] Cities swagger specification This patch adds the swagger definitions for the /cities endpoint Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56802 [details] [review] Bug 17428: [REST] Koha::REST::V1::Cities introduced This patch introduces full CRUD for Koha::Cit(ies|y) classes through the REST api. To test, point your browser to /api/v1/cities to use HTTPRequester/Postman (or the like). And of course, run the unit tests: - Apply the patches - Update your minified swagger file: $ cd kohaclone/ $ misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json \ -d api/v1/swagger/swagger.min.json - Run: $ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests should return green, and no warnings. - Sign off :-D Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 56803 [details] [review] 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> Created attachment 56804 [details] [review] Bug 17428: Remove warning Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Everything works perfectly! Created attachment 56807 [details] [review] Bug 17428: [REST] Cities endpoint unit tests This patch introduces unit tests for the cities endpoint. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56808 [details] [review] Bug 17428: [REST] Cities swagger specification This patch adds the swagger definitions for the /cities endpoint Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56809 [details] [review] Bug 17428: [REST] Koha::REST::V1::Cities introduced This patch introduces full CRUD for Koha::Cit(ies|y) classes through the REST api. To test, point your browser to /api/v1/cities to use HTTPRequester/Postman (or the like). And of course, run the unit tests: - Apply the patches - Update your minified swagger file: $ cd kohaclone/ $ misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json \ -d api/v1/swagger/swagger.min.json - Run: $ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests should return green, and no warnings. - Sign off :-D Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56810 [details] [review] 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> Created attachment 56811 [details] [review] Bug 17428: (QA followup) Remove warning Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56812 [details] [review] Bug 17428: (QA followup) Fix community site in swagger file Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Jonathan Druart from comment #33) > Please try to separate indentation fixes and other changes. Oops, Sorry about that.. been working on another project too long and forgot to comment out the 'tidy before commit' line in my git hooks. Woop, woop! Pushed to master for 16.11, thanks Tomas, Martin, Jonathan! |