Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Personal data query parameter validation #353

Open
wants to merge 3 commits into
base: develop_debrecated
Choose a base branch
from

Conversation

alapto
Copy link
Contributor

@alapto alapto commented Jan 11, 2019

Validate personal data query parameters that can be passed to TSP on read-by-id and options-list requests.

Todos:

  • Write tests

Versioning:

  • Breaking change

"queryParams": {
"type": "object",
"properties": {
"customer[identityId]": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think fact that GET query is flat should not affect JSON representation that we validate.

We had similar situation in ontrack and over there @hieuunguyeen used qs to unserialize nested object that came with GET before validation.

That we way we may reuse validation for same nested structures across GET and POST requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otrack querystring validation schema is here https://github.com/maasglobal/maas-schemas/blob/develop/schemas/tsp/journey-planner/request.json#L5-L8

Not really performing proper validation

Copy link
Contributor

Choose a reason for hiding this comment

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

I just remember discussing with @hieuunguyeen nesting properties, but didn't look into outcome.

Anyway I think we should just reuse same structures across GET and POST requests, and configure schema only for normal object structures not influenced by limitation of a transport method.

Copy link
Contributor Author

@alapto alapto Jan 11, 2019

Choose a reason for hiding this comment

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

Not dismissing your idea, it could work nicely but requires more effort and in that case I would propose postponing this PR and first merging the TSP changes that we need to get for next release and then apply validation on release after that

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that schemas should be validating the raw request coming in, regardless of transportation method.
Defining schema with normal object structure hinting that request is required to go through parser before it could be validated.

Copy link
Contributor

@medikoo medikoo Jan 22, 2019

Choose a reason for hiding this comment

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

I also think that schemas should be validating the raw request coming in,

@hieuunguyeen if we follow this advice, then schema's should validate query strings passed in direct form (so e.g. string "foo=bar&other=etc" should be passed to schema validate).

JSON schemas are purely about JSON objects, and to comply to that, we already do not validate raw request, but parse query string format into JSON before validation. I don't think it should be problematic to improve serialization/deserialization method (we already apply on our ends) so it can also handle nested object structures.

Big downside of what's proposed here is that it forces us to maintain two schemas for exactly same data (once handled in format as { customer: { lastName, firstName } }, and then as { "customer[firstName]" , "customer[lastName]" }.

Design and maintenance wise it looks quite bad to me.

@alapto alapto removed their assignment Jan 11, 2019
@medikoo medikoo assigned alapto and unassigned medikoo Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants