feat(composition): @openfed__queryCache and @openfed__is directives (5/5)#2986
Merged
SkArchon merged 12 commits intoJun 23, 2026
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds two new OpenFED directives— Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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. Comment |
Router-nonroot 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 |
SkArchon
force-pushed
the
milinda/requestScoped-directive
branch
from
2859cba to
faca30a
Compare
SkArchon
force-pushed
the
milinda/queryCache-directive
branch
from
37a198e to
4c90da9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## milinda/requestScoped-directive #2986 +/- ##
===================================================================
+ Coverage 47.04% 47.24% +0.20%
===================================================================
Files 1119 1119
Lines 152204 153380 +1176
Branches 10007 10193 +186
===================================================================
+ Hits 71602 72470 +868
- Misses 78798 79082 +284
- Partials 1804 1828 +24
🚀 New features to boost your workflow:
|
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
composition/src/errors/errors.ts (1)
2066-2328:⚠️ Potential issue | 🟡 MinorReplace string concatenation with template literals in new error message helpers.
Found 17 instances of string concatenation using the
+operator across multiple error message functions. Per the coding guidelines for**/*.{ts,tsx}files: "Use template literals instead of string concatenation." These should be consolidated into single template literals for consistency and readability.🤖 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/errors/errors.ts` around lines 2066 - 2328, Replace string concatenation using the `+` operator with template literals in all the error message helper functions throughout this file. Identify functions like queryCacheOnNonQueryFieldErrorMessage, queryCacheOnNonEntityReturnTypeErrorMessage, cacheInvalidateAndPopulateMutualExclusionErrorMessage, isReferencesUnknownKeyFieldErrorMessage, listArgumentToScalarKeySpecErrorMessage, scalarArgumentToListKeySpecErrorMessage, multipleListArgumentsBatchFactoryMessage, inputObjectCompositeTypeMismatchErrorMessage, inputObjectCompositeMissingFieldErrorMessage, nestedKeyRequiresInputObjectErrorMessage, nestedInputObjectTypeMismatchErrorMessage, nestedInputObjectMissingFieldErrorMessage, and nonInputArgumentCannotTargetCompositeKeyErrorMessage that currently use the `+` operator to join string parts, and convert them to use backticks with embedded expressions instead, consolidating each function's return statement into a single template literal.Source: Coding guidelines
composition/src/v1/normalization/normalization-factory.ts (1)
1-25:⚠️ Potential issue | 🟡 MinorTighten the new helper types: add explicit return annotations and replace
any[]withSelectionNode.Three helper methods lack explicit
voidreturn type annotations (processRootFieldCacheDirectives,extractQueryCacheConfig,validateIsDirectivePlacement), andwalkSelectionsat line 4417 usesreadonly any[]instead of a specific type. ImportSelectionNodefromgraphqland add the missing return type annotations.Per coding guidelines: "Use explicit type annotations for function parameters and return types in TypeScript" and "Avoid
anytype in TypeScript; use specific types or generics".Proposed typing cleanup
import { BREAK, type ConstDirectiveNode, type ConstValueNode, type DefinitionNode, type DirectiveDefinitionNode, type DocumentNode, type EnumValueDefinitionNode, type FieldDefinitionNode, type InputValueDefinitionNode, type InterfaceTypeDefinitionNode, type IntValueNode, Kind, type ListValueNode, type NamedTypeNode, type OperationTypeDefinitionNode, OperationTypeNode, print, + type SelectionNode, type StringValueNode, type TypeDefinitionNode, type TypeExtensionNode, type TypeNode, type ValueNode, visit, } from 'graphql';- processRootFieldCacheDirectives() { + processRootFieldCacheDirectives(): void {extractQueryCacheConfig( parentTypeName: string, configurationTypeName: string, fieldName: string, fieldData: FieldData, operationType: OperationTypeNode | undefined, - ) { + ): void {- validateIsDirectivePlacement(fieldCoords: string, fieldData: FieldData, hasQueryCache: boolean) { + validateIsDirectivePlacement(fieldCoords: string, fieldData: FieldData, hasQueryCache: boolean): void {- const walkSelections = (selections: readonly any[], currentTypeName: string, pathPrefix: string) => { + const walkSelections = (selections: readonly SelectionNode[], currentTypeName: string, pathPrefix: string): void => {🤖 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 1 - 25, Add SelectionNode to the graphql imports at the top of the file. Then locate the three helper methods processRootFieldCacheDirectives, extractQueryCacheConfig, and validateIsDirectivePlacement and add explicit void return type annotations to each one. Finally, find the walkSelections method (around line 4417) and replace the readonly any[] type with readonly SelectionNode[] to use the specific type instead of the generic any type.Source: Coding guidelines
🧹 Nitpick comments (3)
composition/tests/v1/directives/query-cache.test.ts (2)
77-83: ⚡ Quick winGuard array access before dereferencing extracted configs.
Several success-path tests access
rootFieldConfigs![0]before asserting array length; if extraction regresses, this can throw and mask the real mismatch. Add existence/length assertions first, then index.Suggested pattern
const rootFieldConfigs = result.configurationDataByTypeName.get('Query')!.entityCaching?.queryCacheConfigurations; -expect(rootFieldConfigs![0]).toMatchObject({ +expect(rootFieldConfigs).toBeDefined(); +expect(rootFieldConfigs).toHaveLength(1); +expect(rootFieldConfigs![0]).toMatchObject({ fieldName: 'user', maxAgeSeconds: 120, includeHeaders: true, shadowMode: true, });🤖 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/tests/v1/directives/query-cache.test.ts` around lines 77 - 83, The test in query-cache.test.ts accesses rootFieldConfigs![0] without first verifying that rootFieldConfigs exists and contains at least one element. If the extraction regresses, this will throw an error rather than providing a clear test failure message. Add explicit assertions to check that rootFieldConfigs exists and has a length of at least 1 before attempting to access the first element with toMatchObject, ensuring failures are clear and traceable rather than masked by runtime errors.
371-1134: ⚡ Quick winAssert single-error cardinality in failure cases.
Most validation tests only compare
errors[0]; extra unexpected errors would still pass. Addexpect(errors).toHaveLength(1)before checking the exact error payload.Suggested pattern
const { errors } = normalizeSubgraphFailure(...); +expect(errors).toHaveLength(1); expect(errors[0]).toStrictEqual( invalidDirectiveError(...) );🤖 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/tests/v1/directives/query-cache.test.ts` around lines 371 - 1134, Add single-error cardinality assertions to all validation test cases in the file. For each test that validates a failure scenario (where errors[0] is checked), insert `expect(errors).toHaveLength(1)` immediately before the existing `expect(errors[0]).toStrictEqual(...)` assertions. This applies to all test cases within the `@openfed__queryCache` and `@openfed__is` describe blocks, including those testing directive validation, batch mappings, and input-object composite mappings. This ensures that exactly one error is produced rather than silently allowing multiple unexpected errors to pass the test.composition/src/v1/normalization/normalization-factory.ts (1)
4692-4698: Use an interface forArgumentInfo.This is an object shape and should follow the repository's TypeScript interface preference per coding guidelines.
Proposed change
- type ArgumentInfo = { + interface ArgumentInfo { name: string; data: InputValueData; isFieldValue: string | undefined; // `@openfed__is`(fields: "...") value isList: boolean; typeNode: TypeNode; - }; + }🤖 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 4692 - 4698, Convert the ArgumentInfo type definition from a type alias to an interface to align with the repository's TypeScript coding guidelines. Replace the type ArgumentInfo declaration syntax with an interface ArgumentInfo declaration, keeping all the same properties (name, data, isFieldValue, isList, typeNode) and their type annotations intact.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/router-configuration/types.ts`:
- Around line 137-161: Convert the three object-shape type declarations
(FieldMappingConfig, EntityKeyMappingConfig, and QueryCacheConfig) from type
aliases to interfaces for consistency with the coding style guide. Replace the
`type Name = { ... }` syntax with `interface Name { ... }` syntax for each of
these three declarations, removing the equals sign and preserving all property
definitions within the interface body.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4967-4969: The early return when explicitMappings.length === 0
skips validation checks that should also apply to composite-only mappings. Move
the unused-argument validation logic so it runs before the early return
statement that returns compositeMappings, ensuring these checks apply regardless
of whether explicit mappings exist. Additionally, in the buildCompositeIsMapping
function, add a validation check to reject cases where isListReturn is true but
argInfo.isList is false, and perform this validation before emitting any field
mappings to prevent invalid batch mapping configurations.
- Around line 4973-4974: The filter in the extraArgs assignment at
argumentInfos.filter is incorrectly exempting arguments based solely on their
name matching entries in allKeyFieldPaths, without verifying they have proper
annotation. An argument named like a key field but lacking the `@openfed__is`
annotation should still be treated as an extra argument and included in the
filter result, as mappings are explicit-only. Modify the filter condition to not
just check !allKeyFieldPaths.has(a.name), but instead verify that an argument is
only exempted if it is actually properly annotated/mapped as a key field,
otherwise include it in extraArgs to prevent cache collisions.
- Around line 4866-4867: The `namedTypesMatch` function ignores list wrappers,
allowing types with remaining list ranks (like `[ID!]!`) to incorrectly match
scalar key types (like `ID!`) after a single `unwrapListType` call. In the
validation at the `namedTypesMatch` check, after calling
`this.unwrapListType(unwrapped)` to get `innerType`, verify that `innerType`
contains no remaining list wrappers before comparing with `keyFieldTypeNode`, or
explicitly check that the full type structure (including all list ranks)
matches, not just the named type component. This applies to all instances
mentioned in the additional locations as well.
- Around line 5090-5110: The error handling in the normalization factory is
incorrectly reporting incomplete keys even when alternative keys are satisfied.
Instead of immediately pushing an error when a key has explicit mappings but
incomplete fields (the condition checking explicitForThisKey.length > 0 &&
unmappedFields.length > 0), you should accumulate these incomplete key
candidates and only emit the error if no key produced a valid mapping at all.
Move the error reporting logic to occur after evaluating all keys, checking if
the keyFullySatisfied condition was never true across any of the keys being
processed, rather than erroring immediately upon encountering the first
incomplete key.
In `@composition/src/v1/normalization/types/types.ts`:
- Around line 172-186: The type declarations QueryCacheDirectiveNode and
QueryCacheArgumentNode are declared using `type` aliases but should be declared
using `interface` instead, as per the repository's TypeScript conventions for
object-shaped types. Convert both `export type QueryCacheDirectiveNode` and
`export type QueryCacheArgumentNode` declarations to use the `interface`
keyword, keeping all the property definitions exactly as they are, including the
union type in the value property of QueryCacheArgumentNode.
In `@composition/src/v1/warnings/params.ts`:
- Around line 26-30: Replace the `export type` keyword with `export interface`
for all five warning params definitions in this file:
SingleSubgraphInputFieldOneOfWarningParams,
SingleFederatedInputFieldOneOfWarningParams,
InvalidRepeatedComposedDirectiveWarningParams,
RequestScopedSingleFieldWarningParams, and
QueryCacheReturnEntityMissingEntityCacheWarningParams. This aligns with
TypeScript coding guidelines that prefer interfaces over type aliases for object
shapes.
In `@composition/src/v1/warnings/warnings.ts`:
- Around line 264-268: The message property in the warnings section uses string
concatenation with + operators across multiple lines instead of a single
template literal. Refactor this to use a single template literal that spans the
entire message, removing all the + operators and combining all the text portions
(from the fieldCoords field through the final sentence about config extraction)
into one continuous template literal block.
---
Outside diff comments:
In `@composition/src/errors/errors.ts`:
- Around line 2066-2328: Replace string concatenation using the `+` operator
with template literals in all the error message helper functions throughout this
file. Identify functions like queryCacheOnNonQueryFieldErrorMessage,
queryCacheOnNonEntityReturnTypeErrorMessage,
cacheInvalidateAndPopulateMutualExclusionErrorMessage,
isReferencesUnknownKeyFieldErrorMessage,
listArgumentToScalarKeySpecErrorMessage,
scalarArgumentToListKeySpecErrorMessage,
multipleListArgumentsBatchFactoryMessage,
inputObjectCompositeTypeMismatchErrorMessage,
inputObjectCompositeMissingFieldErrorMessage,
nestedKeyRequiresInputObjectErrorMessage,
nestedInputObjectTypeMismatchErrorMessage,
nestedInputObjectMissingFieldErrorMessage, and
nonInputArgumentCannotTargetCompositeKeyErrorMessage that currently use the `+`
operator to join string parts, and convert them to use backticks with embedded
expressions instead, consolidating each function's return statement into a
single template literal.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 1-25: Add SelectionNode to the graphql imports at the top of the
file. Then locate the three helper methods processRootFieldCacheDirectives,
extractQueryCacheConfig, and validateIsDirectivePlacement and add explicit void
return type annotations to each one. Finally, find the walkSelections method
(around line 4417) and replace the readonly any[] type with readonly
SelectionNode[] to use the specific type instead of the generic any type.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4692-4698: Convert the ArgumentInfo type definition from a type
alias to an interface to align with the repository's TypeScript coding
guidelines. Replace the type ArgumentInfo declaration syntax with an interface
ArgumentInfo declaration, keeping all the same properties (name, data,
isFieldValue, isList, typeNode) and their type annotations intact.
In `@composition/tests/v1/directives/query-cache.test.ts`:
- Around line 77-83: The test in query-cache.test.ts accesses
rootFieldConfigs![0] without first verifying that rootFieldConfigs exists and
contains at least one element. If the extraction regresses, this will throw an
error rather than providing a clear test failure message. Add explicit
assertions to check that rootFieldConfigs exists and has a length of at least 1
before attempting to access the first element with toMatchObject, ensuring
failures are clear and traceable rather than masked by runtime errors.
- Around line 371-1134: Add single-error cardinality assertions to all
validation test cases in the file. For each test that validates a failure
scenario (where errors[0] is checked), insert `expect(errors).toHaveLength(1)`
immediately before the existing `expect(errors[0]).toStrictEqual(...)`
assertions. This applies to all test cases within the `@openfed__queryCache` and
`@openfed__is` describe blocks, including those testing directive validation,
batch mappings, and input-object composite mappings. This ensures that exactly
one error is produced rather than silently allowing multiple unexpected errors
to pass the test.
🪄 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: 7a69b121-add2-4b02-8892-ef42f11c2281
⛔ 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 (16)
composition/src/directive-definition-data/directive-definition-data.tscomposition/src/errors/errors.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/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
+4866
to
+4867
| const innerType = this.unwrapListType(unwrapped); | ||
| if (!this.namedTypesMatch(innerType, keyFieldTypeNode)) { |
Contributor
There was a problem hiding this comment.
Preserve list rank when validating mapped argument types.
namedTypesMatch ignores list wrappers, so values like [[ID!]!]! can pass for a scalar ID! key after one unwrap, and deeper list ranks can pass list-key checks. Compare the post-batch element shape against the key shape, or explicitly reject remaining list wrappers for scalar keys.
Example direction
// List of lists - check inner type matches
- const innerType = this.unwrapListType(unwrapped);
- if (!this.namedTypesMatch(innerType, keyFieldTypeNode)) {
+ if (!this.typesMatchIncludingListShape(unwrapped, keyFieldTypeNode)) { // Check element type
const unwrapped = this.unwrapListType(argTypeNode);
- if (!this.namedTypesMatch(unwrapped, keyFieldTypeNode)) {
+ if (isTypeNodeListType(unwrapped) || !this.namedTypesMatch(unwrapped, keyFieldTypeNode)) {- if (!this.namedTypesMatch(argTypeNode, keyFieldTypeNode)) {
+ if (
+ argIsList && keyIsList
+ ? !this.typesMatchIncludingListShape(argTypeNode, keyFieldTypeNode)
+ : !this.namedTypesMatch(argTypeNode, keyFieldTypeNode)
+ ) {Also applies to: 4884-4887, 4945-4946
🤖 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 4866
- 4867, The `namedTypesMatch` function ignores list wrappers, allowing types
with remaining list ranks (like `[ID!]!`) to incorrectly match scalar key types
(like `ID!`) after a single `unwrapListType` call. In the validation at the
`namedTypesMatch` check, after calling `this.unwrapListType(unwrapped)` to get
`innerType`, verify that `innerType` contains no remaining list wrappers before
comparing with `keyFieldTypeNode`, or explicitly check that the full type
structure (including all list ranks) matches, not just the named type component.
This applies to all instances mentioned in the additional locations as well.
Comment on lines
+4967
to
+4969
| if (explicitMappings.length === 0) { | ||
| return compositeMappings; | ||
| } |
Contributor
There was a problem hiding this comment.
Validate composite-only mappings before returning them.
The early return compositeMappings skips the extra-argument and batch checks below. A composite-only field can therefore cache on input @openfed__is(...) while ignoring another argument, and a list-return field can accept a non-list composite input without establishing a batch mapping.
Move the unused-argument validation so it also runs when explicitMappings.length === 0, and reject isListReturn && !argInfo.isList in buildCompositeIsMapping before emitting field mappings.
Also applies to: 5185-5198
🤖 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 4967
- 4969, The early return when explicitMappings.length === 0 skips validation
checks that should also apply to composite-only mappings. Move the
unused-argument validation logic so it runs before the early return statement
that returns compositeMappings, ensuring these checks apply regardless of
whether explicit mappings exist. Additionally, in the buildCompositeIsMapping
function, add a validation check to reject cases where isListReturn is true but
argInfo.isList is false, and perform this validation before emitting any field
mappings to prevent invalid batch mapping configurations.
Comment on lines
+4973
to
+4974
| // Check for extra non-key arguments FIRST (not @openfed__is and not a key field in any key) | ||
| const extraArgs = argumentInfos.filter((a) => !a.isFieldValue && !allKeyFieldPaths.has(a.name)); |
Contributor
There was a problem hiding this comment.
Do not exempt unannotated key-named arguments.
Mappings are explicit-only, so an argument named like another @key field but without @openfed__is is still outside the cache key. The current filter can ignore such arguments and create cache collisions.
Proposed filter change
- 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: 5013-5014
🤖 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 4973
- 4974, The filter in the extraArgs assignment at argumentInfos.filter is
incorrectly exempting arguments based solely on their name matching entries in
allKeyFieldPaths, without verifying they have proper annotation. An argument
named like a key field but lacking the `@openfed__is` annotation should still be
treated as an extra argument and included in the filter result, as mappings are
explicit-only. Modify the filter condition to not just check
!allKeyFieldPaths.has(a.name), but instead verify that an argument is only
exempted if it is actually properly annotated/mapped as a key field, otherwise
include it in extraArgs to prevent cache collisions.
Comment on lines
+5090
to
+5110
| if (!keyFullySatisfied) { | ||
| // If this key has explicit mappings targeting it but is incomplete, error | ||
| if (explicitForThisKey.length > 0 && unmappedFields.length > 0) { | ||
| this.errors.push( | ||
| invalidDirectiveError( | ||
| OPENFED_IS, | ||
| `${fieldCoords}(${explicitForThisKey[0].argumentName}: ...)`, | ||
| FIRST_ORDINAL, | ||
| [ | ||
| explicitIncompleteCompositeKeyErrorMessage( | ||
| fieldCoords, | ||
| explicitForThisKey[0].argumentName, | ||
| explicitForThisKey[0].isFieldValue, | ||
| entityTypeName, | ||
| normalizedFieldSet, | ||
| unmappedFields[0], | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| return []; |
Contributor
There was a problem hiding this comment.
Only report incomplete keys when no alternative key is satisfied.
With overlapping keys such as @key(fields: "id") and @key(fields: "id sku"), a valid id @openfed__is(fields: "id") mapping satisfies the first key but this block still errors on the second key’s missing sku. Accumulate incomplete candidates and emit this error only if no key produced a mapping.
🤖 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 5090
- 5110, The error handling in the normalization factory is incorrectly reporting
incomplete keys even when alternative keys are satisfied. Instead of immediately
pushing an error when a key has explicit mappings but incomplete fields (the
condition checking explicitForThisKey.length > 0 && unmappedFields.length > 0),
you should accumulate these incomplete key candidates and only emit the error if
no key produced a valid mapping at all. Move the error reporting logic to occur
after evaluating all keys, checking if the keyFullySatisfied condition was never
true across any of the keys being processed, rather than erroring immediately
upon encountering the first incomplete key.
Comment on lines
+172
to
+186
| export type QueryCacheDirectiveNode = { | ||
| readonly arguments: ReadonlyArray<QueryCacheArgumentNode>; | ||
| readonly kind: Kind.DIRECTIVE; | ||
| readonly name: NameNode; | ||
| readonly loc?: Location; | ||
| }; | ||
|
|
||
| export type QueryCacheArgumentNode = { | ||
| readonly kind: Kind.ARGUMENT; | ||
| readonly name: NameNode; | ||
| // maxAge is Int; includeHeaders/shadowMode are Boolean. | ||
| // validateDirectives() guarantees each argument's value matches its declared type. | ||
| readonly value: IntValueNode | BooleanValueNode; | ||
| readonly loc?: Location; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check the exact content at lines 172-186
cat -n composition/src/v1/normalization/types/types.ts | sed -n '170,190p'Repository: wundergraph/cosmo
Length of output: 869
🏁 Script executed:
# Check if this file has other type definitions to understand the pattern
grep -n "export type\|export interface" composition/src/v1/normalization/types/types.ts | head -20Repository: wundergraph/cosmo
Length of output: 1067
🏁 Script executed:
# Check usage of these two types in the codebase
rg "QueryCacheDirectiveNode|QueryCacheArgumentNode" --type ts --type tsx -A 2 -B 2Repository: wundergraph/cosmo
Length of output: 89
🏁 Script executed:
# Search for usage of QueryCacheDirectiveNode and QueryCacheArgumentNode
rg "QueryCacheDirectiveNode|QueryCacheArgumentNode" --type ts -A 2 -B 2Repository: wundergraph/cosmo
Length of output: 1986
🏁 Script executed:
# Check the overall pattern in this file: count types vs interfaces
echo "=== Type aliases in file ===" && \
grep -c "^export type" composition/src/v1/normalization/types/types.ts && \
echo "=== Interfaces in file ===" && \
grep -c "^export interface" composition/src/v1/normalization/types/types.tsRepository: wundergraph/cosmo
Length of output: 122
🏁 Script executed:
# Check if these types use union types or discriminated unions (which require `type`)
sed -n '172,186p' composition/src/v1/normalization/types/types.ts | grep -E "\\||&"Repository: wundergraph/cosmo
Length of output: 112
🏁 Script executed:
# Check broader context: are there similar directive node types that use interface?
rg "DirectiveNode|ArgumentNode" composition/src/v1/normalization/types/types.ts -A 5 | head -40Repository: wundergraph/cosmo
Length of output: 943
Consider using interface instead of type for these object shapes.
QueryCacheDirectiveNode and QueryCacheArgumentNode are object-shaped types and should use interface declarations per TypeScript conventions for this repository (**/*.{ts,tsx} files prefer interfaces over type aliases for object shapes). The union type in the value property of QueryCacheArgumentNode does not prevent using an interface.
🤖 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 172 - 186, The
type declarations QueryCacheDirectiveNode and QueryCacheArgumentNode are
declared using `type` aliases but should be declared using `interface` instead,
as per the repository's TypeScript conventions for object-shaped types. Convert
both `export type QueryCacheDirectiveNode` and `export type
QueryCacheArgumentNode` declarations to use the `interface` keyword, keeping all
the property definitions exactly as they are, including the union type in the
value property of QueryCacheArgumentNode.
Source: Coding guidelines
Comment on lines
+264
to
+268
| message: | ||
| `Field "${fieldCoords}" has @openfed__queryCache and returns entity "${entityType}",` + | ||
| ` but "${entityType}" has no @openfed__entityCache directive. Add @openfed__entityCache(maxAge: ...)` + | ||
| ` to "${entityType}" to enable caching, or remove @openfed__queryCache from "${fieldCoords}".` + | ||
| ' The @openfed__queryCache config for this field was not extracted.', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get the context around lines 264-268
cd composition && head -300 src/v1/warnings/warnings.ts | tail -100Repository: wundergraph/cosmo
Length of output: 3588
Use a single template literal for the warning message text.
The message property uses string concatenation with + operators across multiple lines; this should be expressed as a single template literal per project guidelines for **/*.{ts,tsx} files.
Current code (lines 264-268)
message:
`Field "${fieldCoords}" has `@openfed__queryCache` and returns entity "${entityType}",` +
` but "${entityType}" has no `@openfed__entityCache` directive. Add `@openfed__entityCache`(maxAge: ...)` +
` to "${entityType}" to enable caching, or remove `@openfed__queryCache` from "${fieldCoords}".` +
' The `@openfed__queryCache` config for this field was not extracted.',
🤖 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/warnings/warnings.ts` around lines 264 - 268, The message
property in the warnings section uses string concatenation with + operators
across multiple lines instead of a single template literal. Refactor this to use
a single template literal that spans the entire message, removing all the +
operators and combining all the text portions (from the fieldCoords field
through the final sentence about config extraction) into one continuous template
literal block.
Source: Coding guidelines
…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
Reconcile queryCache's cache-extraction (old dispatcher structure) with requestScoped's
refactored inline structure (from cachePopulate review fixes):
- Adopt requestScoped's inline 1-arg extractCacheInvalidateConfig/extractCachePopulateConfig
(called during the parent walk); trim processRootFieldCacheDirectives() to only dispatch
@openfed__queryCache extraction + @openfed__is placement validation, re-add its normalize() call
- extractQueryCacheConfig writes via getOrInitializeEntityCaching (required-field type)
- Restore queryCache's richer maxAgeNotPositiveIntegerErrorMessage({directiveName, value}) and
update entityCache/cachePopulate call sites + tests to the object form
- Dedup imports (ENTITY_CACHE/OPENFED_ENTITY_CACHE/EntityCacheConfiguration/EntityCacheDirectiveNode)
- Regenerate node.pb.go (max_age_seconds non-optional); rename test helper to createSubgraphWithDefaultName
…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
…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
Author
|
It has been replaced by #2997 (identical change, now based on |
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
@openfed__queryCachedirective for per-query entity caching with configurable TTL, header inclusion, and shadow mode settings@openfed__isdirective to map query arguments to entity key fields, supporting scalar, composite, nested, and batch mapping scenariosTests