feat(composition): @openfed__queryCache and @openfed__is directives (4/5)#2997
Draft
SkArchon wants to merge 94 commits into
Draft
feat(composition): @openfed__queryCache and @openfed__is directives (4/5)#2997SkArchon wants to merge 94 commits into
SkArchon wants to merge 94 commits into
Conversation
…add createSubgraphWithDefault test helper
…bgraphWithDefault helper
…bgraphWithDefault helper
…bgraphWithDefault helper
…date-directive # Conflicts: # composition/src/router-configuration/types.ts # composition/src/v1/normalization/normalization-factory.ts # connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go # connect/src/wg/cosmo/node/v1/node_pb.ts # proto/wg/cosmo/node/v1/node.proto # router/gen/proto/wg/cosmo/node/v1/node.pb.go # shared/src/router-config/builder.ts
…pulate-directive # Conflicts: # composition/src/v1/normalization/normalization-factory.ts # composition/src/v1/normalization/types/types.ts # connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go # connect/src/wg/cosmo/node/v1/node_pb.ts # router/gen/proto/wg/cosmo/node/v1/node.pb.go # shared/src/router-config/builder.ts
…oped-directive # Conflicts: # composition/src/v1/normalization/normalization-factory.ts # connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go # connect/src/wg/cosmo/node/v1/node_pb.ts # router/gen/proto/wg/cosmo/node/v1/node.pb.go
…e-directive # Conflicts: # composition/src/v1/normalization/normalization-factory.ts # connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go # connect/src/wg/cosmo/node/v1/node_pb.ts # router/gen/proto/wg/cosmo/node/v1/node.pb.go # shared/src/router-config/builder.ts
…e-directive # Conflicts: # composition/src/router-configuration/types.ts # composition/src/v1/normalization/normalization-factory.ts # composition/src/v1/warnings/warnings.ts # connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go # connect/src/wg/cosmo/node/v1/node_pb.ts # proto/wg/cosmo/node/v1/node.proto # router/gen/proto/wg/cosmo/node/v1/node.pb.go # shared/src/router-config/builder.ts
… into milinda/requestScoped-directive
…s 4/5) Reorders the stack so @openfed__queryCache/@openfed__is sits directly on main as the 4th PR. The @openfed__requestScoped feature is removed here and re-applied on top in the requestScoped branch (now 5/5). The requestScoped assertions in shared/test/entity-caching.test.ts move to that PR. Proto regenerated (request_scoped_configurations field 4 dropped; query_cache remains field 5).
Contributor
WalkthroughAdds ChangesOpenFed query cache and
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
Comment |
Router image scan failed❌ Security vulnerabilities found in image: Please check the security vulnerabilities found in the PR. If you believe this is a false positive, please add the vulnerability to the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
4285-4291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the new helper typings.
extractQueryCacheConfigandvalidateIsDirectivePlacementreturnvoidbut omit return annotations, andwalkSelectionsusesany[]. Add: voidand a concrete GraphQL selection type to keep this new AST traversal type-safe.As per coding guidelines,
**/*.{ts,tsx}should “Use explicit type annotations for function parameters and return types in TypeScript” and “Avoidanytype in TypeScript; use specific types or generics.”Also applies to: 4375-4375, 4403-4403
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4285 - 4291, Add missing return type annotations to the new helper functions for type safety. In extractQueryCacheConfig function, add `: void` return type annotation. In validateIsDirectivePlacement function (around line 4375), add `: void` return type annotation. In walkSelections function (around line 4403), replace the `any[]` type parameter with a concrete GraphQL selection type instead of using the `any` type. This ensures all three functions have explicit type annotations as required by the coding guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4953-4955: The early return when explicitMappings.length === 0
bypasses the extra-argument validation that follows, allowing composite-only
mappings to skip validation of all field arguments. Remove or modify this early
return condition in the normalization logic so that the extra-argument
validation continues to execute even when there are no explicit mappings,
ensuring that composite mappings are validated against all arguments present on
the field to prevent cache-key collisions.
- Around line 5390-5413: The query-cache directive validation logic (including
the check for OPENFED_QUERY_CACHE and the validateIsDirectivePlacement call for
`@openfed__is` misplacement) is currently inside the isObject guard, which skips
validation for interface fields. Move the entire if/else block that checks
fieldData.directivesByName.has(OPENFED_QUERY_CACHE) and calls
extractQueryCacheConfig or validateIsDirectivePlacement outside and after the if
(isObject) block so that query-cache and `@openfed__is` placement validation runs
for all field types, not just object fields.
- Around line 5171-5173: The code currently allows singular composite inputs for
list-return cache fields by setting isBatch=false when isListReturn is true but
argInfo.isList is false. Add validation logic after the isBatch assignment in
the normalization-factory.ts file to reject this invalid configuration. Mirror
the explicit scalar-path validation by checking that when isListReturn is true
and the field is a nested key (indicated by isNestedKey based on
LITERAL_OPEN_BRACE), the argInfo.isList must also be true. If this condition
fails, throw an appropriate error to prevent non-list composite inputs from
being used with list-return query caches.
- Around line 4836-4853: The namedTypesMatch method only compares named types
and ignores list wrappers, allowing mismatched list depths to pass validation.
After unwrapping exactly one batch dimension using this.unwrapListType to get
innerType, replace the namedTypesMatch check with a comparison that validates
both the named types AND the complete type structure including list wrappers
match the keyFieldTypeNode. This ensures that extra nesting in the argument type
is properly rejected before router mappings are emitted, preventing cases like
[[ID!]!]! from incorrectly satisfying a scalar ID batch key requirement.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4285-4291: Add missing return type annotations to the new helper
functions for type safety. In extractQueryCacheConfig function, add `: void`
return type annotation. In validateIsDirectivePlacement function (around line
4375), add `: void` return type annotation. In walkSelections function (around
line 4403), replace the `any[]` type parameter with a concrete GraphQL selection
type instead of using the `any` type. This ensures all three functions have
explicit type annotations as required by the coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d8011ee-5e39-4430-a489-391027019492
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (19)
composition/src/directive-definition-data/directive-definition-data.tscomposition/src/errors/errors.tscomposition/src/errors/types/params.tscomposition/src/router-configuration/types.tscomposition/src/utils/string-constants.tscomposition/src/v1/constants/constants.tscomposition/src/v1/constants/directive-definitions.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types/types.tscomposition/src/v1/normalization/utils.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/tests/v1/directives/cache-populate.test.tscomposition/tests/v1/directives/entity-cache.test.tscomposition/tests/v1/directives/query-cache.test.tsconnect/src/wg/cosmo/node/v1/node_pb.tsproto/wg/cosmo/node/v1/node.protoshared/src/router-config/builder.tsshared/test/entity-caching.test.ts
Comment on lines
+4836
to
+4853
| const unwrapped = this.unwrapListType(argTypeNode); | ||
| if (!isTypeNodeListType(unwrapped)) { | ||
| // Single list, not list of lists | ||
| this.errors.push( | ||
| invalidDirectiveError(OPENFED_IS, `${fieldCoords}(${argInfo.name}: ...)`, FIRST_ORDINAL, [ | ||
| batchListValuedKeyRequiresNestedListsErrorMessage( | ||
| fieldCoords, | ||
| isFieldValue, | ||
| entityTypeName, | ||
| `a single tag list of type "${printTypeNode(argTypeNode)}"`, | ||
| ), | ||
| ]), | ||
| ); | ||
| return []; | ||
| } | ||
| // List of lists - check inner type matches | ||
| const innerType = this.unwrapListType(unwrapped); | ||
| if (!this.namedTypesMatch(innerType, keyFieldTypeNode)) { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Compare list shape, not only named types.
namedTypesMatch ignores list wrappers, so extra nesting can satisfy the wrong key shape, e.g. a [[ID!]!]! argument can pass for a scalar ID batch key. After removing exactly one batch dimension, reject remaining list-depth mismatches before emitting router mappings.
Also applies to: 4872-4873, 4931-4932
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4836
- 4853, The namedTypesMatch method only compares named types and ignores list
wrappers, allowing mismatched list depths to pass validation. After unwrapping
exactly one batch dimension using this.unwrapListType to get innerType, replace
the namedTypesMatch check with a comparison that validates both the named types
AND the complete type structure including list wrappers match the
keyFieldTypeNode. This ensures that extra nesting in the argument type is
properly rejected before router mappings are emitted, preventing cases like
[[ID!]!]! from incorrectly satisfying a scalar ID batch key requirement.
Comment on lines
+4953
to
+4955
| if (explicitMappings.length === 0) { | ||
| return compositeMappings; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Run extra-argument checks for composite-only mappings.
When the only mapping is composite, this early return skips the extra-argument validation below. A field like user(input: UserKeyInput @openfed__is(fields: "id sku"), locale: String) would cache by input while ignoring locale, creating cache-key collisions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4953
- 4955, The early return when explicitMappings.length === 0 bypasses the
extra-argument validation that follows, allowing composite-only mappings to skip
validation of all field arguments. Remove or modify this early return condition
in the normalization logic so that the extra-argument validation continues to
execute even when there are no explicit mappings, ensuring that composite
mappings are validated against all arguments present on the field to prevent
cache-key collisions.
Comment on lines
+5171
to
+5173
| const isBatch = isListReturn && argInfo.isList; | ||
| const isNestedKey = normalizedFieldSet.includes(LITERAL_OPEN_BRACE); | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject singular composite inputs for list-return cache fields.
For list returns, a non-list composite input currently produces mappings with isBatch=false. Mirror the explicit scalar-path validation here so list-return query caches require a batch-capable list argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composition/src/v1/normalization/normalization-factory.ts` around lines 5171
- 5173, The code currently allows singular composite inputs for list-return
cache fields by setting isBatch=false when isListReturn is true but
argInfo.isList is false. Add validation logic after the isBatch assignment in
the normalization-factory.ts file to reject this invalid configuration. Mirror
the explicit scalar-path validation by checking that when isListReturn is true
and the field is a nested key (indicated by isNestedKey based on
LITERAL_OPEN_BRACE), the argInfo.isList must also be true. If this condition
fails, throw an appropriate error to prevent non-list composite inputs from
being used with list-return query caches.
…alue arg
Pass the value directly instead of { directiveName, value }; the error message
no longer includes the directive name. Restores the base signature and updates
all call sites (entityCache/cachePopulate/queryCache handlers + tests).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2997 +/- ##
==========================================
+ Coverage 42.77% 47.31% +4.54%
==========================================
Files 861 1119 +258
Lines 124386 153254 +28868
Branches 9995 10181 +186
==========================================
+ Hits 53204 72514 +19310
- Misses 70837 78923 +8086
- Partials 345 1817 +1472
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
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 (4)
4535-4538: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject extra input-object fields in composite mappings.
validateNestedInputObjectMappingverifies that required key fields exist, but it never rejects input fields that are not part of the mapped key. An input like{ id, sku, locale }mapped withfields: "id sku"emits mappings for onlyidandsku, so differentlocalevalues can collide in the same cache key.Also applies to: 4652-4655
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4535 - 4538, The validateNestedInputObjectMapping function currently validates that required key fields exist but does not reject input object fields that are not part of the mapped keys. After building the fieldMappings array from topLevelGroups in the function, add validation to ensure all input fields from inputData.inputValueDataByName are accounted for in the mapping. Reject any extra input fields that are not part of the mapped keys by throwing an error with a clear message indicating which fields are unmapped. This validation should be added both at the primary location (around lines 4535-4538 where fieldMappings is created) and at the secondary location mentioned in the comment (lines 4652-4655).
4770-4773: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject duplicate key fields when adding composite mappings.
The scalar path checks
mappedKeyFieldToArgumentNamebefore adding a mapping, but this composite branch overwrites existing entries. If a scalar@openfed__is(fields: "id")is processed before a composite@openfed__is(fields: "id sku"), argument order decides whether the duplicate is rejected or emitted.🐛 Proposed fix
for (const mapping of mappings) { for (const fieldMapping of mapping.fieldMappings) { + if (mappedKeyFieldToArgumentName.has(fieldMapping.entityKeyField)) { + this.errors.push( + invalidDirectiveError(OPENFED_IS, `${fieldCoords}(${argInfo.name}: ...)`, FIRST_ORDINAL, [ + duplicateKeyFieldMappingErrorMessage(fieldCoords, fieldMapping.entityKeyField), + ]), + ); + return []; + } mappedKeyFieldToArgumentName.set(fieldMapping.entityKeyField, argInfo.name); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4770 - 4773, The composite mapping loop through fieldMappings is unconditionally adding entries to mappedKeyFieldToArgumentName without checking for duplicates first, unlike the scalar path that validates this. Before calling mappedKeyFieldToArgumentName.set() with fieldMapping.entityKeyField and argInfo.name, check if fieldMapping.entityKeyField already exists in the mappedKeyFieldToArgumentName map. If the key field is already mapped, reject the composite mapping and provide an appropriate error or validation failure instead of silently overwriting the existing entry.
4864-4886: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate scalar companion types in batch mappings.
For list-return fields, scalar arguments paired with a batched list argument skip type validation entirely in this branch. A composite key like
orgId idcan acceptorgId: Int@openfed__is(fields: "orgId")even when the entity key field isID, producing an invalid router key mapping.🐛 Proposed fix
} else { - // Scalar arg in batch mode - could still be valid as a scalar `@openfed__is`, we'll check later + if (!this.namedTypesMatch(argTypeNode, keyFieldTypeNode)) { + this.errors.push( + invalidDirectiveError(OPENFED_IS, `${fieldCoords}(${argInfo.name}: ...)`, FIRST_ORDINAL, [ + explicitTypeMismatchErrorMessage( + argInfo.name, + fieldCoords, + printTypeNode(argTypeNode), + isFieldValue, + entityTypeName, + printTypeNode(keyFieldTypeNode), + ), + ]), + ); + return []; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4864 - 4886, The else branch that handles scalar arguments (when argInfo.isList is false) in batch mode currently skips validation with a placeholder comment. This allows type mismatches to pass through. Add type validation in this branch by comparing the scalar argTypeNode against the keyFieldTypeNode using the namedTypesMatch method, similar to how list arguments are validated above. If the types do not match, push an invalidDirectiveError using explicitTypeMismatchErrorMessage with the same parameters (argInfo.name, fieldCoords, printTypeNode(argTypeNode), and printTypeNode(keyFieldTypeNode)) to ensure scalar companion types in batch mappings are properly validated.
4955-4957: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat every unannotated argument as extra once mappings are explicit.
The builder states that argument names are never matched to
@keyfields, but these filters exempt unannotated arguments whose names equal any key field. For example,user(id: ID!@openfed__is(fields: "id"), sku: String)can emit anidmapping while ignoringskuifskuis also an entity key field, causing cache-key collisions.🐛 Proposed fix
- const extraArgs = argumentInfos.filter((a) => !a.isFieldValue && !allKeyFieldPaths.has(a.name)); + const extraArgs = argumentInfos.filter((a) => !a.isFieldValue); @@ - const globalExtraArgs = argumentInfos.filter((a) => !a.isFieldValue && !allKeyFieldPaths.has(a.name)); + const globalExtraArgs = argumentInfos.filter((a) => !a.isFieldValue);Also applies to: 4995-4997
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4955 - 4957, The filter in the extraArgs assignment (around line 4955) is currently excluding unannotated arguments whose names happen to match key fields in allKeyFieldPaths. Once explicit mappings with `@openfed__is` are used, all unannotated arguments should be treated as extra regardless of whether their names match key field names. Remove the condition !allKeyFieldPaths.has(a.name) from the extraArgs filter so that the filter only checks !a.isFieldValue, ensuring that any unannotated argument is treated as extra and preventing name-based matching to key fields that could cause cache-key collisions. Apply this same logic change to the similar filter at line 4995-4997 that also applies to this same issue.
🧹 Nitpick comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
4281-4287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the new helper signatures.
extractQueryCacheConfigandvalidateIsDirectivePlacementinfervoid, andwalkSelectionsusesany[]. Please add explicit: voidreturn types and replaceanywith a GraphQL selection type or narrow local union. As per coding guidelines, “Use explicit type annotations for function parameters and return types in TypeScript” and “Avoidanytype in TypeScript; use specific types or generics.”♻️ Suggested shape
extractQueryCacheConfig( parentTypeName: string, configurationTypeName: string, fieldName: string, fieldData: FieldData, operationType: OperationTypeNode | undefined, - ) { + ): void { @@ - validateIsDirectivePlacement(fieldCoords: string, fieldData: FieldData) { + validateIsDirectivePlacement(fieldCoords: string, fieldData: FieldData): void { @@ - const walkSelections = (selections: readonly any[], currentTypeName: string, pathPrefix: string) => { + const walkSelections = (selections: readonly SelectionNode[], currentTypeName: string, pathPrefix: string): void => {If
SelectionNodeis not already imported fromgraphql, add it to the existing type import.Also applies to: 4371-4371, 4399-4399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4281 - 4287, The methods extractQueryCacheConfig, validateIsDirectivePlacement, and walkSelections have loose type signatures that rely on type inference and use the any type. Add explicit `: void` return type annotations to extractQueryCacheConfig and validateIsDirectivePlacement methods. For the walkSelections method, replace the `any[]` type with a more specific GraphQL type such as `SelectionNode[]`. If SelectionNode is not already imported from the graphql package, add it to the existing type imports at the top of the file.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4535-4538: The validateNestedInputObjectMapping function currently
validates that required key fields exist but does not reject input object fields
that are not part of the mapped keys. After building the fieldMappings array
from topLevelGroups in the function, add validation to ensure all input fields
from inputData.inputValueDataByName are accounted for in the mapping. Reject any
extra input fields that are not part of the mapped keys by throwing an error
with a clear message indicating which fields are unmapped. This validation
should be added both at the primary location (around lines 4535-4538 where
fieldMappings is created) and at the secondary location mentioned in the comment
(lines 4652-4655).
- Around line 4770-4773: The composite mapping loop through fieldMappings is
unconditionally adding entries to mappedKeyFieldToArgumentName without checking
for duplicates first, unlike the scalar path that validates this. Before calling
mappedKeyFieldToArgumentName.set() with fieldMapping.entityKeyField and
argInfo.name, check if fieldMapping.entityKeyField already exists in the
mappedKeyFieldToArgumentName map. If the key field is already mapped, reject the
composite mapping and provide an appropriate error or validation failure instead
of silently overwriting the existing entry.
- Around line 4864-4886: The else branch that handles scalar arguments (when
argInfo.isList is false) in batch mode currently skips validation with a
placeholder comment. This allows type mismatches to pass through. Add type
validation in this branch by comparing the scalar argTypeNode against the
keyFieldTypeNode using the namedTypesMatch method, similar to how list arguments
are validated above. If the types do not match, push an invalidDirectiveError
using explicitTypeMismatchErrorMessage with the same parameters (argInfo.name,
fieldCoords, printTypeNode(argTypeNode), and printTypeNode(keyFieldTypeNode)) to
ensure scalar companion types in batch mappings are properly validated.
- Around line 4955-4957: The filter in the extraArgs assignment (around line
4955) is currently excluding unannotated arguments whose names happen to match
key fields in allKeyFieldPaths. Once explicit mappings with `@openfed__is` are
used, all unannotated arguments should be treated as extra regardless of whether
their names match key field names. Remove the condition
!allKeyFieldPaths.has(a.name) from the extraArgs filter so that the filter only
checks !a.isFieldValue, ensuring that any unannotated argument is treated as
extra and preventing name-based matching to key fields that could cause
cache-key collisions. Apply this same logic change to the similar filter at line
4995-4997 that also applies to this same issue.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4281-4287: The methods extractQueryCacheConfig,
validateIsDirectivePlacement, and walkSelections have loose type signatures that
rely on type inference and use the any type. Add explicit `: void` return type
annotations to extractQueryCacheConfig and validateIsDirectivePlacement methods.
For the walkSelections method, replace the `any[]` type with a more specific
GraphQL type such as `SelectionNode[]`. If SelectionNode is not already imported
from the graphql package, add it to the existing type imports at the top of the
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d70e1fef-0d05-4fe8-a9a4-49a3c4c26db1
📒 Files selected for processing (3)
composition/src/errors/errors.tscomposition/src/v1/normalization/normalization-factory.tscomposition/tests/v1/directives/query-cache.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- composition/tests/v1/directives/query-cache.test.ts
Contributor
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 (2)
composition/src/v1/normalization/normalization-factory.ts (2)
4953-4995: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat every unmapped argument as cache-key input.
The extra-argument checks ignore any non-
@openfed__isargument whose name matches any entity key path. Since Line 4683 says argument names are never implicitly mapped, such arguments are omitted fromfieldMappingsand can still affect resolver output without being part of the cache key.🐛 Proposed fix
- const extraArgs = argumentInfos.filter((a) => !a.isFieldValue && !allKeyFieldPaths.has(a.name)); + const extraArgs = argumentInfos.filter((a) => !a.isFieldValue); @@ - const globalExtraArgs = argumentInfos.filter((a) => !a.isFieldValue && !allKeyFieldPaths.has(a.name)); + const globalExtraArgs = argumentInfos.filter((a) => !a.isFieldValue);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4953 - 4995, The extra-argument validation in normalization-factory should treat every unmapped non-@openfed__is argument as cache-key input, not exclude ones whose names happen to match any entity key path. Update the batch and singular/composite checks around the explicitMappings/argumentInfos filtering so only actually mapped field values are exempt, and ensure the cache-key accounting uses the same mapping source as fieldMappings rather than allKeyFieldPaths alone.
5369-5385: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the root-type helper so default
Queryfields are accepted.
operationTypeNodeByTypeName.get(parentTypeName)only covers explicitly registered operation types; defaulttype Querycan reachextractQueryCacheConfigwithundefinedand be rejected as non-Query. Use the existing helper that also recognizes default root names.🐛 Proposed fix
- const operationTypeNode = this.operationTypeNodeByTypeName.get(parentTypeName); + const operationTypeNode = this.getOperationTypeNodeForRootTypeName(parentTypeName);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 5369 - 5385, The root-type lookup in normalization is too narrow because `operationTypeNodeByTypeName.get(parentTypeName)` misses default root names like `Query`, which lets `extractQueryCacheConfig` see `undefined` and reject valid fields. Update the logic in `normalization-factory.ts` to use the existing helper that resolves root operation types by both registered nodes and default names, then pass that result through the `operationTypeNode` flow before calling `extractQueryCacheConfig` and related cache handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4706-4710: `buildExplicitMappings` in `normalization-factory` is
using undeclared inputs and an out-of-date helper call, so restore the mapping
helper contract by reintroducing the missing local values (`fieldCoords`,
`isListReturn`, `entityTypeName`) before they are used and update the
`buildCompositeIsMapping(...)` invocation to match its current 3-argument
signature. Check the surrounding `buildExplicitMappings` flow and the helper
definitions it calls so the explicit mapping path compiles again.
In `@composition/src/v1/normalization/types/types.ts`:
- Around line 204-209: `IsDirectiveNode.arguments` is typed incorrectly and
should model a GraphQL directive argument AST instead of a string tuple. Update
the `IsDirectiveNode` definition in `types.ts` so it matches the `@openfed__is`
shape with a `Kind.ARGUMENT` node containing a `StringValueNode` for the
`fields` argument, consistent with the other directive node types and the
normalizer’s AST-based reads. Use the `IsDirectiveNode` and `OPENFED_IS` symbols
to locate and align the type with the existing directive node patterns.
---
Outside diff comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4953-4995: The extra-argument validation in normalization-factory
should treat every unmapped non-@openfed__is argument as cache-key input, not
exclude ones whose names happen to match any entity key path. Update the batch
and singular/composite checks around the explicitMappings/argumentInfos
filtering so only actually mapped field values are exempt, and ensure the
cache-key accounting uses the same mapping source as fieldMappings rather than
allKeyFieldPaths alone.
- Around line 5369-5385: The root-type lookup in normalization is too narrow
because `operationTypeNodeByTypeName.get(parentTypeName)` misses default root
names like `Query`, which lets `extractQueryCacheConfig` see `undefined` and
reject valid fields. Update the logic in `normalization-factory.ts` to use the
existing helper that resolves root operation types by both registered nodes and
default names, then pass that result through the `operationTypeNode` flow before
calling `extractQueryCacheConfig` and related cache handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caf443e7-1d73-4b67-b648-77d7d249feb0
📒 Files selected for processing (10)
composition/src/directive-definition-data/directive-definition-data.tscomposition/src/errors/errors.tscomposition/src/errors/types/params.tscomposition/src/router-configuration/types.tscomposition/src/router-configuration/utils.tscomposition/src/types/types.tscomposition/src/v1/constants/directive-definitions.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types/types.tsshared/src/router-config/builder.ts
✅ Files skipped from review due to trivial changes (1)
- composition/src/errors/types/params.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- composition/src/v1/constants/directive-definitions.ts
- shared/src/router-config/builder.ts
- composition/src/directive-definition-data/directive-definition-data.ts
Comment on lines
+4706
to
+4710
| buildExplicitMappings( | ||
| fieldData: FieldData, | ||
| keyFieldSets: Map<string, KeyFieldSetData>, | ||
| argumentInfos: Array<ArgumentInfo>, | ||
| ): EntityKeyMappingConfig[] { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '4706,5193p' composition/src/v1/normalization/normalization-factory.ts
rg -nP '\bbuildCompositeIsMapping\s*\(' composition/src/v1/normalization/normalization-factory.tsRepository: wundergraph/cosmo
Length of output: 19048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the start of buildExplicitMappings and the full buildCompositeIsMapping signature/body.
sed -n '4706,4795p' composition/src/v1/normalization/normalization-factory.ts
printf '\n---\n'
sed -n '5118,5198p' composition/src/v1/normalization/normalization-factory.tsRepository: wundergraph/cosmo
Length of output: 6824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the start of buildExplicitMappings and the full buildCompositeIsMapping signature/body.
sed -n '4706,4795p' composition/src/v1/normalization/normalization-factory.ts
printf '\n---\n'
sed -n '5118,5198p' composition/src/v1/normalization/normalization-factory.tsRepository: wundergraph/cosmo
Length of output: 6824
Restore the mapping helper contract. buildExplicitMappings still references fieldCoords, isListReturn, and entityTypeName before declaring them, and the buildCompositeIsMapping(...) call no longer matches its 3-argument signature. This leaves the normalization path uncompilable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4706
- 4710, `buildExplicitMappings` in `normalization-factory` is using undeclared
inputs and an out-of-date helper call, so restore the mapping helper contract by
reintroducing the missing local values (`fieldCoords`, `isListReturn`,
`entityTypeName`) before they are used and update the
`buildCompositeIsMapping(...)` invocation to match its current 3-argument
signature. Check the surrounding `buildExplicitMappings` flow and the helper
definitions it calls so the explicit mapping path compiles again.
Comment on lines
+204
to
+209
| export type IsDirectiveNode = { | ||
| readonly arguments: readonly [string]; | ||
| readonly kind: Kind.DIRECTIVE; | ||
| readonly name: NameNode & { readonly value: typeof OPENFED_IS }; | ||
| readonly loc?: Location; | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts '\bIsDirectiveNode\b' -C3
# Inspect a representative FieldsArgumentNode / argument node type for the intended shape
rg -nP --type=ts '(FieldsArgumentNode|MaxAgeArgumentNode)\b' -C3Repository: wundergraph/cosmo
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and related type definitions.
git ls-files 'composition/src/v1/normalization/types/*.ts' | sed -n '1,20p'
echo '---'
sed -n '1,280p' composition/src/v1/normalization/types/types.ts
echo '---'
rg -n "type (IsDirectiveNode|QueryCacheDirectiveNode|CachePopulateDirectiveNode|FieldsArgumentNode|MaxAgeArgumentNode|ArgumentInfo)" composition/src/v1/normalization/types -C 3Repository: wundergraph/cosmo
Length of output: 10034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and related type definitions.
git ls-files 'composition/src/v1/normalization/types/*.ts' | sed -n '1,20p'
echo '---'
sed -n '1,280p' composition/src/v1/normalization/types/types.ts
echo '---'
rg -n "type (IsDirectiveNode|QueryCacheDirectiveNode|CachePopulateDirectiveNode|FieldsArgumentNode|MaxAgeArgumentNode|ArgumentInfo)" composition/src/v1/normalization/types -C 3Repository: wundergraph/cosmo
Length of output: 10034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "OPENFED_IS|IsDirectiveNode|OPENFED_QUERY_CACHE|OPENFED_CACHE_POPULATE|EntityCacheDirectiveNode|QueryCacheDirectiveNode|CachePopulateDirectiveNode" composition/src -C 3
echo '---'
rg -n "arguments: readonly \[string\]|readonly \[string\]" composition/src -C 2Repository: wundergraph/cosmo
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused evidence for the `@openfed__is` directive shape and any consumer/cast of IsDirectiveNode.
rg -n "IsDirectiveNode|as IsDirectiveNode|OPENFED_IS_DEFINITION|`@openfed__is`|fields: String!" composition/src -C 3
echo '---'
# Look for any dedicated argument-node type for `@openfed__is` or similar patterns.
rg -n "FieldsArgumentNode|fieldsArgument|OPENFED_IS.*ArgumentNode|ArgumentNode" composition/src/v1/normalization/types composition/src/v1/constants/directive-definitions.ts -C 2Repository: wundergraph/cosmo
Length of output: 29498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused evidence for the `@openfed__is` directive shape and any consumer/cast of IsDirectiveNode.
rg -n "IsDirectiveNode|as IsDirectiveNode|OPENFED_IS_DEFINITION|`@openfed__is`|fields: String!" composition/src -C 3
echo '---'
# Look for any dedicated argument-node type for `@openfed__is` or similar patterns.
rg -n "FieldsArgumentNode|fieldsArgument|OPENFED_IS.*ArgumentNode|ArgumentNode" composition/src/v1/normalization/types composition/src/v1/constants/directive-definitions.ts -C 2Repository: wundergraph/cosmo
Length of output: 29498
IsDirectiveNode.arguments should be a directive argument node, not string.
@openfed__is is defined as fields: String!, and the normalizer reads directive arguments as GraphQL AST nodes. This should model the fields argument shape (Kind.ARGUMENT + StringValueNode) to match the other directive node types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composition/src/v1/normalization/types/types.ts` around lines 204 - 209,
`IsDirectiveNode.arguments` is typed incorrectly and should model a GraphQL
directive argument AST instead of a string tuple. Update the `IsDirectiveNode`
definition in `types.ts` so it matches the `@openfed__is` shape with a
`Kind.ARGUMENT` node containing a `StringValueNode` for the `fields` argument,
consistent with the other directive node types and the normalizer’s AST-based
reads. Use the `IsDirectiveNode` and `OPENFED_IS` symbols to locate and align
the type with the existing directive node patterns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@openfed__queryCache+@openfed__is— query-field caching with argument-to-@keymappings for cache-key construction. AddsQueryCacheConfiguration/EntityKeyMapping/EntityCacheFieldMappingproto (field 5) + bindings, config types, normalization (@openfed__ismapping incl. batch), error messages, a warning, and the query-cache + serializer tests.More documentation on this: https://app.notion.com/p/wundergraph/Directives-37bb19e0a0ec80b9b505d896beed5897
Summary by CodeRabbit
New Features
Bug Fixes
Tests