chore: selectively inject directive defs/deps #2307
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughReorganized v1 constants into dedicated modules, consolidated directive-definition shapes to DirectiveDefinitionNode, widened several type signatures to use TypeName/FieldName, introduced centralized persisted-directive extraction, adjusted normalization/federation flows, and updated many tests to use new helpers and exported SDL fragments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Areas needing extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
composition/tests/v1/directives/semantic-non-null.test.ts (1)
409-419: Fix client-schema expectation to use semanticNonNullDefinition (consistent with earlier tests).Earlier cases use
semanticNonNullDefinitionfor client schema; this block usesSEMANTIC_NON_NULL_DIRECTIVE. Align to prevent assertion failures.Apply:
- SCHEMA_QUERY_DEFINITION + - SEMANTIC_NON_NULL_DIRECTIVE + + SCHEMA_QUERY_DEFINITION + + semanticNonNullDefinition + ` type Query { a: [[[ID]]] @semanticNonNull(levels: [0, 2]) } `,composition/tests/v1/types/arguments.test.ts (1)
122-126: Fix stray backticks in test titlesRemove the trailing backtick from these test names.
- test('that if arguments of the same name are not the same type, an error is returned`', () => { + test('that if arguments of the same name are not the same type, an error is returned', () => { @@ - test('that if arguments have different boolean default values, an error is returned`', () => { + test('that if arguments have different boolean default values, an error is returned', () => {Also applies to: 154-161
composition/src/v1/normalization/walkers.ts (1)
184-191: Bug: duplicateOperationTypeDefinitionError is droppedThe error is created but never pushed, so duplicates won’t be reported.
- if (definitionNode) { - duplicateOperationTypeDefinitionError( - operationType, - namedTypeName, - getTypeNodeNamedTypeName(definitionNode.type), - ); - return false; - } + if (definitionNode) { + nf.errors.push( + duplicateOperationTypeDefinitionError( + operationType, + namedTypeName, + getTypeNodeNamedTypeName(definitionNode.type), + ), + ); + return false; + }composition/tests/v1/directives/directives.test.ts (1)
99-125: Fix invalid GraphQL value in @z(list: …) input object.
{ name: String }is a type name used as a value; GraphQL input values require a string literal like"test".Apply:
@@ - type Query { - dummy: String! @z(list: {name: String}) - } + type Query { + dummy: String! @z(list: { name: "test" }) + } @@ const ne: Subgraph = { name: 'ne', url: '', definitions: parse(` directive @z(list: [[Input!]!]!) on FIELD_DEFINITION type Query { - dummy: String! @z(list: { name: String }) + dummy: String! @z(list: { name: "test" }) } input Input { name: String! } `), };Also applies to: 458-471
composition/tests/v1/types/inputs.test.ts (1)
395-406: Fix expected error field name in skipped test.The fixture defines “field: String! …”, but the assertion references “Input field "name"”. Correct the expectation to “field” to avoid future flakes when the test is enabled.
- incompatibleInputValueDefaultValueTypeError('Input field "name"', 'Input.name', 'String!', 'null'), + incompatibleInputValueDefaultValueTypeError('Input field "field"', 'Input.field', 'String!', 'null'),composition/tests/v1/types/objects.test.ts (1)
445-463: Duplicate scenario — both tests pass the same subgraph order.“#1.2” repeats the exact inputs as “#1.1”. If the intent is permutation coverage, swap the order in one test.
-const { federatedGraphSchema } = federateSubgraphsSuccess( - [subgraphI, subgraphL, subgraphM], - ROUTER_COMPATIBILITY_VERSION_ONE, -); +const { federatedGraphSchema } = federateSubgraphsSuccess( + [subgraphI, subgraphM, subgraphL], + ROUTER_COMPATIBILITY_VERSION_ONE, +);
🧹 Nitpick comments (31)
composition/src/v1/normalization/types.ts (1)
80-89: Include error details in ExecutionFailure for diagnosability.Expose minimal context (messages or code) on failures.
Apply:
-export type ExecutionFailure = { - success: false; -}; +export type ExecutionFailure = { + success: false; + errorMessages?: string[]; +};If you prefer stricter shape, make
errorMessages: string[]required.composition/src/schema-building/utils.ts (1)
301-310: Good: export upsertTagDirectives.Centralizes tag merging; keeps last-wins by name.
Consider a brief TSDoc noting last-write-wins semantics.
composition/tests/v1/types/interfaces.test.ts (1)
459-459: Duplicate test title (“#1.1”) — rename one for clarity.Both tests use the same name; prefer “#1.2” for the latter to keep titles unique.
- test('that an Interface type and extension definition federate successfully #1.1', () => { + test('that an Interface type and extension definition federate successfully #1.2', () => {Also applies to: 481-481
composition/tests/utils/utils.ts (2)
20-20: Simplify normalizeString; avoid redundant alternation and replaceAll.Use one whitespace pass; works for all whitespace including newlines.
-export function normalizeString(input: string): string { - return input.replaceAll(/\s+|\\n\s*/g, ' ').trim(); -} +export function normalizeString(input: string): string { + return input.replace(/\s+/g, ' ').trim(); +}
45-47: Gate debug logging to reduce noisy CI output.Only log when explicitly enabled.
- if (!result.success) { - console.dir(result.errors); - } + if (!result.success && process.env.DEBUG_COSMO_TESTS === '1') { + console.dir(result.errors, { depth: null }); + }composition/tests/v1/directives/one-of.test.ts (1)
105-118: Deduplicate repeated SDL prefixesMultiple expectations prepend the same SCHEMA_QUERY_DEFINITION + Query block. Extract a small helper (e.g., buildSDL(fragment: string)) to reduce duplication and future drift.
Also applies to: 119-132
composition/src/v1/normalization/walkers.ts (1)
554-560: Consistent traversal short‑circuit for ENTITY_UNIONDefinition returns (continues traversal) while Extension returns false (stops). Prefer returning false in both places for consistency.
UnionTypeDefinition: { enter(node) { if (node.name.value === ENTITY_UNION) { - return; + return false; } nf.upsertUnionByNode(node); }, },composition/tests/v1/directives/shareable.test.ts (1)
75-86: Use OPENFED_FIELD_SET constant for consistencyYou import OPENFED_FIELD_SET; leverage it instead of hardcoding the scalar.
- scalar openfed__FieldSet - `, + ` + + OPENFED_FIELD_SET,composition/tests/v1/directives/provides.test.ts (2)
21-21: Align parse import with other testsUse parse from '../../../src' for consistency with the rest of the test suite (and any custom parsing options).
-import { parse } from 'graphql'; +import { parse } from '../../../src';
942-943: Prefer toHaveLength for array assertionsUnify on expect(warnings).toHaveLength(0) for readability and consistency.
- expect(warnings.length).toBe(0); + expect(warnings).toHaveLength(0);Also applies to: 1132-1133
composition/tests/v1/directives/fieldset-directives.test.ts (2)
37-43: Use OPENFED_FIELD_SET constant instead of hard-coding 'scalar openfed__FieldSet'.Keeps assertions aligned with shared constants and reduces drift.
Apply:
import { EXTERNAL_DIRECTIVE, INACCESSIBLE_DIRECTIVE, KEY_DIRECTIVE, REQUIRES_DIRECTIVE, SCHEMA_QUERY_DEFINITION, + OPENFED_FIELD_SET, } from '../utils/utils'; … - scalar openfed__FieldSet + ` + OPENFED_FIELD_SETAlso applies to: 845-905
907-968: Unify Map key generic across the file (TypeName vs string).This block uses
Map<TypeName, ConfigurationData>while earlier assertions useMap<string, ConfigurationData>. Pick one (prefer TypeName if it’s the new public alias) and standardize for consistency.composition/tests/v1/contracts.test.ts (1)
3-8: Prefer explicit constants over versionOneRouterDefinitions.To align with “selective injection,” build expectations with
SCHEMA_QUERY_DEFINITIONplus only the directive constants actually needed (e.g.,TAG_DIRECTIVE,INACCESSIBLE_DIRECTIVE). This reduces coupling and keeps tests resilient to directive set changes.If you want, I can generate a follow-up diff to replace the aggregator in these spots with explicit constants matching each test’s needs.
Also applies to: 1078-1090, 1114-1126, 1149-1162
composition/tests/v1/normalization.test.ts (1)
929-935: Use KEY constant instead of string literal 'key'.Unifies directive-name references and avoids accidental case mismatches.
Apply:
- expect(errors[0]).toStrictEqual( - invalidDirectiveError('key', 'User', FIRST_ORDINAL, [ + expect(errors[0]).toStrictEqual( + invalidDirectiveError(KEY, 'User', FIRST_ORDINAL, [ invalidDirectiveLocationErrorMessage(KEY, INPUT_OBJECT_UPPER), ]), );composition/tests/v1/types/enums.test.ts (1)
92-118: Optional: reduce repetition in TAG_DIRECTIVE expectations.Many blocks concatenate TAG_DIRECTIVE + repeated enum SDL. Consider a small helper like expectNormalizedSDLWithTag(nodeSDL) to cut duplication and ease future changes.
Also applies to: 120-146, 148-160
composition/src/v1/constants/constants.ts (3)
160-168: Unify map key typing to DirectiveName.EVENT_DRIVEN_DIRECTIVE_DEFINITIONS_BY_DIRECTIVE_NAME uses Map<string,...> while other maps use Map<DirectiveName,...>. Align to DirectiveName for type-safety and consistency.
Apply:
-export const EVENT_DRIVEN_DIRECTIVE_DEFINITIONS_BY_DIRECTIVE_NAME = new Map<string, DirectiveDefinitionNode>([ +export const EVENT_DRIVEN_DIRECTIVE_DEFINITIONS_BY_DIRECTIVE_NAME = new Map<DirectiveName, DirectiveDefinitionNode>([
120-136: Clarify “BASE” directive membership; avoid drift between Map vs Array.BASE_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME includes EDFS_* directives, but BASE_DIRECTIVE_DEFINITIONS[] excludes them. If “BASE” means core federation v1 directives, exclude EDFS_* from the map or rename the map to reflect scope. Alternatively, derive both from a single source to keep them in sync.
Example to derive arrays from the map:
-export const BASE_DIRECTIVE_DEFINITIONS: DirectiveDefinitionNode[] = [ - DEPRECATED_DEFINITION, - EXTENDS_DEFINITION, - EXTERNAL_DEFINITION, - KEY_DEFINITION, - PROVIDES_DEFINITION, - REQUIRES_DEFINITION, - SPECIFIED_BY_DEFINITION, - TAG_DEFINITION, -]; +export const BASE_DIRECTIVE_DEFINITIONS: DirectiveDefinitionNode[] = Array.from( + BASE_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME.values() +);Also applies to: 149-158
73-106: Guard against accidental mutation and future divergence.
- Export maps/sets as ReadonlyMap/ReadonlySet consistently and avoid exposing mutable instances.
- Consider freezing arrays: Object.freeze([...]) to harden immutability.
- Prefer constructing subset maps/arrays from a single canonical DIRECTIVE_DEFINITION_BY_NAME to avoid copy‑paste divergence.
Also applies to: 120-148, 149-158, 170-178
composition/tests/v1/directives/inaccessible.test.ts (2)
650-663: Duplicate test titles “#1.3” — rename or parametrize.Multiple tests share the exact name “that an @inaccessible only needs to be declared on a single field #1.3”. Rename distinctly or use it.each to parametrize order permutations.
Also applies to: 655-663, 660-663
122-139: Avoid non‑null assertion on federatedGraphClientSchema.The success helper should always return both schemas; drop the “!” to keep tests strict and type‑safe.
- expect(schemaToSortedNormalizedString(federatedGraphClientSchema!)).toBe( + expect(schemaToSortedNormalizedString(federatedGraphClientSchema)).toBe(composition/tests/v1/types/inputs.test.ts (1)
99-116: Standardize on normalizeSubgraphSuccess helper.These blocks still use normalizeSubgraph + NormalizationSuccess casting. For consistency with the rest of the suite, switch to normalizeSubgraphSuccess and destructure { schema }.
-const result = normalizeSubgraph( - subgraphR.definitions, - subgraphR.name, - undefined, - ROUTER_COMPATIBILITY_VERSION_ONE, -) as NormalizationSuccess; -expect(result.success).toBe(true); -expect(schemaToSortedNormalizedString(result.schema)).toBe( +const { schema } = normalizeSubgraphSuccess(subgraphR, ROUTER_COMPATIBILITY_VERSION_ONE); +expect(schemaToSortedNormalizedString(schema)).toBe(Also applies to: 118-136, 138-156, 158-176, 178-196, 198-216, 218-236
composition/tests/v1/types/objects.test.ts (1)
112-114: Remove unnecessary cast.normalizeSubgraphSuccess already returns the needed shape; the “as NormalizationSuccess” cast is redundant.
-const { schema } = normalizeSubgraphSuccess(subgraphS, ROUTER_COMPATIBILITY_VERSION_ONE) as NormalizationSuccess; +const { schema } = normalizeSubgraphSuccess(subgraphS, ROUTER_COMPATIBILITY_VERSION_ONE);composition/src/v1/constants/strings.ts (1)
62-74: Make exported sets/map truly read‑only at the type level (and arrays immutable).
- Prefer ReadonlySet for all exported sets to prevent accidental mutation by consumers.
- Use ReadonlyMap<DirectiveName, ReadonlyArray> so dependency lists can’t be mutated.
- Minor: the comment above DEPENDENCIES_BY_DIRECTIVE_NAME contradicts the entry for EDFS_NATS_SUBSCRIBE; clarify it.
Apply:
-export const TYPE_SYSTEM_DIRECTIVE_LOCATIONS = new Set<string>([ +export const TYPE_SYSTEM_DIRECTIVE_LOCATIONS: ReadonlySet<string> = new Set<string>([ @@ -export const EVENT_DIRECTIVE_NAMES = new Set<string>([ +export const EVENT_DIRECTIVE_NAMES: ReadonlySet<string> = new Set<string>([ @@ -export const STREAM_CONFIGURATION_FIELD_NAMES = new Set<string>([ +export const STREAM_CONFIGURATION_FIELD_NAMES: ReadonlySet<string> = new Set<string>([ @@ -export const SUBSCRIPTION_FILTER_INPUT_NAMES = new Set<string>([AND_UPPER, IN_UPPER, NOT_UPPER, OR_UPPER]); -export const SUBSCRIPTION_FILTER_LIST_INPUT_NAMES = new Set<string>([AND_UPPER, OR_UPPER]); +export const SUBSCRIPTION_FILTER_INPUT_NAMES: ReadonlySet<string> = new Set<string>([AND_UPPER, IN_UPPER, NOT_UPPER, OR_UPPER]); +export const SUBSCRIPTION_FILTER_LIST_INPUT_NAMES: ReadonlySet<string> = new Set<string>([AND_UPPER, OR_UPPER]); @@ -export const COMPOSITE_OUTPUT_NODE_KINDS: ReadonlySet<Kind> = new Set<Kind>([ +export const COMPOSITE_OUTPUT_NODE_KINDS: ReadonlySet<Kind> = new Set<Kind>([ @@ -export const DEPENDENCIES_BY_DIRECTIVE_NAME: ReadonlyMap<DirectiveName, Array<DefinitionNode>> = new Map< - DirectiveName, - Array<DefinitionNode> ->([ +// Dependencies injected when a directive is present in a subgraph +export const DEPENDENCIES_BY_DIRECTIVE_NAME: ReadonlyMap<DirectiveName, ReadonlyArray<DefinitionNode>> = new Map< + DirectiveName, + ReadonlyArray<DefinitionNode> +>([ @@ -export const IGNORED_FEDERATED_TYPE_NAMES: ReadonlySet<TypeName> = new Set<TypeName>([ +export const IGNORED_FEDERATED_TYPE_NAMES: ReadonlySet<TypeName> = new Set<TypeName>([ @@ -export const CLIENT_PERSISTED_DIRECTIVE_NAMES: ReadonlySet<DirectiveName> = new Set<DirectiveName>([ +export const CLIENT_PERSISTED_DIRECTIVE_NAMES: ReadonlySet<DirectiveName> = new Set<DirectiveName>([Also consider freezing the value arrays:
- [KEY, [FIELD_SET_SCALAR_DEFINITION]], + [KEY, Object.freeze([FIELD_SET_SCALAR_DEFINITION] as const)],And update the stale comment:
-// `edfs__natsSubscribe` dependencies are added in `validateEventDrivenSubgraph` +// Dependencies automatically injected for specific directives (e.g., edfs__natsSubscribe requires edfs__NatsStreamConfiguration).Also applies to: 76-85, 86-94, 95-101, 123-136, 138-142
composition/src/v1/constants/non-directive-definitions.ts (2)
1-1: Unify GraphQL imports to one module path.Mixing 'graphql/index' and 'graphql' can cause dual module instances and type incompatibilities. Import from 'graphql' only.
-import { InputObjectTypeDefinitionNode, Kind, ScalarTypeDefinitionNode } from 'graphql/index'; +import { InputObjectTypeDefinitionNode, Kind, ScalarTypeDefinitionNode, EnumTypeDefinitionNode } from 'graphql'; @@ -import { EnumTypeDefinitionNode } from 'graphql';Also applies to: 27-28
36-69: Optional: reorder fields to match normalized SDL expectations.Your tests assert edfs__NatsStreamConfiguration with consumerInactiveThreshold first. If the normalizer does not sort input fields, this order difference can fail string comparisons. Reorder defensively.
export const EDFS_NATS_STREAM_CONFIGURATION_DEFINITION: InputObjectTypeDefinitionNode = { kind: Kind.INPUT_OBJECT_TYPE_DEFINITION, name: stringToNameNode(EDFS_NATS_STREAM_CONFIGURATION), fields: [ - { - kind: Kind.INPUT_VALUE_DEFINITION, - name: stringToNameNode(CONSUMER_NAME), - type: { - kind: Kind.NON_NULL_TYPE, - type: stringToNamedTypeNode(STRING_SCALAR), - }, - }, - { - kind: Kind.INPUT_VALUE_DEFINITION, - name: stringToNameNode(STREAM_NAME), - type: { - kind: Kind.NON_NULL_TYPE, - type: stringToNamedTypeNode(STRING_SCALAR), - }, - }, { kind: Kind.INPUT_VALUE_DEFINITION, name: stringToNameNode(CONSUMER_INACTIVE_THRESHOLD), type: { kind: Kind.NON_NULL_TYPE, type: stringToNamedTypeNode(INT_SCALAR), }, defaultValue: { kind: Kind.INT, value: DEFAULT_CONSUMER_INACTIVE_THRESHOLD.toString(), }, }, + { + kind: Kind.INPUT_VALUE_DEFINITION, + name: stringToNameNode(CONSUMER_NAME), + type: { + kind: Kind.NON_NULL_TYPE, + type: stringToNamedTypeNode(STRING_SCALAR), + }, + }, + { + kind: Kind.INPUT_VALUE_DEFINITION, + name: stringToNameNode(STREAM_NAME), + type: { + kind: Kind.NON_NULL_TYPE, + type: stringToNamedTypeNode(STRING_SCALAR), + }, + }, ], };Confirm whether schemaToSortedNormalizedString sorts input field definitions. If it already sorts, this change is optional.
composition/tests/v1/events.test.ts (1)
603-633: Remove ad‑hoc console output and give the test a descriptive name.console.log in tests is noisy in CI; the case name 'test' is not descriptive.
-test('test', () => { +test('prints custom input type unchanged by normalization helpers', () => { @@ - console.log(printSchema(s)); + // No assertions here; keep this purely for local inspection if needed. + // Consider adding an assertion or removing this block entirely.Or delete the console.log line if the block must stay.
composition/src/v1/constants/directive-definitions.ts (2)
565-581: Unify FieldSet type construction in @requires for consistency
REQUIRES_DEFINITIONhand-buildsNON_NULL(FIELD_SET_SCALAR)whileKEY_DEFINITION/PROVIDES_DEFINITIONuseREQUIRED_FIELDSET_TYPE_NODE. Prefer the shared node for consistency and fewer AST variants.Apply:
export const REQUIRES_DEFINITION: DirectiveDefinitionNode = { arguments: [ { kind: Kind.INPUT_VALUE_DEFINITION, name: stringToNameNode(FIELDS), - type: { - kind: Kind.NON_NULL_TYPE, - type: stringToNamedTypeNode(FIELD_SET_SCALAR), - }, + type: REQUIRED_FIELDSET_TYPE_NODE, }, ], kind: Kind.DIRECTIVE_DEFINITION, locations: [stringToNameNode(FIELD_DEFINITION_UPPER)], name: stringToNameNode(REQUIRES), repeatable: false, };
307-308: Minor: fix @edfs__natsRequest comment typoMissing colon after
providerId. Should read:providerId: String! = "default".-// directive @edfs__natsRequest(subject: String!, providerId String! = "default") on FIELD_DEFINITION +// directive @edfs__natsRequest(subject: String!, providerId: String! = "default") on FIELD_DEFINITIONcomposition/src/v1/normalization/normalization-factory.ts (2)
2990-3004: Defensive guard for unlikely failed upsertComment says “cannot actually be unsuccessful”, but we proceed assuming success. Add a small guard to avoid
result.dataaccess if this assumption breaks.- if (result.success) { + if (result.success) { for (const fieldNode of EDFS_NATS_STREAM_CONFIGURATION_DEFINITION.fields!) { this.addInputValueDataByNode({ fieldName: fieldNode.name.value, isArgument: false, inputValueDataByName: result.data.inputValueDataByName, node: fieldNode, originalParentTypeName: EDFS_NATS_STREAM_CONFIGURATION, }); } - } + } else { + // Defensive: should not happen; skip injection to prevent undefined access + return; + }
2945-3029: Dead variable:nonEntityExtensionTypeNamesis never populatedThe set is declared and checked but never written to, so the error branch is unreachable. Either wire the producer or remove the variable and its check.
composition/src/v1/federation/federation-factory.ts (1)
317-353: Centralized persisted-directive extraction is solid; add tiny safetyLogic cleanly separates auth, deprecated, tag, and generic persisted directives and records references for later def injection. Add a guard for unexpected empty arrays.
Apply:
- switch (directiveName) { + if (!directiveNodes || directiveNodes.length === 0) { + continue; + } + switch (directiveName) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/test/__snapshots__/router.config.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (50)
composition-go/composition_test.go(1 hunks)composition/src/index.ts(1 hunks)composition/src/schema-building/ast.ts(1 hunks)composition/src/schema-building/utils.ts(7 hunks)composition/src/utils/utils.ts(1 hunks)composition/src/v1/constants/constants.ts(1 hunks)composition/src/v1/constants/directive-definitions.ts(8 hunks)composition/src/v1/constants/non-directive-definitions.ts(1 hunks)composition/src/v1/constants/strings.ts(1 hunks)composition/src/v1/constants/type-nodes.ts(1 hunks)composition/src/v1/federation/federation-factory.ts(25 hunks)composition/src/v1/federation/params.ts(1 hunks)composition/src/v1/federation/utils.ts(1 hunks)composition/src/v1/normalization/directive-definition-data.ts(1 hunks)composition/src/v1/normalization/normalization-factory.ts(22 hunks)composition/src/v1/normalization/types.ts(2 hunks)composition/src/v1/normalization/utils.ts(1 hunks)composition/src/v1/normalization/walkers.ts(6 hunks)composition/src/v1/utils/string-constants.ts(0 hunks)composition/src/v1/utils/utils.ts(1 hunks)composition/tests/utils/utils.ts(2 hunks)composition/tests/v1/contracts.test.ts(134 hunks)composition/tests/v1/directives/authorization-directives.test.ts(36 hunks)composition/tests/v1/directives/configure-description.test.ts(16 hunks)composition/tests/v1/directives/directives.test.ts(12 hunks)composition/tests/v1/directives/external.test.ts(32 hunks)composition/tests/v1/directives/fieldset-directives.test.ts(4 hunks)composition/tests/v1/directives/inaccessible.test.ts(29 hunks)composition/tests/v1/directives/interface-object.test.ts(10 hunks)composition/tests/v1/directives/one-of.test.ts(13 hunks)composition/tests/v1/directives/override.test.ts(29 hunks)composition/tests/v1/directives/provides.test.ts(31 hunks)composition/tests/v1/directives/require-fetch-reasons.test.ts(11 hunks)composition/tests/v1/directives/semantic-non-null.test.ts(22 hunks)composition/tests/v1/directives/shareable.test.ts(12 hunks)composition/tests/v1/directives/subscription-filter.test.ts(4 hunks)composition/tests/v1/directives/v2-directives.test.ts(3 hunks)composition/tests/v1/entities.test.ts(25 hunks)composition/tests/v1/entity-interface.test.ts(7 hunks)composition/tests/v1/events.test.ts(27 hunks)composition/tests/v1/federation-factory.test.ts(20 hunks)composition/tests/v1/normalization.test.ts(73 hunks)composition/tests/v1/resolvability.test.ts(38 hunks)composition/tests/v1/router-configuration.test.ts(8 hunks)composition/tests/v1/types/arguments.test.ts(8 hunks)composition/tests/v1/types/enums.test.ts(11 hunks)composition/tests/v1/types/inputs.test.ts(13 hunks)composition/tests/v1/types/interfaces.test.ts(29 hunks)composition/tests/v1/types/objects.test.ts(12 hunks)composition/tests/v1/types/scalars.test.ts(4 hunks)
💤 Files with no reviewable changes (1)
- composition/src/v1/utils/string-constants.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/tests/v1/directives/subscription-filter.test.tscomposition/tests/v1/normalization.test.tscomposition/tests/v1/directives/shareable.test.ts
🔇 Additional comments (61)
composition/src/v1/utils/utils.ts (2)
33-33: AI summary mentions non-existent function.The AI-generated summary for this file claims a new exported function
addOptionalIterableValuesToSetwas added, but this function is not present in the provided code. The actual change is just an import path update forMAX_OR_SCOPES.
33-37: LGTM! Import paths updated correctly.The import path updates align with the constants reorganization into dedicated
v1/constants/modules. BothMAX_OR_SCOPESandCOMPOSITE_OUTPUT_NODE_KINDSare actively used in this file (lines 314, 335-336, 399 and line 444 respectively).composition/tests/v1/directives/v2-directives.test.ts (4)
10-10: LGTM: Clean constant extraction.The import of
SCHEMA_QUERY_DEFINITIONcentralizes the base schema definition, reducing duplication and improving maintainability across test files.
20-26: LGTM: Improved readability through destructuring.The destructuring pattern for
federatedGraphSchemaand the use ofSCHEMA_QUERY_DEFINITIONas a schema prefix make the test more readable and maintainable. The test logic remains intact while adapting cleanly to the updated API surface.
43-49: LGTM: Consistent refactoring pattern.The second test follows the same clean destructuring and constant usage pattern as the first test, ensuring consistency across the test suite.
65-67: LGTM: Safe error handling with destructuring.The destructuring pattern for error handling is clean and safe. The length check on line 66 properly validates the errors array before accessing
errors[0], preventing potential undefined access issues.composition/tests/v1/entity-interface.test.ts (3)
185-190: Good addition: validating warnings.The test now properly validates that no warnings are produced during federation, which improves test coverage and ensures the operation completes cleanly.
18-18: SCHEMA_QUERY_DEFINITION import verified successfully.The verification confirms that
SCHEMA_QUERY_DEFINITIONis properly exported fromcomposition/tests/v1/utils/utils.tsat line 169 with the expected schema query definition (schema { query: Query }). The import path./utils/utilsin the test file correctly resolves to this export.
30-31: No issues found – destructuring pattern is fully compatible with return type.The
federateSubgraphsSuccessfunction returnsFederationSuccess, which contains all properties being destructured in the test:federatedGraphSchema(GraphQLSchema),subgraphConfigBySubgraphName(Map<string, SubgraphConfig>), andwarnings(Array). The refactored destructuring pattern is correct and properly typed.composition/src/v1/normalization/types.ts (1)
6-6: LGTM.composition/src/schema-building/utils.ts (3)
287-299: Good: export upsertDeprecatedDirective for reuse.Makes persisted deprecation handling composable across modules.
749-751: Nice addition: input object runtime guard.Matches pattern of other guards; improves narrowing.
523-523: No signature widening issues detected.The function parameter
definitions: (MutableDefinitionNode | DefinitionNode)[]at composition/src/schema-building/utils.ts:523 is already compatible with its callers. The only caller passingthis.routerDefinitionsin federation-factory.ts (lines 2794, 3107) is already typed as(MutableDefinitionNode | DefinitionNode)[](federation-factory.ts:290), matching the parameter type exactly. All callers are compatible.composition/src/v1/federation/utils.ts (1)
16-16: Import path update verified—no stale imports remain.All BASE_SCALARS imports across the codebase have been consistently updated to the new path
../constants/constants. The new path is valid, and no references to the old path exist. The change is safe and complete.composition/src/schema-building/ast.ts (1)
266-274: ****The review comment assumes
MutableDefinitionNodewas renamed fromMutableTypeDefinitionNode, but git history showsMutableTypeDefinitionNodehas never existed in this codebase. There is no evidence of a rename occurring, therefore no breaking change to mitigate. The back-compat alias suggestion should be disregarded.Likely an incorrect or invalid review comment.
composition/tests/v1/types/interfaces.test.ts (2)
20-20: Good move to centralized test helpers/constants.Imports of INACCESSIBLE_DIRECTIVE, SCHEMA_QUERY_DEFINITION, TAG_DIRECTIVE and normalizeSubgraphSuccess/Failure align tests with the new API surface. No issues.
Also applies to: 25-27
1657-1709: New invalid‑implementation fixtures (naaaa, nbaaa) look solid.They exercise argument type/arity, accessibility, and unimplemented fields well; assertions above match the shapes.
To guard against accidental future edits, consider pinning a snapshot of the expected error shapes for these two cases.
composition/tests/v1/router-configuration.test.ts (1)
22-24: Destructuring configurationDataByTypeName from normalizeSubgraphSuccess is clean.Matches the updated test helper contract; assertions remain intact.
composition/src/v1/normalization/utils.ts (1)
19-19: Import path update verified—no changes needed.Both exports exist with correct types: BASE_SCALARS is a Set and EDFS_ARGS_REGEXP is a RegExp. The import path correctly resolves and the change is safe.
composition/src/v1/normalization/directive-definition-data.ts (1)
77-109: Imports verified—code changes are correct.All imports originate from their expected modules with the correct AST node shapes. REQUIRED_*_TYPE_NODE constants are properly typed NonNull wrappers, and all DEFINITION constants are DirectiveDefinitionNode types as expected. The refactoring to v1/constants modules is consistent and complete.
composition-go/composition_test.go (1)
72-100: LGTM! SDL expectations updated to reflect selective directive injection.The expected SDL now includes only the directives that are actively used (
@requiresScopesand@authenticated), along with the newopenfed__Scopescalar. This aligns with the PR objective of selectively injecting directive definitions and dependencies rather than including all possible directives upfront.composition/tests/v1/directives/interface-object.test.ts (3)
29-29: LGTM! Updated to use new test constants.The import of
SCHEMA_QUERY_DEFINITIONfrom the shared utils aligns with the broader refactor to centralize directive and schema definition constants.
33-37: LGTM! Cleaner error destructuring pattern.The updated pattern directly destructures
errorsfrom the failure result, simplifying access compared to the previousresult.errorsapproach.
47-87: LGTM! Consistent migration to SCHEMA_QUERY_DEFINITION.All test expectations now use
SCHEMA_QUERY_DEFINITIONas the canonical SDL prefix, replacing the previousversionTwoRouterDefinitions. This standardization improves test maintainability and aligns with the new constant organization.Also applies to: 169-209, 291-331, 414-454, 534-562, 636-667, 675-699, 707-731, 774-822
composition/src/v1/constants/type-nodes.ts (1)
1-13: LGTM! Well-structured TypeNode constants.These reusable TypeNode constants (
REQUIRED_STRING_TYPE_NODEandREQUIRED_FIELDSET_TYPE_NODE) follow GraphQL AST patterns correctly and will help ensure consistency across directive definitions that require these common type structures.composition/src/v1/federation/params.ts (1)
1-15: LGTM! Clean type definition for directive extraction parameters.The new
ExtractPersistedDirectivesParamstype provides well-structured parameters for the directive extraction flow, supporting the selective injection feature described in the PR objectives.composition/src/utils/utils.ts (1)
64-71: LGTM! Useful optional variant of existing utility.The
addOptionalIterableValuesToSetfunction is a clean addition that mirrors the existingaddIterableValuesToSetpattern while handling optional sources gracefully with an early guard clause.composition/tests/v1/directives/require-fetch-reasons.test.ts (3)
13-13: LGTM! Updated imports for new test constants.The imports now include
REQUEST_FETCH_REASONS_DIRECTIVEandSCHEMA_QUERY_DEFINITION, replacing the previous consolidated definition imports. This aligns with the selective directive injection approach.
23-33: LGTM! Consistent use of SCHEMA_QUERY_DEFINITION.All federated graph schema expectations now use
SCHEMA_QUERY_DEFINITIONas the base, replacingversionOneRouterDefinitions. This standardization improves test clarity and maintainability.Also applies to: 67-82, 117-132, 167-182, 217-232, 267-282, 317-332, 367-384
392-409: LGTM! Explicit directive inclusion in subgraph schemas.The subgraph-specific schemas now explicitly concatenate
SCHEMA_QUERY_DEFINITIONwithREQUEST_FETCH_REASONS_DIRECTIVE, reflecting the selective directive injection where directives are only included when actually used.Also applies to: 438-457
composition/tests/v1/directives/external.test.ts (5)
19-19: LGTM! Updated imports for new directive constants.The imports now include individual directive constants (
EXTERNAL_DIRECTIVE,KEY_DIRECTIVE,OPENFED_FIELD_SET) andSCHEMA_QUERY_DEFINITION, replacing the previous consolidated router definition imports.
32-60: LGTM! Explicit directive fragments in normalized schema expectations.The test expectations now explicitly include directive fragments (
EXTERNAL_DIRECTIVE,KEY_DIRECTIVE) rather than relying on base definition sets. This makes the expected output more explicit and aligns with selective directive injection. Note the subgraph reference changed fromnatonaaaa(defined at line 1171).Also applies to: 63-91
94-108: LGTM! Proper directive and scalar fragment composition.The expected output correctly combines
EXTERNAL_DIRECTIVE,KEY_DIRECTIVE, andOPENFED_FIELD_SETto construct the complete schema expectation.
337-370: LGTM! Systematic migration to SCHEMA_QUERY_DEFINITION.All federation test expectations consistently use
SCHEMA_QUERY_DEFINITIONas the base SDL prefix. The systematic nature of these changes demonstrates a well-executed refactor across the entire test suite.Also applies to: 374-407, 411-447, 450-486, 489-525, 528-564, 567-603, 606-642, 645-665, 668-688, 702-720, 723-742, 745-764, 767-819, 824-885, 888-944, 970-1022, 1025-1086, 1089-1145
1171-1197: Subgraph renamed fromnatonaaaafor consistency.The subgraph definition was renamed, and all references in the test file have been updated accordingly. This maintains internal consistency within the test suite.
composition/src/index.ts (1)
30-35: LGTM! Improved constants organization with centralized exports.The new exports consolidate v1 constants into dedicated modules (constants, directive-definitions, integers, non-directive-definitions, strings, type-nodes) rather than scattering them across utility modules. This improves discoverability and maintainability of the public API.
Note: This represents a public API surface change. If there are external consumers importing from the previous
v1/utils/constantspaths, they will need to update their imports.composition/tests/v1/types/arguments.test.ts (1)
37-41: Good switch to direct destructuringUsing { federatedGraphSchema } improves clarity and avoids leaking helper internals. LGTM.
Also applies to: 61-66
composition/tests/v1/directives/shareable.test.ts (1)
110-133: Permutation tests read wellOrder-invariance across subgraphs is covered comprehensively. LGTM.
composition/src/v1/normalization/walkers.ts (1)
61-62: Rename ripple check verified: customDirectiveDefinitionByNameNo remaining references to the old property name exist. The new property
customDirectiveDefinitionByNameis properly defined (normalization-factory.ts:389), written to (walkers.ts:61), and read from (normalization-factory.ts:3447). The rename is complete and consistent.composition/tests/v1/types/scalars.test.ts (1)
22-26: Good migration to new normalization/federation helpers.Destructured results and constant fragments are used cleanly; assertions look correct.
Also applies to: 33-38, 46-49, 71-80
composition/tests/v1/directives/subscription-filter.test.ts (1)
1-33: The review comment is incorrect. NormalizationSuccess is still publicly exported and the cast is necessary.NormalizationSuccess is re-exported from the main index via
export * from './normalization/types', making it part of the public API. The cast innormalizeSubgraphSuccessis not redundant—it serves a purpose: TypeScript needs it to narrow the return type after theexpect()assertion, sincenormalizeSubgraph()returns a union type (success | failure). Removing the import and cast would break the test utilities and dependent tests.Likely an incorrect or invalid review comment.
composition/tests/v1/normalization.test.ts (1)
789-861: Nice shift to public constants and selective injection.Using
SCHEMA_QUERY_DEFINITION,KEY_DIRECTIVE,SHAREABLE_DIRECTIVE,TAG_DIRECTIVE, andOPENFED_FIELD_SET/OPENFED_SCOPEkeeps tests aligned with the new public surface.composition/tests/v1/directives/authorization-directives.test.ts (1)
12-17: Selective directive injection looks consistent.Good move to explicit AUTHENTICATED/REQUIRES_SCOPES constants and appending OPENFED_SCOPE only when needed.
Consider adding one negative test asserting OPENFED_SCOPE is not present when no
@requiresScopesis used, to guard over‑injection regressions.Also applies to: 300-318, 575-592
composition/tests/v1/types/enums.test.ts (1)
13-21: Good migration to helper-based test APIs.Switch to normalizeSubgraphSuccess/Failure and shared directive constants improves readability and reduces boilerplate. No issues found.
composition/src/v1/constants/constants.ts (1)
170-178: Confirm intentional omission of LINK from VERSION_TWO_DIRECTIVE_DEFINITIONS.LINK is present in V2 map but not in VERSION_TWO_DIRECTIVE_DEFINITIONS[]. If excluded by design (e.g., injected separately), add a brief comment to prevent re-introduction. If not, include it.
[suggest_minor_issue]
composition/tests/v1/directives/inaccessible.test.ts (1)
33-41: LGTM on helper migration and SDL assembly.Use of federateSubgraphsSuccess/Failure and concatenated SCHEMA_QUERY_DEFINITION + INACCESSIBLE_DIRECTIVE matches the new public test utilities and reads clean.
Also applies to: 266-275, 308-316, 356-364, 405-414, 477-486, 521-529, 579-587, 755-763, 795-803, 835-843, 855-873
composition/tests/v1/types/inputs.test.ts (1)
32-47: LGTM on helper migration and use of SCHEMA_QUERY_DEFINITION/TAG_DIRECTIVE.Assertions align with new return shapes and shared constants; expectations look correct.
Also applies to: 238-260, 305-324, 342-359, 361-380
composition/tests/v1/types/objects.test.ts (1)
31-41: Helper migration and SDL assertions look solid.The normalization/federation tests consistently use the new helpers and constants; expectations match the updated flows.
Also applies to: 43-55, 58-69, 71-82, 84-96, 98-109, 139-151, 153-165, 167-179, 181-193, 243-253, 288-309, 311-331, 333-337, 339-359, 361-381, 401-419, 466-486, 487-506
composition/tests/v1/directives/override.test.ts (1)
35-42: LGTM: normalization error surface.Using normalizeSubgraphFailure and destructuring { errors } aligns with the new API surface.
composition/tests/v1/federation-factory.test.ts (2)
582-591: LGTM: schema prefix constants and client schema assertions.Switch to SCHEMA_* and persisted directive constants is consistent across tests; client vs server schema checks look sound.
Also applies to: 610-621, 626-639, 642-699
564-572: Fix Map length assertions (use .size).directiveDefinitionByDirectiveName appears to be a Map/Set (has method). toHaveLength expects a length property and will fail.
Apply:
-expect(subgraphAConfig!.directiveDefinitionByDirectiveName).toHaveLength(2); +expect(subgraphAConfig!.directiveDefinitionByDirectiveName.size).toBe(2); @@ -expect(subgraphBConfig!.directiveDefinitionByDirectiveName).toHaveLength(3); +expect(subgraphBConfig!.directiveDefinitionByDirectiveName.size).toBe(3);Also applies to: 568-571
⛔ Skipped due to learnings
Learnt from: Aenimus PR: wundergraph/cosmo#2287 File: composition/tests/v1/federation-factory.test.ts:569-583 Timestamp: 2025-10-17T16:35:24.723Z Learning: In the wundergraph/cosmo repository's Vitest test suite, `toHaveLength()` works with Map objects and `expect(map.has(key))` without an explicit matcher correctly asserts the boolean result in the test assertions for `composition/tests/v1/federation-factory.test.ts`.composition/src/v1/constants/non-directive-definitions.ts (1)
2-2: Import path confirmed — no change required.'../../ast/utils' resolves to composition/src/ast/utils.ts (defines stringToNameNode and stringToNamedTypeNode) and the same import is used across composition/src/v1; leave as-is.
composition/src/v1/constants/directive-definitions.ts (1)
480-514: @link(as) nullability: comment vs implementation mismatchThe doc comment says
as: String!but the implementation makesasoptional. Please confirm the intended spec and align comment/AST accordingly. Ifasmust be required, wrap inNON_NULL. If optional, update the comment.composition/src/v1/normalization/normalization-factory.ts (2)
167-172: No issues found—type annotation confirms AST nodes are exportedThe export declaration at line 103 of
composition/src/v1/constants/strings.tscorrectly types the values asArray<DefinitionNode>(AST nodes, not strings), matching the expected usage. No type/runtime mismatch exists.
2669-2755: ****The review comment is based on a faulty assumption. The
edfsDirectiveReferencesset IS being populated inwalkers.tsline 50 during the Directive visitor traversal.In the
normalize()method,upsertDirectiveSchemaAndEntityDefinitions()is called first at line 3455. This walker uses graphql-js'svisit()function to traverse the entire document, including all Directive nodes (both global and field-level). When a directive name matchesEVENT_DIRECTIVE_NAMES, it's added toedfsDirectiveReferencesat line 50.By the time
extractEventDirectivesToConfiguration()is invoked at line 348 ofwalkers.tsduring field processing, the set is already populated. Adding the suggested insertion would constitute redundant work, as the directives have already been registered by the earlier visitor pass.The existing code flow is correct. No changes are required.
Likely an incorrect or invalid review comment.
composition/src/v1/federation/federation-factory.ts (6)
256-257: TypeName-centric maps and definition arrays: good upgradeSwitch to Map<TypeName,…> and widening of router/client definition arrays improves type-safety and flexibility. Seeded persistedDirectiveDefinitionByDirectiveName looks correct.
Also applies to: 260-264, 276-287, 291-291
649-653: Consistent wiring of extractPersistedDirectives across copy/upsert pathsGood consistency; persisted directives now flow to enums, args, fields, and parent nodes uniformly.
Also applies to: 680-684, 864-868, 968-972, 990-994, 1019-1023, 1079-1083, 1235-1239
2246-2247: Good placement of directive def/deps injectionRunning after tag-data generation and before auth propagation ensures definitions exist before schema assembly.
2283-2297: I'll search for more specific information about the dependencies and how client directives are filtered to verify the concerns.The web search wasn't specific to the codebase. Let me search directly in the code for overlapping dependencies.Based on my analysis of the code search results and the review comment, I now have enough information to generate my final response:Confirm that deduplication is necessary and the client set alignment is valid
Looking at the code search results:
DEPENDENCIES_BY_DIRECTIVE_NAMEentries show that multiple directives can share dependencies (e.g., multiple directives depend on scalar types and input object types)CLIENT_PERSISTED_DIRECTIVE_NAMES= {DEPRECATED, ONE_OF, SEMANTIC_NON_NULL}getClientPersistedDirectiveNodes()filters for these same three directive names- The current code does not deduplicate, so shared dependencies would be added multiple times
The review concern is valid and accurate. The suggested refactor addresses a real issue. The client directive alignment is also correct - the same directive set that gets added to client definitions is the one filtered by
getClientPersistedDirectiveNodes().Avoid duplicate definition insertion and align client directive surface
The current implementation risks appending shared dependencies multiple times to
routerDefinitionsandclientDefinitionsarrays when multiple directives reference the same scalar or type dependencies. The suggested de-duplication using Sets to track already-added definitions is appropriate and necessary.The client directive alignment is correct:
CLIENT_PERSISTED_DIRECTIVE_NAMES({DEPRECATED,ONE_OF,SEMANTIC_NON_NULL}) matches whatgetClientPersistedDirectiveNodes()emits, ensuring consistency between the directive set added toclientDefinitionsand what the client schema ultimately receives.The refactor is sound and addresses legitimate concerns.
1946-1948: IGNORED_FEDERATED_TYPE_NAMES guards are correctly scoped to applicable type casesBased on code inspection, the current guard placement is correct and intentional. The
IGNORED_FEDERATED_TYPE_NAMESset contains only internal federation metadata types: 5 scalars (BOOLEAN_SCALAR, ID_SCALAR, INT_SCALAR, FLOAT_SCALAR, STRING_SCALAR), 1 enum (LINK_PURPOSE), and 7 input objects (e.g., SUBSCRIPTION_FIELD_CONDITION, EDFS_NATS_STREAM_CONFIGURATION).The guards are positioned only where these types can occur:
- Line 1946: Inside
Kind.ENUM_TYPE_DEFINITIONcase (enum LINK_PURPOSE is guarded)- Line 2015: Inside
Kind.INPUT_OBJECT_TYPE_DEFINITIONcase (input objects like SUBSCRIPTION_FIELD_CONDITION are guarded)- Line 2155: Inside
Kind.SCALAR_TYPE_DEFINITIONcase (scalars like BOOLEAN_SCALAR are guarded)The
OBJECT_TYPE_DEFINITION,INTERFACE_TYPE_DEFINITION, andUNION_TYPE_DEFINITIONcases correctly have no guards because no composite output types appear in the IGNORED set—these are user-defined types that must always be processed.
279-287: Seeded directive-definition map is correctly synchronized with injection registryAll seven directives in
persistedDirectiveDefinitionByDirectiveName(AUTHENTICATED, DEPRECATED, INACCESSIBLE, ONE_OF, REQUIRES_SCOPES, SEMANTIC_NON_NULL, TAG) are present inDIRECTIVE_DEFINITION_BY_NAMEand reference the identical definition objects fromdirective-definitions.ts.Since both maps use the same object references rather than duplicating definitions, semantic consistency of repeatable and locations properties is guaranteed by design—there is no risk of mismatch. The current implementation correctly prevents the drift concern raised in the review comment.
|
@coderabbitai pause |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
✅ Actions performedReviews paused. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
composition/src/v1/normalization/directive-definition-data.ts (1)
407-447: Include streamConfiguration in optional argsNATS subscribe defines STREAM_CONFIGURATION in argument types but not in optionalArgumentNames. Add it for completeness and clearer validation semantics.
export const NATS_SUBSCRIBE_DEFINITION_DATA: DirectiveDefinitionData = { argumentTypeNodeByName: new Map<string, ArgumentData>([ @@ ]), isRepeatable: false, locations: new Set<string>([FIELD_DEFINITION_UPPER]), name: EDFS_NATS_SUBSCRIBE, node: EDFS_NATS_SUBSCRIBE_DEFINITION, - optionalArgumentNames: new Set<string>([PROVIDER_ID]), + optionalArgumentNames: new Set<string>([PROVIDER_ID, STREAM_CONFIGURATION]), requiredArgumentNames: new Set<string>([SUBJECTS]), };composition/src/v1/normalization/walkers.ts (1)
182-191: Bug: duplicate operation types error is not recordedduplicateOperationTypeDefinitionError(...) is called but not pushed to nf.errors, so duplicates won’t surface.
- if (definitionNode) { - duplicateOperationTypeDefinitionError( - operationType, - namedTypeName, - getTypeNodeNamedTypeName(definitionNode.type), - ); - return false; - } + if (definitionNode) { + nf.errors.push( + duplicateOperationTypeDefinitionError( + operationType, + namedTypeName, + getTypeNodeNamedTypeName(definitionNode.type), + ), + ); + return false; + }composition/tests/v1/directives/authorization-directives.test.ts (1)
1655-1660: Fix subgraph name typo (breaks warning/error provenance).Variable is
fdabut itsnameis "ng". Use a consistent name to avoid confusing diagnostics.-const fda: Subgraph = { - name: 'ng', +const fda: Subgraph = { + name: 'fda',composition/src/v1/normalization/normalization-factory.ts (1)
1085-1125: Custom directive definitions not emitted; populate customDirectiveDefinitionByName#addDirectiveDefinitionsToDocument emits this.customDirectiveDefinitionByName, but addDirectiveDefinitionDataByNode never sets it for custom directives. Result: custom @Directives defined in SDL won’t appear in the output AST.
Add:
addDirectiveDefinitionDataByNode(node: DirectiveDefinitionNode): boolean { const name = node.name.value; if (this.definedDirectiveNames.has(name)) { this.errors.push(duplicateDirectiveDefinitionError(name)); return false; } this.definedDirectiveNames.add(name); // Normalize federation directives by replacing them with predefined definitions const definition = V2_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME.get(name); // Add the V2 directive definitions regardless of use so the subgraph can be recognised as a V2 subgraph. if (definition) { this.directiveDefinitionByName.set(name, definition); // Uses of the directive also set this boolean in the walker. this.isSubgraphVersionTwo = true; return false; } // Other known directives will only be added if they are used. if (DIRECTIVE_DEFINITION_BY_NAME.has(name)) { return false; } - this.directiveDefinitionByName.set(name, node); + this.directiveDefinitionByName.set(name, node); + this.customDirectiveDefinitionByName.set(name as DirectiveName, node);
♻️ Duplicate comments (4)
composition/tests/v1/resolvability.test.ts (1)
1059-1059: The typo has already been corrected.The past review comment suggested changing "frm" to "from", but the test title already correctly uses "from" in the current code.
composition/src/v1/constants/constants.ts (1)
108-118: Fix BASE_SCALARS: remove non-existent “_Entities” scalar and use shared ANY constant“_Entities” isn’t a scalar in federation; leave only real scalars. Also, avoid the literal '_Any' and use the shared ANY_SCALAR constant for consistency.
@@ -import { DirectiveDefinitionNode } from 'graphql'; +import { DirectiveDefinitionNode } from 'graphql'; @@ - BOOLEAN_SCALAR, + BOOLEAN_SCALAR, COMPOSE_DIRECTIVE, @@ STRING_SCALAR, SUBSCRIPTION_FILTER, TAG, } from '../../utils/string-constants'; +// Optional: if available in this module, import ANY_SCALAR for consistency +// (already used elsewhere, e.g., in walkers.ts) +import { ANY_SCALAR } from '../../utils/string-constants'; @@ -export const BASE_SCALARS: ReadonlySet<string> = new Set<string>([ - '_Any', - '_Entities', - BOOLEAN_SCALAR, - FLOAT_SCALAR, - ID_SCALAR, - INT_SCALAR, - FIELD_SET_SCALAR, - SCOPE_SCALAR, - STRING_SCALAR, -]); +export const BASE_SCALARS: ReadonlySet<string> = new Set<string>([ + ANY_SCALAR, + BOOLEAN_SCALAR, + FLOAT_SCALAR, + ID_SCALAR, + INT_SCALAR, + FIELD_SET_SCALAR, + SCOPE_SCALAR, + STRING_SCALAR, +]);composition/tests/v1/directives/configure-description.test.ts (1)
670-675: Title corrected (“Scalar” propagation).Previous misnaming is fixed. Thanks for aligning the title with the assertions.
composition/src/v1/normalization/normalization-factory.ts (1)
3441-3457: Referenced V2 directives are not flagged/injected; set isSubgraphVersionTwo and load from V2 mapWhen a V2 directive is referenced (common), we should inject its definition and flip the version flag. Current code only checks DIRECTIVE_DEFINITION_BY_NAME and never sets isSubgraphVersionTwo here, so V2-only references are missed.
Apply:
#addDirectiveDefinitionsToDocument(definitions: Array<DefinitionNode>): void { const dependencies = new Set<DefinitionNode>(); for (const directiveName of this.referencedDirectiveNames) { - const definition = DIRECTIVE_DEFINITION_BY_NAME.get(directiveName); - if (!definition) { - continue; - } - this.directiveDefinitionByName.set(directiveName, definition); + let definition = DIRECTIVE_DEFINITION_BY_NAME.get(directiveName); + if (!definition) { + // Handle federation V2 directives referenced (but not defined) in SDL + definition = V2_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME.get(directiveName); + if (!definition) { + continue; + } + this.isSubgraphVersionTwo = true; + } + this.directiveDefinitionByName.set(directiveName, definition); addOptionalIterableValuesToSet(dependencies, DEPENDENCIES_BY_DIRECTIVE_NAME.get(directiveName)); definitions.push(definition); } // Always include custom directive definitions regardless of use. for (const definition of this.customDirectiveDefinitionByName.values()) { definitions.push(definition); } definitions.push(...dependencies); }
🧹 Nitpick comments (33)
composition/tests/v1/types/interfaces.test.ts (5)
20-28: Unify utils import source to avoid split-brain.You import constants from '../utils/utils' and functions from '../../utils/utils'. If feasible, re-export the functions in v1/utils and use a single source to reduce drift.
Example (only if v1 utils can re-export the shared helpers):
-import { INACCESSIBLE_DIRECTIVE, SCHEMA_QUERY_DEFINITION, TAG_DIRECTIVE } from '../utils/utils'; -// ... -import { - federateSubgraphsFailure, - federateSubgraphsSuccess, - normalizeString, - normalizeSubgraphFailure, - normalizeSubgraphSuccess, - schemaToSortedNormalizedString, -} from '../../utils/utils'; +import { + INACCESSIBLE_DIRECTIVE, + SCHEMA_QUERY_DEFINITION, + TAG_DIRECTIVE, + federateSubgraphsFailure, + federateSubgraphsSuccess, + normalizeString, + normalizeSubgraphFailure, + normalizeSubgraphSuccess, + schemaToSortedNormalizedString, +} from '../utils/utils';
245-331: Reduce brittleness of deep Map/Set equality in error assertions.Strict equality over nested Map/Set relies on stable insertion/iteration order. If producers change ordering, tests may flake. Consider canonicalizing before compare (sort keys/convert Sets to sorted arrays) or matching structurally with partials.
Minimal pattern (conceptual):
// helpers (tests utils) const canon = (v: unknown): unknown => v instanceof Map ? Array.from(v.entries()).sort(([a], [b]) => String(a).localeCompare(String(b))).map(([k, val]) => [k, canon(val)]) : v instanceof Set ? Array.from(v.values()).sort() : Array.isArray(v) ? v.map(canon) : typeof v === 'object' && v !== null ? Object.fromEntries(Object.entries(v).sort(([a],[b]) => a.localeCompare(b)).map(([k,val])=>[k, canon(val)])) : v; // then: expect(canon(errors[0])).toStrictEqual(canon(expected0)); expect(canon(errors[1])).toStrictEqual(canon(expected1));Also applies to: 333-409
459-501: De‑duplicate order-variance tests with test.each.Both tests assert identical SDL with different subgraph order. Use parameterized cases to reduce duplication and ease future edits.
Apply:
-test('that an Interface type and extension definition federate successfully #1.1', () => { - const { federatedGraphSchema } = federateSubgraphsSuccess( - [subgraphQ, subgraphR, subgraphU], - ROUTER_COMPATIBILITY_VERSION_ONE, - ); - expect(schemaToSortedNormalizedString(federatedGraphSchema)).toBe( - normalizeString( - SCHEMA_QUERY_DEFINITION + - ` - interface Interface { - age: Int! - name: String! - } - - type Query { - dummy: String! - } - `, - ), - ); -}); - -test('that an Interface type and extension definition federate successfully #1.2', () => { - const { federatedGraphSchema } = federateSubgraphsSuccess( - [subgraphQ, subgraphU, subgraphR], - ROUTER_COMPATIBILITY_VERSION_ONE, - ); - expect(schemaToSortedNormalizedString(federatedGraphSchema)).toBe( - normalizeString( - SCHEMA_QUERY_DEFINITION + - ` - interface Interface { - age: Int! - name: String! - } - - type Query { - dummy: String! - } - `, - ), - ); -}); +test.each([ + [[subgraphQ, subgraphR, subgraphU], '#1.1'], + [[subgraphQ, subgraphU, subgraphR], '#1.2'], +])('that an Interface type and extension definition federate successfully %s', (order) => { + const { federatedGraphSchema } = federateSubgraphsSuccess(order, ROUTER_COMPATIBILITY_VERSION_ONE); + expect(schemaToSortedNormalizedString(federatedGraphSchema)).toBe( + normalizeString( + SCHEMA_QUERY_DEFINITION + + ` + interface Interface { + age: Int! + name: String! + } + + type Query { + dummy: String! + } + `, + ), + ); +});
431-437: Track the TODO with an issue or inline link.Convert the TODO into a tracked task (issue/ADR) so future changes to @inaccessible/entity interfaces don’t leave tests stale.
I can open a follow-up issue with a short checklist if helpful.
1657-1709: Rename new fixtures for clarity (or add brief comments).Identifiers naaaa/nbaaa are opaque. Prefer descriptive names (e.g., subgraphInvalidImplA/subgraphInvalidImplB) or add a comment above each block summarizing its role.
Apply (plus update references at Lines 245 and 333):
-const naaaa: Subgraph = { +const subgraphInvalidImplA: Subgraph = { name: 'naaaa', ... -const nbaaa: Subgraph = { +const subgraphInvalidImplB: Subgraph = { name: 'nbaaa',composition/tests/v1/events.test.ts (2)
157-196: Prefer using shared SDL constants to avoid driftInline input edfs__NatsStreamConfiguration is duplicated here. Use the shared fragment to reduce maintenance and ensure consistency.
Apply:
- expect(schemaToSortedNormalizedString(schema)).toBe( - normalizeString( - SCHEMA_ALL_ROOTS_DEFINITION + - EDFS_NATS_PUBLISH_DIRECTIVE + - EDFS_NATS_REQUEST_DIRECTIVE + - EDFS_NATS_SUBSCRIBE_DIRECTIVE + - EXTERNAL_DIRECTIVE + - KEY_DIRECTIVE + - ` + expect(schemaToSortedNormalizedString(schema)).toBe( + normalizeString( + SCHEMA_ALL_ROOTS_DEFINITION + + EDFS_NATS_PUBLISH_DIRECTIVE + + EDFS_NATS_REQUEST_DIRECTIVE + + EDFS_NATS_SUBSCRIBE_DIRECTIVE + + EXTERNAL_DIRECTIVE + + KEY_DIRECTIVE + + ` type Entity @key(fields: "id", resolvable: false) { id: ID! @external } type Mutation { updateEntity(id: ID!, name: String!): edfs__PublishResult! @edfs__natsPublish(subject: "updateEntity.{{ args.id }}") } type Query { findEntity(id: ID!): Entity! @edfs__natsRequest(subject: "findEntity.{{ args.id }}") } type Subscription { entitySubscription(id: ID!): Entity! @edfs__natsSubscribe(subjects: ["entities.{{ args.id }}"], providerId: "my-provider") entitySubscriptionTwo(firstID: ID!, secondID: ID!): Entity! @edfs__natsSubscribe(subjects: ["firstSub.{{ args.firstID }}", "secondSub.{{ args.secondID }}"], providerId: "double", streamConfiguration: {consumerName: "consumer", streamName: "streamName", consumerInactiveThreshold: 300}) } - input edfs__NatsStreamConfiguration { - consumerInactiveThreshold: Int! = 30 - consumerName: String! - streamName: String! - } - type edfs__PublishResult { success: Boolean! } scalar openfed__FieldSet `, + ) + EDFS_NATS_STREAM_CONFIGURATION_INPUT + OPENFED_FIELD_SET - ), + , );
1705-1718: Normalize subgraph name casing for consistencyThe name 'subgraph-aH' mixes cases while others are lowercase. Consider 'subgraph-ah' for consistency across fixtures. Behavior is unaffected but reduces noise in diffs/logs.
-const subgraphAH: Subgraph = { - name: 'subgraph-aH', +const subgraphAH: Subgraph = { + name: 'subgraph-ah',composition/tests/v1/directives/provides.test.ts (2)
341-387: Reduce brittleness in deep Map/Set equalityAsserting full ConditionalFieldData structures is fragile. Prefer partial shape checks with objectContaining/maps to focus on intent.
Example:
- expect(conditionalFieldDataByCoordinates).toStrictEqual(new Map([...full structure...])); + expect(conditionalFieldDataByCoordinates.get('NestedObject.age')).toMatchObject({ + providedBy: expect.arrayContaining([ + expect.objectContaining({ fieldPath: ['entity','object','nestedObject','age'] }), + expect.objectContaining({ fieldPath: ['entities','object','nestedObject','age'] }), + ]), + }); + expect(conditionalFieldDataByCoordinates.get('NestedObject.name')).toMatchObject({ + providedBy: expect.arrayContaining([ + expect.objectContaining({ fieldPath: ['entity','object','nestedObject','name'] }), + expect.objectContaining({ fieldPath: ['entities','object','nestedObject','name'] }), + ]), + });
479-511: Warning count assertions: be explicit about content, not just lengthVerifying warnings length followed by element equality can be simplified to checking for the specific warning presence, which is more robust.
- expect(warnings).toHaveLength(1); - expect(warnings[0]).toStrictEqual( + expect(warnings).toEqual([ nonExternalConditionalFieldWarning(`Query.entity`, o.name, `Entity.id`, `id`, PROVIDES), - ); + ]);composition/tests/v1/federation-factory.test.ts (2)
565-573: Avoid brittle size checks on directive definitionsChecking exact map size is fragile as new built-ins may appear. Assert presence of required directives instead.
-expect(subgraphAConfig!.directiveDefinitionByDirectiveName).toHaveLength(2); -expect(subgraphAConfig!.directiveDefinitionByDirectiveName.has(KEY)).toBe(true); -expect(subgraphAConfig!.directiveDefinitionByDirectiveName.has(SHAREABLE)).toBe(true); +expect(subgraphAConfig!.directiveDefinitionByDirectiveName.has(KEY)).toBe(true); +expect(subgraphAConfig!.directiveDefinitionByDirectiveName.has(SHAREABLE)).toBe(true);And similarly for subgraphB: assert 'a', KEY, SHAREABLE exist without asserting total size.
94-99: Keep SDL fragments centralizedGood use of SCHEMA_ALL_ROOTS_DEFINITION/AUTHENTICATED/REQUIRES_SCOPES. Consider centralizing OPENFED_SCOPE and any new directive defs in the same utils aggregator to prevent future drift.
composition/src/v1/normalization/utils.ts (1)
117-125: Make field-set normalization more robust to formattingslice(2, -2) assumes leading/trailing "{ " / " }". Consider trimming by locating outer braces to be resilient to printer changes.
-export function getNormalizedFieldSet(documentNode: DocumentNode): string { - return print(lexicographicallySortDocumentNode(documentNode)).replaceAll(/\s+/g, ' ').slice(2, -2); -} +export function getNormalizedFieldSet(documentNode: DocumentNode): string { + const s = print(lexicographicallySortDocumentNode(documentNode)).replaceAll(/\s+/g, ' ').trim(); + const start = s.indexOf('{'); + const end = s.lastIndexOf('}'); + return start >= 0 && end > start ? s.slice(start + 1, end).trim() : s; +}composition/src/v1/federation/utils.ts (1)
97-113: Minor: early-continue to reduce nestingYou can invert conditions to reduce nesting in validateImplicitFieldSets and keep the happy-path linear; functionality unchanged.
composition/tests/utils/utils.ts (1)
36-46: Improve debug output for failed normalizationMatch federation logging by adding depth for better visibility.
- if (!result.success) { - console.dir(result.errors); - } + if (!result.success) { + console.dir(result.errors, { depth: null }); + }composition/src/utils/utils.ts (1)
64-71: Consider aligning parameter order with the existing function.The new
addOptionalIterableValuesToSethas parameters ordered as(target, source?), while the existingaddIterableValuesToSet(lines 58-62) uses(source, target). This inconsistency may cause confusion when developers switch between the two similar functions.Apply this diff to align the parameter order:
-export function addOptionalIterableValuesToSet<T>(target: Set<T>, source?: Array<T> | Iterable<T>) { +export function addOptionalIterableValuesToSet<T>(source: Array<T> | Iterable<T> | undefined, target: Set<T>) { if (!source) { return; } for (const value of source) { target.add(value); } }composition/tests/v1/types/arguments.test.ts (2)
122-136: Minor: stray backticks in test titlesTitles like “that if arguments of the same name are not the same type, an error is returned`” have a trailing backtick. Drop it for consistency.
- test('that if arguments of the same name are not the same type, an error is returned`', () => { + test('that if arguments of the same name are not the same type, an error is returned', () => {
36-53: Consistent result handling style in testsMost tests now destructure outputs; a few still check result.success/fields directly. Consider standardizing on destructuring for readability.
Also applies to: 57-75, 78-96, 98-120, 185-205
composition/tests/v1/directives/fieldset-directives.test.ts (1)
430-446: LGTM; consider a small helper for Map/Set-heavy expectationsThe repeated new Map<TypeName, ConfigurationData>(...) patterns are verbose. A tiny expectConfig helper could reduce duplication, but current assertions are fine.
composition/tests/v1/entity-interface.test.ts (1)
15-15: Use a type-only import for TypeName.This is only used as a type. Prefer
import type { TypeName }to avoid emitting a runtime import.-import { +import type { TypeName, } from '../../src';composition/src/schema-building/utils.ts (2)
287-300: Harden upsertDeprecatedDirective for external callers.Now that it’s exported, guard against missing/non-string reasons to avoid runtime errors if used outside the normalized path.
export function upsertDeprecatedDirective( persistedDirectivesData: PersistedDirectivesData, incomingDirectiveNode: ConstDirectiveNode, ) { - if (!incomingDirectiveNode.arguments?.length) { - return; - } - // The argument was already validated in the normalization factory, so it can be safely cast - const incomingReasonString = (incomingDirectiveNode.arguments[0].value as StringValueNode).value; + const arg = incomingDirectiveNode.arguments?.find((a) => a.name.value === REASON); + if (!arg || arg.value.kind !== Kind.STRING) { + return; + } + const incomingReasonString = (arg.value as StringValueNode).value; if (persistedDirectivesData.deprecatedReason.length < incomingReasonString.length) { persistedDirectivesData.deprecatedReason = incomingReasonString; } }
301-310: Harden upsertTagDirectives for missing/invalid @tag(name: …).Add simple guards so exported usage can’t throw on malformed nodes.
export function upsertTagDirectives( persistedDirectivesData: PersistedDirectivesData, incomingDirectiveNodes: ConstDirectiveNode[], ) { for (const incomingDirectiveNode of incomingDirectiveNodes) { - // The argument was already validated in the normalization factory, so it can be safely cast - const incomingNameString = (incomingDirectiveNode.arguments![0].value as StringValueNode).value; - persistedDirectivesData.tagDirectiveByName.set(incomingNameString, incomingDirectiveNode); + const arg = incomingDirectiveNode.arguments?.find((a) => a.name.value === 'name'); + if (!arg || arg.value.kind !== Kind.STRING) { + continue; + } + const incomingNameString = (arg.value as StringValueNode).value; + persistedDirectivesData.tagDirectiveByName.set(incomingNameString, incomingDirectiveNode); } }composition/tests/v1/entities.test.ts (2)
14-15: Use a type-only import for TypeName.It’s only used in generics; prefer
import type { TypeName }to keep runtime clean.-import { +import type { TypeName, } from '../../src';
608-671: Reduce duplication with parameterized tests (optional).The #1.x and #2.x suites repeat the same assertions across order permutations. Consider
test.eachto table-drive inputs while keeping a single assertion block.// Example sketch: test.each([ { order: ['subgraph-u', 'subgraph-v'] }, { order: ['subgraph-v', 'subgraph-u'] }, ])('that if a target key can be satisfied, it will be included in the router configuration (%#)', ({ order }) => { const graphs = order.map((n) => ({ subgraphU, subgraphV } as any)[n]); const { federatedGraphSchema, subgraphConfigBySubgraphName } = federateSubgraphsSuccess(graphs, ROUTER_COMPATIBILITY_VERSION_ONE); // shared SDL and config assertions... });Also applies to: 673-736, 738-874, 876-1012, 1014-1150, 1152-1288, 1290-1426, 1428-1564
composition/tests/v1/directives/authorization-directives.test.ts (2)
61-76: Reduce SDL boilerplate with a tiny helper.Repeated
SCHEMA_QUERY_DEFINITION + [fragments] + bodyclutters tests. Extract a helper likewithBaseSDL(body, ...fragments)to assemble once and reuse across this suite. Keeps assertions terse and consistent.
1897-1911: Remove unused fixturesubgraphQor reference it in a test.Dead fixtures slow readers and risk drift. If not used, drop it.
composition/tests/v1/directives/external.test.ts (2)
342-345: Extract a base-SDL builder to cut repetition.You repeatedly do
SCHEMA_QUERY_DEFINITION + \...`. AwithBaseSDL(body, ...fragments)` helper used across directive tests will reduce duplication and inconsistent whitespace.
236-275: TypeName is currently juststring; suggestion is optional/defensive.TypeName is defined as
export type TypeName = string;in bothcomposition/src/resolvability-graph/types/types.tsandcomposition/src/types/types.ts. String literals like'Entity'are valid and will type‑check correctly as-is.The suggestion to add a helper (e.g.,
const tn = (s: string) => s as TypeName;) is a defensive pattern to future‑proof against potential TypeName changes (e.g., if it becomes a branded type), but it's not required for the current codebase.composition/tests/v1/directives/inaccessible.test.ts (1)
650-663: Unify success assertions with the new API.Elsewhere you destructure
federateSubgraphsSuccessresults; here you still checkresult.success. Prefer a consistent pattern (either destructure or explicitly assert.success) for readability.composition/tests/v1/directives/configure-description.test.ts (1)
85-91: Factor shared fragments into a helper.Combine
SCHEMA_QUERY_DEFINITIONandCONFIGURE_DESCRIPTION_DIRECTIVEvia a smallwithBaseSDLhelper to keep expectations short and uniform across tests.composition/tests/v1/directives/shareable.test.ts (2)
107-131: Parametrize permutation tests to reduce duplication.The six “functions with extensions correctly” tests differ only in subgraph order. Wrap the orders in a table and loop to cut repetition and maintenance cost.
115-118: Introduce base-SDL helper for federation expectations.Repeated
SCHEMA_QUERY_DEFINITION + \...`patterns can be centralized (e.g.,withBaseSDL`), improving consistency across directive suites.composition/src/v1/federation/federation-factory.ts (1)
141-169: Name shadowing: imported vs. methodgetNodeForRouterSchemaByDataThis file imports a function named
getNodeForRouterSchemaByDataand also defines a method with the same name. Calls withoutthis.resolve to the import, which is easy to misread.Suggestion: rename the class method to
getRouterNodeForData(or alias the import) to avoid ambiguity. Low risk readability win.Also applies to: 1834-1839
composition/src/v1/normalization/normalization-factory.ts (1)
3728-3753: Naming consistency: consider aligning result key with internal mapResult still exports directiveDefinitionByDirectiveName while the internal is directiveDefinitionByName. Keeping for BC is fine; consider aliasing/renaming in a future major to avoid confusion.
Confirm external consumers still rely on directiveDefinitionByDirectiveName before any rename.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/test/__snapshots__/router.config.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (50)
composition-go/composition_test.go(1 hunks)composition/src/index.ts(1 hunks)composition/src/normalization/types.ts(2 hunks)composition/src/schema-building/ast.ts(1 hunks)composition/src/schema-building/utils.ts(7 hunks)composition/src/subgraph/types.ts(1 hunks)composition/src/utils/utils.ts(1 hunks)composition/src/v1/constants/constants.ts(1 hunks)composition/src/v1/constants/directive-definitions.ts(9 hunks)composition/src/v1/constants/non-directive-definitions.ts(1 hunks)composition/src/v1/constants/strings.ts(1 hunks)composition/src/v1/constants/type-nodes.ts(1 hunks)composition/src/v1/federation/federation-factory.ts(25 hunks)composition/src/v1/federation/params.ts(1 hunks)composition/src/v1/federation/utils.ts(1 hunks)composition/src/v1/normalization/directive-definition-data.ts(1 hunks)composition/src/v1/normalization/normalization-factory.ts(22 hunks)composition/src/v1/normalization/types.ts(2 hunks)composition/src/v1/normalization/utils.ts(1 hunks)composition/src/v1/normalization/walkers.ts(7 hunks)composition/src/v1/utils/string-constants.ts(0 hunks)composition/src/v1/utils/utils.ts(1 hunks)composition/tests/utils/utils.ts(2 hunks)composition/tests/v1/contracts.test.ts(134 hunks)composition/tests/v1/directives/authorization-directives.test.ts(36 hunks)composition/tests/v1/directives/configure-description.test.ts(17 hunks)composition/tests/v1/directives/directives.test.ts(12 hunks)composition/tests/v1/directives/external.test.ts(34 hunks)composition/tests/v1/directives/fieldset-directives.test.ts(21 hunks)composition/tests/v1/directives/inaccessible.test.ts(30 hunks)composition/tests/v1/directives/interface-object.test.ts(14 hunks)composition/tests/v1/directives/one-of.test.ts(13 hunks)composition/tests/v1/directives/override.test.ts(32 hunks)composition/tests/v1/directives/provides.test.ts(32 hunks)composition/tests/v1/directives/require-fetch-reasons.test.ts(11 hunks)composition/tests/v1/directives/semantic-non-null.test.ts(22 hunks)composition/tests/v1/directives/shareable.test.ts(12 hunks)composition/tests/v1/directives/subscription-filter.test.ts(2 hunks)composition/tests/v1/directives/v2-directives.test.ts(3 hunks)composition/tests/v1/entities.test.ts(45 hunks)composition/tests/v1/entity-interface.test.ts(8 hunks)composition/tests/v1/events.test.ts(29 hunks)composition/tests/v1/federation-factory.test.ts(22 hunks)composition/tests/v1/normalization.test.ts(73 hunks)composition/tests/v1/resolvability.test.ts(38 hunks)composition/tests/v1/router-configuration.test.ts(12 hunks)composition/tests/v1/types/arguments.test.ts(8 hunks)composition/tests/v1/types/enums.test.ts(11 hunks)composition/tests/v1/types/inputs.test.ts(6 hunks)composition/tests/v1/types/interfaces.test.ts(29 hunks)
💤 Files with no reviewable changes (1)
- composition/src/v1/utils/string-constants.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/tests/v1/normalization.test.ts
🔇 Additional comments (50)
composition/tests/v1/directives/semantic-non-null.test.ts (1)
24-24: LGTM! Good refactoring to centralize test utilities.The consolidation of schema definition constants into reusable imports improves maintainability and consistency across tests.
composition/tests/v1/types/interfaces.test.ts (2)
33-35: LGTM: migration to normalizeSubgraphSuccess/Failure.Adoption of the higher-level helpers improves readability and reduces branching on result.success. Keep it consistent across the file.
773-802: Nice fix: varied subgraph order now truly differs across “#1.1”/“#1.2”.This resolves the earlier copy/paste oversight. Good.
composition/src/v1/utils/utils.ts (1)
33-33: LGTM! Clean import path updates.The import paths have been updated to reflect the reorganized constants structure, aligning with the broader refactoring to centralize v1 constants into dedicated modules.
Also applies to: 37-37
composition/src/subgraph/types.ts (1)
27-27: LGTM! Type strengthening with TypeName.The map key type has been narrowed from
stringtoTypeName, improving type safety and aligning with the broader pattern of usingTypeNamethroughout the codebase.composition/src/normalization/types.ts (1)
14-14: LGTM! Consistent type strengthening.The addition of the
TypeNameimport and the narrowing ofconfigurationDataByTypeName's key type fromstringtoTypeNameimproves type safety and maintains consistency with related interfaces likeInternalSubgraph.Also applies to: 26-26
composition/src/v1/constants/type-nodes.ts (1)
1-13: LGTM! Well-structured reusable type node constants.These constants centralize commonly used required scalar type nodes (
String!andopenfed__FieldSet!), promoting code reuse and consistency across the GraphQL type construction logic.composition/tests/v1/normalization.test.ts (3)
51-59: LGTM! Centralized test constants improve maintainability.Importing reusable directive and schema definition constants from the test utils reduces duplication and makes tests easier to maintain.
66-71: LGTM! Cleaner test patterns with destructuring.The refactoring to destructure
{ errors }or{ schema }directly from normalization results makes the tests more concise and readable, while maintaining explicit type assertions.Also applies to: 88-113
162-171: LGTM! Consistent use of SDL constants in test expectations.Using centralized constants like
KEY_DIRECTIVEandOPENFED_FIELD_SETin expected test outputs improves maintainability and ensures consistency across tests.Also applies to: 190-199
composition/tests/v1/directives/require-fetch-reasons.test.ts (2)
6-6: LGTM! Consolidated imports align with public API.The import of
parsefrom the public API and the use of centralized test constants (REQUEST_FETCH_REASONS_DIRECTIVE,SCHEMA_QUERY_DEFINITION) improve consistency and maintainability.Also applies to: 13-13
23-34: LGTM! Consistent use of schema definition constants.The consistent application of
SCHEMA_QUERY_DEFINITIONandREQUEST_FETCH_REASONS_DIRECTIVEthroughout test expectations ensures that schema definition changes only need to be made in one place.Also applies to: 67-83, 392-410
composition/tests/v1/types/enums.test.ts (2)
13-13: LGTM! Improved test utilities and constants.The imports of test helper functions (
normalizeSubgraphSuccess,normalizeSubgraphFailure) and centralized constants (INACCESSIBLE_DIRECTIVE,SCHEMA_QUERY_DEFINITION,TAG_DIRECTIVE) make tests more maintainable and easier to read.Also applies to: 18-21
26-36: LGTM! Cleaner test patterns with helper functions.Using
normalizeSubgraphSuccessandnormalizeSubgraphFailurehelpers with destructuring provides a cleaner and more consistent test pattern compared to checkingresult.successflags.Also applies to: 191-194
composition/src/v1/constants/non-directive-definitions.ts (3)
35-68: LGTM! Well-structured input object with default value.The
EDFS_NATS_STREAM_CONFIGURATION_DEFINITIONis properly constructed with three required fields, including a sensible default value forconsumerInactiveThreshold.
71-109: LGTM! Scalar and enum definitions are correct.The scalar type definitions (
FIELD_SET_SCALAR_DEFINITION,LINK_IMPORT_DEFINITION,SCOPE_SCALAR_DEFINITION) and theLINK_PURPOSE_DEFINITIONenum are properly structured with appropriate AST node kinds and values.
116-186: LGTM! Properly structured recursive filter definitions.The subscription filter definitions correctly model a recursive filtering structure:
SUBSCRIPTION_FIELD_CONDITION_DEFINITIONdefines field-level conditionsSUBSCRIPTION_FILTER_CONDITION_DEFINITIONallows composing conditions with AND/OR/NOT operators and correctly references itself for nested logicThe recursive structure is intentional and appropriate for building complex filter expressions.
composition/src/schema-building/ast.ts (1)
266-274: Verify public API status of old type name before adding backcompat aliasThe old type name
MutableTypeDefinitionNodehas no references anywhere in the codebase, including in generated.d.tsfiles or barrel exports. The backcompat alias is a defensive measure, but its necessity depends on whether this name was ever part of the public API in a previous release. Confirm the following before implementing:
- Was
MutableTypeDefinitionNodeexported as part of a public release?- Are there known external consumers using the old name?
- Is this PR renaming an existing public export, or introducing a new internal type?
If the old name was genuinely public, add the backcompat alias as suggested. If not, the alias may be unnecessary.
composition/src/v1/federation/params.ts (1)
12-15: LGTM!The new
ExtractPersistedDirectivesParamstype is well-defined and follows TypeScript best practices. The fields are appropriately typed and align with the centralized directive extraction flow mentioned in the PR objectives.composition/tests/v1/directives/interface-object.test.ts (2)
16-30: LGTM!The import updates align with the PR's objective to use public API exports and centralized schema definitions. The migration from direct GraphQL imports to the public API surface (
parse,TypeName,SCHEMA_QUERY_DEFINITION) improves consistency across the test suite.
93-93: LGTM!The type annotation update from
Map<string, ConfigurationData>toMap<TypeName, ConfigurationData>provides stronger typing and aligns with the PR's migration to TypeName-based keys.composition-go/composition_test.go (1)
73-77: LGTM!The SDL updates correctly reflect the new schema surface, including the introduction of the
openfed__Scopescalar and adjusted directive definitions. The test assertions remain intact and valid.composition/src/v1/normalization/types.ts (1)
80-89: LGTM!The discriminated union pattern for
UpsertInputObjectResultis a robust approach that enables type-safe error handling. Thesuccessflag allows TypeScript to narrow the type and provide compile-time safety when accessing thedatafield.composition/tests/v1/router-configuration.test.ts (1)
23-25: LGTM!The destructuring pattern simplifies test assertions and the
TypeName-based typing provides stronger type safety. These changes align well with the PR's migration to a more robust public API surface.composition/tests/v1/directives/v2-directives.test.ts (1)
10-10: LGTM!The import and usage of
SCHEMA_QUERY_DEFINITIONcentralizes schema fragment definitions and aligns with the broader test suite refactoring.composition/tests/v1/resolvability.test.ts (1)
20-20: LGTM!The import of centralized constants (
INACCESSIBLE_DIRECTIVE,SCHEMA_QUERY_DEFINITION) improves consistency across the test suite and aligns with the PR's migration to a unified public API surface.composition/tests/v1/directives/one-of.test.ts (1)
78-86: Comment/expectation alignment looks good“directive definition” wording now matches the preserved SDL usage of @OneOf. LGTM.
composition/tests/v1/contracts.test.ts (1)
1068-1076: Centralizing SDL fragments via SCHEMA_QUERY_DEFINITION/TAG/INACCESSIBLE is cleanThe refactor improves readability and consistency across tests. LGTM.
composition/tests/v1/directives/subscription-filter.test.ts (2)
60-87: Normalization expectations for injected OPENFED types look correctSwitch to normalizeSubgraphSuccess/Failure + OPENFED_* fragments is consistent and clear. LGTM.
456-465: EDG extension scenarioThe federated schema expectation for entity extension in EDG matches the intended shape. LGTM.
composition/src/index.ts (1)
29-35: No re-export collisions detectedVerification confirms that
composition/src/utils/string-constants.ts(164 exports) andcomposition/src/v1/constants/strings.ts(9 exports) have zero overlapping identifiers. The two modules export completely distinct symbol names, so no duplicate export errors or ambiguous surfaces will occur.Likely an incorrect or invalid review comment.
composition/tests/v1/entity-interface.test.ts (2)
31-32: Good move to the new destructured federation result.The
{ federatedGraphSchema }shape aligns with the updated API and keeps assertions clear.
88-110: Typed configuration maps look correct.Using
Map<TypeName, ConfigurationData>with explicit Sets/keys matches the new config surface. No issues spotted.Also applies to: 112-141
composition/src/schema-building/utils.ts (2)
749-751: Nice export for runtime narrowing.
isInputObjectDefinitionDataimproves type guards downstream; LGTM.
523-540: Broadened parameter type can break callers; add overloads or verify all call sites.Changing to
(MutableDefinitionNode | DefinitionNode)[]means aMutableDefinitionNode[]can no longer be passed safely (the callee pushes a possibly wider element). Either add overloads or confirm all callers are updated to the union.Optionally add overloads to maintain source compatibility:
-export function addValidPersistedDirectiveDefinitionNodeByData( - definitions: (MutableDefinitionNode | DefinitionNode)[], +export function addValidPersistedDirectiveDefinitionNodeByData( + definitions: MutableDefinitionNode[], + data: PersistedDirectiveDefinitionData, + persistedDirectiveDefinitionByDirectiveName: Map<DirectiveName, DirectiveDefinitionNode>, + errors: Error[], +): void; +export function addValidPersistedDirectiveDefinitionNodeByData( + definitions: (MutableDefinitionNode | DefinitionNode)[], data: PersistedDirectiveDefinitionData, persistedDirectiveDefinitionByDirectiveName: Map<DirectiveName, DirectiveDefinitionNode>, errors: Error[], -) { +): void { const argumentNodes: MutableInputValueNode[] = []; if (!addValidatedArgumentNodes(argumentNodes, data, persistedDirectiveDefinitionByDirectiveName, errors)) { return; } definitions.push({ arguments: argumentNodes, kind: Kind.DIRECTIVE_DEFINITION, locations: setToNameNodeArray(data.executableLocations), name: stringToNameNode(data.name), repeatable: data.repeatable, description: data.description, }); }composition/tests/v1/types/inputs.test.ts (3)
32-33: Helper-based normalization reads cleaner.
normalizeSubgraphSuccess(...).schemamakes intent explicit and reduces casting noise.
102-117: Correct directive definition prefixing.Using
TAG_DIRECTIVEin normalization outputs ensures SDL validity without overexposing router fragments.Also applies to: 126-137, 140-152
268-270: Right place to include SCHEMA_QUERY_DEFINITION.Federated SDL expectations should be prefixed; this aligns with other suites.
composition/tests/v1/directives/directives.test.ts (3)
14-15: Public test surface trimmed appropriately.Importing
INACCESSIBLE_DIRECTIVEandSCHEMA_QUERY_DEFINITIONkeeps the suite decoupled from internal constants.
153-155: Correctly include @inaccessible definition in expectations.Prepending the directive definition avoids false negatives from undefined directives.
350-351: Router schema fragment inclusion is appropriate here.Federated output should include
SCHEMA_QUERY_DEFINITION; assertion matches.composition/tests/v1/entities.test.ts (1)
51-56: Destructured results + router schema prefix are consistent.
{ federatedGraphSchema }andSCHEMA_QUERY_DEFINITIONusage are aligned with the new API.Also applies to: 55-81
composition/tests/v1/directives/shareable.test.ts (1)
71-86: Nice: no duplicate @Shareable at field level.Expectation matches the intended behavior precisely.
composition/tests/v1/directives/authorization-directives.test.ts (1)
480-488: The review comment is incorrect. The test code is correct as written.The test "that merged scopes remove any superfluous scopes #3" properly validates the normalization function:
- SDL input (line 500):
[["a", "b"], ["a"]]— the unmerged source data- Data output (line 488):
[["a"]]— the expected result after removing superfluous setsThis is the correct test pattern: input with redundant scopes → output with merged/reduced scopes. Changing the SDL to match the expected output would break the merge validation entirely, reducing the test to a trivial pass-through check.
Likely an incorrect or invalid review comment.
composition/tests/v1/directives/override.test.ts (1)
21-31: Test API shifts look correctSwitch to
TypeNamekeys and new{ errors }/destructured federation results reads well and matches the new public surface.Also applies to: 36-43
composition/src/v1/federation/federation-factory.ts (1)
2888-2931: Node 22+ already enforced in engines.node—review concern is mitigatedThe code does use
isDisjointFrom(a Stage-3 Set method) at lines 2888, 2917, 2949, 2996, 3013, 3046, and 3086. However, yourpackage.jsonalready specifiesengines.node: ">=22.11.0", which means the runtime compatibility risk identified in the review is already addressed. No changes are required.The review comment appears to have been written without checking the existing engine constraint, or before it was added.
composition/src/v1/constants/directive-definitions.ts (1)
226-249: Remove review comment — GraphQL spec allows default values on Non-Null directive argumentsDirective argument definitions may be Non-Null and still provide a default value. If a Non-Null argument definition has a defaultValue, it is treated as optional (the default is used when the argument is omitted). The current code defining
providerId: String! = "default"across all EDFS directives is valid per the GraphQL specification and requires no changes.Likely an incorrect or invalid review comment.
composition/src/v1/normalization/normalization-factory.ts (3)
1401-1451: Return-type change for upsertInputObjectByNode is clean and used correctlyThe UpsertInputObjectResult with success/data simplifies callers and is handled properly in EDGS injection. LGTM.
747-791: Switch to directiveDefinitionDataByName in validation looks correctValidation now consults the ByName map; consistent with initializeDirectiveDefinitionDatas(). Good change.
2938-3010: Ensure NATS stream config type is emitted via dependencies (skip in main loop)Confirmed: DEPENDENCIES_BY_DIRECTIVE_NAME includes EDFS_NATS_SUBSCRIBE -> EDFS_NATS_STREAM_CONFIGURATION_DEFINITION (composition/src/v1/constants/strings.ts), and normalization-factory imports/uses that map to add the input type dependency. No change required.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Checklist