Description
Martin Renvoize (ashimema)
2016-10-14 08:06:01 UTC
Created attachment 56487 [details] [review] BUG 17455: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Created attachment 56488 [details] [review] BUG 17455: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body Tests coming in followup Created attachment 56500 [details] [review] BUG 17445: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body Created attachment 56546 [details] [review] Bug 17445: Just some refactoring The idea is to make the code more readable moving the code to its own subroutine Created attachment 56547 [details] [review] Bug 17445: 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 ;) (In reply to Jonathan Druart from comment #5) > Created attachment 56546 [details] [review] [review] > Bug 17445: Just some refactoring > > The idea is to make the code more readable moving the code to its own > subroutine Totally agree with this.. I was intending on doing this one myself but wanted to get the quick and dirty patch up here as quick as possible to get feedback on the general method :) (In reply to Jonathan Druart from comment #6) > Created attachment 56547 [details] [review] [review] > Bug 17445: 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 ;) Also agree with this change.. it didn't even occur to me to think about error code presidency in this case.. Generically I think you tend to just work backwards down the error codes, so your checking for a 403 failure before a more generic 400 is perfect in this case. Good spot! :) Created attachment 56634 [details] [review] [SIGNED-OFF] BUG 17445: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body https://bugs.koha-community.org/show_bug.cgi?id=17445 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56635 [details] [review] [SIGNED-OFF] Bug 17445: Just some refactoring The idea is to make the code more readable moving the code to its own subroutine Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56636 [details] [review] [SIGNED-OFF] Bug 17445: 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 ;) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56659 [details] [review] Bug 17445: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body https://bugs.koha-community.org/show_bug.cgi?id=17445 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56660 [details] [review] Bug 17445: Just some refactoring The idea is to make the code more readable moving the code to its own subroutine Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56661 [details] [review] Bug 17445: 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 ;) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 16.11, thanks Jonathan, Martin! |