feat: add @oneOf composition support #2225
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughAdds support for the @OneOf directive across normalization and federation: new constants/definitions, validation that input-object oneOf fields are all optional, error and warning factories, directive registration/emission, restored directive-definition datas, and accompanying tests and test helpers. 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
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-08T20:57:07.946ZApplied to files:
🧬 Code graph analysis (4)composition/src/v1/normalization/params.ts (3)
composition/src/v1/federation/params.ts (3)
composition/src/v1/federation/federation-factory.ts (8)
composition/src/v1/normalization/normalization-factory.ts (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (9)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Router-nonroot 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: 3
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/normalization-factory.ts (1)
3479-3531: Address switch declaration scope issues and improve code organization.The static analysis correctly identifies that variables declared in switch cases can be accessed by other cases. Additionally, the @OneOf validation logic would benefit from extraction into a dedicated method.
Apply this diff to fix the scope issues and improve maintainability:
case Kind.INPUT_OBJECT_TYPE_DEFINITION: + { const fieldCount = parentData.inputValueDataByName.size; if (fieldCount < 1) { this.errors.push(noInputValueDefinitionsError(parentTypeName)); break; } const definesOneOf = parentData.directivesByDirectiveName.has(ONE_OF); const requiredFieldNames = new Set<FieldName>(); for (const valueData of parentData.inputValueDataByName.values()) { if (isTypeRequired(valueData.type)) { requiredFieldNames.add(valueData.name); } // Base Scalars have already been set if (valueData.namedTypeKind !== Kind.NULL) { continue; } const namedTypeData = this.parentDefinitionDataByTypeName.get(valueData.namedTypeName); if (!namedTypeData) { // undefined types are handled elsewhere continue; } if (!isInputNodeKind(namedTypeData.kind)) { this.errors.push( invalidNamedTypeError({ data: valueData, namedTypeData, nodeType: `${kindToNodeType(parentData.kind)} field`, }), ); continue; } valueData.namedTypeKind = namedTypeData.kind; } if (definesOneOf) { if (requiredFieldNames.size > 0) { this.errors.push( oneOfRequiredFieldsError({ requiredFieldNames: Array.from(requiredFieldNames), typeName: parentTypeName, }), ); break; } if (fieldCount === 1) { this.warnings.push( singleSubgraphInputFieldOneOfWarning({ fieldName: getFirstEntry(parentData.inputValueDataByName)?.name ?? 'unknown', subgraphName: this.subgraphName, typeName: parentTypeName, }), ); } } definitions.push(this.getInputObjectNodeByData(parentData)); break; + }Consider extracting the @OneOf validation logic to a dedicated method for better organization:
private validateOneOfDirective( parentData: InputObjectDefinitionData, parentTypeName: string, fieldCount: number, requiredFieldNames: Set<FieldName> ): void { if (requiredFieldNames.size > 0) { this.errors.push( oneOfRequiredFieldsError({ requiredFieldNames: Array.from(requiredFieldNames), typeName: parentTypeName, }), ); return; } if (fieldCount === 1) { this.warnings.push( singleSubgraphInputFieldOneOfWarning({ fieldName: getFirstEntry(parentData.inputValueDataByName)?.name ?? 'unknown', subgraphName: this.subgraphName, typeName: parentTypeName, }), ); } }composition/src/v1/federation/federation-factory.ts (3)
1485-1489: Bug: impossible condition (size < 0) prevents error path.
interfaceDataByTypeName.size < 0can never be true; it should be< 1. This can mask incompatible named type errors.Apply:
- if (interfaceDataByTypeName.size < 0 && !unionTypeName) { + if (interfaceDataByTypeName.size < 1 && !unionTypeName) { this.errors.push(incompatibleFederatedFieldNamedTypeError(fieldCoordinates, subgraphNamesByNamedTypeName)); continue; }
1949-2026: Fix switch-case declarations scope (Biome noSwitchDeclarations) and add missing @OneOf default-value validation.
- Declarations inside
case Kind.INPUT_OBJECT_TYPE_DEFINITION:should be wrapped in a block to avoid leaking across cases.- Spec requires oneOf input fields to be nullable and have no default value; code enforces nullability but not “no default value”. Add a check and error out when any field has a default.
Apply:
- case Kind.INPUT_OBJECT_TYPE_DEFINITION: - const invalidRequiredInputs = new Array<InvalidRequiredInputValueData>(); - const inputValueNodes = new Array<MutableInputValueNode>(); - const clientInputValueNodes = new Array<MutableInputValueNode>(); - const definesOneOf = parentDefinitionData.directivesByDirectiveName.has(ONE_OF); - const requiredFieldNames = new Set<FieldName>(); + case Kind.INPUT_OBJECT_TYPE_DEFINITION: { + const invalidRequiredInputs = new Array<InvalidRequiredInputValueData>(); + const inputValueNodes = new Array<MutableInputValueNode>(); + const clientInputValueNodes = new Array<MutableInputValueNode>(); + const definesOneOf = parentDefinitionData.directivesByDirectiveName.has(ONE_OF); + const requiredFieldNames = new Set<FieldName>(); + const defaultedFieldNames = new Set<FieldName>(); for (const [inputValueName, inputValueData] of parentDefinitionData.inputValueDataByName) { if (isTypeRequired(inputValueData.type)) { requiredFieldNames.add(inputValueName); } + if (inputValueData.defaultValue !== undefined) { + defaultedFieldNames.add(inputValueName); + } if (parentDefinitionData.subgraphNames.size === inputValueData.subgraphNames.size) { inputValueNodes.push(this.getNodeWithPersistedDirectivesByInputValueData(inputValueData)); if (isNodeDataInaccessible(inputValueData)) { continue; } clientInputValueNodes.push({ ...inputValueData.node, directives: getClientPersistedDirectiveNodes(inputValueData), }); } else if (isTypeRequired(inputValueData.type)) { invalidRequiredInputs.push({ inputValueName, missingSubgraphs: getEntriesNotInHashSet( parentDefinitionData.subgraphNames, inputValueData.subgraphNames, ), requiredSubgraphs: [...inputValueData.requiredSubgraphNames], }); } } if (invalidRequiredInputs.length > 0) { this.errors.push( invalidRequiredInputValueError(INPUT_OBJECT, parentTypeName, invalidRequiredInputs, false), ); - break; + break; } if (definesOneOf) { + if (defaultedFieldNames.size > 0) { + this.errors.push( + new Error( + `The "@oneOf" directive defined on Input Object "${parentTypeName}" is invalid because all Input fields must not define default values; however, the following field(s) define defaults: "` + + [...defaultedFieldNames].join('", "') + + `".`, + ), + ); + break; + } if (requiredFieldNames.size > 0) { this.errors.push( oneOfRequiredFieldsError({ requiredFieldNames: Array.from(requiredFieldNames), typeName: parentTypeName, }), ); break; } if (inputValueNodes.length === 1) { this.warnings.push( singleFederatedInputFieldOneOfWarning({ fieldName: inputValueNodes[0]!.name.value, typeName: parentTypeName, }), ); } } parentDefinitionData.node.fields = inputValueNodes; this.routerDefinitions.push(this.getNodeForRouterSchemaByData(parentDefinitionData)); if (isNodeDataInaccessible(parentDefinitionData)) { this.validateReferencesOfInaccessibleType(parentDefinitionData); break; } if (clientInputValueNodes.length < 1) { this.errors.push( allChildDefinitionsAreInaccessibleError( kindToNodeType(parentDefinitionData.kind), parentTypeName, 'Input field', ), ); break; } this.clientDefinitions.push({ ...parentDefinitionData.node, directives: getClientPersistedDirectiveNodes(parentDefinitionData), fields: clientInputValueNodes, }); - break; + break; + }Notes:
- Spec excerpt: “If the Input Object is a OneOf Input Object then: The type of the input field must be nullable. The input field must not have a default value.”
2957-2974: Logic bug: variable shadowing causes every argument to be marked @inaccessible.Inside
handleChildTagExclusionsyou shadow thetagNamesparameter withfor (const [argumentName, tagNames] ...), then compare the set to itself:!tagNames.isDisjointFrom(tagNames)is always true. This hides valid arguments.Apply:
- for (const [argumentName, tagNames] of childTagData.tagNamesByArgumentName) { + for (const [argumentName, argumentTagNames] of childTagData.tagNamesByArgumentName) { const inputValueData = getOrThrowError( fieldData.argumentDataByName, argumentName, `${fieldName}.argumentDataByArgumentName`, ); if (isNodeDataInaccessible(inputValueData)) { continue; } - if (!tagNames.isDisjointFrom(tagNames)) { + if (!tagNames.isDisjointFrom(argumentTagNames)) { getValueOrDefault( inputValueData.persistedDirectivesData.directivesByDirectiveName, INACCESSIBLE, () => [generateSimpleDirective(INACCESSIBLE)], ); this.inaccessibleCoords.add(inputValueData.federatedCoords); } }
🧹 Nitpick comments (8)
composition/tests/v1/utils/utils.ts (1)
37-45: Test helper for semanticNonNull — OK; consider dedupLooks good. Consider composing this from baseDirectiveDefinitions plus only the delta to reduce duplication across test helpers.
composition/tests/v1/directives/semantic-non-null.test.ts (1)
471-480: naf Subgraph fixture — OK; minor naming nitReadable as-is. Optionally rename to a more descriptive id (e.g., nn_success) for future maintainers.
composition/src/errors/types.ts (1)
33-36: Type for oneOf required fields — OK; prefer readonlyIf these params are not mutated, prefer ReadonlyArray for stronger typing.
-export type OneOfRequiredFieldsErrorParams = { - requiredFieldNames: Array<FieldName>; +export type OneOfRequiredFieldsErrorParams = { + requiredFieldNames: ReadonlyArray<FieldName>; typeName: TypeName; };composition/src/errors/errors.ts (1)
1666-1675: oneOfRequiredFieldsError helper — OK; edge‑case guard (optional)Message format/pluralization are consistent. If there’s any chance of an empty requiredFieldNames array, consider guarding to avoid
"".-export function oneOfRequiredFieldsError({ requiredFieldNames, typeName }: OneOfRequiredFieldsErrorParams): Error { +export function oneOfRequiredFieldsError({ requiredFieldNames, typeName }: OneOfRequiredFieldsErrorParams): Error { + const names = requiredFieldNames.length ? requiredFieldNames.join(QUOTATION_JOIN) : '<none>'; return new Error( - `The "@oneOf" directive defined on Input Object "${typeName}" is invalid because all Input fields must be` + + `The "@oneOf" directive defined on Input Object "${typeName}" is invalid because all Input fields must be` + ` optional (nullable); however, the following Input field` + (requiredFieldNames.length > 1 ? `s are` : ` is`) + - ` required (non-nullable): "` + - requiredFieldNames.join(QUOTATION_JOIN) + + ` required (non-nullable): "` + + names + `".`, ); }composition/src/v1/warnings/params.ts (1)
1-12: Warning param types — OK; prefer readonly fieldsIf immutable, mark fields as readonly to better express intent.
-export type SingleSubgraphInputFieldOneOfWarningParams = { - fieldName: FieldName; - subgraphName: SubgraphName; - typeName: TypeName; -}; +export type SingleSubgraphInputFieldOneOfWarningParams = { + readonly fieldName: FieldName; + readonly subgraphName: SubgraphName; + readonly typeName: TypeName; +}; -export type SingleFederatedInputFieldOneOfWarningParams = { - fieldName: FieldName; - typeName: TypeName; -}; +export type SingleFederatedInputFieldOneOfWarningParams = { + readonly fieldName: FieldName; + readonly typeName: TypeName; +};composition/src/v1/warnings/warnings.ts (1)
190-203: Minor: Missing return type annotation.Consider adding an explicit return type for consistency with the other warning functions in this file.
export function singleFederatedInputFieldOneOfWarning({ fieldName, typeName, -}: SingleFederatedInputFieldOneOfWarningParams) { +}: SingleFederatedInputFieldOneOfWarningParams): Warning {composition/tests/v1/directives/one-of.test.ts (1)
550-555: Fix inconsistent indentation in test fixture.The indentation in the
fbacfixture is inconsistent with the rest of the test file.definitions: parse(` - input Input { - a: ID! - b: Boolean - } -`), + input Input { + a: ID! + b: Boolean + } + `),composition/src/v1/normalization/normalization-factory.ts (1)
3525-3525: Potential null handling improvement for field name extraction.Using the nullish coalescing operator with a fallback to 'unknown' is good defensive programming. However, consider logging when this fallback occurs for debugging purposes.
- fieldName: getFirstEntry(parentData.inputValueDataByName)?.name ?? 'unknown', + fieldName: (() => { + const firstEntry = getFirstEntry(parentData.inputValueDataByName); + if (!firstEntry?.name) { + console.warn(`Unable to retrieve field name for @oneOf on ${parentTypeName}`); + return 'unknown'; + } + return firstEntry.name; + })(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
composition/src/errors/errors.ts(2 hunks)composition/src/errors/types.ts(2 hunks)composition/src/utils/string-constants.ts(3 hunks)composition/src/v1/federation/federation-factory.ts(8 hunks)composition/src/v1/normalization/directive-definition-data.ts(5 hunks)composition/src/v1/normalization/normalization-factory.ts(8 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/src/v1/utils/constants.ts(3 hunks)composition/src/v1/warnings/params.ts(1 hunks)composition/src/v1/warnings/warnings.ts(2 hunks)composition/tests/v1/directives/external.test.ts(0 hunks)composition/tests/v1/directives/one-of.test.ts(1 hunks)composition/tests/v1/directives/semantic-non-null.test.ts(3 hunks)composition/tests/v1/router-configuration.test.ts(0 hunks)composition/tests/v1/utils/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- composition/tests/v1/router-configuration.test.ts
- composition/tests/v1/directives/external.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/tests/v1/directives/semantic-non-null.test.ts
🧬 Code graph analysis (12)
composition/src/v1/warnings/params.ts (1)
composition/src/types/types.ts (3)
FieldName(5-5)SubgraphName(9-9)TypeName(11-11)
composition/src/errors/types.ts (1)
composition/src/types/types.ts (2)
FieldName(5-5)TypeName(11-11)
composition/tests/v1/directives/one-of.test.ts (7)
composition/tests/utils/utils.ts (6)
normalizeSubgraphFailure(31-38)normalizeSubgraphSuccess(40-47)schemaToSortedNormalizedString(27-29)normalizeString(19-21)federateSubgraphsSuccess(59-72)federateSubgraphsFailure(49-57)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/errors/errors.ts (1)
oneOfRequiredFieldsError(1666-1675)composition/src/utils/string-constants.ts (1)
INPUT(68-68)composition/tests/v1/utils/utils.ts (3)
baseDirectiveDefinitions(18-25)versionOneRouterDefinitions(118-118)schemaQueryDefinition(94-97)composition/src/v1/warnings/warnings.ts (2)
singleSubgraphInputFieldOneOfWarning(174-188)singleFederatedInputFieldOneOfWarning(190-204)composition/src/ast/utils.ts (1)
parse(272-274)
composition/src/utils/string-constants.ts (1)
composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/src/errors/errors.ts (2)
composition/src/errors/types.ts (1)
OneOfRequiredFieldsErrorParams(33-36)composition/src/utils/string-constants.ts (1)
QUOTATION_JOIN(117-117)
composition/src/v1/warnings/warnings.ts (2)
composition/src/v1/warnings/params.ts (2)
SingleSubgraphInputFieldOneOfWarningParams(3-7)SingleFederatedInputFieldOneOfWarningParams(9-12)composition/src/warnings/types.ts (1)
Warning(9-17)
composition/src/v1/normalization/utils.ts (2)
composition/src/utils/string-constants.ts (1)
ONE_OF(102-102)composition/src/v1/normalization/directive-definition-data.ts (1)
ONE_OF_DEFINITION_DATA(450-458)
composition/src/v1/utils/constants.ts (2)
composition/src/utils/string-constants.ts (2)
ONE_OF(102-102)INPUT_OBJECT_UPPER(72-72)composition/src/ast/utils.ts (2)
stringArrayToNameNodeArray(84-90)stringToNameNode(77-82)
composition/tests/v1/directives/semantic-non-null.test.ts (4)
composition/tests/utils/utils.ts (3)
normalizeSubgraphSuccess(40-47)schemaToSortedNormalizedString(27-29)normalizeString(19-21)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/tests/v1/utils/utils.ts (2)
schemaQueryDefinition(94-97)baseDirectiveDefinitionsWithSemanticNonNull(37-45)composition/src/ast/utils.ts (1)
parse(272-274)
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 (14)
INPUT_OBJECT_UPPER(72-72)ONE_OF(102-102)CHANNEL(12-12)PROVIDER_ID(112-112)DEFAULT_EDFS_PROVIDER_ID(21-21)FIELD_DEFINITION_UPPER(54-54)EDFS_REDIS_PUBLISH(36-36)CHANNELS(13-13)EDFS_REDIS_SUBSCRIBE(37-37)OBJECT_UPPER(105-105)REQUIRE_FETCH_REASONS(120-120)LEVELS(80-80)INT_SCALAR(74-74)SEMANTIC_NON_NULL(132-132)composition/src/v1/utils/constants.ts (6)
ONE_OF_DEFINITION(650-655)REQUIRED_STRING_TYPE_NODE(107-110)EDFS_REDIS_PUBLISH_DEFINITION(427-448)EDFS_REDIS_SUBSCRIBE_DEFINITION(451-478)REQUIRE_FETCH_REASONS_DEFINITION(676-681)SEMANTIC_NON_NULL_DEFINITION(721-752)composition/src/ast/utils.ts (1)
stringToNamedTypeNode(100-105)
composition/src/v1/federation/federation-factory.ts (8)
composition/src/utils/string-constants.ts (1)
ONE_OF(102-102)composition/src/v1/utils/constants.ts (1)
ONE_OF_DEFINITION(650-655)composition/src/utils/types.ts (1)
InvalidRequiredInputValueData(33-37)composition/src/schema-building/ast.ts (1)
MutableInputValueNode(108-115)composition/src/types/types.ts (1)
FieldName(5-5)composition/src/schema-building/utils.ts (1)
isTypeRequired(124-126)composition/src/errors/errors.ts (1)
oneOfRequiredFieldsError(1666-1675)composition/src/v1/warnings/warnings.ts (1)
singleFederatedInputFieldOneOfWarning(190-204)
composition/src/v1/normalization/normalization-factory.ts (7)
composition/src/utils/string-constants.ts (2)
ONE_OF(102-102)SEMANTIC_NON_NULL(132-132)composition/src/v1/utils/constants.ts (2)
ONE_OF_DEFINITION(650-655)SEMANTIC_NON_NULL_DEFINITION(721-752)composition/src/errors/errors.ts (1)
oneOfRequiredFieldsError(1666-1675)composition/src/types/types.ts (1)
FieldName(5-5)composition/src/schema-building/utils.ts (1)
isTypeRequired(124-126)composition/src/v1/warnings/warnings.ts (1)
singleSubgraphInputFieldOneOfWarning(174-188)composition/src/utils/utils.ts (1)
getFirstEntry(251-257)
🪛 Biome (2.1.2)
composition/src/v1/federation/federation-factory.ts
[error] 1950-1950: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1951-1951: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1952-1952: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1953-1953: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1954-1954: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
composition/src/v1/normalization/normalization-factory.ts
[error] 3480-3480: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 3485-3485: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 3486-3486: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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). (3)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
🔇 Additional comments (25)
composition/src/utils/string-constants.ts (3)
102-102: Add ONE_OF constant — OKName matches spec and usage elsewhere.
190-190: Mark @OneOf as non‑repeatable — OKThis aligns with its spec semantics and repo’s expectations.
172-172: Persist @OneOf to clients — verifiedONE_OF is in PERSISTED_CLIENT_DIRECTIVES and is registered where persisted directive definitions are assembled (composition/src/utils/string-constants.ts:172; composition/src/schema-building/utils.ts — getClientPersistedDirectiveNodes; composition/src/v1/federation/federation-factory.ts — persistedDirectiveDefinitionByDirectiveName includes ONE_OF_DEFINITION).
composition/tests/v1/directives/semantic-non-null.test.ts (3)
21-21: Import normalizeSubgraphSuccess — OKImport is used; no issues.
25-25: Import test directive block — OKMatches usage in expectations.
104-119: Happy‑path normalization assertion — OKStable, uses normalized/lexicographic helpers; assertion is deterministic.
composition/src/errors/types.ts (1)
2-2: New imports — OKcomposition/src/errors/errors.ts (1)
12-12: Import OneOfRequiredFieldsErrorParams — OKcomposition/src/v1/normalization/utils.ts (3)
40-41: Import ONE_OF_DEFINITION_DATA — OK
74-75: Import ONE_OF — OK
411-411: Register @OneOf in directive definition datas — definition not found; verify location/repeatabilityrg returned no matches for ONE_OF_DEFINITION_DATA / name: ONE_OF / locations: INPUT_OBJECT / isRepeatable: false. Either point to the file/path where ONE_OF_DEFINITION_DATA is defined or confirm the directive is declared with locations: [INPUT_OBJECT] and isRepeatable: false.
composition/src/v1/utils/constants.ts (2)
71-71: Import ONE_OF — OK
517-517: Expose ONE_OF in built‑ins — OKKeeps name discoverable across flows.
composition/src/v1/warnings/warnings.ts (1)
173-187: LGTM! Clear and actionable warning messages.The warning function correctly guides users to remove @OneOf when only a single optional field is present, suggesting to make the field required instead.
composition/tests/v1/directives/one-of.test.ts (2)
21-102: Comprehensive test coverage for @OneOf normalization.The tests effectively validate error scenarios for required fields and warning conditions for single-field @OneOf definitions.
346-364: Good test coverage for cross-subgraph @OneOf validation.The tests correctly verify that @OneOf constraints are enforced across federated subgraphs.
composition/src/v1/normalization/normalization-factory.ts (2)
68-71: Proper integration of new directive definitions.The @OneOf and @semanticNonNull directive definitions are correctly imported and will be added to the schema when referenced.
3438-3452: Good conditional loading of directive definitions.The code correctly adds directive definitions only when they are actually referenced in the schema, avoiding unnecessary bloat.
composition/src/v1/federation/federation-factory.ts (4)
273-274: Registering ONE_OF in persisted directives looks good.Adding [ONE_OF, ONE_OF_DEFINITION] ensures @OneOf instances are carried through during federation. No issues spotted here.
2013-2016: Message text tweak LGTM.“Input field” capitalization aligns with surrounding messages.
1738-1761: Repeatable directive validation flows are correct.Flattening persisted directives and guarding non‑repeatables via the stored definition is the right approach; it will honor @OneOf’s non‑repeatability once the constant is fixed.
269-279: ONE_OF omission is safe — graphql v16.9.0 provides @OneOfpackage.json shows graphql overridden to 16.9.0, and graphql-js v16.9.0 implemented the built‑in @OneOf directive, so leaving ONE_OF out of persistedDirectiveDefinitions will not remove @OneOf at runtime/router SDL. (github.com)
If you need to support older graphql versions or want the router SDL to always embed the directive definition, add ONE_OF to persistedDirectiveDefinitions in composition/src/v1/federation/federation-factory.ts (lines ~269–279).
composition/src/v1/normalization/directive-definition-data.ts (3)
692-726: SEMANTIC_NON_NULL data restoration LGTM.Levels type/default align with the corresponding definition; non‑repeatable on FIELD_DEFINITION is consistent.
585-647: Restored EDFS Redis and RequireFetchReasons directive datas look consistent.Argument shapes, defaults, locations, and repeatability match the imported SDL definitions.
Also applies to: 649-657
21-21: Imports updated correctly for ONE_OF.No issues with the added imports.
Also applies to: 81-81
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist