Skip to content
This repository has been archived by the owner on Apr 15, 2020. It is now read-only.

null error annotating children with errors at stitching #26

Closed
chess777 opened this issue Nov 6, 2019 · 4 comments
Closed

null error annotating children with errors at stitching #26

chess777 opened this issue Nov 6, 2019 · 4 comments

Comments

@chess777
Copy link

chess777 commented Nov 6, 2019

I'm using graphql-tools-fork v7.0.3
NodeJS v12.8.1, tried also on v13.0.1
I have a following schema:

type Query {
  getType1(): Type1
}

type Type1 {
  type2: [Type2]!
}

type Type2{
  mandatoryField: String!
}

If I by error don't fill mandatoryField, then annotateWithChildrenErrors will fail with error:

{
  "stack": "TypeError: Cannot set property 'Symbol(subSchemaErrors)' of null\n    at annotateWithChildrenErrors (<path>api\\node_modules\\graphql-tools-fork\\src\\stitching\\errors.ts:73:23)\n    at <path>api\\node_modules\\graphql-tools-fork\\src\\stitching\\errors.ts:68:37\n    at Array.forEach (<anonymous>)\n    at Object.annotateWithChildrenErrors (<path>api\\node_modules\\graphql-tools-fork\\src\\stitching\\errors.ts:68:12)\n    at Object.handleResult (<path>api\\node_modules\\graphql-tools-fork\\src\\stitching\\checkResultAndHandleErrors.ts:63:5)\n    at defaultMergedResolver (<path>api\\node_modules\\graphql-tools-fork\\src\\stitching\\defaultMergedResolver.ts:24:10)\n    at field.resolve (<path>api\\node_modules\\graphql-tools-fork\\src\\stitching\\mergeSchemas.ts:275:16)\n    at field.resolve (<path>api\\node_modules\\graphql-extensions\\src\\index.ts:274:18)\n    at resolveFieldValueOrError (<path>api\\node_modules\\graphql\\execution\\execute.js:467:18)\n    at resolveField (<path>api\\node_modules\\graphql\\execution\\execute.js:434:16)\n    at executeFields (<path>api\\node_modules\\graphql\\execution\\execute.js:275:18)\n    at collectAndExecuteSubfields (<path>api\\node_modules\\graphql\\execution\\execute.js:713:10)\n    at completeObjectValue (<path>api\\node_modules\\graphql\\execution\\execute.js:703:10)\n    at completeValue (<path>api\\node_modules\\graphql\\execution\\execute.js:591:12)\n    at completeValue (<path>api\\node_modules\\graphql\\execution\\execute.js:557:21)\n    at <path>api\\node_modules\\graphql\\execution\\execute.js:492:16",
  "message": "Cannot set property 'Symbol(subSchemaErrors)' of null"
}

After validation to annotateWithChildrenErrors comes object:

{
  type2: [
    null
  ]
}

And childrenErrors:

[
  {
    message: 'Cannot return null for non-nullable field Type2.mandatoryField.',
    path: ['type2', 0, 'mandatoryField']
  }
]

As error will be relocated it will reach such state when object is null and childrenErrors will still be an array - this will lead to null exception on line 73.
I've checked the code - v7.1.0 does not have it fixed.

yaacovCR added a commit that referenced this issue Nov 6, 2019
yaacovCR added a commit that referenced this issue Nov 6, 2019
@yaacovCR
Copy link
Owner

yaacovCR commented Nov 6, 2019

Fix only passes along the error if schemas is changed to

type Query {
  getType1(): Type1
}

type Type1 {
  type2: [Type2!]! // instead of [Type2]!
}

type Type2{
  mandatoryField: String!
}

@yaacovCR
Copy link
Owner

yaacovCR commented Nov 6, 2019

Does that work for now? I believe total fix requires wrapping stitched result instead of annotating it, because you can't annotate null, we get around that by throwing for fields immediately when null, but doesn't work with lists.

@chess777
Copy link
Author

chess777 commented Nov 7, 2019

Thank you! The fix works as you've described.

@chess777 chess777 closed this as completed Nov 7, 2019
yaacovCR added a commit that referenced this issue Nov 7, 2019
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
yaacovCR added a commit that referenced this issue Nov 7, 2019
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
@yaacovCR
Copy link
Owner

yaacovCR commented Nov 7, 2019

Should be fixed for reals now in 7.1.2.

yaacovCR added a commit that referenced this issue Dec 31, 2019
yaacovCR added a commit that referenced this issue Dec 31, 2019
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
yaacovCR added a commit that referenced this issue Dec 31, 2019
yaacovCR added a commit that referenced this issue Dec 31, 2019
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
yaacovCR added a commit that referenced this issue Jan 8, 2020
yaacovCR added a commit that referenced this issue Jan 8, 2020
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
yaacovCR added a commit that referenced this issue Jan 21, 2020
yaacovCR added a commit that referenced this issue Jan 21, 2020
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
yaacovCR added a commit that referenced this issue Feb 27, 2020
yaacovCR added a commit that referenced this issue Feb 27, 2020
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
yaacovCR added a commit that referenced this issue Mar 26, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
yaacovCR added a commit that referenced this issue Mar 26, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants