feat(executor): add schema coordinate to graphql errors #7588
Conversation
🦋 Changeset detectedLatest commit: 2b67fa6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
EmrysMyrddin
force-pushed
the
feat/executor/error-schema-coordinates
branch
from
42bb1f4 to
78dd816
Compare
📝 WalkthroughSummary by CodeRabbitRelease NotesNew Features
Tests
WalkthroughThis change adds an opt-in flag Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Execute as execute()
participant BuildCtx as buildExecutionContext()
participant Resolver as Field Resolver
participant ErrUtils as locatedError / errors.ts
Client->>Execute: execute(document, variables, { schemaCoordinateInErrors: true })
Execute->>BuildCtx: buildExecutionContext(args)
BuildCtx-->>Execute: ExecutionContext{ schemaCoordinateInErrors: true }
Activate Resolver
Resolver->>Execute: throws error / returns rejected
Execute->>ErrUtils: locatedError(rawError, nodes, path, info)
alt schemaCoordinateInErrors enabled
ErrUtils->>ErrUtils: compute coordinate from info (Parent.field)
ErrUtils-->>Execute: GraphQLError(with coordinate)
else
ErrUtils-->>Execute: GraphQLError(without coordinate)
end
Execute-->>Client: ExecutionResult(errors: [...])
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
💻 Website PreviewThe latest changes are available as preview in: https://pr-7588.graphql-tools.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/executor/src/execution/execute.ts(10 hunks)packages/executor/src/execution/locatedError.ts(1 hunks)packages/utils/src/errors.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/executor/src/execution/locatedError.ts (2)
packages/utils/src/types.ts (1)
Maybe(29-29)packages/utils/src/Interfaces.ts (1)
GraphQLResolveInfo(71-73)
packages/utils/src/errors.ts (2)
packages/utils/src/types.ts (1)
Maybe(29-29)packages/utils/src/Interfaces.ts (1)
GraphQLResolveInfo(71-73)
packages/executor/src/execution/execute.ts (3)
packages/executor/src/execution/locatedError.ts (1)
locatedError(5-14)packages/utils/src/Path.ts (1)
pathToArray(23-31)packages/executor/src/execution/coerceError.ts (1)
coerceError(1-24)
🪛 GitHub Check: Type Check on GraphQL v15
packages/executor/src/execution/locatedError.ts
[failure] 11-11:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/executor/src/execution/locatedError.ts
[failure] 11-11:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: deployment
- GitHub Check: Unit Test on Bun
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
🔇 Additional comments (9)
packages/utils/src/errors.ts (3)
2-2: LGTM: Import statement is correct.The import of
GraphQLResolveInfofrom the local Interfaces module is appropriate for the new parameter.
64-68: LGTM: Backward-compatible signature change.The addition of the optional
infoparameter maintains backward compatibility with existing callers while enabling schema coordinate enrichment.
75-80: Ignore null-safety concerns forinfo.parentTypeandinfo.fieldName. Both are required properties ofGraphQLResolveInfoand thus always defined wheninfois present.Likely an incorrect or invalid review comment.
packages/executor/src/execution/locatedError.ts (1)
5-14: Verify null safety for info properties.Similar to the concern in
errors.ts, ensure thatinfo.parentTypeandinfo.fieldNameare always defined when called, as there are no null checks before accessing these properties.Based on learnings from the GraphQL specification,
GraphQLResolveInfoshould always haveparentTypeandfieldNamedefined during field resolution. However, confirm this by reviewing the interface definition.packages/executor/src/execution/execute.ts (5)
60-60: LGTM: Import correctly switched to local wrapper.The import now uses the local
locatedErrorwrapper that enriches errors with schema coordinates instead of the graphql-js version.
749-749: LGTM: Consistent error enrichment in executeField.All error handling paths in
executeFieldnow pass theinfoparameter to enable schema coordinate enrichment. Theinfovariable is correctly defined at line 707 and is available in all these error handling blocks.Also applies to: 756-756, 768-768, 775-775
1044-1044: LGTM: Consistent error enrichment in list completion.Error handling paths in
completeAsyncIteratorValueandcompleteListItemValuecorrectly pass theinfoparameter, which is available in scope from the function parameters.Also applies to: 1204-1204, 1217-1217
1792-1792: LGTM: Consistent error enrichment in subscription execution.Subscription error handling at lines 1792 and 1798 correctly passes the
infoparameter, which is defined at line 1768.Also applies to: 1798-1798
1924-1924: LGTM: Consistent error enrichment in stream execution.All stream-related error handling paths correctly pass the
infoparameter, which is available from function parameters inexecuteStreamFieldandexecuteStreamIteratorItem.Also applies to: 1932-1932, 1980-1980, 1999-1999, 2007-2007
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/executor/src/execution/execute.ts(10 hunks)packages/executor/src/execution/locatedError.ts(1 hunks)packages/utils/src/errors.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/executor/src/execution/execute.ts (3)
packages/executor/src/execution/locatedError.ts (1)
locatedError(5-14)packages/utils/src/Path.ts (1)
pathToArray(23-31)packages/executor/src/execution/coerceError.ts (1)
coerceError(1-24)
packages/executor/src/execution/locatedError.ts (2)
packages/utils/src/types.ts (1)
Maybe(29-29)packages/utils/src/Interfaces.ts (1)
GraphQLResolveInfo(71-73)
packages/utils/src/errors.ts (2)
packages/utils/src/types.ts (1)
Maybe(29-29)packages/utils/src/Interfaces.ts (1)
GraphQLResolveInfo(71-73)
🪛 GitHub Check: Type Check on GraphQL v15
packages/executor/src/execution/locatedError.ts
[failure] 11-11:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/executor/src/execution/locatedError.ts
[failure] 11-11:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: alpha / snapshot
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: deployment
- GitHub Check: Unit Test on Bun
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Full Check on GraphQL v16
🔇 Additional comments (3)
packages/executor/src/execution/execute.ts (2)
60-60: Import of wrapper is correctUsing the local locatedError wrapper centralizes augmentation. No issues.
745-757: Good adoption of locatedError(info) across error pathsPassing info enables consistent schemaCoordinates enrichment. The changes keep original control flow intact.
Also applies to: 756-760, 768-776, 775-779, 1044-1046, 1202-1209, 1216-1221, 1792-1793, 1798-1799, 1924-1935, 1980-1984
packages/utils/src/errors.ts (1)
2-2: Non‑breaking extension to relocatedError(info) looks goodSignature change is backward‑compatible; extensions merge preserves existing keys and adds schemaCoordinates when info is provided. Fits createGraphQLError across graphql v15/v16/v17.
Also applies to: 64-81
EmrysMyrddin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/utils/src/errors.ts (1)
85-109: Remove duplicate schema coordinate serialization.The
relocatedErrorfunction adds schema coordinates in two ways:
- As a serializable
schemaCoordinatesfield in extensions (line 99)- As a non-serializable Symbol via
addSchemaCoordinateToError(line 105)This creates inconsistency with the stated design goal of using Symbols to prevent serialization. The serializable field should be removed.
Apply this diff to use only the Symbol-based approach:
const error = createGraphQLError(originalError.message, { nodes: originalError.nodes, source: originalError.source, positions: originalError.positions, path: path == null ? originalError.path : path, originalError, - extensions: info - ? { - ...originalError.extensions, - schemaCoordinates: `${info.parentType.name}.${info.fieldName}`, - } - : originalError.extensions, + extensions: originalError.extensions, }); if (info) { addSchemaCoordinateToError(error, info); }packages/executor/src/execution/execute.ts (1)
2062-2073: Consider coercing errors before locating for consistency.In the
executeStreamIteratorItemfunction,rawErroris passed directly tolocatedErrorin the promise rejection handler (line 2063), while other error paths in this file applycoerceErrorfirst. This inconsistency was also noted in a past review comment.Apply this diff for consistency:
if (isPromise(completedItem)) { completedItem = completedItem.then(undefined, rawError => { + rawError = coerceError(rawError); const error = locatedError( rawError, fieldNodes, pathToArray(itemPath), exeContext.schemaCoordinateInErrors && info, );Also applies to: 2076-2081
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/executor/src/execution/execute.ts(14 hunks)packages/executor/src/execution/normalizedExecutor.ts(1 hunks)packages/utils/src/Interfaces.ts(1 hunks)packages/utils/src/errors.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/utils/src/errors.ts (1)
packages/utils/src/types.ts (1)
Maybe(29-29)
packages/executor/src/execution/execute.ts (3)
packages/utils/src/errors.ts (1)
locatedError(70-83)packages/utils/src/Path.ts (1)
pathToArray(23-31)packages/executor/src/execution/coerceError.ts (1)
coerceError(1-24)
🪛 GitHub Actions: pr
packages/executor/src/execution/execute.ts
[error] 60-60: TS2307: Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Actions: website
packages/executor/src/execution/execute.ts
[error] 60-60: Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: alpha / snapshot
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: deployment
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: Full Check on GraphQL v16
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: Type Check on GraphQL v15
packages/utils/src/errors.ts
[failure] 76-76:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: Unit Test on Bun
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/utils/src/errors.ts
[failure] 76-76:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
🪛 GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
packages/executor/src/execution/execute.ts
[failure] 60-60:
Cannot find module './locatedError.js' or its corresponding type declarations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
🔇 Additional comments (4)
packages/utils/src/Interfaces.ts (1)
96-102: LGTM! Clear documentation of security consideration.The JSDoc appropriately documents the security implication of exposing schema coordinates using a Symbol to prevent serialization. This is a well-designed addition to the public API.
packages/executor/src/execution/normalizedExecutor.ts (1)
52-52: LGTM!The flag is correctly propagated through the normalized execution path, maintaining consistency with other request fields.
packages/executor/src/execution/execute.ts (2)
130-130: LGTM! Flag correctly propagated through execution context.The
schemaCoordinateInErrorsflag is properly threaded through ExecutionContext, ExecutionArgs, and buildExecutionContext, enabling conditional schema coordinate attachment to errors throughout the execution pipeline.Also applies to: 251-251, 424-424, 537-537
753-759: Error handling pattern is consistently applied across all error paths.All 14
locatedErrorcalls in the file (including those at lines 753, 765, 782, 794, 1068, 1233, 1251, 1831, 1842, 1973, 1986, 2039, 2063, and 2076) use the identical conditional pattern:exeContext.schemaCoordinateInErrors && infofor the 4th argument. No inconsistencies were found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/@graphql-tools_import-7588-dependencies.md (1)
5-5: Fix markdown list indentation.The list item indentation should be 0 spaces, not 2, according to the markdownlint rule MD007.
Apply this diff:
dependencies updates: - - Updated dependency [`@theguild/federation-composition@^0.20.2` ↗︎](https://www.npmjs.com/package/@theguild/federation-composition/v/0.20.2) (from `^0.20.1`, in `dependencies`) +- Updated dependency [`@theguild/federation-composition@^0.20.2` ↗︎](https://www.npmjs.com/package/@theguild/federation-composition/v/0.20.2) (from `^0.20.1`, in `dependencies`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/@graphql-tools_import-7588-dependencies.md(1 hunks)packages/executor/src/execution/execute.ts(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/executor/src/execution/execute.ts (3)
packages/utils/src/errors.ts (1)
locatedError(70-83)packages/utils/src/Path.ts (1)
pathToArray(23-31)packages/executor/src/execution/coerceError.ts (1)
coerceError(1-24)
🪛 markdownlint-cli2 (0.18.1)
.changeset/@graphql-tools_import-7588-dependencies.md
5-5: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (12)
packages/executor/src/execution/execute.ts (12)
45-45: LGTM!The import of
locatedErrorfrom@graphql-tools/utilsis correct and aligns with the new error handling functionality.
130-130: LGTM!The optional
schemaCoordinateInErrorsflag inExecutionContextcorrectly enables opt-in behavior for including schema coordinates in errors.
251-251: LGTM!The optional
schemaCoordinateInErrorsflag inExecutionArgsprovides the opt-in interface for users to enable schema coordinate tracking in errors, as intended by the PR objectives.
424-424: LGTM!Correctly destructures the
schemaCoordinateInErrorsflag from the execution arguments.
537-537: LGTM!Correctly includes the
schemaCoordinateInErrorsflag in the returned execution context, completing the opt-in mechanism.
2062-2081: Verify error coercion consistency.Most call sites in this file coerce rawError using
coerceError()before passing tolocatedError, ensuring proper Error shape (name/stack/cause). However, lines 2062-2068 and 2075-2081 pass rawError directly without coercing first. While a past review comment indicated this was addressed, the current code still shows this pattern.Verify whether these two locations need coercion for consistency, or if there's a reason they're handled differently.
Compare with consistent pattern at lines 1971-1978 and 1984-1991:
rawError = coerceError(rawError); const error = locatedError(rawError, fieldNodes, pathToArray(itemPath), ...);
753-799: LGTM!Excellent error handling in
executeField. All error paths (AggregateError iteration and single errors, both async and sync) properly:
- Coerce raw errors to Error instances
- Pass info conditionally via
exeContext.schemaCoordinateInErrors && info- Call locatedError with correct parameters
This enables schema coordinate tracking when opted-in.
1068-1073: LGTM!Correct error handling in
completeAsyncIteratorValue. The error is properly coerced before being passed tolocatedErrorwith conditional info parameter.
1233-1256: LGTM!Both async and sync error paths in
completeListItemValueproperly coerce errors before callinglocatedErrorwith the conditional info parameter.
1831-1847: LGTM!Error handling in
executeSubscriptioncorrectly passes the conditional info parameter tolocatedError, enabling schema coordinate tracking for subscription errors when opted-in.
1973-1991: LGTM!Both error paths in
executeStreamFieldproperly coerce raw errors and pass the conditional info parameter tolocatedError.
2039-2044: LGTM!The catch block in
executeStreamIteratorItemcorrectly coerces the error before callinglocatedErrorwith conditional info.
EmrysMyrddin
force-pushed
the
feat/executor/error-schema-coordinates
branch
from
88d2c47 to
9a80d88
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/executor/src/execution/execute.ts (2)
1827-1848: Apply coerceError before locatedError for consistency.Unlike other error handling paths in this file, the subscription error handlers (lines 1831-1836 and 1842-1847) pass errors directly to
locatedErrorwithout first callingcoerceError. This is inconsistent with the pattern established inexecuteField,completeListItemValue, and other locations.Apply this diff:
if (isPromise(result)) { return result .then(result => assertEventStream(result, exeContext.signal, exeContext.onSignalAbort)) - .then(undefined, error => { - throw locatedError( - error, + .then(undefined, rawError => { + const coercedError = coerceError(rawError); + throw locatedError( + coercedError, fieldNodes, pathToArray(path), exeContext.schemaCoordinateInErrors && info, ); }); } return assertEventStream(result, exeContext.signal, exeContext.onSignalAbort); - } catch (error) { + } catch (rawError) { + const coercedError = coerceError(rawError); throw locatedError( - error, + coercedError, fieldNodes, pathToArray(path), exeContext.schemaCoordinateInErrors && info, ); }
2020-2086: Add missing error coercion in executeStreamIteratorItem.The function has inconsistent error handling:
- Line 2038 correctly coerces in the first catch block
- Lines 2062-2068 (promise rejection) and 2076-2081 (catch block) pass
rawErrordirectly tolocatedErrorwithout coercing firstThis is inconsistent with the pattern used throughout the file and can result in improperly shaped errors (missing name, stack, or cause properties).
Apply this diff:
if (isPromise(completedItem)) { completedItem = completedItem.then(undefined, rawError => { + rawError = coerceError(rawError); const error = locatedError( rawError, fieldNodes, pathToArray(itemPath), exeContext.schemaCoordinateInErrors && info, ); const handledError = handleFieldError(error, itemType, asyncPayloadRecord.errors); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); return handledError; }); } return { done: false, value: completedItem }; } catch (rawError) { + const coercedError = coerceError(rawError); const error = locatedError( - rawError, + coercedError, fieldNodes, pathToArray(itemPath), exeContext.schemaCoordinateInErrors && info, ); const value = handleFieldError(error, itemType, asyncPayloadRecord.errors); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); return { done: false, value }; }
♻️ Duplicate comments (1)
packages/utils/src/errors.ts (1)
70-83: Fix type incompatibility with GraphQL's locatedError.The
nodesparameter includesnullin its type, but GraphQL'slocatedErroronly acceptsASTNode | ReadonlyArray<ASTNode> | undefined(notnull). This causes type errors in GraphQL v15.Apply this diff:
export function locatedError( rawError: unknown, - nodes: ASTNode | ReadonlyArray<ASTNode> | null | undefined, + nodes: ASTNode | ReadonlyArray<ASTNode> | undefined, path: Maybe<ReadonlyArray<string | number>>, info: SchemaCoordinateInfo | false | null | undefined, ) { - const error = _locatedError(rawError, nodes, path); + const error = _locatedError(rawError, nodes ?? undefined, path); if (info) { addSchemaCoordinateToError(error, info); } return error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/executor/src/execution/execute.ts(14 hunks)packages/executor/src/execution/normalizedExecutor.ts(1 hunks)packages/utils/src/Interfaces.ts(1 hunks)packages/utils/src/errors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/executor/src/execution/normalizedExecutor.ts
- packages/utils/src/Interfaces.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/executor/src/execution/execute.ts (3)
packages/utils/src/errors.ts (1)
locatedError(70-83)packages/utils/src/Path.ts (1)
pathToArray(23-31)packages/executor/src/execution/coerceError.ts (1)
coerceError(1-24)
packages/utils/src/errors.ts (1)
packages/utils/src/types.ts (1)
Maybe(29-29)
🪛 GitHub Check: Type Check on GraphQL v15
packages/utils/src/errors.ts
[failure] 76-76:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/utils/src/errors.ts
[failure] 76-76:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Bun
- GitHub Check: deployment
🔇 Additional comments (3)
packages/executor/src/execution/execute.ts (3)
115-131: LGTM! Clean API extension for schema coordinates.The new
schemaCoordinateInErrorsflag is properly threaded throughExecutionArgs,ExecutionContext, andbuildExecutionContext. The opt-in approach allows backward compatibility.Also applies to: 240-252, 410-539
745-803: LGTM! Consistent error coercion and coordinate attachment.All error paths in
executeFieldproperly coerce errors before callinglocatedErrorwith the conditional schema coordinate info. The pattern is applied consistently across both promise rejections and synchronous catches, includingAggregateErrorhandling.
1021-1096: LGTM! Proper error handling in list completion.Both
completeAsyncIteratorValueandcompleteListItemValueconsistently coerce errors before attaching schema coordinates. The pattern is correctly applied to iterator errors, promise rejections, and synchronous exceptions.Also applies to: 1190-1263
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/utils/src/errors.ts (1)
85-109: Remove redundant schema coordinate assignment.This issue was flagged in a previous review and remains unresolved. The function assigns the schema coordinate twice using different keys:
- String key
schemaCoordinates(line 99)- Symbol key via
addSchemaCoordinateToError(line 105)Keep only the helper approach for consistency.
Apply this diff:
const error = createGraphQLError(originalError.message, { nodes: originalError.nodes, source: originalError.source, positions: originalError.positions, path: path == null ? originalError.path : path, originalError, - extensions: info - ? { - ...originalError.extensions, - schemaCoordinates: `${info.parentType.name}.${info.fieldName}`, - } - : originalError.extensions, + extensions: originalError.extensions, }); if (info) { addSchemaCoordinateToError(error, info); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/utils/src/errors.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/utils/src/errors.ts (1)
packages/utils/src/types.ts (1)
Maybe(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Bun
- GitHub Check: deployment
- GitHub Check: Full Check on GraphQL v16
🔇 Additional comments (2)
packages/utils/src/errors.ts (2)
64-68: LGTM!Good use of
Symbol.forfor cross-realm compatibility, and the helper correctly encapsulates the schema coordinate formatting logic.
70-83: LGTM!The critical type incompatibility issue from the previous review has been fixed—
nodesno longer acceptsnull, matching GraphQL's signature. The function correctly delegates to the originallocatedErrorand conditionally adds schema coordinate information.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/utils/src/errors.ts (1)
63-63: Export theSchemaCoordinateInfotype.This type appears in the public signatures of
locatedErrorandrelocatedErrorbut isn't exported, preventing TypeScript consumers from properly using these functions.Apply this diff:
-type SchemaCoordinateInfo = { fieldName: string; parentType: { name: string } }; +export type SchemaCoordinateInfo = { fieldName: string; parentType: { name: string } };
🧹 Nitpick comments (1)
packages/utils/src/errors.ts (1)
70-83: Consider simplifying theinfoparameter type.The parameter accepts
SchemaCoordinateInfo | false | null | undefined, which is unusual. TypicallySchemaCoordinateInfo | undefinedsuffices for optional parameters. The inclusion offalseandnulladds complexity without clear benefit unless there's a specific semantic distinction between "not provided" (undefined) and "explicitly disabled" (false/null).If there's no semantic reason for the tri-state logic, consider:
export function locatedError( rawError: unknown, nodes: ASTNode | ReadonlyArray<ASTNode> | undefined, path: Maybe<ReadonlyArray<string | number>>, - info: SchemaCoordinateInfo | false | null | undefined, + info?: SchemaCoordinateInfo, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/utils/src/errors.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/utils/src/errors.ts (1)
packages/utils/src/types.ts (1)
Maybe(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: deployment
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
🔇 Additional comments (2)
packages/utils/src/errors.ts (2)
1-1: LGTM: Clean wrapper pattern.Aliasing the imported
locatedErroras_locatedErroris a standard approach for creating an enhanced wrapper function.
85-104: LGTM: Consistent error relocation with schema coordinates.The function correctly relocates the error while preserving all original properties and consistently uses the
addSchemaCoordinateToErrorhelper when coordinate info is provided. The redundant assignment issue from previous reviews has been resolved.
|
Instead of relying on this implementation within the executor for the otel plugin, is it possible to instead retrieve the schema coordinate from the execution result? E.g. is it possible to resolve a graphql errors array path property within the the GraphQLError to the schema coordinate? That would prevent tight coupling otel reporting with graphql-tools. |
|
It is probably possible, but not easy to do. Path can be very tricky to walk, because of aliases, unions, interfaces and lists. It would be possible to visit the result with typeinfo, but the result doesn't always contain the error path (because of non-null fields). |
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/executor |
1.5.0-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-apollo-link |
2.0.5-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-envelop |
4.0.5-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-legacy-ws |
1.1.24-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-urql-exchange |
1.0.27-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-yoga |
3.0.35-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-tag-pluck |
8.3.26-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
graphql-tools |
9.0.25-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/import |
7.1.7-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/links |
10.0.5-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/load |
8.1.7-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/apollo-engine-loader |
8.0.27-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/code-file-loader |
8.1.27-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/git-loader |
8.0.31-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/github-loader |
9.0.5-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-file-loader |
8.1.7-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/json-file-loader |
8.0.25-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/module-loader |
8.0.25-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/url-loader |
9.0.5-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/merge |
9.1.6-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/mock |
9.1.4-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/node-require |
7.0.32-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/relay-operation-optimizer |
7.0.26-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/resolvers-composition |
7.0.25-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/schema |
10.0.30-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/utils |
10.11.0-alpha-20251114153833-2b67fa69527eca2c25a79f3cf204b3badffa843b |
npm ↗︎ unpkg ↗︎ |
In which scenario is that the case? I just tested it the following way: import { execute, parse } from 'graphql';
import * as yoga from 'graphql-yoga';
const schema = yoga.createSchema({
typeDefs: /* GraphQL */ `
type Query {
user: User
}
type User {
id: ID!
b: String!
}
`,
resolvers: {
Query: {
user: () => ({}),
},
User: {
id: () => {
throw new Error('A');
},
b: () => {
throw new Error('B');
},
},
},
});
const document = parse(/* GraphQL */ `
query {
user {
id
b
}
}
`);
const res = execute({ schema, document });
console.log(JSON.stringify(res, null, 2));And it seems like the path is to the exact field? |
|
Yes exactly. The path points to |
|
So imagine the user have a required field with a union type. An error if an error is thrown in any resolver under this field, you will not be able to actually know from which actual type it was thrown. |
EmrysMyrddin
force-pushed
the
feat/executor/error-schema-coordinates
branch
from
6cec35f to
eae3c31
Compare
EmrysMyrddin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/executor/src/execution/execute.ts (1)
1829-1836: Coerce raw errors before passing them to locatedError in subscription/stream pathsMost error paths now normalize errors via
coerceErrorbefore callinglocatedError, but these cases still pass the raw value:
- Subscription event stream rejection and synchronous
executeSubscriptioncatch.- The rejection handler and catch in
executeStreamIteratorItem.For consistent error shaping (name/stack/cause) across all paths, consider coercing here as well:
- return result - .then(result => assertEventStream(result, exeContext.signal, exeContext.onSignalAbort)) - .then(undefined, error => { - throw locatedError( - error, - fieldNodes, - pathToArray(path), - exeContext.schemaCoordinateInErrors && info, - ); - }); + return result + .then(result => assertEventStream(result, exeContext.signal, exeContext.onSignalAbort)) + .then(undefined, rawError => { + const coercedError = coerceError(rawError); + throw locatedError( + coercedError, + fieldNodes, + pathToArray(path), + exeContext.schemaCoordinateInErrors && info, + ); + }); @@ - } catch (error) { - throw locatedError( - error, - fieldNodes, - pathToArray(path), - exeContext.schemaCoordinateInErrors && info, - ); + } catch (rawError) { + const coercedError = coerceError(rawError); + throw locatedError( + coercedError, + fieldNodes, + pathToArray(path), + exeContext.schemaCoordinateInErrors && info, + ); }- if (isPromise(completedItem)) { - completedItem = completedItem.then(undefined, rawError => { - const error = locatedError( - rawError, - fieldNodes, - pathToArray(itemPath), - exeContext.schemaCoordinateInErrors && info, - ); + if (isPromise(completedItem)) { + completedItem = completedItem.then(undefined, rawError => { + const coercedError = coerceError(rawError); + const error = locatedError( + coercedError, + fieldNodes, + pathToArray(itemPath), + exeContext.schemaCoordinateInErrors && info, + ); @@ - } catch (rawError) { - const error = locatedError( - rawError, - fieldNodes, - pathToArray(itemPath), - exeContext.schemaCoordinateInErrors && info, - ); + } catch (rawError) { + const coercedError = coerceError(rawError); + const error = locatedError( + coercedError, + fieldNodes, + pathToArray(itemPath), + exeContext.schemaCoordinateInErrors && info, + );Also applies to: 1842-1847, 2061-2081
🧹 Nitpick comments (1)
packages/utils/tests/errors.test.ts (1)
83-88: Make JSON coordinate assertion less brittleRelying on the exact JSON string means a harmless change in graphql-js’s property ordering could break this test. You can keep the intent while avoiding order sensitivity by asserting on the parsed object:
- const error = createGraphQLError('message', { - coordinate: 'Query.test', - }); - expect(JSON.stringify(error)).toBe('{"message":"message","coordinate":"Query.test"}'); + const error = createGraphQLError('message', { + coordinate: 'Query.test', + }); + expect(JSON.parse(JSON.stringify(error))).toEqual({ + message: 'message', + coordinate: 'Query.test', + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.changeset/floppy-women-poke.md(1 hunks).github/workflows/pr.yml(1 hunks)packages/executor/src/execution/__tests__/executor-test.ts(1 hunks)packages/executor/src/execution/execute.ts(14 hunks)packages/executor/src/execution/normalizedExecutor.ts(1 hunks)packages/utils/src/Interfaces.ts(1 hunks)packages/utils/src/errors.ts(5 hunks)packages/utils/tests/createGraphQLError.test.ts(0 hunks)packages/utils/tests/errors.test.ts(1 hunks)packages/utils/tests/relocatedError.test.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/utils/tests/relocatedError.test.ts
- packages/utils/tests/createGraphQLError.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/executor/src/execution/normalizedExecutor.ts
- .github/workflows/pr.yml
- packages/utils/src/Interfaces.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T19:58:05.749Z
Learnt from: ardatan
Repo: ardatan/graphql-tools PR: 0
File: :0-0
Timestamp: 2025-01-29T19:58:05.749Z
Learning: The `isUrl` helper in graphql-tools/utils should be tested with both URL.canParse and new URL() paths, covering various URL formats (http, https, file) and invalid cases. Tests should properly mock and restore URL.canParse to ensure consistent behavior across environments.
Applied to files:
packages/utils/tests/errors.test.tspackages/executor/src/execution/__tests__/executor-test.ts
🧬 Code graph analysis (3)
packages/utils/tests/errors.test.ts (2)
packages/utils/src/errors.ts (4)
createGraphQLError(63-98)relocatedError(126-140)getSchemaCoordinate(102-104)locatedError(106-124)packages/testing/utils.ts (2)
describeIf(87-89)testIf(91-93)
packages/executor/src/execution/execute.ts (3)
packages/utils/src/errors.ts (1)
locatedError(106-124)packages/utils/src/Path.ts (1)
pathToArray(23-31)packages/executor/src/execution/coerceError.ts (1)
coerceError(1-24)
packages/utils/src/errors.ts (1)
packages/utils/src/types.ts (1)
Maybe(29-29)
🪛 LanguageTool
.changeset/floppy-women-poke.md
[grammar] ~12-~12: Ensure spelling is correct
Context: ... about your schema. If you need to keep you schema private and secret, you should s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Bun
- GitHub Check: deployment
🔇 Additional comments (5)
packages/executor/src/execution/__tests__/executor-test.ts (2)
1275-1307: Executor coordinate toggle test looks correctThe test accurately exercises
schemaCoordinateInErrorsbeing on vs off and asserts the expected'Query.foo'coordinate when enabled. Optional only: you could add a case without the flag to document the default behavior, but this is not required.
1309-1361: Abstract-type coordinate coverage is solidThis test nicely validates that, for both an interface field and a union field, the coordinate resolves to the concrete runtime type (
'A.f'), which matches how the executor now derives coordinates frominfo.parentType/info.fieldName.packages/utils/tests/errors.test.ts (1)
27-54: Good coverage of locatedError and getSchemaCoordinateThe tests for
locatedErrorandgetSchemaCoordinateeffectively verify that nodes/path are set, coordinates are attached when requested, and thatgetSchemaCoordinateworks even whencoordinateis added externally. This lines up with the new error utility behavior.packages/executor/src/execution/execute.ts (1)
45-45: schemaCoordinateInErrors is plumbed consistently through executionImporting
locatedErrorfrom utils and threading the optionalschemaCoordinateInErrorsflag throughExecutionArgs,buildExecutionContext,ExecutionContext, and the per-event execution context keeps the option available everywhere it’s needed without affecting existing callers. This wiring looks coherent and backwards compatible.Also applies to: 115-131, 240-252, 410-425, 522-538
packages/utils/src/errors.ts (1)
56-61: Coordinate plumbing in error helpers is version- and spec-friendlyUsing
versionInfo.majorto select the appropriateGraphQLErrorconstructor, and only definingcoordinate/toJSONwhen the constructed error doesn’t already expose a coordinate, keepscreateGraphQLError,locatedError, andrelocatedErrorcompatible with graphql-js 15, 16+, and future versions that may add native coordinate support. The sharedtoJSONhelper also guarantees coordinates serialize consistently when present.Also applies to: 90-95, 106-123
| Add optional schema coordinate in error extensions. This extension allows to precisely identify the | ||
| source of the error by automated tools like tracing or monitoring. | ||
|
|
||
| This new feature is opt-in, you have to enable it using `schemaCoordinateInErrors` executor option. | ||
|
|
||
| **Caution:** This feature, when enabled, will expose information about your schema. If you need to | ||
| keep you schema private and secret, you should strip this attribute at serialization time before | ||
| sending errors to the client. | ||
|
|
||
| ```ts | ||
| import { parse } from 'graphql' | ||
| import { normalizedExecutor } from '@graphql-tools/executor' | ||
| import schema from './schema' | ||
|
|
||
| // You can also use `Symbol.for('graphql.error.schemaCoordinate')` to get the symbol if you don't | ||
| // want to depend on `@graphql-tools/utils` | ||
|
|
||
| const result = await normalizedExecutor({ | ||
| schema, | ||
| document: parse(gql`...`), | ||
| schemaCoordinateInErrors: true // enable adding schema coordinate to graphql errors | ||
| }) | ||
|
|
||
| if (result.errors) { | ||
| for (const error of result.errors) { | ||
| console.log('Error in resolver ', error.coordinate, ':', error.message) | ||
| } | ||
| } |
There was a problem hiding this comment.
Polish changeset wording and example
A few small doc fixes will make this clearer:
- Line 12: change “keep you schema private and secret” → “keep your schema private and secret”.
- The comment about
Symbol.for('graphql.error.schemaCoordinate')suggests a symbol-based API, while the current example and implementation exposeerror.coordinatedirectly; clarifying how/if the symbol is actually used would avoid confusion. - Optional: the snippet uses
parse(gql\...`)without importinggql; either add the import or switch to a plain string literal inparse` so the example is self-contained.
🧰 Tools
🪛 LanguageTool
[grammar] ~12-~12: Ensure spelling is correct
Context: ... about your schema. If you need to keep you schema private and secret, you should s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
| type SchemaCoordinateInfo = { fieldName: string; parentType: { name: string } }; | ||
|
|
||
| export function getSchemaCoordinate(error: GraphQLError): string | undefined { | ||
| return error.coordinate; | ||
| } | ||
|
|
||
| export function locatedError( | ||
| rawError: unknown, | ||
| nodes: ASTNode | ReadonlyArray<ASTNode> | undefined, | ||
| path: Maybe<ReadonlyArray<string | number>>, | ||
| info: SchemaCoordinateInfo | false | null | undefined, | ||
| ): GraphQLError { | ||
| const error = _locatedError(rawError, nodes, path) as GraphQLError; | ||
|
|
||
| // `graphql` locatedError is only changing path and nodes if it is not already defined | ||
| if (!error.coordinate && info && error.coordinate == null) { | ||
| const coordinate = `${info.parentType.name}.${info.fieldName}`; | ||
| Object.defineProperties(error, { | ||
| coordinate: { value: coordinate, enumerable: true, configurable: true }, | ||
| toJSON: { value: toJSON }, | ||
| }); | ||
| } | ||
|
|
||
| return error; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Make locatedError’s info parameter optional to preserve API compatibility
The exported locatedError now has a required info parameter, whereas the graphql-js version only accepts (rawError, nodes, path). Any existing callers that import locatedError from @graphql-tools/utils and still use the 3‑argument form will now fail type-checking even though the runtime call works (since info is unused when falsy). You can keep the new coordinate behavior while preserving the old call shape by making info optional:
-export function locatedError(
- rawError: unknown,
- nodes: ASTNode | ReadonlyArray<ASTNode> | undefined,
- path: Maybe<ReadonlyArray<string | number>>,
- info: SchemaCoordinateInfo | false | null | undefined,
-): GraphQLError {
+export function locatedError(
+ rawError: unknown,
+ nodes: ASTNode | ReadonlyArray<ASTNode> | undefined,
+ path: Maybe<ReadonlyArray<string | number>>,
+ info?: SchemaCoordinateInfo | false | null,
+): GraphQLError {The existing if (info && error.coordinate == null) guard already handles undefined, so no further changes are needed.
I'll verify this review comment by checking the codebase for usage patterns and confirming the graphql-js API signature.
[/run_scripts]
[/web_search]
🏁 Script executed:
#!/bin/bash
# Find all imports and usages of locatedError
rg -n "locatedError" --type=ts --type=js -B2 -A2Length of output: 10090
🏁 Script executed:
#!/bin/bash
# Check the current errors.ts file to confirm the exact signature
cat packages/utils/src/errors.ts | head -130Length of output: 3655
🏁 Script executed:
#!/bin/bash
# Search for test files that might show expected usage patterns
fd -e test.ts -e spec.ts -e test.js | xargs grep -l "locatedError" 2>/dev/null | head -5Length of output: 76
🌐 Web query:
graphql-js locatedError function signature documentation
💡 Result:
Signature (GraphQL.js API):
function locatedError(error: Error, nodes: Array): GraphQLError. It produces a GraphQLError aware of the document location(s) responsible for the original error. [1][2]
Sources:
[1] GraphQL.js API — Error module.
[2] GraphQL.js (alternate API docs).
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check if the executor.ts calls have any type assertions or casts to bypass type errors
rg -n "locatedError\(" packages/executor/src/execution/execute.ts -A4 | grep -E "as |any|@ts-"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at a specific locatedError call with more context to see if there are type issues
sed -n '750,770p' packages/executor/src/execution/execute.tsLength of output: 743
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for any locatedError calls with exactly 3 arguments (no 4th arg)
rg -n "locatedError\(" packages/ -A3 | grep -v "schemaCoordinateInErrors" | rg "locatedError\([^)]*\)$" Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check what is exported from @graphql-tools/utils main index
fd -name "index.ts" -o -name "index.js" | xargs grep -l "locatedError" 2>/dev/nullLength of output: 230
🏁 Script executed:
#!/bin/bash
# Look at the package exports for utils
cat packages/utils/src/index.ts | grep -i locatedLength of output: 0
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find and show the main index file
find packages/utils/src -name "index.ts" -o -name "index.js"Length of output: 27
🏁 Script executed:
#!/bin/bash
# Show packages/utils/src/index.ts content
cat packages/utils/src/index.tsLength of output: 2324
🏁 Script executed:
#!/bin/bash
# Check package.json to see what's exported
cat packages/utils/package.json | grep -A 20 '"exports"'Length of output: 547
Make locatedError's info parameter optional to preserve API compatibility
The exported locatedError function currently has a required info parameter, breaking compatibility with the graphql-js API that accepts only (rawError, nodes, path). Any external callers using the 3-argument form will fail type-checking even though runtime behavior works correctly (since info is unused when falsy).
Changing the parameter to optional preserves backward compatibility:
export function locatedError(
rawError: unknown,
nodes: ASTNode | ReadonlyArray<ASTNode> | undefined,
path: Maybe<ReadonlyArray<string | number>>,
- info: SchemaCoordinateInfo | false | null | undefined,
+ info?: SchemaCoordinateInfo | false | null,
): GraphQLError {The existing guard already handles undefined: if (!error.coordinate && info && error.coordinate == null).
🤖 Prompt for AI Agents
In packages/utils/src/errors.ts around lines 100 to 124, the locatedError
function declares its info parameter as required which breaks API compatibility
with graphql-js's three-argument form; change the function signature to make
info optional (e.g. add a ? to the parameter) so its type becomes
SchemaCoordinateInfo | false | null | undefined and update any affected type
annotations accordingly; no other logic changes are needed because the existing
guard already handles undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/utils/tests/errors.test.ts (1)
11-28: Previous review feedback addressed!The path assertion at line 22 successfully addresses the previous review comment. The test now validates both the path relocation and the coordinate update.
🧹 Nitpick comments (1)
packages/utils/tests/errors.test.ts (1)
30-44: Consider adding edge case test for locatedError without coordinate info.The current test only covers the scenario where coordinate info is provided. Consider adding a test case that verifies
locatedErrorbehavior wheninfoisnull,undefined, orfalseto ensure the coordinate is not added in those cases.Example test case:
it('should not add coordinate when info is not provided', () => { const originalError = createGraphQLError('test'); const nodes: ASTNode[] = [{ kind: Kind.DOCUMENT, definitions: [] }]; const error = locatedError(originalError, nodes, ['test'], null); expect(error.nodes).toBe(nodes); expect(error.path).toEqual(['test']); if (versionInfo.major >= 16) { expect(error.coordinate).toBeUndefined(); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/utils/tests/errors.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T19:58:05.749Z
Learnt from: ardatan
Repo: ardatan/graphql-tools PR: 0
File: :0-0
Timestamp: 2025-01-29T19:58:05.749Z
Learning: The `isUrl` helper in graphql-tools/utils should be tested with both URL.canParse and new URL() paths, covering various URL formats (http, https, file) and invalid cases. Tests should properly mock and restore URL.canParse to ensure consistent behavior across environments.
Applied to files:
packages/utils/tests/errors.test.ts
🧬 Code graph analysis (1)
packages/utils/tests/errors.test.ts (2)
packages/utils/src/errors.ts (4)
createGraphQLError(63-98)relocatedError(126-140)getSchemaCoordinate(102-104)locatedError(106-124)packages/testing/utils.ts (2)
describeIf(87-89)testIf(91-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Full Check on GraphQL v16
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Bun
- GitHub Check: alpha / snapshot
- GitHub Check: deployment
🔇 Additional comments (3)
packages/utils/tests/errors.test.ts (3)
1-8: LGTM!The imports are well-organized and all utilities are properly imported for testing.
46-57: LGTM!The test suite correctly validates
getSchemaCoordinatebehavior across different error construction methods. The version gating andts-expect-errorusage are appropriate.
59-92: LGTM!The test suite thoroughly validates
createGraphQLError:
- Non-Error
originalErrorwrapping ensures proper error object construction- Coordinate handling is correctly version-gated
- JSON serialization test confirms the custom
toJSONimplementation works as intended
Description
Add a new extension to graphql errors to easily locate the source of error in schema.
This information can be useful to create metrics and point out field in schema with hight rates of errors.
This feature is needed in the context of the new Console Tracing feature.
See graphql/graphql-spec#1200
Related to GW-501
Type of change