feat: add @semanticNonNull support #2221
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughAdds support for a new @semanticNonNull(levels: [Int!]! = [0]) directive: parsing and validation during normalization, per-subgraph null-level tracking on FieldData, federation/client emission of a generated directive, new error helpers/types/constants/utilities, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
composition/src/schema-building/types.ts (1)
111-111: Define and enforce invariants for nullLevelsBySubgraphName.Please confirm normalization enforces:
- levels are non‑negative integers,
- de‑duplicated (Set helps) and within the field’s type nullability depth,
- empty set semantics (unset vs. empty) are well‑defined.
Also verify any FieldData copy/merge paths deep‑copy this Map<SubgraphName, Set> to avoid aliasing.
composition/src/utils/utils.ts (2)
196-219: Sort numerically to avoid lexicographic mis-order (e.g., 10 before 2).Current
.sort()is string-based. Use a numeric comparator.Apply:
-export function generateSemanticNonNullDirective(levels: Set<number>): ConstDirectiveNode { - const sortedLevels = Array.from(levels).sort(); +export function generateSemanticNonNullDirective(levels: Set<number>): ConstDirectiveNode { + const sortedLevels = Array.from(levels).sort((a, b) => a - b);
251-257: Nit: function name/param don’t match accepted types.
getSingleHashSetEntrytakes a Set or Map; consider renaming togetFirstEntry(andhashSet->collection) for clarity. No behavior change.composition/src/errors/errors.ts (1)
1646-1657: Fix missing space and ensure numeric sort in message.There’s no space before “is invalid…”, and
.sort()is lexicographic.Apply:
-export function semanticNonNullInconsistentLevelsError(data: FieldData): Error { +export function semanticNonNullInconsistentLevelsError(data: FieldData): Error { const coords = `${data.renamedParentTypeName}.${data.name}`; let message = - `The "@semanticNonNull" directive defined on field "${coords}"` + + `The "@semanticNonNull" directive defined on field "${coords}" ` + `is invalid due to inconsistent values provided to the "levels" argument across the following subgraphs:\n`; for (const [subgraphName, levels] of data.nullLevelsBySubgraphName) { - message += ` Subgraph "${subgraphName}" defines levels ${Array.from(levels).sort()}.\n`; + message += ` Subgraph "${subgraphName}" defines levels ${Array.from(levels).sort((a, b) => a - b)}.\n`; }composition/src/utils/string-constants.ts (1)
171-172: Include auth directives in client-exposed set or adjust comment elsewhere.
getClientPersistedDirectiveNodescomment says to include@authenticatedand@requiresScopes, butPERSISTED_CLIENT_DIRECTIVESomits them.Apply if exposure is intended:
-export const PERSISTED_CLIENT_DIRECTIVES = new Set<string>([DEPRECATED, SEMANTIC_NON_NULL]); +export const PERSISTED_CLIENT_DIRECTIVES = new Set<string>([ + AUTHENTICATED, + DEPRECATED, + REQUIRES_SCOPES, + SEMANTIC_NON_NULL, +]);If not intended, update the comment in
getClientPersistedDirectiveNodes.composition/src/schema-building/utils.ts (1)
458-465: Comment/behavior mismatch about client-visible directives.Comment says include
@authenticated/@requiresScopes, but the filter set currently excludes them. Align with constants or update the comment.See suggested change in string-constants.ts.
composition/tests/v1/directives/semantic-non-null.test.ts (1)
15-15: Prefer project’s parse wrapper for consistent noLocation handling.Use the internal
parseutil to align with other tests and avoid location noise:import { parse } from '../../../src';.composition/src/v1/normalization/normalization-factory.ts (3)
692-699: Call the semanticNonNull handler once per directive, not once per argument.Currently invoked inside the arguments loop, which re-calls the handler if future optional args are added. Move it after argument validation.
- if (isSemanticNonNull && data.kind === Kind.FIELD_DEFINITION) { - this.handleSemanticNonNullDirective({ - data, - directiveNode, - errorMessages, - }); - continue; - } + // no-op here; handled once after the loop for @semanticNonNullfor (const argumentNode of directiveNode.arguments) { ... } + if (isSemanticNonNull && isField) { + this.handleSemanticNonNullDirective({ + data, + directiveNode, + errorMessages, + }); + }
2175-2228: Don’t assume argument order; pick “levels” by name.Indexing
arguments![0]is brittle if directive arguments expand or reorder. Select by name to harden parsing.- const values = (directiveNode.arguments![0].value as ConstListValueNode).values as ReadonlyArray<IntValueNode>; + const levelsArg = directiveNode.arguments?.find((a) => a.name.value === 'levels'); + if (!levelsArg || levelsArg.value.kind !== Kind.LIST) { + // types are validated earlier; if we get here, bail gracefully + return; + } + const values = (levelsArg.value as ConstListValueNode).values as ReadonlyArray<IntValueNode>;
162-165: Unreachable NaN branch is harmless but dead.
semanticNonNullLevelsNaNIndexErrorMessagecan’t trigger because the directive arg is validated asInt. You may keep it for defense-in-depth or remove later.composition/src/v1/federation/federation-factory.ts (1)
1731-1757: Directive emission path looks solid; minor robustness note on level sorting.
generateSemanticNonNullDirectivesorts levels lexicographically. If levels ever reach double digits, prefer numeric sort.To apply (in composition/src/v1/utils/utils.ts):
// change const sortedLevels = Array.from(levels).sort(); // to const sortedLevels = Array.from(levels).sort((a, b) => a - b);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
composition/src/errors/errors.ts(2 hunks)composition/src/errors/types.ts(1 hunks)composition/src/schema-building/types.ts(1 hunks)composition/src/schema-building/utils.ts(5 hunks)composition/src/types/types.ts(1 hunks)composition/src/utils/string-constants.ts(7 hunks)composition/src/utils/utils.ts(3 hunks)composition/src/v1/federation/federation-factory.ts(19 hunks)composition/src/v1/normalization/directive-definition-data.ts(4 hunks)composition/src/v1/normalization/normalization-factory.ts(11 hunks)composition/src/v1/normalization/types.ts(3 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/src/v1/utils/constants.ts(4 hunks)composition/src/v1/utils/utils.ts(2 hunks)composition/tests/v1/directives/semantic-non-null.test.ts(1 hunks)composition/tests/v1/utils/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
composition/src/v1/normalization/types.ts
🧬 Code graph analysis (13)
composition/src/v1/normalization/types.ts (2)
composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/schema-building/types.ts (1)
FieldData(97-118)
composition/src/schema-building/types.ts (1)
composition/src/types/types.ts (1)
SubgraphName(9-9)
composition/tests/v1/directives/semantic-non-null.test.ts (7)
composition/tests/utils/utils.ts (5)
normalizeSubgraphFailure(31-38)federateSubgraphsSuccess(59-72)schemaToSortedNormalizedString(27-29)normalizeString(19-21)federateSubgraphsFailure(49-57)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/errors/errors.ts (4)
invalidDirectiveError(233-244)semanticNonNullLevelsNonNullErrorMessage(1638-1643)semanticNonNullLevelsIndexOutOfBoundsErrorMessage(1630-1636)semanticNonNullInconsistentLevelsError(1645-1657)composition/src/utils/string-constants.ts (3)
SEMANTIC_NON_NULL(131-131)ID_SCALAR(62-62)QUERY(114-114)composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/schema-building/types.ts (1)
FieldData(97-118)composition/src/ast/utils.ts (1)
parse(272-274)
composition/src/errors/errors.ts (2)
composition/src/errors/types.ts (2)
SemanticNonNullLevelsIndexOutOfBoundsErrorParams(21-25)SemanticNonNullLevelsNonNullErrorParams(27-30)composition/src/schema-building/types.ts (1)
FieldData(97-118)
composition/src/schema-building/utils.ts (3)
composition/src/utils/string-constants.ts (3)
NON_REPEATABLE_PERSISTED_DIRECTIVES(189-189)SEMANTIC_NON_NULL(131-131)PERSISTED_CLIENT_DIRECTIVES(171-171)composition/src/utils/utils.ts (2)
generateSemanticNonNullDirective(196-219)getSingleHashSetEntry(251-257)composition/src/schema-building/types.ts (4)
ChildData(249-249)NodeData(265-265)SchemaData(220-226)FieldData(97-118)
composition/src/v1/utils/constants.ts (3)
composition/src/utils/string-constants.ts (4)
SEMANTIC_NON_NULL(131-131)LEVELS(80-80)INT_SCALAR(74-74)FIELD_DEFINITION_UPPER(54-54)composition/src/schema-building/ast.ts (1)
MutableDirectiveDefinitionNode(22-29)composition/src/ast/utils.ts (2)
stringToNameNode(77-82)stringToNamedTypeNode(100-105)
composition/src/v1/normalization/directive-definition-data.ts (4)
composition/src/schema-building/types.ts (2)
DirectiveDefinitionData(41-50)ArgumentData(30-34)composition/src/utils/string-constants.ts (4)
LEVELS(80-80)INT_SCALAR(74-74)FIELD_DEFINITION_UPPER(54-54)SEMANTIC_NON_NULL(131-131)composition/src/ast/utils.ts (1)
stringToNamedTypeNode(100-105)composition/src/v1/utils/constants.ts (1)
SEMANTIC_NON_NULL_DEFINITION(711-742)
composition/src/v1/normalization/utils.ts (2)
composition/src/utils/string-constants.ts (1)
SEMANTIC_NON_NULL(131-131)composition/src/v1/normalization/directive-definition-data.ts (1)
SEMANTIC_NON_NULL_DATA(522-556)
composition/src/utils/string-constants.ts (1)
composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/src/v1/utils/utils.ts (1)
composition/src/schema-building/types.ts (1)
NodeData(265-265)
composition/src/v1/normalization/normalization-factory.ts (6)
composition/src/schema-building/utils.ts (2)
isFieldData(781-783)isTypeRequired(124-126)composition/src/utils/string-constants.ts (1)
SEMANTIC_NON_NULL(131-131)composition/src/errors/errors.ts (3)
semanticNonNullLevelsNonNullErrorMessage(1638-1643)semanticNonNullLevelsNaNIndexErrorMessage(1626-1628)semanticNonNullLevelsIndexOutOfBoundsErrorMessage(1630-1636)composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/v1/normalization/types.ts (1)
HandleSemanticNonNullDirectiveParams(64-68)composition/src/schema-building/ast.ts (1)
MutableTypeNode(205-205)
composition/src/utils/utils.ts (2)
composition/src/ast/utils.ts (1)
stringToNameNode(77-82)composition/src/utils/string-constants.ts (2)
SEMANTIC_NON_NULL(131-131)LEVELS(80-80)
composition/src/v1/federation/federation-factory.ts (7)
composition/src/types/types.ts (2)
TypeName(11-11)DirectiveName(3-3)composition/src/v1/utils/constants.ts (1)
SEMANTIC_NON_NULL_DEFINITION(711-742)composition/src/schema-building/types.ts (2)
NodeData(265-265)FieldData(97-118)composition/src/utils/utils.ts (2)
getSingleHashSetEntry(251-257)generateSemanticNonNullDirective(196-219)composition/src/v1/utils/utils.ts (1)
getNodeCoords(447-460)composition/src/schema-building/utils.ts (1)
isFieldData(781-783)composition/src/errors/errors.ts (1)
semanticNonNullInconsistentLevelsError(1645-1657)
⏰ 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). (15)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
composition/src/types/types.ts (1)
3-4: LGTM: DirectiveName alias is fine.Simple widening of the public type surface; no runtime impact.
composition/src/errors/types.ts (1)
21-31: LGTM: new error param types.Shapes align with intended error helpers; no conflicts observed.
composition/src/v1/normalization/directive-definition-data.ts (1)
28-28: LGTM: @semanticNonNull definition wired.
- levels: [Int!]! with default [0] and optional arg semantics looks correct.
- Non‑repeatable and FIELD_DEFINITION location match intent.
Please confirm normalization de‑duplicates/sorts provided levels and validates bounds against the field type depth.
Also applies to: 70-70, 75-75, 93-93, 522-557
composition/src/v1/normalization/types.ts (1)
12-12: LGTM: stronger typing and new handler params.Using SubgraphName for targetSubgraphName and the new HandleSemanticNonNullDirectiveParams look good.
Also applies to: 55-56, 64-68
composition/src/v1/normalization/utils.ts (1)
47-47: LGTM: registration in initializeDirectiveDefinitionDatas.Directive is now discoverable by normalization.
Also applies to: 80-81, 414-415
composition/src/v1/utils/constants.ts (1)
64-64: Decide whether SEMANTIC_NON_NULL should be added to the base directive definitions.
- SEMANTIC_NON_NULL_DEFINITION is present and SEMANTIC_NON_NULL was added to ALL_IN_BUILT_DIRECTIVE_NAMES and PERSISTED_CLIENT_DIRECTIVES, but it is NOT added to BASE_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME or BASE_DIRECTIVE_DEFINITIONS (composition/src/v1/utils/constants.ts).
- Effect: the normalization/emit path treats the name as a built‑in (so custom declarations are skipped) but, since it isn’t in the BASE_* maps, its directive definition will not be pre‑registered nor included in the persisted/directive‑definition emission.
- Action: either add SEMANTIC_NON_NULL to BASE_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME (and BASE_DIRECTIVE_DEFINITIONS) if the definition must be emitted, or confirm this omission is intentional (v1‑only / no emitted definition).
composition/src/utils/string-constants.ts (1)
189-190: Good call on non-repeatable persisted directives.Deduping
@inaccessibleand@semanticNonNullearly avoids noisy repeats.composition/src/schema-building/utils.ts (3)
337-339: Early dedupe for non-repeatable persisted directives looks good.Prevents duplicate accumulation before validation.
450-457: Client emission uses a single subgraph’s levels; ensure consistency upstream.
getSingleHashSetEntry(nullLevelsBySubgraphName)picks one subgraph arbitrarily. This is fine only if normalization guarantees equal sets across subgraphs (and errors otherwise).If not already enforced, wire
semanticNonNullInconsistentLevelsErrorinto normalization before this path.
781-783: Type guard widening LGTM.Expanding
isFieldDatato includeSchemaDatasimplifies call sites.composition/tests/v1/utils/utils.ts (1)
106-112: Fix invalid SDL: remove default from non-nulllevelsor make it nullableFile: composition/tests/v1/utils/utils.ts Lines: 106-112 —
levels: [Int!]! = [0]is invalid SDL (defaults are not allowed on non-null types) and will fail parsing.Option A (keep non-null, drop default):
-export const semanticNonNullDefinition = `directive @semanticNonNull(levels: [Int!]! = [0]) on FIELD_DEFINITION`; +export const semanticNonNullDefinition = `directive @semanticNonNull(levels: [Int!]!) on FIELD_DEFINITION`;Option B (allow default, make list nullable):
-export const semanticNonNullDefinition = `directive @semanticNonNull(levels: [Int!]! = [0]) on FIELD_DEFINITION`; +export const semanticNonNullDefinition = `directive @semanticNonNull(levels: [Int!] = [0]) on FIELD_DEFINITION`;If a default is required choose B; otherwise choose A to keep the argument required.
⛔ Skipped due to learnings
Learnt from: Noroth PR: wundergraph/cosmo#2108 File: protographic/tests/sdl-validation/01-basic-validation.test.ts:20-57 Timestamp: 2025-08-05T13:00:30.288Z Learning: In the protographic SDL validation system, the validation logic for nullable list items only refers to the actual list type and not the nesting. For example, `[[String!]]` (nullable list of non-nullable lists) would not trigger a nullable list warning because the validation focuses on the immediate list structure rather than nested list structures.composition/tests/v1/directives/semantic-non-null.test.ts (1)
29-343: Excellent test coverage for normalization and federation paths.Covers key happy/error paths, persistence to router/client schemas, and cross‑subgraph consistency. LGTM.
composition/src/v1/normalization/normalization-factory.ts (1)
640-652: Good defaulting for @semanticNonNull with no arguments.Correctly treats default levels as
[0]and guards non‑null root types.composition/src/v1/federation/federation-factory.ts (3)
1006-1010: Correctly flags presence of @semanticNonNull for definition emission.Setting
referencedPersistedDirectiveNamesensures the directive definition is emitted only when used. Nice.
1851-1870: Cross‑subgraph levels consistency check: LGTM.Set equality check is correct and early‑exits on mismatch.
2192-2203: Conditional directive definition inclusion: LGTM.Emits
SEMANTIC_NON_NULL_DEFINITIONonly when referenced. Matches tests’ expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
composition/src/v1/federation/federation-factory.ts (4)
702-712: Enforce single-subgraph invariant before using getSingleHashSetEntryComment says there should be a single entry, but we don’t assert it. Guard to prevent ambiguous state.
Apply this diff:
- const subgraphName = getSingleHashSetEntry(incomingData.subgraphNames); - if (subgraphName === undefined) { + if (incomingData.subgraphNames.size !== 1) { this.errors.push(unknownFieldSubgraphNameError(incomingData.federatedCoords)); return; } + const subgraphName = getSingleHashSetEntry(incomingData.subgraphNames)!;
1735-1739: Levels sort is lexicographic in generator; ensure numeric ordering
generateSemanticNonNullDirectivesorts with default.sort(), which is lexicographic for numbers (e.g.,[0,10,2]). Emit numerically sorted levels for stable output.Update the helper in composition/src/utils/utils.ts:
export function generateSemanticNonNullDirective(levels: Set<number>): ConstDirectiveNode { - const sortedLevels = Array.from(levels).sort(); + const sortedLevels = Array.from(levels).sort((a, b) => a - b); const values = new Array<ConstValueNode>(); for (const level of sortedLevels) {
1851-1870: Nit: short‑circuit validation if directive absentTiny win: skip the loop when the field doesn’t carry
@semanticNonNull.Apply this diff:
- validateSemanticNonNull(data: FieldData) { + validateSemanticNonNull(data: FieldData) { + if (!data.directivesByDirectiveName.has(SEMANTIC_NON_NULL)) { + return; + } let comparison: Set<number> | undefined; for (const levels of data.nullLevelsBySubgraphName.values()) {
2193-2211: Be careful overwriting routerDefinitions; prefer additive updateReassigning
this.routerDefinitionscan be brittle if call order changes later. Consider prepending missing directive definitions instead of replacing the whole array.Example approach (conceptual):
- Build a Set of required directive names based on
isVersionTwoandreferencedPersistedDirectiveNames.- Prepend any missing definitions while keeping existing entries intact and deduplicated by
name.value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
composition/src/v1/federation/federation-factory.ts(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
composition/src/v1/federation/federation-factory.ts (8)
composition/src/types/types.ts (2)
TypeName(11-11)DirectiveName(3-3)composition/src/utils/string-constants.ts (1)
SEMANTIC_NON_NULL(131-131)composition/src/v1/utils/constants.ts (1)
SEMANTIC_NON_NULL_DEFINITION(711-742)composition/src/schema-building/types.ts (3)
PersistedDirectiveDefinitionData(192-199)NodeData(265-265)FieldData(97-118)composition/src/utils/utils.ts (3)
getSingleHashSetEntry(251-257)addMapEntries(245-249)generateSemanticNonNullDirective(196-219)composition/src/v1/utils/utils.ts (1)
getNodeCoords(447-462)composition/src/schema-building/utils.ts (1)
isFieldData(781-783)composition/src/errors/errors.ts (1)
semanticNonNullInconsistentLevelsError(1648-1660)
⏰ 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). (15)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
composition/src/v1/federation/federation-factory.ts (5)
889-889: Confirm merge semantics for nullLevelsBySubgraphName
addMapEntriesoverwrites existing keys with incomingSet<number>references. If the same subgraph key could appear across merges, you probably want a union (for level of incoming => target.add(level)), not replacement. If keys are guaranteed unique per field, please confirm.
1006-1009: Good: tracking directive usage for definition emissionAdding
SEMANTIC_NON_NULLtoreferencedPersistedDirectiveNameson copy ensures the definition is emitted only when needed.
1731-1741: Avoid silently defaulting to levels [0] when none recordedIf a field has
@semanticNonNullbutnullLevelsBySubgraphNameis empty, defaulting to[0]could mask a normalization gap. Prefer explicit validation error or skip emission.Apply this localized tweak if you choose to skip emission instead of defaulting:
- persistedDirectiveNodes.push( - generateSemanticNonNullDirective( - getSingleHashSetEntry(data.nullLevelsBySubgraphName) ?? new Set<number>([0]), - ), - ); + const levels = getSingleHashSetEntry(data.nullLevelsBySubgraphName); + if (levels) { + persistedDirectiveNodes.push(generateSemanticNonNullDirective(levels)); + } else { + // levels missing: rely on validation to report or skip emission + continue; + }
2012-2014: Good: validate before directive emissionValidating cross‑subgraph level consistency prior to building field nodes prevents emitting contradictory directives.
270-276: No change required — persistedDirectiveDefinitions is unusedrg output shows persistedDirectiveDefinitions only at its initialization in composition/src/v1/federation/federation-factory.ts (line 273). persistedDirectiveDefinitionByDirectiveName already contains SEMANTIC_NON_NULL and referencedPersistedDirectiveNames is used to drive inclusion of @semanticNonNull, so adding SEMANTIC_NON_NULL to persistedDirectiveDefinitions is unnecessary.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composition/src/v1/federation/federation-factory.ts (1)
814-819: Ensure @semanticNonNull definition gets emitted when added by later subgraphs
referencedPersistedDirectiveNamesis only updated incopyFieldData. If the first seen instance of a field lacks@semanticNonNullbut a later subgraph adds it, the directive definition may be missing. Also update the flag here.Apply this diff:
extractPersistedDirectives( targetData.persistedDirectivesData, incomingData.directivesByDirectiveName, this.persistedDirectiveDefinitionByDirectiveName, ); + if (incomingData.directivesByDirectiveName.has(SEMANTIC_NON_NULL)) { + this.referencedPersistedDirectiveNames.add(SEMANTIC_NON_NULL); + }
♻️ Duplicate comments (2)
composition/src/v1/federation/federation-factory.ts (2)
1034-1034: Fix shared reference: deep‑copy nullLevelsBySubgraphName to avoid cross‑mutationCurrent code assigns the map by reference, causing mutations to leak between copies.
Apply this diff:
- nullLevelsBySubgraphName: sourceData.nullLevelsBySubgraphName, + nullLevelsBySubgraphName: new Map( + Array.from(sourceData.nullLevelsBySubgraphName, ([subgraph, levels]) => [subgraph, new Set(levels)]), + ),
888-889: Deep‑merge null level sets instead of assigning Set references
addMapEntriescopies Set references verbatim. Use a safe merge to prevent future mutations in one instance affecting others.Apply this diff:
- addMapEntries(incomingData.nullLevelsBySubgraphName, targetData.nullLevelsBySubgraphName); + for (const [subgraphName, levels] of incomingData.nullLevelsBySubgraphName) { + const targetLevels = getValueOrDefault( + targetData.nullLevelsBySubgraphName, + subgraphName, + () => new Set<number>(), + ); + addIterableValuesToSet(levels, targetLevels); + }
🧹 Nitpick comments (2)
composition/src/v1/normalization/normalization-factory.ts (1)
2178-2236: Index derivation and validation look correct; minor nits.
- Use radix for parseInt to avoid implicit base.
- The fallback error "Argument "levels" validation error." is likely unreachable due to earlier type validation; consider removing or switching to the standard invalidArgumentValueErrorMessage for consistency.
Apply this diff:
- for (const { value } of values) { - const int = parseInt(value); + for (const { value } of values) { + const int = parseInt(value, 10);Optional:
- if (!levelsArg || levelsArg.value.kind !== Kind.LIST) { - errorMessages.push(`Argument "${LEVELS}" validation error.`); - return; - } + if (!levelsArg || levelsArg.value.kind !== Kind.LIST) { + // Type validation should have already failed earlier; defensively bail. + return; + }composition/src/v1/federation/federation-factory.ts (1)
1849-1868: Use a snapshot of the comparison set to guard against accidental mutationsTaking a copy avoids edge cases if the original Set is mutated elsewhere.
- validateSemanticNonNull(data: FieldData) { - let comparison: Set<number> | undefined; + validateSemanticNonNull(data: FieldData) { + let comparison: Set<number> | undefined; for (const levels of data.nullLevelsBySubgraphName.values()) { if (!comparison) { - comparison = levels; + comparison = new Set(levels); continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
composition/src/errors/errors.ts(2 hunks)composition/src/schema-building/utils.ts(5 hunks)composition/src/utils/utils.ts(3 hunks)composition/src/v1/federation/federation-factory.ts(19 hunks)composition/src/v1/normalization/normalization-factory.ts(12 hunks)composition/tests/v1/directives/semantic-non-null.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- composition/src/utils/utils.ts
- composition/src/errors/errors.ts
- composition/tests/v1/directives/semantic-non-null.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
composition/src/v1/federation/federation-factory.ts (8)
composition/src/types/types.ts (2)
TypeName(11-11)DirectiveName(3-3)composition/src/utils/string-constants.ts (1)
SEMANTIC_NON_NULL(131-131)composition/src/v1/utils/constants.ts (1)
SEMANTIC_NON_NULL_DEFINITION(711-742)composition/src/schema-building/types.ts (3)
PersistedDirectiveDefinitionData(192-199)NodeData(265-265)FieldData(97-118)composition/src/utils/utils.ts (3)
getFirstEntry(251-257)addMapEntries(245-249)generateSemanticNonNullDirective(196-219)composition/src/v1/utils/utils.ts (1)
getNodeCoords(447-462)composition/src/schema-building/utils.ts (1)
isFieldData(779-781)composition/src/errors/errors.ts (1)
semanticNonNullInconsistentLevelsError(1648-1660)
composition/src/schema-building/utils.ts (3)
composition/src/utils/string-constants.ts (3)
NON_REPEATABLE_PERSISTED_DIRECTIVES(189-189)SEMANTIC_NON_NULL(131-131)PERSISTED_CLIENT_DIRECTIVES(171-171)composition/src/utils/utils.ts (2)
generateSemanticNonNullDirective(196-219)getFirstEntry(251-257)composition/src/schema-building/types.ts (4)
ChildData(249-249)NodeData(265-265)SchemaData(220-226)FieldData(97-118)
composition/src/v1/normalization/normalization-factory.ts (6)
composition/src/schema-building/utils.ts (2)
isFieldData(779-781)isTypeRequired(124-126)composition/src/utils/string-constants.ts (2)
SEMANTIC_NON_NULL(131-131)LEVELS(80-80)composition/src/errors/errors.ts (3)
semanticNonNullLevelsNonNullErrorMessage(1641-1646)semanticNonNullLevelsNaNIndexErrorMessage(1626-1628)semanticNonNullLevelsIndexOutOfBoundsErrorMessage(1630-1639)composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/v1/normalization/types.ts (1)
HandleSemanticNonNullDirectiveParams(64-68)composition/src/schema-building/ast.ts (1)
MutableTypeNode(205-205)
⏰ 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). (14)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
composition/src/schema-building/utils.ts (3)
36-36: Imports align with broadened type guard and new directive utilities.Looks good and consistent with downstream usage.
Also applies to: 61-61, 68-68, 74-80
337-339: De-duping non-repeatable persisted directives: confirm intended validation behavior.By skipping additional instances for names in NON_REPEATABLE_PERSISTED_DIRECTIVES, duplicate occurrences won’t reach getValidFlattenedPersistedDirectiveNodeArray’s non‑repeatable check. If upstream validation already flags duplicates at extract/validate time, this is fine; otherwise you might mask errors. Please confirm the pipeline guarantees duplicates are still surfaced to users.
779-781: Broadened isFieldData signature is correct.Accepting SchemaData avoids narrowing issues at call sites; type guard remains precise.
composition/src/v1/normalization/normalization-factory.ts (7)
13-13: Import of IntValueNode is appropriate for levels parsing.
161-164: New semanticNonNull error helpers wired correctly.
233-234: New imports (isFieldData, MutableTypeNode, SubgraphName) align with added logic.Also applies to: 256-257, 371-371
301-301: LEVELS and SEMANTIC_NON_NULL constants imported as expected.Also applies to: 329-329
629-633: Directive predicates are cleanly factored.Using isFieldData avoids accidental schema/location handling.
641-652: Default no-arg handling for @semanticNonNull is sound.
- Disallows level 0 when the field is already non‑null.
- Defaults to [0] for nullable fields.
683-701: Inline handling dispatch for @semanticNonNull is correct.composition/src/v1/federation/federation-factory.ts (4)
1730-1739: LGTM: generated @semanticNonNull directive emissionSelecting the first levels set after validation is correct;
generateSemanticNonNullDirectivesorts values.
2010-2011: LGTM: validate before field node emissionValidating consistency prior to emitting nodes is the right place.
262-262: LGTM: stronger typing for namedInputValueTypeNamesSwitch to
Set<TypeName>is appropriate and aligns with public types.
270-271: LGTM: register SEMANTIC_NON_NULL definitionAdding it to
persistedDirectiveDefinitionByDirectiveNameenables extraction/persistence.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
composition/src/errors/errors.ts (1)
1631-1640: Out-of-bounds message reads well for non-list typesGood handling of the maxIndex === 0 case with “the only valid index is 0.” This addresses the earlier concern about “0–0 inclusive.”
composition/src/v1/federation/federation-factory.ts (2)
1037-1038: Fix shared reference: deep‑copy nullLevelsBySubgraphName to avoid cross‑mutationnullLevelsBySubgraphName is assigned by reference; copies can mutate each other and corrupt results.
Apply this diff:
- nullLevelsBySubgraphName: sourceData.nullLevelsBySubgraphName, + nullLevelsBySubgraphName: new Map( + Array.from( + sourceData.nullLevelsBySubgraphName, + ([subgraph, levels]) => [subgraph, new Set<number>(levels)], + ), + ),#!/bin/bash # Find any remaining by-reference copies of nullLevelsBySubgraphName rg -nP 'nullLevelsBySubgraphName\s*:\s*sourceData\.nullLevelsBySubgraphName' -C2 # Find any merges that assign Sets by reference for this map rg -nP 'addMapEntries\s*\(\s*[^,]*nullLevelsBySubgraphName' -C2
891-892: Do not use addMapEntries for Map<SubgraphName, Set>; union into copiesaddMapEntries assigns Set objects by reference. This can couple targetData to incomingData and allow accidental cross‑mutation later. Union the values into per‑key copies instead.
Apply this diff:
- addMapEntries(incomingData.nullLevelsBySubgraphName, targetData.nullLevelsBySubgraphName); + for (const [subgraphName, levels] of incomingData.nullLevelsBySubgraphName) { + const targetLevels = getValueOrDefault( + targetData.nullLevelsBySubgraphName, + subgraphName, + () => new Set<number>(), + ); + addIterableValuesToSet(levels, targetLevels); + }
🧹 Nitpick comments (6)
composition/src/errors/errors.ts (1)
1651-1663: Stabilize and clarify per-subgraph levels listingFor deterministic error messages and better readability, sort subgraph names and render levels with brackets.
Apply this diff:
- for (const [subgraphName, levels] of data.nullLevelsBySubgraphName) { - message += ` Subgraph "${subgraphName}" defines levels ${Array.from(levels).sort((a, b) => a - b)}.\n`; - } + const subgraphs = [...data.nullLevelsBySubgraphName.keys()].sort(); + for (const subgraphName of subgraphs) { + const levels = data.nullLevelsBySubgraphName.get(subgraphName)!; + const sorted = [...levels].sort((a, b) => a - b); + message += ` Subgraph "${subgraphName}" defines levels [${sorted.join(', ')}].\n`; + }composition/src/v1/normalization/normalization-factory.ts (2)
696-703: Invoke handler only once per directive to avoid duplicate workCurrently, the handler is called for each argument node of the directive. If the directive ever gains additional arguments, this can cause duplicate validation and repeated errors.
Apply this diff to gate execution on the levels argument name:
- if (isSemanticNonNull && isField) { - this.handleSemanticNonNullDirective({ - data, - directiveNode, - errorMessages, - }); - continue; - } + if (isSemanticNonNull && isField && argumentName === LEVELS) { + this.handleSemanticNonNullDirective({ + data, + directiveNode, + errorMessages, + }); + continue; + }
2179-2238: Use a single source of truth for the field typeYou traverse data.node.type but print data.type. If these ever diverge, the message could reference a different type than the one examined. Use data.type for both.
Apply this diff:
- const nonNullIndices = new Set<number>(); - let currentType: MutableTypeNode | null = data.node.type; + const nonNullIndices = new Set<number>(); + let currentType: MutableTypeNode | null = data.type; @@ - const values = levelsArg.value.values as ReadonlyArray<IntValueNode>; - const typeString = printTypeNode(data.type); + const values = levelsArg.value.values as ReadonlyArray<IntValueNode>; + const typeString = printTypeNode(data.type);composition/src/v1/federation/federation-factory.ts (3)
1733-1758: Router emission: generate canonical @semanticNonNull onceApproach looks good. Minor safety: guard against empty/null maps explicitly to document intent.
Apply this small tweak (no behavior change):
- if (directiveName === SEMANTIC_NON_NULL && isFieldData(data)) { - persistedDirectiveNodes.push( - generateSemanticNonNullDirective(getFirstEntry(data.nullLevelsBySubgraphName) ?? new Set<number>([0])), - ); + if (directiveName === SEMANTIC_NON_NULL && isFieldData(data)) { + const levels = getFirstEntry(data.nullLevelsBySubgraphName); + persistedDirectiveNodes.push( + generateSemanticNonNullDirective(levels ?? new Set<number>([0])), + ); continue; }
1852-1871: Validate only when directive is present; keep loop as-isCurrent logic works; add a cheap guard to avoid doing work on fields without @semanticNonNull (and to future‑proof if the map were ever populated incorrectly).
Apply this diff:
- validateSemanticNonNull(data: FieldData) { + validateSemanticNonNull(data: FieldData) { + if (!data.directivesByDirectiveName.has(SEMANTIC_NON_NULL)) return; let comparison: Set<number> | undefined; for (const levels of data.nullLevelsBySubgraphName.values()) {Also applies to: 2013-2014
2195-2214: LGTM: usage‑gated definition emission for v1/v2Emission logic and routerDefinitions resets are consistent. Consider a tiny dedup helper to DRY the SEMANTIC_NON_NULL_DEFINITION push across branches later.
Also applies to: 2217-2225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composition/src/errors/errors.ts(3 hunks)composition/src/v1/federation/federation-factory.ts(20 hunks)composition/src/v1/normalization/normalization-factory.ts(12 hunks)composition/tests/v1/directives/semantic-non-null.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composition/tests/v1/directives/semantic-non-null.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
composition/src/v1/federation/federation-factory.ts
🧬 Code graph analysis (3)
composition/src/errors/errors.ts (3)
composition/src/errors/types.ts (2)
SemanticNonNullLevelsIndexOutOfBoundsErrorParams(21-25)SemanticNonNullLevelsNonNullErrorParams(27-30)composition/src/utils/string-constants.ts (1)
LEVELS(80-80)composition/src/schema-building/types.ts (1)
FieldData(97-118)
composition/src/v1/federation/federation-factory.ts (8)
composition/src/types/types.ts (2)
TypeName(11-11)DirectiveName(3-3)composition/src/utils/string-constants.ts (1)
SEMANTIC_NON_NULL(131-131)composition/src/v1/utils/constants.ts (1)
SEMANTIC_NON_NULL_DEFINITION(711-742)composition/src/schema-building/types.ts (3)
PersistedDirectiveDefinitionData(192-199)NodeData(265-265)FieldData(97-118)composition/src/utils/utils.ts (3)
getFirstEntry(251-257)addMapEntries(245-249)generateSemanticNonNullDirective(196-219)composition/src/v1/utils/utils.ts (1)
getNodeCoords(447-462)composition/src/schema-building/utils.ts (1)
isFieldData(779-781)composition/src/errors/errors.ts (1)
semanticNonNullInconsistentLevelsError(1651-1663)
composition/src/v1/normalization/normalization-factory.ts (6)
composition/src/schema-building/utils.ts (2)
isFieldData(779-781)isTypeRequired(124-126)composition/src/utils/string-constants.ts (4)
OVERRIDE(106-106)REQUIRES_SCOPES(121-121)SEMANTIC_NON_NULL(131-131)LEVELS(80-80)composition/src/errors/errors.ts (4)
semanticNonNullLevelsNonNullErrorMessage(1642-1647)semanticNonNullArgumentErrorMessage(1649-1649)semanticNonNullLevelsNaNIndexErrorMessage(1627-1629)semanticNonNullLevelsIndexOutOfBoundsErrorMessage(1631-1640)composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/v1/normalization/types.ts (1)
HandleSemanticNonNullDirectiveParams(64-68)composition/src/schema-building/ast.ts (1)
MutableTypeNode(205-205)
⏰ 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). (15)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (8)
composition/src/v1/normalization/normalization-factory.ts (3)
642-653: Default semantics for missing arguments look correctDefaulting to [0] only when the field is nullable, and erroring when index 0 is already non-null, matches the directive’s intent.
1151-1155: FieldData gains nullLevelsBySubgraphNameGood place to store per-subgraph levels for later cross-subgraph consistency checks.
2179-2238: ```shell
#!/bin/bash
set -euo pipefailecho "== git status (brief) =="
git rev-parse --is-inside-work-tree >/dev/null 2>&1 && git rev-parse --abbrev-ref HEAD || echo "not a git repo"echo "== Searching repository for semanticNonNull occurrences =="
Broad search for directive name, handler name, related constants and error message identifiers
rg -n --hidden --no-ignore -S '@semanticNonNull|semanticNonNull|handleSemanticNonNullDirective|semanticNonNullArgumentErrorMessage|semanticNonNullLevelsNaNIndexErrorMessage|semanticNonNullLevelsIndexOutOfBoundsErrorMessage|semanticNonNullLevelsNonNullErrorMessage|LEVELS' || true
FILE='composition/src/v1/normalization/normalization-factory.ts'
if [ -f "$FILE" ]; then
echo "== Showing $FILE (lines 2160-2250) =="
sed -n '2160,2250p' "$FILE"
else
echo "File $FILE not found"
fiecho "== Searching composition/tests for semanticNonNull occurrences =="
rg -n --hidden --no-ignore -S 'semanticNonNull|@semanticNonNull' composition/tests || trueecho "== Summary counts (by file) =="
rg -n --hidden --no-ignore -S 'semanticNonNull|@semanticNonNull' -c || true</blockquote></details> <details> <summary>composition/src/v1/federation/federation-factory.ts (5)</summary><blockquote> `56-74`: **LGTM: wiring and type-surface for @semanticNonNull** Imports, constants, and added types (DirectiveName/TypeName) are correctly wired for the new directive. Also applies to: 89-101, 117-119, 153-170, 209-216, 218-231, 239-240 --- `702-713`: **LGTM: stable subgraph source for subscriptionFilter** Using getFirstEntry(Set) with an explicit undefined guard keeps behavior deterministic and error‑aware. --- `806-809`: **LGTM: usage‑based gating for directive definitions** Recording SEMANTIC_NON_NULL in referencedPersistedDirectiveNames in both upsert and copy paths ensures correct emission in v1/v2. Also applies to: 1009-1012 --- `1024-1024`: **LGTM: stricter typing for inheritedDirectiveNames** Switch to Set<DirectiveName> improves clarity and future refactors. --- `1765-1766`: **LGTM: single source for persisted directive assembly** Delegating to getValidFlattenedPersistedDirectiveNodeArray keeps router/client paths symmetrical and maintainable (router side here). </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist