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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX release] Validate JSON API docs returned by normalizeResponse #3608

Merged
merged 1 commit into from
Aug 3, 2015
Merged

[BUGFIX release] Validate JSON API docs returned by normalizeResponse #3608

merged 1 commit into from
Aug 3, 2015

Conversation

mike-north
Copy link
Contributor

Fixes #3603

JSON API documents returned by normalizeResponse now are validated in the following ways

Ensure that...

  • the document is an object
  • at least one of the following top-level keys exist in said object: data, meta, errors
  • top-level keys "data" and "errors" are not both present
  • data, if present, is either null, an array or an object
  • errors, if present, is an array
  • links, if present, is an object
  • meta, if present, is an object
  • include, if present, is an array
  • jsonapi, if present, is an object

@mike-north mike-north force-pushed the json-api-validation branch from c93a4f1 to 926b966 Compare July 29, 2015 21:09
stefanpenner
stefanpenner reviewed Jul 29, 2015
View reviewed changes
packages/ember-data/lib/system/store/serializer-response.js
*/
export function hasJsonApiDocumentValidationErrors(doc) {
let errors = [];
if (Ember.typeOf(doc) !== 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

doc && typeof object === 'object

@mike-north mike-north force-pushed the json-api-validation branch 2 times, most recently from 99feb94 to e983e26 Compare July 29, 2015 21:43
@mike-north
Copy link
Contributor Author

Thanks for feedback @stefanpenner . I have addressed it, and also reformatted the error messages
screen shot 2015-07-29 at 2 42 53 pm

In the future, if we get more sophisticated about JSON API validation, there may be multiple validation errors returned for a document

stefanpenner
stefanpenner reviewed Jul 29, 2015
View reviewed changes
packages/ember-data/lib/system/store/serializer-response.js
!('meta' in doc)) {
errors.push('One or more of the following keys must be present: "data", "errors", "meta".');
} else {
if (('data' in doc) && ('errors' in doc)) {
Copy link
Member

Choose a reason for hiding this comment

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

do we also need to check that the properties have object values, or are they allow to be of any type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

arrays or objects are allowed as values. I'm not sure where we want this to end, because we could then validate the objects (or arrays) themselves, including relationships, etc...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with just checking that the keys exists for now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I'll go ahead and implement these validations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now I have validations in place for meta, data, errors, links, jsonapi and included. This should provide complete validation of the top-level of a JSON API document

@mike-north mike-north force-pushed the json-api-validation branch 2 times, most recently from 5505ee9 to 7cf819f Compare July 29, 2015 22:49
@mike-north
Copy link
Contributor Author

This seems to now be complete AFAIK.

bmac
bmac reviewed Jul 30, 2015
View reviewed changes
packages/ember-data/lib/system/store/serializer-response.js
}
if ('data' in doc) {
if (!(doc.data === null || Ember.isArray(doc.data) || typeof doc.data === 'object')) {
errors.push('data must be null, an object, or an array');
Copy link
Member

Choose a reason for hiding this comment

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

Whats the difference between this error and the one on line 31?

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 29-33 seems identical with 34-38.

@bmac
Copy link
Member

bmac commented Jul 30, 2015

Looks good. Do we want to prefix this commit with [BUGFIX release] and include in in 1.13.x?

@stefanpenner
Copy link
Member

Looks good. Do we want to prefix this commit with [BUGFIX release] and include in in 1.13.x?
Show all checks

yes please (just pinged mike)

@mike-north mike-north force-pushed the json-api-validation branch from 7cf819f to 0c6032b Compare July 30, 2015 18:53
@mike-north mike-north changed the title Validate JSON API docs returned by normalizeResponse [BUGFIX release] Validate JSON API docs returned by normalizeResponse Jul 30, 2015
@mike-north
Copy link
Contributor Author

@bmac done

stefanpenner
stefanpenner reviewed Jul 30, 2015
View reviewed changes
packages/ember-data/lib/system/store/serializer-response.js
@@ -16,7 +80,8 @@ const get = Ember.get;
*/
export function normalizeResponseHelper(serializer, store, modelClass, payload, id, requestType) {
let normalizedResponse = serializer.normalizeResponse(store, modelClass, payload, id, requestType);

let validationErrors = validateDocumentStructure(normalizedResponse);
Copy link
Member

Choose a reason for hiding this comment

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

this call should happen in the assert block, or it wont get stripped for prod builds

@stefanpenner
Copy link
Member

left one blocking comment.

@mike-north mike-north force-pushed the json-api-validation branch from 0c6032b to 353889a Compare July 30, 2015 21:21
@mike-north
Copy link
Contributor Author

@stefanpenner should be good now. I need the data in the assert description, so wrapped the whole thing in a Ember.runInDebug

@stefanpenner
Copy link
Member

no longer caching node_modules #3616

@mike-north
Copy link
Contributor Author

No idea why this continues to fail, but it seems to be unrelated to the proposed changes

Stef clarified this for me. The way defeatureify works internally disallows nested "defeaturees" (i.e., an assert in a runInDebug)

@mike-north mike-north force-pushed the json-api-validation branch from 353889a to 58dbdb7 Compare July 30, 2015 22:13
@wecc
Copy link
Contributor

wecc commented Jul 31, 2015

Let's remove one of the duplicate checks (mentioned above) and merge this :shipit:

@mike-north mike-north force-pushed the json-api-validation branch from 58dbdb7 to 782c5b1 Compare July 31, 2015 22:33
@bmac
Copy link
Member

bmac commented Aug 3, 2015

Looks good. Thanks @mike-north

bmac added a commit that referenced this pull request Aug 3, 2015
@bmac
Merge pull request #3608 from mike-north/json-api-validation
3366e85 
[BUGFIX release] Validate JSON API docs returned by normalizeResponse
@bmac bmac merged commit 3366e85 into emberjs:master Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mike-north @bmac @stefanpenner @wecc