feat(composition): validation for transforms with better errors and backward-compat API#9398
Merged
feat(composition): validation for transforms with better errors and backward-compat API#9398
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughComposes annotated subgraphs and returns both supergraph SDL and annotated subgraphs; adds validator for federation directives ( Changes
Sequence DiagramsequenceDiagram
actor CLI as CLI Process
participant Config as Config Loader
participant Compose as Composition Engine
participant Transforms as SubgraphTransforms
participant Validate as Validator
participant Output as Writer
CLI->>Config: Load config
Config->>Compose: getComposedResultFromConfig(config)
Compose->>Transforms: getAnnotatedSubgraphs()\n(apply transforms with try/catch)
Transforms-->>Compose: annotatedSubgraphs
Compose->>Compose: composeAnnotatedSubgraphs(annotatedSubgraphs)
Compose-->>Validate: supergraphSdl, subgraphs
Validate->>Validate: Parse SDL, inspect `@merge/`@resolveTo/@key\nverify subgraph/type/field/args, compute suggestions
alt Validation errors found
Validate-->>CLI: GraphQLError[]
CLI->>CLI: Log failures and messages
CLI->>CLI: process.exit(1)
else No errors
Validate-->>Output: Validated supergraphSdl
Output->>CLI: Write output
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Collaborator
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-mesh/compose-cli |
1.5.27-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-composition |
0.8.43-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/include |
0.3.36-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/incontext-sdk-codegen |
0.0.14-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/cli |
0.100.48-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/config |
0.108.42-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/grpc |
0.108.39-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/odata |
0.106.41-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/openapi |
0.109.48-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/soap |
0.107.42-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/http |
0.106.40-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/migrate-config-cli |
1.7.35-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@omnigraph/grpc |
0.2.20-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@omnigraph/odata |
0.2.41-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@omnigraph/openapi |
0.109.47-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@omnigraph/soap |
0.107.41-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-hive |
0.105.11-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-newrelic |
0.104.40-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-response-cache |
0.104.41-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-statsd |
0.104.40-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-grpc |
0.3.37-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-odata |
0.2.41-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-soap |
0.10.41-alpha-20260409103243-cbac018ee4286fe3fcdc933c84d5769289fe3ea6 |
npm ↗︎ unpkg ↗︎ |
Contributor
💻 Website PreviewThe latest changes are available as preview in: https://9998d7f6.graphql-mesh.pages.dev |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds additional validation and clearer error reporting during composition—especially around transforms (e.g. Federation transform) and directive-driven wiring (@merge, @resolveTo)—and surfaces these validation errors in the compose CLI.
Changes:
- Add validation for Federation transform
resolveReferenceconfig, including root field existence + suggestions. - Improve transform error reporting by wrapping failures with transform/subgraph context.
- Add compose-cli post-compose validation for
@merge/@resolveToand integrate it into the CLI flow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fusion/composition/src/transforms/federation.ts | Adds validation for @key/resolveReference config (operation/root-field existence + suggestions). |
| packages/fusion/composition/src/compose.ts | Wraps transform application errors to include transform + subgraph context (also introduces an unused import). |
| packages/compose-cli/src/validate.ts | New supergraph validation pass for @merge/@resolveTo references (currently has several runtime-breaking issues). |
| packages/compose-cli/src/run.ts | Runs validation after composition and exits on errors (currently breaks --subgraph). |
| packages/compose-cli/src/getComposedSchemaFromConfig.ts | Changes return shape to include { supergraphSdl, subgraphs } for downstream validation. |
| .changeset/some-pianos-smoke.md | Declares patch releases for compose-cli and fusion-composition for the validation/error-message improvements. |
Comments suppressed due to low confidence (1)
packages/fusion/composition/src/compose.ts:26
createGraphQLErroris imported but never used in this file. This will trigger unused-import linting in many setups; either remove the import or use it when wrapping transform failures (e.g. to preserve locations/cause).
import { snakeCase } from 'snake-case';
import { stitchingDirectives } from '@graphql-tools/stitching-directives';
import {
createGraphQLError,
getDirectiveExtensions,
getDocumentNodeFromSchema,
MapperKind,
mapSchema,
type FieldMapper,
} from '@graphql-tools/utils';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/compose-cli/src/run.ts
Outdated
Comment on lines
+131
to
+137
| const errors = validateSupergraphSdl(supergraphSdl, subgraphs); | ||
| if (errors.length) { | ||
| log.error(`Composition validation failed with the following errors:`); | ||
| for (const error of errors) { | ||
| log.error(error.message); | ||
| } | ||
| process.exit(1); |
There was a problem hiding this comment.
validateSupergraphSdl is executed unconditionally, but when --subgraph is used getComposedSchemaFromConfig returns that subgraph’s SDL (not a composed supergraph). This will throw (e.g. missing join__Graph) and break the --subgraph workflow. Consider skipping validateSupergraphSdl when config.subgraph is set, or returning a real supergraph SDL for that mode.
Suggested change
| const errors = validateSupergraphSdl(supergraphSdl, subgraphs); | |
| if (errors.length) { | |
| log.error(`Composition validation failed with the following errors:`); | |
| for (const error of errors) { | |
| log.error(error.message); | |
| } | |
| process.exit(1); | |
| if (!config.subgraph) { | |
| const errors = validateSupergraphSdl(supergraphSdl, subgraphs); | |
| if (errors.length) { | |
| log.error(`Composition validation failed with the following errors:`); | |
| for (const error of errors) { | |
| log.error(error.message); | |
| } | |
| process.exit(1); | |
| } |
Comment on lines
+114
to
+118
| } | ||
| // Args validation | ||
| if (resolveToDirective.sourceArgs) { | ||
| const argOfFields = | ||
| 'arguments' in fieldDef && fieldDef.arguments |
There was a problem hiding this comment.
typeDef can be undefined when sourceTypeName doesn’t exist in the referenced subgraph, but the code uses it immediately with the in operator ('fields' in typeDef), which will throw at runtime. Add an explicit if (!typeDef) branch that reports a GraphQL error with suggestions for the type name.
Comment on lines
+117
to
+123
| const argOfFields = | ||
| 'arguments' in fieldDef && fieldDef.arguments | ||
| ? fieldDef.arguments.map(arg => arg.name.value) | ||
| : []; | ||
| for (const argName in resolveToDirective.sourceArgs) { | ||
| const argNameInField = argOfFields.find(arg => arg === argName); | ||
| if (!argNameInField) { |
There was a problem hiding this comment.
fields is built as a string[] (field names), but later it’s treated as if it contained AST field nodes (fields.map(f => f.name.value)). This will crash when trying to build suggestions. Use the string[] directly for suggestionList (or keep both the field nodes and the names separately).
Comment on lines
+117
to
+142
| const argOfFields = | ||
| 'arguments' in fieldDef && fieldDef.arguments | ||
| ? fieldDef.arguments.map(arg => arg.name.value) | ||
| : []; | ||
| for (const argName in resolveToDirective.sourceArgs) { | ||
| const argNameInField = argOfFields.find(arg => arg === argName); | ||
| if (!argNameInField) { | ||
| const suggestions = suggestionList(argName, argOfFields); | ||
| const suggestionStr = suggestions.length | ||
| ? ` Did you mean "${suggestions.join(' or ')}"?` | ||
| : ''; | ||
| errors.push( | ||
| createGraphQLError( | ||
| `@resolveTo directive on type ${typeName} references unknown argument ${argName} in field ${fieldName} of subgraph ${subgraphName} ${suggestionStr}`, | ||
| { | ||
| nodes: type.astNode, | ||
| }, | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
fieldDef is derived from fields.find(...) where fields is a string[], so fieldDef is a string. The subsequent args validation treats fieldDef like a field AST node (fieldDef.arguments), which means argument validation will never work and may misreport every sourceArgs entry as unknown. Keep the actual field definition node (from typeDef.fields) so you can read its arguments correctly.
Comment on lines
+117
to
133
| const annotatedSubgraphs = getAnnotatedSubgraphs(subgraphConfigsForComposition, { | ||
| ignoreSemanticConventions: config.ignoreSemanticConventions, | ||
| alwaysAddTransportDirective: config.subgraph != null, | ||
| }); | ||
| if (config.subgraph) { | ||
| const annotatedSubgraphs = getAnnotatedSubgraphs(subgraphConfigsForComposition, { | ||
| ignoreSemanticConventions: config.ignoreSemanticConventions, | ||
| alwaysAddTransportDirective: true, | ||
| }); | ||
| const subgraph = annotatedSubgraphs.find(sg => sg.name === config.subgraph); | ||
| if (!subgraph) { | ||
| logger.error(`Subgraph ${config.subgraph} not found`); | ||
| process.exit(1); | ||
| } | ||
| return print(subgraph.typeDefs); | ||
| return { | ||
| supergraphSdl: print(subgraph.typeDefs), | ||
| subgraphs: annotatedSubgraphs, | ||
| }; | ||
| } | ||
| const result = composeSubgraphs(subgraphConfigsForComposition, { | ||
| ignoreSemanticConventions: config.ignoreSemanticConventions, | ||
| alwaysAddTransportDirective: false, | ||
| }); | ||
| const result = composeAnnotatedSubgraphs(annotatedSubgraphs); | ||
| if (result.errors?.length) { |
There was a problem hiding this comment.
The function return type changed from a SDL string to an object { supergraphSdl, subgraphs }, but getComposedSchemaFromConfig is re-exported from the package entrypoint. This is a breaking change for programmatic consumers and doesn’t match the patch-level changeset. Consider keeping the old return type (and adding a new API for additional data) or bumping the version appropriately.
Comment on lines
+329
to
+330
| throw new Error( | ||
| `Failed to apply transform "${transform.name}" on subgraph "${subgraphName}": ${e.message}`, |
There was a problem hiding this comment.
Wrapping transform errors with new Error(...) drops the original error type and stack trace, and e.message can be undefined if something non-Error is thrown. Prefer preserving the original error as cause (and stringify non-Error throws) so the resulting failure is still debuggable.
Suggested change
| throw new Error( | |
| `Failed to apply transform "${transform.name}" on subgraph "${subgraphName}": ${e.message}`, | |
| const cause = e instanceof Error ? e : new Error(String(e)); | |
| const errorMessage = e instanceof Error ? e.message : String(e); | |
| throw new Error( | |
| `Failed to apply transform "${transform.name}" on subgraph "${subgraphName}": ${errorMessage}`, | |
| { cause }, |
Comment on lines
+288
to
+325
| // Validate merge directive | ||
| // First validate operation config | ||
| const operationType = | ||
| keyConfig.resolveReference?.operation || ('query' as OperationTypeNode); | ||
| if ( | ||
| operationType !== 'query' && | ||
| operationType !== 'mutation' && | ||
| operationType !== 'subscription' | ||
| ) { | ||
| throw new Error( | ||
| `Invalid operation type "${operationType}" in resolveReference config for @key directive on ${type.name} type. Expected "query", "mutation" or "subscription".`, | ||
| ); | ||
| } | ||
| const targetFieldName = keyConfig.resolveReference.fieldName; | ||
| if (!targetFieldName) { | ||
| throw new Error( | ||
| `Missing fieldName in resolveReference config for @key directive on ${type.name} type.`, | ||
| ); | ||
| } | ||
| const rootType = getDefinedRootType(subgraphSchema, operationType); | ||
| if (!rootType) { | ||
| throw new Error( | ||
| `Root type for operation "${operationType}" not found in schema for @key directive on ${type.name} type.`, | ||
| ); | ||
| } | ||
| const rootTypeFields = rootType.getFields(); | ||
| if (!rootTypeFields[targetFieldName]) { | ||
| const suggestions = suggestionList( | ||
| targetFieldName, | ||
| Object.keys(rootTypeFields) as string[], | ||
| ); | ||
| const suggestionStr = suggestions.length | ||
| ? ` Did you mean "${suggestions.join(' or ')}"?` | ||
| : ''; | ||
| throw new Error( | ||
| `Field "${targetFieldName}" not found in root type "${rootType.name}" for @key directive on ${type.name} type. ${suggestionStr}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new resolveReference validation throws plain Errors, while the rest of this transform uses TransformValidationError for configuration problems. Using the same error class here keeps error handling consistent (and avoids losing the semantic error type when callers catch by name).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/compose-cli/src/validate.ts (1)
29-43: Use@join__graph(name:)values for subgraph identity, not enum value names.
subgraphNamesis currently populated from enum value identifiers (value.name), which can differ from the actual subgraph name carried in@join__graph(name: ...). This can produce false “unknown subgraph” errors.Proposed fix
- const subgraphNames = new Set(); + const subgraphNames = new Set<string>(); for (const value of joinGraphEnum.getValues()) { - subgraphNames.add(value.name); const directives = getDirectiveExtensions(value, schema); if (!directives?.join__graph) { throw new FatalCompositionError( `Expected join__Graph enum value ${value.name} to have `@join__graph` directive`, ); } for (const directive of directives.join__graph) { if (!directive.name) { throw new FatalCompositionError( `Expected `@join__graph` directive on join__Graph enum value ${value.name} to have a name argument`, ); } + subgraphNames.add(directive.name); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compose-cli/src/validate.ts` around lines 29 - 43, The code currently adds enum value identifiers (value.name) to subgraphNames, but the canonical subgraph identity is the `@join__graph`(name: ...) argument; update the logic in the loop over joinGraphEnum.getValues() (around joinGraphEnum, subgraphNames, getDirectiveExtensions, directives.join__graph) to extract and add the name argument from each join__graph directive (e.g., directive.name) to subgraphNames instead of value.name, and still throw a FatalCompositionError via the same checks if a join__graph directive or its name argument is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/compose-cli/src/run.ts`:
- Around line 130-138: The code unconditionally calls validateSupergraphSdl on
the value returned by getComposedSchemaFromConfig, which breaks subgraph-only
mode because supergraphSdl in that path is not a composed supergraph; update the
logic to skip validation when running in subgraph-only mode (check
config.subgraph) or when supergraphSdl is not a composed supergraph, e.g., only
call validateSupergraphSdl(supergraphSdl, subgraphs) when config.subgraph is
falsy (or when a composed marker like join__Graph exists), and ensure you do not
process.exit(1) for subgraph-only output so --subgraph continues to work.
In `@packages/compose-cli/src/validate.ts`:
- Around line 114-145: The AST lookup for
resolveToDirective.sourceTypeName/sourceFieldName mistakenly maps typeDef.fields
to strings so fieldDef becomes a string; revert fields to the AST
FieldDefinition nodes (do not map to f.name.value), then find fieldDef by
comparing f.name.value === fieldName; when building suggestions call
suggestionList(fieldName, fields.map(f => f.name.value)) (i.e., map only for
suggestions), and for arg validation use the actual fieldDef.arguments (e.g.,
const argOfFields = fieldDef.arguments ? fieldDef.arguments.map(a =>
a.name.value) : []), ensuring subsequent checks like 'arguments' in fieldDef or
accessing fieldDef.arguments operate on the AST node rather than a string.
- Around line 46-76: The loop currently validates only type-level directives via
getDirectiveExtensions(type, schema) and misses field-level `@merge` and
`@resolveTo`; update the validation to also iterate each type's fields (use
type.getFields() inside the for (const typeName in typeMap) block), call
getDirectiveExtensions(field, schema) for each field, and apply the same checks:
for `@merge` verify mergeDirective.subgraph exists and is in subgraphNames, and
for `@resolveTo` verify resolveTo.sourceName/subgraph (or the expected arguments)
are present and valid. When pushing errors use createGraphQLError with nodes:
field.astNode (or the appropriate field AST node) and keep existing error
messages/formats; reuse same symbols mergeDirective.subgraph,
resolveTo.sourceName, subgraphNames, createGraphQLError to locate where to add
these checks.
---
Nitpick comments:
In `@packages/compose-cli/src/validate.ts`:
- Around line 29-43: The code currently adds enum value identifiers (value.name)
to subgraphNames, but the canonical subgraph identity is the `@join__graph`(name:
...) argument; update the logic in the loop over joinGraphEnum.getValues()
(around joinGraphEnum, subgraphNames, getDirectiveExtensions,
directives.join__graph) to extract and add the name argument from each
join__graph directive (e.g., directive.name) to subgraphNames instead of
value.name, and still throw a FatalCompositionError via the same checks if a
join__graph directive or its name argument is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2820b1f6-d674-4758-b2cd-ba027c5c818a
📒 Files selected for processing (6)
.changeset/some-pianos-smoke.mdpackages/compose-cli/src/getComposedSchemaFromConfig.tspackages/compose-cli/src/run.tspackages/compose-cli/src/validate.tspackages/fusion/composition/src/compose.tspackages/fusion/composition/src/transforms/federation.ts
Comment on lines
+46
to
+76
| for (const typeName in typeMap) { | ||
| const type = typeMap[typeName]; | ||
| if (type) { | ||
| const directives = getDirectiveExtensions(type, schema); | ||
| if (directives?.merge) { | ||
| for (const mergeDirective of directives.merge) { | ||
| const subgraphName = mergeDirective.subgraph; | ||
| if (!subgraphName) { | ||
| errors.push( | ||
| createGraphQLError( | ||
| `Expected @merge directive on type ${typeName} to have a subgraph argument`, | ||
| { | ||
| nodes: type.astNode, | ||
| }, | ||
| ), | ||
| ); | ||
| continue; | ||
| } else if (!subgraphNames.has(subgraphName)) { | ||
| errors.push( | ||
| createGraphQLError( | ||
| `@merge directive on type ${typeName} references unknown subgraph ${subgraphName}`, | ||
| { | ||
| nodes: type.astNode, | ||
| }, | ||
| ), | ||
| ); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| if (directives?.resolveTo) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the validate.ts file and examine its content
fd -t f "validate.ts" packages/compose-cli/src/Repository: ardatan/graphql-mesh
Length of output: 100
🏁 Script executed:
# Read the validate.ts file to understand full context
cat -n packages/compose-cli/src/validate.tsRepository: ardatan/graphql-mesh
Length of output: 6560
🏁 Script executed:
# Search for `@merge` directive usage in test files and examples
rg "@merge" --type graphql --type typescript --type json -A 2 -B 2 | head -100Repository: ardatan/graphql-mesh
Length of output: 95
🏁 Script executed:
# Search for `@merge` directive usage across the codebase
rg "@merge" -A 2 -B 2 | head -150Repository: ardatan/graphql-mesh
Length of output: 13079
🏁 Script executed:
# Search for `@resolveTo` directive usage
rg "@resolveTo" -A 2 -B 2 | head -100Repository: ardatan/graphql-mesh
Length of output: 7572
🏁 Script executed:
# Search for field-level directive validation
rg "getDirectiveExtensions" -A 3 -B 1Repository: ardatan/graphql-mesh
Length of output: 31371
🏁 Script executed:
# Check if there are any tests for the validate function
rg "validateSupergraphSdl" -A 10 -B 2 | head -100Repository: ardatan/graphql-mesh
Length of output: 2560
🏁 Script executed:
# Search for test files related to validate
fd -t f "*test*" packages/compose-cli/src/ | head -20Repository: ardatan/graphql-mesh
Length of output: 349
🏁 Script executed:
# Search for test files related to validate
fd --glob "*test*" packages/compose-cli/Repository: ardatan/graphql-mesh
Length of output: 150
🏁 Script executed:
# Check test directory structure and content
find packages/compose-cli/tests -type f -name "*.ts" | head -20Repository: ardatan/graphql-mesh
Length of output: 122
🏁 Script executed:
# Search for field iteration in validation or processing of directives
rg "MapperKind.FIELD|\.fields\[|for.*field" packages/compose-cli/src/ -A 2 -B 2Repository: ardatan/graphql-mesh
Length of output: 46
🏁 Script executed:
# Search for any validation of field-level `@merge` or `@resolveTo` directives
rg "fieldConfig|field.*merge|field.*resolveTo" packages/compose-cli/ -A 2 -B 2Repository: ardatan/graphql-mesh
Length of output: 471
🏁 Script executed:
# Check GraphQL type structure to verify if fields can be iterated
rg "getFields|\.fields" packages/compose-cli/src/ -A 2Repository: ardatan/graphql-mesh
Length of output: 1849
Field-level @merge and @resolveTo directives are not validated.
The validation loop only checks type-level directives via getDirectiveExtensions(type, schema). Documentation shows both directives are primarily used on fields (e.g., author(id: ID): Author @merge(...) and author: Author @resolveTo(...)), but field-level instances with missing or invalid subgraph/sourceName arguments will silently pass validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/compose-cli/src/validate.ts` around lines 46 - 76, The loop
currently validates only type-level directives via getDirectiveExtensions(type,
schema) and misses field-level `@merge` and `@resolveTo`; update the validation to
also iterate each type's fields (use type.getFields() inside the for (const
typeName in typeMap) block), call getDirectiveExtensions(field, schema) for each
field, and apply the same checks: for `@merge` verify mergeDirective.subgraph
exists and is in subgraphNames, and for `@resolveTo` verify
resolveTo.sourceName/subgraph (or the expected arguments) are present and valid.
When pushing errors use createGraphQLError with nodes: field.astNode (or the
appropriate field AST node) and keep existing error messages/formats; reuse same
symbols mergeDirective.subgraph, resolveTo.sourceName, subgraphNames,
createGraphQLError to locate where to add these checks.
Contributor
Apollo Federation Subgraph Compatibility Results
Learn more: |
Owner
Author
|
@copilot apply changes based on the comments in this thread and also add tests for those changes |
ardatan
force-pushed
the
validation-transform
branch
from
963a07d to
b2b46ed
Compare
…nError, backward-compat API, and add tests Agent-Logs-Url: https://github.com/ardatan/graphql-mesh/sessions/c0546f66-b604-46e4-bbb2-9c0ef51f6a4d Co-authored-by: ardatan <20847995+ardatan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ardatan/graphql-mesh/sessions/c0546f66-b604-46e4-bbb2-9c0ef51f6a4d Co-authored-by: ardatan <20847995+ardatan@users.noreply.github.com>
Contributor
Applied all review changes in commits Review fixes applied:
Tests added:
|
Copilot
AI
changed the title
feat(composition): validation for transforms
feat(composition): validation for transforms with better errors and backward-compat API
Apr 9, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/compose-cli/src/validate.ts (1)
7-13:Error.captureStackTraceis V8-specific and may not exist in all environments.While this works in Node.js, it could throw in non-V8 environments (e.g., some edge runtimes). Consider adding a guard.
♻️ Suggested defensive guard
export class FatalCompositionError extends Error { constructor(message: string) { super(`Fatal composition error: ${message}`); this.name = 'FatalCompositionError'; - Error.captureStackTrace(this, this.constructor); + if (Error.captureStackTrace) { + Error.captureStackTrace(this, this.constructor); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compose-cli/src/validate.ts` around lines 7 - 13, The class FatalCompositionError currently calls Error.captureStackTrace in its constructor which is V8-specific and may throw in non-V8 runtimes; update the FatalCompositionError constructor to check for the existence of Error.captureStackTrace (e.g., typeof Error.captureStackTrace === 'function') before calling it and fall back silently when unavailable so the code is safe in edge/non-V8 environments while preserving the message and name assignments.packages/fusion/composition/tests/transforms/federation.test.ts (1)
122-138: Minor inefficiency: transform is called twice per test case.Each test invokes
transform(schema, subgraphConfig)twice—once to check the error type and once for the message pattern. Consider capturing the error once usingexpect().toThrow()with a function that validates both, or use a try/catch pattern to assert on both properties from a single invocation.♻️ Suggested optimization (optional)
it('throws TransformValidationError when fieldName is missing', () => { const transform = createFederationTransform({ Foo: { key: { fields: 'id', resolveReference: { fieldName: '', keyArg: 'id', }, }, }, }); - expect(() => transform(schema, subgraphConfig)).toThrow(TransformValidationError); - expect(() => transform(schema, subgraphConfig)).toThrow( - /Missing fieldName in resolveReference config for `@key` directive on Foo type/, - ); + expect(() => transform(schema, subgraphConfig)).toThrow( + expect.objectContaining({ + name: 'TransformValidationError', + message: expect.stringMatching(/Missing fieldName in resolveReference config for `@key` directive on Foo type/), + }), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fusion/composition/tests/transforms/federation.test.ts` around lines 122 - 138, The test currently calls transform(schema, subgraphConfig) twice; change it to invoke transform once and capture the thrown error (e.g., via try/catch) and then assert both that the error is an instance of TransformValidationError and that its message matches /Missing fieldName.../; this involves updating the test that uses createFederationTransform (variable transform) to call transform(schema, subgraphConfig) a single time and then run expect(err).toBeInstanceOf(TransformValidationError) and expect(err.message).toMatch(/Missing fieldName in resolveReference config for `@key` directive on Foo type/).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/compose-cli/src/validate.ts`:
- Around line 7-13: The class FatalCompositionError currently calls
Error.captureStackTrace in its constructor which is V8-specific and may throw in
non-V8 runtimes; update the FatalCompositionError constructor to check for the
existence of Error.captureStackTrace (e.g., typeof Error.captureStackTrace ===
'function') before calling it and fall back silently when unavailable so the
code is safe in edge/non-V8 environments while preserving the message and name
assignments.
In `@packages/fusion/composition/tests/transforms/federation.test.ts`:
- Around line 122-138: The test currently calls transform(schema,
subgraphConfig) twice; change it to invoke transform once and capture the thrown
error (e.g., via try/catch) and then assert both that the error is an instance
of TransformValidationError and that its message matches /Missing fieldName.../;
this involves updating the test that uses createFederationTransform (variable
transform) to call transform(schema, subgraphConfig) a single time and then run
expect(err).toBeInstanceOf(TransformValidationError) and
expect(err.message).toMatch(/Missing fieldName in resolveReference config for
`@key` directive on Foo type/).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 358f95fa-0411-482f-931b-95c9459832c0
📒 Files selected for processing (9)
packages/compose-cli/src/getComposedSchemaFromConfig.tspackages/compose-cli/src/run.tspackages/compose-cli/src/validate.tspackages/compose-cli/tests/validate.test.tspackages/fusion/composition/src/compose.tspackages/fusion/composition/src/transforms/federation.tspackages/fusion/composition/tests/transforms/federation.test.tspackages/legacy/runtime/test/reproduction-8688/sourceService.tspackages/plugins/hive/tests/hive.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/legacy/runtime/test/reproduction-8688/sourceService.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/fusion/composition/src/transforms/federation.ts
- packages/fusion/composition/src/compose.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds validation for Federation transform
resolveReferenceconfig and@merge/@resolveTodirectives with actionable error messages (including field/argument suggestions). Fixes several bugs introduced in the initial implementation and restores API backward compatibility.Fixes
getComposedSchemaFromConfig: Restored return type toPromise<string>(was changed to{ supergraphSdl, subgraphs }, breaking programmatic consumers). Added internalgetComposedResultFromConfigused byrun.tsinstead.run.ts: SkipvalidateSupergraphSdlwhen--subgraphflag is set (single-subgraph SDL is not a supergraph).federation.ts: Changedthrow new Error(...)→throw new TransformValidationError(...)in allresolveReferencevalidation to be consistent with the rest of the transform.compose.ts: Preserve original error ascausewhen wrapping transform failures; stringify non-Errorthrows.validate.ts: Fixed three bugs in@resolveTofield/arg validation:typeDefwas used without an undefined check (throws whensourceTypeNamedoesn't exist)fieldswasFieldDefinitionNode[]butsuggestionListwas called with.map(f => f.name.value)on already-string valuesfieldDefwas a string (fromfields.find) but accessed as.arguments(an AST node property) — arg validation never actually ranif (!subgraph)block and unused importsTests Added
federation.test.ts: Validation forresolveReference— missingfieldName, non-existent field (with suggestion), invalid operation type all throwTransformValidationError.validate.test.ts(new):validateSupergraphSdlcoverage for@merge(unknown subgraph) and@resolveTo(unknown subgraph, type, field with suggestion, argument with suggestion).