fix: support __typename in field sets #2348
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughRefactors error helpers to accept typed object parameters, adds LITERAL_PERIOD and TYPENAME constants (replacing PERIOD), introduces typeNameAlreadyProvidedErrorMessage and HandleNonExternalConditionalFieldParams, centralizes non-external conditional-field handling, updates path parsing to use LITERAL_PERIOD, and expands tests for __typename scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composition/tests/v1/directives/provides.test.ts (1)
440-479: Fix theisTypeNamecondition to explicitly check for__typenameThe bug in
composition/src/errors/errors.tsat line 1490 is confirmed. The lineconst isTypeName = segments[segments.length - 1];assigns the last segment string (always truthy) rather than checking if it equals__typename. This causes both__typenameand regular fields likeNestedObject.ageto incorrectly trigger the__typename-specific error message.Change line 1490 to explicitly compare against the
TYPENAMEconstant:const isTypeName = segments[segments.length - 1] === TYPENAME;You'll need to import
TYPENAMEfrom../utils/string-constantsif not already imported.
🧹 Nitpick comments (3)
composition/src/v1/normalization/utils.ts (1)
76-77:__typenamehandling and period constant usage in key-field validation
- Importing and using
LITERAL_PERIODforcurrentPath.join(...)and related joins is consistent with other modules and keeps the delimiter centralized.- In
validateKeyFieldSets:
- Skipping fields where
fieldName === TYPENAMEavoids looking up synthetic__typenameinfieldDataByNameand allows mixed keys likeid __typenameto validate purely on real fields.- The new
SelectionSetbranch forlastFieldName === TYPENAMEcorrectly reports a non-compositeStringscalar usinginvalidSelectionSetDefinitionErrorMessage, without relying on schema metadata.- One behavioral change worth double-checking: a key field set that consists only of
__typenamewill now:
- Be traversed without any errors (the field is ignored in
Field.enter), and- Still result in a
RequiredFieldConfigurationbeing pushed for thatfieldSet.If the intention is to allow
__typenameto appear in key field sets but still require at least one non-__typenamefield to form an actual key, you may want to enforce thatkeyFieldSetPaths(or some non-typename counter) is non-empty before adding a configuration.Also applies to: 87-91, 145-145, 208-210, 244-245, 272-273, 288-297
composition/src/v1/normalization/normalization-factory.ts (2)
1680-1710: Centralized non-external conditional-field handling is soundThe new private
#handleNonExternalConditionalFieldcleanly centralizes the V1 vs V2 behavior for non-external conditional fields:
- In V2 (
isSubgraphVersionTwo === true), it appends anonExternalConditionalFieldErrorwithdirectiveCoords,directiveName,fieldSet,subgraphName, andtargetCoords: currentFieldCoords.- In V1, it appends a
nonExternalConditionalFieldWarningwith the same coordinates and field set, matching the behavior already asserted in the tests.Both scalar/enum leaf nodes without external ancestors and bare
__typenamein a field set now delegate to this helper, which makes the behavior consistent across provides/requires and reduces duplication invalidateConditionalFieldSet. The call sites also correctly avoid invoking this when there is at least one external ancestor, which is important for nested__typenameunder external parents.Also applies to: 1813-1820
2052-2109: Typed accumulators invalidateProvidesOrRequiresimprove clarity without changing behaviorChanging
allErrorMessagesandconfigurationsto explicitly typedArray<string>andArray<RequiredFieldConfiguration>is a small but useful clarity improvement. The method still:
- Aggregates per-field error message strings and, when any are present, pushes a single
invalidProvidesOrRequiresDirectivesError(...)ontothis.errors.- Returns
undefinedwhen there are no configurations and no errors, preserving the “redundant directive” behavior relied on by tests where all fields are effectively unconditionally provided.No functional issues here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
composition/src/errors/errors.ts(5 hunks)composition/src/errors/types.ts(2 hunks)composition/src/utils/string-constants.ts(2 hunks)composition/src/v1/federation/federation-factory.ts(6 hunks)composition/src/v1/federation/utils.ts(2 hunks)composition/src/v1/normalization/normalization-factory.ts(8 hunks)composition/src/v1/normalization/params.ts(1 hunks)composition/src/v1/normalization/utils.ts(7 hunks)composition/tests/v1/directives/fieldset-directives.test.ts(9 hunks)composition/tests/v1/directives/provides.test.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 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/provides.test.tscomposition/src/v1/federation/federation-factory.tscomposition/tests/v1/directives/fieldset-directives.test.tscomposition/src/v1/normalization/normalization-factory.ts
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
composition/tests/v1/directives/provides.test.ts
📚 Learning: 2025-10-17T16:35:24.723Z
Learnt from: Aenimus
Repo: wundergraph/cosmo PR: 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`.
Applied to files:
composition/tests/v1/directives/provides.test.tscomposition/tests/v1/directives/fieldset-directives.test.ts
📚 Learning: 2025-09-03T21:12:18.149Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: cli/src/commands/subgraph/commands/link.ts:21-28
Timestamp: 2025-09-03T21:12:18.149Z
Learning: Subgraph names in the Cosmo platform can contain slashes, so when parsing formats like `<namespace>/<subgraph-name>`, only the first slash should be treated as the delimiter between namespace and subgraph name. The subgraph name portion can contain additional slashes.
Applied to files:
composition/tests/v1/directives/provides.test.ts
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
composition/tests/v1/directives/fieldset-directives.test.tscomposition/src/v1/normalization/normalization-factory.ts
🧬 Code graph analysis (9)
composition/src/v1/federation/utils.ts (1)
composition/src/utils/string-constants.ts (1)
TYPENAME(156-156)
composition/src/errors/types.ts (1)
composition/src/types/types.ts (2)
TypeName(11-11)SubgraphName(9-9)
composition/src/v1/normalization/params.ts (2)
composition/src/v1/normalization/normalization-factory.ts (1)
HandleNonExternalConditionalFieldParams(1680-1710)composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/tests/v1/directives/provides.test.ts (6)
composition/src/errors/errors.ts (4)
nonExternalConditionalFieldError(1482-1500)invalidProvidesOrRequiresDirectivesError(667-674)incompatibleTypeWithProvidesErrorMessage(690-699)typeNameAlreadyProvidedErrorMessage(1600-1605)composition/src/utils/string-constants.ts (2)
PROVIDES(115-115)ID_SCALAR(64-64)composition/tests/utils/utils.ts (1)
normalizeSubgraphFailure(27-34)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/subgraph/types.ts (1)
Subgraph(18-22)composition/src/ast/utils.ts (1)
parse(272-274)
composition/src/v1/federation/federation-factory.ts (2)
composition/src/utils/string-constants.ts (1)
LITERAL_PERIOD(89-89)composition/src/resolvability-graph/constants/string-constants.ts (1)
LITERAL_PERIOD(5-5)
composition/src/v1/normalization/utils.ts (4)
composition/src/router-configuration/types.ts (1)
RequiredFieldConfiguration(76-81)composition/src/utils/string-constants.ts (3)
TYPENAME(156-156)LITERAL_PERIOD(89-89)STRING_SCALAR(142-142)composition/src/errors/errors.ts (1)
invalidSelectionSetDefinitionErrorMessage(607-619)composition/src/utils/utils.ts (1)
kindToNodeType(82-151)
composition/tests/v1/directives/fieldset-directives.test.ts (8)
composition/tests/utils/utils.ts (4)
normalizeSubgraphSuccess(36-46)normalizeString(19-21)normalizeSubgraphFailure(27-34)federateSubgraphsSuccess(58-71)composition/tests/v1/utils/utils.ts (5)
KEY_DIRECTIVE(67-69)OPENFED_FIELD_SET(71-71)SCHEMA_QUERY_DEFINITION(158-162)REQUIRES_DIRECTIVE(99-101)EXTERNAL_DIRECTIVE(46-48)composition/src/errors/errors.ts (4)
invalidDirectiveError(239-250)invalidSelectionSetDefinitionErrorMessage(607-619)nonExternalConditionalFieldError(1482-1500)invalidProvidesOrRequiresDirectivesError(667-674)composition/src/utils/utils.ts (2)
numberToOrdinal(44-57)kindToNodeType(82-151)composition/src/v1/warnings/warnings.ts (1)
nonExternalConditionalFieldWarning(57-76)composition/src/types/types.ts (2)
TypeName(11-11)FieldName(5-5)composition/src/router-configuration/types.ts (1)
ConfigurationData(83-95)composition/src/subgraph/types.ts (1)
Subgraph(18-22)
composition/src/errors/errors.ts (2)
composition/src/errors/types.ts (2)
IncompatibleTypeWithProvidesErrorMessageParams(45-49)NonExternalConditionalFieldErrorParams(51-57)composition/src/utils/string-constants.ts (1)
LITERAL_PERIOD(89-89)
composition/src/v1/normalization/normalization-factory.ts (7)
composition/src/v1/constants/constants.ts (1)
BASE_SCALARS(111-121)composition/src/errors/errors.ts (5)
incompatibleTypeWithProvidesErrorMessage(690-699)unknownNamedTypeErrorMessage(1525-1527)nonExternalConditionalFieldError(1482-1500)typeNameAlreadyProvidedErrorMessage(1600-1605)invalidSelectionSetDefinitionErrorMessage(607-619)composition/src/v1/normalization/params.ts (1)
HandleNonExternalConditionalFieldParams(17-22)composition/src/v1/warnings/warnings.ts (1)
nonExternalConditionalFieldWarning(57-76)composition/src/utils/string-constants.ts (2)
TYPENAME(156-156)STRING_SCALAR(142-142)composition/src/utils/utils.ts (1)
kindToNodeType(82-151)composition/src/router-configuration/types.ts (1)
RequiredFieldConfiguration(76-81)
⏰ 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). (13)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
composition/src/errors/types.ts (1)
45-57: New error parameter types look consistent with callers
IncompatibleTypeWithProvidesErrorMessageParamsandNonExternalConditionalFieldErrorParamscorrectly model the payloads used inerrors.tsand normalization code (coords, fieldSet, directiveName, subgraphName, etc.). No issues from a typing or cohesion perspective.composition/src/errors/errors.ts (2)
690-699: Object-parameter refactor for provides error is soundSwitching
incompatibleTypeWithProvidesErrorMessageto acceptIncompatibleTypeWithProvidesErrorMessageParamsand includingsubgraphNamein the message improves clarity and call-site readability. No functional issues spotted.
1600-1605:typeNameAlreadyProvidedErrorMessagehelper is consistent with existing wordingThe new helper mirrors
fieldAlreadyProvidedErrorMessagebut specializes on@providesand omits the legacy@externalcaveat. This keeps error construction centralized and aligns with the new structured error flows.composition/src/v1/federation/utils.ts (1)
26-27: Skipping__typenamein implicit key inference looks correctGuarding on
fieldName === TYPENAMEinvalidateImplicitFieldSetsprevents__typenamefrom contributing to implicit key field sets or unconditionally-provided checks, which matches the goal of “supporting__typenamein field sets” without treating it as a real key field. Traversal state (currentDepth,definedFields) remains consistent.Just be aware that a key field set consisting only of
__typenamewill now be considered implicitly valid here (no errors raised); confirm this aligns with your expectations for implicit key generation.Also applies to: 141-143
composition/src/v1/normalization/params.ts (1)
17-22: Parameter object for non-external conditional field handling is well-scoped
HandleNonExternalConditionalFieldParamscleanly groups the coords, directive name, and field set used by#handleNonExternalConditionalField, matching the usage innormalization-factory.ts. No issues.composition/src/v1/federation/federation-factory.ts (1)
208-209: Centralizing period handling viaLITERAL_PERIODis consistentAll updated
split(LITERAL_PERIOD)calls operate on coordinates/paths that are constructed with.${...}elsewhere, so this is a mechanical refactor that keeps the delimiter definition centralized instring-constants.tswithout changing behavior. No issues spotted.Also applies to: 600-602, 1485-1486, 2352-2352, 2403-2403
composition/src/utils/string-constants.ts (1)
89-90: NewLITERAL_PERIODandTYPENAMEconstants fit existing patternsAdding
LITERAL_PERIODandTYPENAMEaligns with other literal constants (LITERAL_NEW_LINE,LITERAL_SPACE) and supports shared usage in federation/normalization utilities. Names and values look correct.Also applies to: 156-156
composition/tests/v1/directives/provides.test.ts (2)
440-479: Object-formnonExternalConditionalFieldErrorexpectations look consistentSwitching the expectations to use the object-parameter form of
nonExternalConditionalFieldErrorwith explicitdirectiveCoords,directiveName,fieldSet,subgraphName, andtargetCoordsmatches the new helper signature and keeps the tests resilient to message formatting changes. No issues spotted with coordinates or field-set strings for the four error cases on subgraphnand the V2 case onp.Also applies to: 516-527
792-816: Leaf-node @provides and__typename@provides tests correctly pin error behaviorThe new fixtures
nakaaandnalaaplus their tests align well with the normalization logic:
- For a leaf-node
@providesonQuery.a: ID, the expectation thatgetFieldSetParent(true, ...)yields anincompatibleTypeWithProvidesErrorMessageusingfieldCoords: "Query.a"andresponseType: ID_SCALARis correct and guards future regressions in base-scalar detection.- For
@provides(fields: "__typename")onQuery.a: Object, expecting a singleinvalidProvidesOrRequiresDirectivesError(PROVIDES, [ " On field \"Query.a\":\n -" + typeNameAlreadyProvidedErrorMessage("Object.__typename", nalaa.name) ])matches the newvalidateConditionalFieldSetbranch that special-casesTYPENAMEfor provides.These tests accurately capture the intended semantics and are a good addition to the coverage for error helpers.
Also applies to: 2029-2052
composition/tests/v1/directives/fieldset-directives.test.ts (5)
253-275: Empty-selection-set parse test matches field-set parsing behaviorThe updated test for
"id { }"now clearly asserts a singleinvalidDirectiveErrorwrappingunparsableFieldSetErrorMessage('id { }', SyntaxError(...)). That lines up with howextractKeyFieldSetsfeeds raw field sets intosafeParse('{'+fieldSet+'}'). The message and raw field-set string in the expectation look correct and should guard against regressions in the parser error reporting.
433-487:__typenamekey semantics and entity-link tests are well specifiedThe new KEY-focused tests and fixtures cover important edge-cases:
naaadasserts that@key(fields: "__typename")is accepted and normalized into the expected schema withKEY_DIRECTIVE+OPENFED_FIELD_SET.naaaechecks that adding a selection set to__typenamein a key produces aninvalidSelectionSetDefinitionErrorMessagewithSTRING_SCALARandkindToNodeType(Kind.SCALAR_TYPE_DEFINITION), which matches the intended “non-composite type” messaging.sbaaa/sbaabandsbaac/sbaadverify that explicit and implicit__typenamekeys create the right federated entity link and resulting schema.These expectations align with the new
TYPENAMEhandling and should provide solid regression coverage for typename-as-key behavior.Also applies to: 2171-2222, 2276-2322
449-463: Selection set on__typenametests align with new scalar-typename guardrailsBoth tests that assert errors when a selection set is added to
__typename(for KEY and REQUIRES) correctly:
- Use
invalidSelectionSetDefinitionErrorMessagewith:
- the raw field-set string (including trailing space where present),
fieldCoordinatesPathpointing atEntity.__typename,selectionSetTypeName=STRING_SCALAR,fieldTypeString=kindToNodeType(Kind.SCALAR_TYPE_DEFINITION).- Couple this with a
nonExternalConditionalFieldWarningin the V1 REQUIRES case (naaac), reflecting the separate “non-external conditional field” concern.This matches the new
validateConditionalFieldSetlogic that special-casesTYPENAMEfor selection sets while still reusing the generic invalid-selection-set machinery.Also applies to: 1515-1538
1047-1512: REQUIRES +__typenameV1/V2 matrix is comprehensive and consistentThe new block of REQUIRES tests around fixtures
naaaa–naaacandsaaaa–saaaeexercises a nuanced matrix:
- Direct
__typenamein a V1 field set yields anonExternalConditionalFieldWarningand no requires configuration (naaaa), whereas the same in a V2 subgraph yields anonExternalConditionalFieldError(naaab).- Nested
__typenameunder external vs non-external parents is covered:
- Valid and conditional when the parent field is external (saaaa/saaaae, with requires configuration and no warnings).
- Producing only warnings and no requires configuration when the parent chain is non-external (saaab/saaac/saaad variants).
- The federated test with
saaaa/saaaeconfirms that nested__typenamein a requires field set composes correctly across subgraphs.The configuration expectations (presence/absence of
requires,externalFieldNames, and field-name sets) match what NormalizationFactory’s#handleNonExternalConditionalFieldandvalidateConditionalFieldSetare designed to produce. Coverage here looks strong and should catch regressions in future refactors.Also applies to: 2128-2274
1543-1564: Router configuration implicit-key test refactor is behavior-preservingSwitching this router-configuration test to destructure
subgraphConfigBySubgraphNamefromfederateSubgraphsSuccesswhile leaving the configuration expectations unchanged keeps the semantics identical and improves consistency with other tests that already use the same helper. No behavioral risk here.composition/src/v1/normalization/normalization-factory.ts (2)
1637-1678:getFieldSetParentrefactor correctly wires object-form provides error messagesThe updated
getFieldSetParentlogic forisProvides === truenow:
- Derives
fieldCoordsonce (${parentTypeName}.${fieldName}) and uses it consistently.- Returns an
incompatibleTypeWithProvidesErrorMessageconstructed from{ fieldCoords, responseType: fieldNamedTypeName, subgraphName: this.subgraphName }when the field’s named type is a base scalar or a non-object/interface named type.- Uses
unknownNamedTypeErrorMessage(fieldCoords, fieldNamedTypeName)when the named type is missing fromparentDefinitionDataByTypeName.This matches the new leaf-node @provides test for ID and keeps the error context precise at the directive field, not the parent type. The change is localized and backward-compatible for the object/interface success path.
1769-1783:__typename-specific handling in conditional field-set validation matches new testsThe additions around
TYPENAMEinvalidateConditionalFieldSetlook correct and align with the new test cases:
- In the
Fieldvisitor:
- When
fieldName === TYPENAMEandisProvidesis true, you pushtypeNameAlreadyProvidedErrorMessage(currentFieldCoords, nf.subgraphName)intoerrorMessagesandBREAKthe traversal, which is exactly what the “__typename can be provided” test asserts viainvalidProvidesOrRequiresDirectivesError(PROVIDES, [...]).- When
fieldName === TYPENAME,isProvidesis false, and there are no external ancestors, you delegate to#handleNonExternalConditionalField, which produces either a warning (V1) or an error (V2), matching the REQUIRES tests for direct__typenamefield sets.- In the
SelectionSetvisitor:
- When a selection set is entered with
lastFieldName === TYPENAME, you emitinvalidSelectionSetDefinitionErrorMessage(fieldSet, fieldCoordsPath, STRING_SCALAR, kindToNodeType(Kind.SCALAR_TYPE_DEFINITION))andBREAK, which matches the KEY/REQUIRES tests that attach selection sets to__typename.The early
TYPENAMEchecks also ensure__typenamenever falls through to the regularfieldDataByName/unknown-type path, so it won’t be misreported as an undefined field. Overall, the behavior for__typenamein provides/requires field sets is now explicit and well-covered by the tests.Also applies to: 1972-1989
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist