-
Notifications
You must be signed in to change notification settings - Fork 34
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
alapto
wants to merge
3
commits into
maasglobal:develop_debrecated
Choose a base branch
from
alapto:tsp-retrieve-customer-data
base: develop_debrecated
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| { | ||
| "$id": "http://maasglobal.com/core/components/personal-data-query.json", | ||
| "description": "MaaS personal data query parameters schema", | ||
| "definitions": { | ||
| "queryParams": { | ||
| "type": "object", | ||
| "properties": { | ||
| "customer[identityId]": { | ||
| "$ref": "http://maasglobal.com/core/components/units.json#/definitions/identityId" | ||
| }, | ||
| "customer[firstName]": { | ||
| "description": "First name of the customer (e.g. John)", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/personalName" | ||
| }, | ||
| "customer[lastName]": { | ||
| "description": "Last name of the customer (e.g. Doe)", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/personalName" | ||
| }, | ||
| "customer[phone]": { | ||
| "description": "ITU-T E.164 phone number", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/phone" | ||
| }, | ||
| "customer[email]": { | ||
| "description": "Rough validation of a valid e-mail address", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/email" | ||
| }, | ||
| "email": { | ||
| "description": "Rough validation of a valid e-mail address", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/email" | ||
| }, | ||
| "customer[address]": { | ||
| "$ref": "http://maasglobal.com/core/components/address.json#/definitions/address" | ||
| }, | ||
| "customer[city]": { | ||
| "$ref": "http://maasglobal.com/core/components/address.json#/definitions/city" | ||
| }, | ||
| "customer[country]": { | ||
| "$ref": "http://maasglobal.com/core/components/address.json#/definitions/country" | ||
| }, | ||
| "customer[zipCode]": { | ||
| "$ref": "http://maasglobal.com/core/components/address.json#/definitions/zipCode" | ||
| }, | ||
| "customer[locale]": { | ||
| "$ref": "http://maasglobal.com/core/components/i18n.json#/definitions/locale" | ||
| }, | ||
| "customer[appInstanceId]": { | ||
| "description": "An id specific to a user device", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/appInstanceId" | ||
| }, | ||
| "customer[opaqueId]": { | ||
| "description": "Typically the hash of the identityId", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/opaqueId" | ||
| }, | ||
| "customer[clientId]": { | ||
| "description": "An id indicating the source of the client", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/clientId" | ||
| }, | ||
| "customer[dob]": { | ||
| "description": "The customer's date of birth or boolean indicating if the value is already in DB", | ||
| "$ref": "http://maasglobal.com/core/components/units.json#/definitions/isoDate" | ||
| }, | ||
| "customer[ssid]": { | ||
| "description": "Social Security ID", | ||
| "$ref": "http://maasglobal.com/core/components/common.json#/definitions/ssid" | ||
| }, | ||
| "customer[subscriberType]": { | ||
| "description": "Can indicate if subscriber is using paid or free plan", | ||
| "type": "string", | ||
| "minLength": 3, | ||
| "maxLength": 255 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "mode": "TAXI", | ||
| "startTime": 1475850000000, | ||
| "endTime": 1475860000000, | ||
| "from": "-60.00,24.00", | ||
| "to": "+60.05,-24.05", | ||
| "fromRadius": 100, | ||
| "toRadius": 10, | ||
| "customer[opaqueId]": "1ca7f4853bc724caa58645552c842ea21543337fe4731e2222de7e1668e1facf" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "tspId": "tsp-id", | ||
| "customer[notInSchema]": "passedThrough", | ||
| "customer[phone]": "+35855544433" | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
ontrackand 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.