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

Schema stitching fails when type/field names collide with JS built-ins #1107

Closed
gnidan opened this issue Apr 16, 2019 · 1 comment · Fixed by #1336
Closed

Schema stitching fails when type/field names collide with JS built-ins #1107

gnidan opened this issue Apr 16, 2019 · 1 comment · Fixed by #1336

Comments

@gnidan
Copy link

gnidan commented Apr 16, 2019

When using the field name constructor in my schema, graphql-tools fails to perform transformations inside delegateToSchema: it incorrectly thinks here that the mapping already exists, and thus throws an exception.

To inline the if statement:

if (this.mapping[actualTypeName][field]) {
  this.mapping[actualTypeName][field].push(parsedFragment);
} else {
  this.mapping[actualTypeName][field] = [parsedFragment];
}

In this case, since field is equal to "constructor", the object this.mapping[actualTypeName] has a regular JS constructor by default, so the if case executes even though the else case would be more correct.

I'm about to open a PR with a possible fix, to use JS Map objects instead of plain {} objects. Please advise if another approach would be better!

Thanks!

yaacovCR added a commit that referenced this issue Apr 3, 2020
yaacovCR added a commit that referenced this issue Apr 3, 2020
closes #1107 and #1108

 - Use Object.create(null) for maps to remove prototype, which allows use
of in operator, former modelled after use in graphql-js, latter code
style choice to show that object is being used as a map.
 - Create utility for Object.prototype.hasOwnProperty.call
 - Use graphql-js toObjMap, keyMap, and keyValMap helpers to streamline
code.
yaacovCR added a commit that referenced this issue Apr 3, 2020
closes #1107 and #1108

 - Use Object.create(null) for maps to remove prototype, which allows use
of in operator, former modelled after use in graphql-js, latter code
style choice to show that object is being used as a map.
 - Create utility for Object.prototype.hasOwnProperty.call
 - Use graphql-js toObjMap, keyMap, and keyValMap helpers to streamline
code.
@yaacovCR
Copy link
Collaborator

yaacovCR commented Apr 7, 2020

Should be fixed by #1336, track #1308.

Thank you for bringing attention to this.

@yaacovCR yaacovCR closed this as completed Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants