Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Raise 400 error for invalid contributor param type #433

Merged
merged 1 commit into from Mar 29, 2019

Conversation

kellyi
Copy link
Contributor

@kellyi kellyi commented Mar 28, 2019

Overview

Raise a 400 error if not all of the contributor querystring parameters
are integers, sending back a detailed error response about the type.

The other custom parameters already expect strings, so they don't fail
in the same way, and the pagination params have their own error
handling. Therefore we don't need to add similar handling to the other
errors.

Connects #424

Demo

Screen Shot 2019-03-28 at 4 25 35 PM

Screen Shot 2019-03-28 at 4 25 48 PM

Testing Instructions

  • resetdb then processfixtures
  • serve this branch
  • visit http://localhost:8081/api/docs/#!/facilities/facilities_list and try entering a string value in the contributors input, then search and verify that it returns 400
  • change the string value to and int, like 2, and verify that it returns 200 and that you see a FeatureCollection
  • visit the main app on 6543 then search for facilities in various ways and verify that that also still works

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

@kellyi kellyi force-pushed the ki/return-400-for-incorrect-api-params branch from 0b1d533 to e27725c Compare March 28, 2019 20:30
@kellyi kellyi requested a review from jwalgran March 28, 2019 20:37
Copy link
Contributor

@rajadain rajadain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 tested, works well.

I tried briefly to see if this could be done with validators, but they would require having a serializer for the payload which we currently don't.

On one hand that feels like a heavier lift. On another, maybe it would be a good idea to have well-defined parameters. Maybe for a future task.

@kellyi
Copy link
Contributor Author

kellyi commented Mar 29, 2019

Good point. It appears that we can do this using a serializer on its own -- https://tinyendian.com/articles/parsing-query-parameters-in-rest-framework/ -- so I will update this to do that.

@kellyi kellyi force-pushed the ki/return-400-for-incorrect-api-params branch 2 times, most recently from 494e2d0 to bbf02ed Compare March 29, 2019 15:56
@kellyi
Copy link
Contributor Author

kellyi commented Mar 29, 2019

bbf02ed is an amended commit that users a custom serializer + the built-in ValidationError for handling errors related to the queryparams

Add a FacilityQueryParamsSerializer to validate the types of the
querystring parameters provided to the /api/facilities/ endpoint & send
back a 400 error if any of the parameters has an invalid type.
@kellyi kellyi force-pushed the ki/return-400-for-incorrect-api-params branch from bbf02ed to c3f867b Compare March 29, 2019 16:08
@kellyi
Copy link
Contributor Author

kellyi commented Mar 29, 2019

Rebased on develop

Copy link
Contributor

@rajadain rajadain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 clean, elegant solution.

@kellyi kellyi removed the request for review from jwalgran March 29, 2019 16:15
@kellyi kellyi assigned kellyi and unassigned jwalgran Mar 29, 2019
@kellyi
Copy link
Contributor Author

kellyi commented Mar 29, 2019

Thanks!

@kellyi kellyi merged commit 6be9551 into develop Mar 29, 2019
@kellyi kellyi deleted the ki/return-400-for-incorrect-api-params branch March 29, 2019 16:16
Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was delighted by how simple and declarative the serializer solution was but I wondered how it was dealing with the fact that a query string argument could appear multiple times. This lead me to an edge case:

This returns 400 like we want
http://localhost:8081/api/facilities/?contributors=1&contributors=two

This, unfortunately, still returns 500
http://localhost:8081/api/facilities/?contributors=one&contributors=2

@kellyi
Copy link
Contributor Author

kellyi commented Mar 29, 2019

This lead me to an edge case:

Oof, I merged the PR before I saw this. Will make a new PR to fix.

kellyi pushed a commit that referenced this pull request Mar 29, 2019
Fixes a bug discovered in #433 whereby a list of queryparamters would
not typecheck correctly.

Connects #424
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants