Fix semantic conventions option #8720
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughFor compose-cli, forwards ignoreSemanticConventions into the composition path (and sets alwaysAddTransportDirective explicitly), adds e2e fixtures/tests verifying presence/absence of @merge directives, adds a package for the e2e tests, schema, and a changeset for a patch release. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as mesh-compose (CLI)
participant GetComposed as getComposedSchemaFromConfig
participant Compose as composeSubgraphs
participant FS as Filesystem
CLI->>GetComposed: invoke with composeConfig (may include ignoreSemanticConventions)
GetComposed->>Compose: composeSubgraphs(subgraphConfigs, { ignoreSemanticConventions: config.ignoreSemanticConventions, alwaysAddTransportDirective: false })
alt ignoreSemanticConventions = true
Note over Compose: Compose omits automatic semantic merge directives
else ignoreSemanticConventions = false
Note over Compose: Compose may add semantic merge directives
end
Compose-->>GetComposed: supergraph SDL
GetComposed->>FS: write supergraph output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(No out-of-scope functional changes identified.) Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.changeset/lucky-bats-lead.md (1)
5-5: Optionally reference the linked issue for traceability.Consider appending “Fixes #8714.” to the body to auto-link the issue in release notes.
packages/compose-cli/src/getComposedSchemaFromConfig.ts (2)
119-122: Good move centralizing shared composition options. Add an explicit type and coerce the boolean.Typing catches typos at compile-time and documents intent. Coercing to boolean avoids passing undefined.
Apply:
@@ -import { - composeSubgraphs, - futureAdditions, - getAnnotatedSubgraphs, - type FutureAddition, - type SubgraphConfig, -} from '@graphql-mesh/fusion-composition'; +import { + composeSubgraphs, + futureAdditions, + getAnnotatedSubgraphs, + type FutureAddition, + type SubgraphConfig, + type ComposeSubgraphsOptions, +} from '@graphql-mesh/fusion-composition'; @@ - const options = { - ignoreSemanticConventions: config.ignoreSemanticConventions, - alwaysAddTransportDirective: true, - }; + const options: ComposeSubgraphsOptions = { + ignoreSemanticConventions: Boolean(config.ignoreSemanticConventions), + alwaysAddTransportDirective: true, + };
121-121: Double-check side effects of alwaysAddTransportDirective: true.Setting this unconditionally means minimal @transport directives won’t be stripped even when headers/options are empty. If that’s intentional for CLI output stability, great; otherwise consider exposing a CLI flag or deferring to library defaults.
e2e/semantic-conventions/package.json (1)
1-8: Align GraphQL dependency strategy with the monorepo.If other e2e packages use workspace protocol for GraphQL, consider switching to keep versions in lockstep; otherwise this is fine as-is.
Would you like me to scan sibling e2e packages and open a small follow-up to standardize?
e2e/semantic-conventions/mesh.config.conventions-disabled.ts (1)
6-18: Optional: Add a companion test for the config.subgraph flow.The bug was in the subgraphs[] branch, which you now cover. For completeness against future regressions, consider a second minimal config using config.subgraph to ensure both code paths honor ignoreSemanticConventions.
e2e/semantic-conventions/semantic-conventions.test.ts (3)
4-4: Regex is brittle; allow optional whitespace and avoid greedy captureThe current
/@merge\(.*\)/can miss matches if a space appears before(or if the printer introduces formatting tweaks. A quick, minimal improvement:Apply this diff to make the regex more resilient:
-const regExp = /@merge\(.*\)/; +const regExp = /@merge\s*\([^)]*\)/m;Note: If you adopt the AST-based approach suggested below, this regex becomes unnecessary.
1-14: Prefer AST-based assertion over regex to avoid false positives/negativesRegex can (1) falsely match directive definitions (e.g.,
directive @merge(...)) and (2) break with printer formatting. Parsing the SDL and scanning for actual directive usages is robust and intention-revealing.Apply this diff to parse the SDL and assert on directive usage:
-import { createTenv } from '@e2e/tenv'; +import { createTenv } from '@e2e/tenv'; +import { parse, visit } from 'graphql'; const { compose, fs } = createTenv(__dirname); -const regExp = /@merge\(.*\)/; + +function hasDirectiveUsage(sdl: string, name: string): boolean { + const ast = parse(sdl); + let used = false; + visit(ast, { + Directive(node) { + if (node.name.value === name) used = true; + }, + }); + return used; +} -it('should generate merge directives', async () => { +it('should generate merge directive usages when conventions are enabled', async () => { const { output } = await compose({ output: 'graphql', args: ['-c', 'mesh.config.conventions-enabled.ts'] }); - await expect(fs.read(output)).resolves.toMatch(regExp); + const sdl = await fs.read(output); + expect(hasDirectiveUsage(sdl, 'merge')).toBe(true); }); -it('should not generate merge directives', async () => { +it('should not generate merge directive usages when conventions are disabled', async () => { const { output } = await compose({ output: 'graphql', args: ['-c', 'mesh.config.conventions-disabled.ts'] }); - await expect(fs.read(output)).resolves.not.toMatch(regExp); + const sdl = await fs.read(output); + expect(hasDirectiveUsage(sdl, 'merge')).toBe(false); });
6-14: Add coverage for multi-subgraphs (the original bug surface)The issue fixed here manifests when composing multiple subgraphs via
config.subgraphs. The current tests exercise only a single subgraph. Please add an e2e that composes at least two subgraphs and verifies:
- enabled: usages of
@mergeare present,- disabled (
ignoreSemanticConventions: true): no usages of@mergeare present.I can draft the additional configs (e.g.,
mesh.config.subgraphs-conventions-enabled.tsandmesh.config.subgraphs-conventions-disabled.ts) and, if needed, minimal additional SDL (or reuse the same SDL for two subgraphs) plus corresponding test blocks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.changeset/lucky-bats-lead.md(1 hunks)e2e/semantic-conventions/mesh.config.conventions-disabled.ts(1 hunks)e2e/semantic-conventions/mesh.config.conventions-enabled.ts(1 hunks)e2e/semantic-conventions/package.json(1 hunks)e2e/semantic-conventions/schema.graphql(1 hunks)e2e/semantic-conventions/semantic-conventions.test.ts(1 hunks)packages/compose-cli/src/getComposedSchemaFromConfig.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
e2e/semantic-conventions/semantic-conventions.test.ts (1)
e2e/utils/tenv.ts (2)
createTenv(216-655)compose(303-358)
packages/compose-cli/src/getComposedSchemaFromConfig.ts (1)
packages/fusion/composition/src/compose.ts (2)
getAnnotatedSubgraphs(77-547)composeSubgraphs(562-572)
e2e/semantic-conventions/mesh.config.conventions-enabled.ts (3)
e2e/semantic-conventions/mesh.config.conventions-disabled.ts (1)
composeConfig(6-18)packages/compose-cli/src/types.ts (1)
defineConfig(45-47)packages/compose-cli/src/loadGraphQLHTTPSubgraph.ts (1)
loadGraphQLHTTPSubgraph(203-368)
e2e/semantic-conventions/mesh.config.conventions-disabled.ts (3)
e2e/semantic-conventions/mesh.config.conventions-enabled.ts (1)
composeConfig(6-17)packages/compose-cli/src/types.ts (1)
defineConfig(45-47)packages/compose-cli/src/loadGraphQLHTTPSubgraph.ts (1)
loadGraphQLHTTPSubgraph(203-368)
🔇 Additional comments (5)
.changeset/lucky-bats-lead.md (1)
1-5: Changeset looks good and targets the right package.Patch bump on @graphql-mesh/compose-cli with a concise message aligns with the scope of this fix.
packages/compose-cli/src/getComposedSchemaFromConfig.ts (1)
124-124: Bug fix confirmed: options now flow through both single- and multi-subgraph paths.Previously only the single-subgraph branch respected ignoreSemanticConventions; this resolves #8714’s core complaint for subgraphs[]. Nicely done.
Also applies to: 132-132
e2e/semantic-conventions/mesh.config.conventions-disabled.ts (1)
6-18: Config correctly disables semantic conventions via subgraphs[].This exercises the path that previously ignored the option. Looks good.
e2e/semantic-conventions/mesh.config.conventions-enabled.ts (1)
6-17: Enabled config mirrors the disabled variant appropriately.Good parity between the two configs to validate presence vs. absence of @merge.
e2e/semantic-conventions/schema.graphql (1)
1-7: LGTM: minimal schema is sufficient for exercising semantic conventionsThe simple Query and Book types are adequate for the e2e scenario. No issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/lucky-bats-lead.md (2)
5-5: Polish wording and capture both forwarded flags for accuracy.Current phrasing is a bit awkward and only mentions
ignoreSemanticConventions, while the PR also forwardsalwaysAddTransportDirective. Consider this tighter copy:-`ignoreSemanticConventions` option was ignored when you compose subgraphs into a supergraph. +Fix: Respect `ignoreSemanticConventions` (and `alwaysAddTransportDirective`) when composing via `composeConfig.subgraphs`. +Previously, these options were only applied when using the single `subgraph` path.
5-6: Remove stray trailing character.There appears to be an orphaned “6” on Line 6. It’s harmless but untidy in release notes.
-`ignoreSemanticConventions` option was ignored when you compose subgraphs into a supergraph. -6 +`ignoreSemanticConventions` option was ignored when you compose subgraphs into a supergraph.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
.changeset/lucky-bats-lead.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/lucky-bats-lead.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...you compose subgraphs into a supergraph.
(QB_NEW_EN)
⏰ 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: apollo-federation-compatibility
- GitHub Check: e2e / node v20
- GitHub Check: integration / node 18
- GitHub Check: integration / node 22
- GitHub Check: integration / node 20
- GitHub Check: check
- GitHub Check: unit / node 22
- GitHub Check: unit / node 18
🔇 Additional comments (1)
.changeset/lucky-bats-lead.md (1)
1-4: Changeset front‑matter looks correct for a patch bump.Package name and release type align with the PR scope (compose-cli only).
| } | ||
| const options = { | ||
| ignoreSemanticConventions: config.ignoreSemanticConventions, | ||
| alwaysAddTransportDirective: true, |
There was a problem hiding this comment.
Now I realized, this should not be true for regular subgraph composition.
ardatan
force-pushed
the
fix-semantic-conventions
branch
from
e8daa6d to
701c927
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/compose-cli/src/getComposedSchemaFromConfig.ts (1)
131-134: Correctly propagate ignoreSemanticConventions and disable transport directives in multi-subgraph compositionThis aligns the multi-subgraph path with the intended behavior and fixes the reported issue where merge directives appeared despite
ignoreSemanticConventions: true. Good catch.
🧹 Nitpick comments (4)
.changeset/lucky-bats-lead.md (1)
5-5: Tighten phrasing and clarify scope in the changeset entryMinor grammar fix and make it explicit this affects the subgraphs array path; also mention the transport directive toggle to preempt questions when scanning release notes.
-`ignoreSemanticConventions` option was ignored when you compose subgraphs into a supergraph. +`ignoreSemanticConventions` option was ignored when composing multiple subgraphs (`composeConfig.subgraphs`) into a supergraph. +The composition path now forwards this flag and disables `alwaysAddTransportDirective` by default.packages/compose-cli/src/getComposedSchemaFromConfig.ts (1)
131-134: Document why transport directives differ between annotated vs. composed pathsFuture readers may wonder why
alwaysAddTransportDirectiveistruefor the annotated single-subgraph path butfalsehere. A short comment helps avoid regressions.- const result = composeSubgraphs(subgraphConfigsForComposition, { + // In regular supergraph composition we avoid synthetic transport directives; + // they are only needed when printing a single annotated subgraph (config.subgraph). + const result = composeSubgraphs(subgraphConfigsForComposition, { ignoreSemanticConventions: config.ignoreSemanticConventions, alwaysAddTransportDirective: false, });e2e/semantic-conventions/semantic-conventions.test.ts (2)
4-4: Make the directive detection regex more robust to whitespace/formattingGraphQL printers can vary spacing/line-wrapping. This regex avoids potential false negatives.
-const regExp = /@merge\(.*\)/; +const regExp = /@merge\s*\([^)]*\)/m;
6-20: Add a multi-subgraph scenario to reflect the original failure modeThe linked issue was about the
subgraphsarray. The current fixtures use a single subgraph configuration, which is good, but a second test that composes two simple subgraphs (one providing an entity and another referencing it) would harden coverage and prevent regressions.If you’d like, I can draft:
- two tiny SDLs (Book + Review),
- corresponding
mesh.config.conventions-enabled|disabled.2-subgraphs.tsfixtures,- and an extra test that asserts presence/absence of
@mergeacross the composed supergraph.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.changeset/lucky-bats-lead.md(1 hunks)e2e/semantic-conventions/mesh.config.conventions-disabled.ts(1 hunks)e2e/semantic-conventions/mesh.config.conventions-enabled.ts(1 hunks)e2e/semantic-conventions/package.json(1 hunks)e2e/semantic-conventions/schema.graphql(1 hunks)e2e/semantic-conventions/semantic-conventions.test.ts(1 hunks)packages/compose-cli/src/getComposedSchemaFromConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/semantic-conventions/mesh.config.conventions-disabled.ts
- e2e/semantic-conventions/package.json
- e2e/semantic-conventions/schema.graphql
- e2e/semantic-conventions/mesh.config.conventions-enabled.ts
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/semantic-conventions/semantic-conventions.test.ts (1)
e2e/utils/tenv.ts (1)
compose(303-358)
packages/compose-cli/src/getComposedSchemaFromConfig.ts (1)
packages/fusion/composition/src/compose.ts (1)
composeSubgraphs(562-572)
🪛 LanguageTool
.changeset/lucky-bats-lead.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...you compose subgraphs into a supergraph.
(QB_NEW_EN)
🔇 Additional comments (1)
packages/compose-cli/src/getComposedSchemaFromConfig.ts (1)
131-134: ✅ All composeSubgraphs call sites are properly guarded
- Only two production‐code invocations remain:
• In packages/fusion/composition/src/compose.ts (getUnifiedGraphGracefully) using defaults
• In packages/compose-cli/src/getComposedSchemaFromConfig.ts (one for single‐subgraph via getAnnotatedSubgraphs with alwaysAddTransportDirective: true, another for multi-subgraph via composeSubgraphs with alwaysAddTransportDirective: false)- No other unguarded composeSubgraphs calls were found outside of tests.
- Every E2E mesh.config.ts fixture now uses the subgraphs array, confirming the new multi-subgraph path is exercised.
2 participants
Description
The option to disable semantic conventions currently is only passed down if a single subgraph is composed (config.subgraph). It should also be possible to disable the option when composing multiple subgraphs to a supergraph (config.subgraphs).
Fixes #8714
Type of change
How Has This Been Tested?
Manual tests are described in #8714. I basically used a schema that contains queries conforming to semantic conventions and disabled the feature. The directives were genereted anyway.
Additionally I implemented a basic e2e test (semantic-conventions).
Test Environment:
@graphql-mesh/compose-cli: 1.4.12Checklist:
CONTRIBUTING doc and the
style guidelines of this project
changeset using
yarn changesetthat bumps the version