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

Fix: Don't crash when mediaItem.URLs is null #4982

Merged
merged 1 commit into from
Feb 13, 2019
Merged

Conversation

focusaurus
Copy link
Contributor

Impact: minor
Type: bugfix

Issue

With some imperfect development data we have some catalog products that have a primaryImage.URLs: null. This situation causes the destructuring to fail with this error:

GraphQL error: Cannot destructure property `large` of 'undefined' or 'null'.
    at new ApolloError (/usr/local/src/node_modules/apollo-client/errors/ApolloError.js:25:1)

Solution

Return null early and avoid the exception.

Testing

This module was previously barren of unit tests. I added some basic tests that confirm this fix plus the base case behavior.

@focusaurus focusaurus added the bug For issues that describe a defect or regression in the released software label Feb 12, 2019
@focusaurus focusaurus self-assigned this Feb 12, 2019
@aldeed
Copy link
Contributor

aldeed commented Feb 13, 2019

@focusaurus Very happy to merge unit tests, but as a general rule I don't think we should have to guard against values that the schema doesn't allow. In the ImageInfo SimpleSchema, URLs is required (but can be an empty object). So my bigger concern would be finding out where we are not validating against that schema and fixing it to validate. We should not let such bad data be saved in the first place.

@aldeed aldeed merged commit dbbaf79 into develop Feb 13, 2019
@aldeed aldeed deleted the fix-media-without-URLs branch February 13, 2019 15:41
@dancastellon
Copy link
Contributor

Following up here - URLs isn't present for Cloudinary images because that object is build in the GraphQL resolver.

@aldeed
Copy link
Contributor

aldeed commented Feb 13, 2019

@dancastellon It would still need to be set to {}, though, or it will fail the schema validation. Otherwise we can make that field optional in the schema, but I'm not sure that makes sense to do outside of the Cloudinary integration use case.

@jeffcorpuz jeffcorpuz mentioned this pull request Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For issues that describe a defect or regression in the released software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants