feat(executor): add schema coordinates extension to graphql errors #7588
Conversation
🦋 Changeset detectedLatest commit: a08dcdf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 CodeRabbit
WalkthroughThreads an opt-in boolean flag Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutorFactory as executorFromSchema
participant NormExec as normalizedExecutor
participant Build as buildExecutionContext
participant ExecCtx as ExecutionContext
participant Resolver as Field Resolver
participant ErrUtil as utils.locatedError
Client->>ExecutorFactory: request (may include schemaCoordinateInErrors)
ExecutorFactory-->>NormExec: invoke normalizedExecutor(request with flag)
Client->>Build: buildExecutionContext(args with schemaCoordinateInErrors)
Build-->>ExecCtx: return ExecutionContext (contains schemaCoordinateInErrors)
Resolver->>Resolver: resolve field
alt resolver throws
Resolver->>ExecCtx: check schemaCoordinateInErrors
alt true and ResolveInfo present
ExecCtx->>ErrUtil: locatedError(err, nodes, path, SchemaCoordinateInfo)
ErrUtil-->>Resolver: GraphQLError (with symbol extension)
else
ExecCtx->>ErrUtil: locatedError(err, nodes, path)
ErrUtil-->>Resolver: GraphQLError
end
Resolver-->>Client: return error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (8)
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
| path: Maybe<ReadonlyArray<string | number>>, | ||
| info: GraphQLResolveInfo, | ||
| ) { | ||
| const error = _locatedError(rawError, nodes, path); |
There was a problem hiding this comment.
Fix type incompatibility: null handling for nodes parameter.
The static analysis correctly identifies that nodes accepts null but the upstream _locatedError expects ASTNode | ReadonlyArray<ASTNode> | undefined. You need to handle the null case explicitly.
Apply this diff to fix the type mismatch:
const error = _locatedError(rawError, nodes, path);
+ const error = _locatedError(rawError, nodes ?? undefined, path);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Type Check on GraphQL v15
[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
[failure] 11-11:
Argument of type 'ASTNode | readonly ASTNode[] | null | undefined' is not assignable to parameter of type 'ASTNode | readonly ASTNode[] | undefined'.
🤖 Prompt for AI Agents
In packages/executor/src/execution/locatedError.ts around line 11, the call to
_locatedError(rawError, nodes, path) passes nodes which can be null while
_locatedError expects ASTNode | ReadonlyArray<ASTNode> | undefined; change the
call to explicitly handle the null case by passing undefined when nodes is null
(e.g., use a null-coalescing or conditional expression) so the argument type
matches the upstream function signature.
| info: GraphQLResolveInfo, | ||
| ) { | ||
| const error = _locatedError(rawError, nodes, path); | ||
| error.extensions['schemaCoordinates'] = `${info.parentType.name}.${info.fieldName}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Initialize extensions if undefined to prevent runtime errors.
Direct assignment to error.extensions['schemaCoordinates'] will fail if extensions is undefined. The GraphQL error object should have extensions initialized, but it's safer to ensure it exists.
Apply this diff to safely initialize extensions:
- error.extensions['schemaCoordinates'] = `${info.parentType.name}.${info.fieldName}`;
+ if (!error.extensions) {
+ error.extensions = {};
+ }
+ error.extensions['schemaCoordinates'] = `${info.parentType.name}.${info.fieldName}`;Alternatively, use a more concise approach:
- error.extensions['schemaCoordinates'] = `${info.parentType.name}.${info.fieldName}`;
+ error.extensions = {
+ ...error.extensions,
+ schemaCoordinates: `${info.parentType.name}.${info.fieldName}`,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error.extensions['schemaCoordinates'] = `${info.parentType.name}.${info.fieldName}`; | |
| if (!error.extensions) { | |
| error.extensions = {}; | |
| } | |
| error.extensions['schemaCoordinates'] = `${info.parentType.name}.${info.fieldName}`; |
🤖 Prompt for AI Agents
In packages/executor/src/execution/locatedError.ts around line 12, the code
assigns to error.extensions['schemaCoordinates'] without ensuring extensions is
defined; update the code to first ensure error.extensions is an object (create
an empty object when undefined) and then set the schemaCoordinates property on
it so the assignment cannot throw when extensions is missing.
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.
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Bumps the actions-deps group with 1 update: [webpack](https://github.com/webpack/webpack). Updates `webpack` from 5.99.1 to 5.99.2 - [Release notes](https://github.com/webpack/webpack/releases) - [Commits](webpack/webpack@v5.99.1...v5.99.2) --- updated-dependencies: - dependency-name: webpack dependency-version: 5.99.2 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: actions-deps ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Bumps [next](https://github.com/vercel/next.js) from 15.4.2 to 15.4.3. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v15.4.2...v15.4.3) --- updated-dependencies: - dependency-name: next dependency-version: 15.4.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-apollo-link |
2.0.1-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-envelop |
4.0.1-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-legacy-ws |
1.1.20-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-urql-exchange |
1.0.23-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-yoga |
3.0.31-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-tag-pluck |
8.3.22-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
graphql-tools |
9.0.21-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/import |
7.1.3-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/links |
10.0.1-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/load |
8.1.3-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/apollo-engine-loader |
8.0.23-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/code-file-loader |
8.1.23-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/git-loader |
8.0.27-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/github-loader |
9.0.1-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-file-loader |
8.1.3-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/json-file-loader |
8.0.21-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/module-loader |
8.0.21-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/url-loader |
9.0.1-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/merge |
9.1.2-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/mock |
9.1.0-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/node-require |
7.0.28-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/relay-operation-optimizer |
7.0.22-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/resolvers-composition |
7.0.21-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/schema |
10.0.26-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/utils |
10.10.0-alpha-20251027163100-a08dcdfd6eea95538ad051088417b90371b660ff |
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. |
|
isnt |
|
You have the equivalent to visit the result too. You give it the result and the schema, it allows to visit the result with the type information at each step. But even without this util, I don't really see how to workaround the fact that it is possible that the result will not contain the data needed to disambiguate the path if there is any Union or interface involved. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (2)
.changeset/floppy-women-poke.md(1 hunks)packages/utils/src/errors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/src/errors.ts
⏰ 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: deployment
- GitHub Check: Full Check on 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 Bun
- 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 Node 18 (windows-latest) and GraphQL v16
🔇 Additional comments (2)
.changeset/floppy-women-poke.md (2)
1-3: Changeset metadata looks correct.The version bump type (
minor) aligns with the feature being non-breaking and opt-in.
5-11: Clear and well-explained feature description.The documentation effectively communicates the purpose, opt-in nature, and the security rationale for using
Symbol. This provides good context for end users.
3 participants
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.
Type of change