feat(composition): @openfed__requestScoped directive (5/5)#2985
Open
SkArchon wants to merge 2 commits into
Open
feat(composition): @openfed__requestScoped directive (5/5)#2985SkArchon wants to merge 2 commits into
SkArchon wants to merge 2 commits into
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 ChangesRequest-scoped and query-cache directives
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
SkArchon
force-pushed
the
milinda/requestScoped-directive
branch
from
2859cba to
faca30a
Compare
SkArchon
force-pushed
the
milinda/cachePopulate-directive
branch
from
c7044af to
b52c0a2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## milinda/queryCache-directive #2985 +/- ##
================================================================
+ Coverage 47.31% 47.37% +0.05%
================================================================
Files 1119 1119
Lines 153254 153374 +120
Branches 10181 10193 +12
================================================================
+ Hits 72514 72654 +140
+ Misses 78923 78916 -7
+ Partials 1817 1804 -13
🚀 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: 1
🧹 Nitpick comments (4)
composition/src/v1/normalization/types/types.ts (1)
142-154: ⚡ Quick winUse interfaces for the new directive AST object shapes.
Lines 142-154 define object-shape
typealiases; convert them tointerfacedeclarations to match the TypeScript convention in the guidelines.As per coding guidelines,
**/*.{ts,tsx}should prefer interfaces over type aliases for object shapes.🤖 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 142 - 154, Convert the type aliases RequestScopedDirectiveNode and RequestScopedArgumentNode to interface declarations to align with TypeScript coding guidelines that prefer interfaces for object shapes. Change each `type` keyword to `interface`, remove the equals sign, and keep all the properties and their readonly modifiers intact within the interface body.Source: Coding guidelines
composition/src/router-configuration/types.ts (1)
89-98: ⚡ Quick winPrefer an interface for this new object-shape type.
Line 89 introduces an object-shape
typealias; switch it to aninterfaceto align with repository TypeScript rules.As per coding guidelines,
**/*.{ts,tsx}should prefer interfaces over type aliases for object shapes.Proposed change
-export type RequestScopedFieldConfig = { +export interface RequestScopedFieldConfig { fieldName: FieldName; typeName: TypeName; // L1 cache key used to store/lookup this field's value for the duration of a request. // Format: "{subgraphName}.{key}" where `key` is the `@openfed__requestScoped`(key:) argument. // All fields in the same subgraph declaring `@openfed__requestScoped` with the same key share // the same L1 entry — the first one to resolve populates it, subsequent ones inject // from it (subject to widening checks and alias-aware normalization). l1Key: string; -}; +}🤖 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/router-configuration/types.ts` around lines 89 - 98, The RequestScopedFieldConfig definition uses a type alias for an object shape, but repository TypeScript conventions prefer interfaces for object shapes. Convert the type alias declaration to an interface by replacing the equals sign syntax with interface syntax, keeping all the properties and their documentation comments intact.Source: Coding guidelines
composition/src/v1/normalization/normalization-factory.ts (1)
4250-4261: ⚡ Quick winAdd an explicit return type and avoid object-shape type aliases in the new method.
Line 4250 should declare an explicit return type, and Lines 4255-4261 should use an
interface(or equivalent) instead of an object-shapetypealias.As per coding guidelines, TypeScript functions should use explicit type annotations for return types, and object shapes should prefer interfaces over type aliases.
Proposed change
- extractRequestScopedFields() { + extractRequestScopedFields(): void { @@ - type Collected = { + interface Collected { typeName: string; fieldName: string; fieldCoords: string; key: string; l1Key: string; - }; + }🤖 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 4250 - 4261, The extractRequestScopedFields method lacks an explicit return type annotation, and the Collected type alias with object shape should be converted to an interface according to coding guidelines. Add a return type annotation to the extractRequestScopedFields function signature that returns an array or collection of the Collected items, and convert the Collected type alias definition to an interface declaration instead of a type alias.Source: Coding guidelines
composition/src/v1/warnings/params.ts (1)
20-24: ⚡ Quick winPrefer an interface for warning params shape.
Line 20 adds an object-shape
typealias; please switch this to aninterfacefor consistency with the TypeScript rules.As per coding guidelines,
**/*.{ts,tsx}should prefer interfaces over type aliases for object shapes.🤖 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/params.ts` around lines 20 - 24, The RequestScopedSingleFieldWarningParams is currently defined as a type alias but should be converted to an interface for consistency with TypeScript coding guidelines that prefer interfaces over type aliases for object shapes. Replace the type keyword with interface while keeping the same object shape definition with subgraphName, key, and fieldCoords properties unchanged.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 4300-4302: In the loop iterating over coordsByKey, the condition
checking coordsList.length uses the loose equality operator (==) instead of
strict equality (===). Replace the == operator with === in the coordsList.length
== 1 comparison to comply with strict equality requirements and avoid potential
type coercion issues.
---
Nitpick comments:
In `@composition/src/router-configuration/types.ts`:
- Around line 89-98: The RequestScopedFieldConfig definition uses a type alias
for an object shape, but repository TypeScript conventions prefer interfaces for
object shapes. Convert the type alias declaration to an interface by replacing
the equals sign syntax with interface syntax, keeping all the properties and
their documentation comments intact.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4250-4261: The extractRequestScopedFields method lacks an explicit
return type annotation, and the Collected type alias with object shape should be
converted to an interface according to coding guidelines. Add a return type
annotation to the extractRequestScopedFields function signature that returns an
array or collection of the Collected items, and convert the Collected type alias
definition to an interface declaration instead of a type alias.
In `@composition/src/v1/normalization/types/types.ts`:
- Around line 142-154: Convert the type aliases RequestScopedDirectiveNode and
RequestScopedArgumentNode to interface declarations to align with TypeScript
coding guidelines that prefer interfaces for object shapes. Change each `type`
keyword to `interface`, remove the equals sign, and keep all the properties and
their readonly modifiers intact within the interface body.
In `@composition/src/v1/warnings/params.ts`:
- Around line 20-24: The RequestScopedSingleFieldWarningParams is currently
defined as a type alias but should be converted to an interface for consistency
with TypeScript coding guidelines that prefer interfaces over type aliases for
object shapes. Replace the type keyword with interface while keeping the same
object shape definition with subgraphName, key, and fieldCoords properties
unchanged.
🪄 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: 97ff9f05-9eb4-445c-902b-3c0c09d00fb7
⛔ 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 (14)
composition/src/directive-definition-data/directive-definition-data.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/request-scoped.test.tsconnect/src/wg/cosmo/node/v1/node_pb.tsproto/wg/cosmo/node/v1/node.protoshared/src/router-config/builder.ts
Restacks @openfed__requestScoped on top of the queryCache branch. The resulting tree is identical to the previous combined stack tip; this commit re-applies the requestScoped feature (including its assertions in shared/test/entity-caching.test.ts) on top of queryCache.
SkArchon
changed the title
This was referenced Jun 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
4290-4491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten TypeScript typing in the new helper set (
any+ implicit returns).The newly added helpers rely on inferred return types, and Line 4408 uses
readonly any[]. This weakens type safety in a critical normalization path.♻️ Suggested typing direction
- extractQueryCacheConfig( + 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: ReadonlyArray<SelectionNode>, + currentTypeName: string, + pathPrefix: string, + ): void => {// If not already present: import type { SelectionNode } from 'graphql';As per coding guidelines,
**/*.{ts,tsx}requires explicit function return annotations and avoidinganyin TypeScript.🤖 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 4290 - 4491, The new helper methods lack explicit return type annotations and the walkSelections function uses `readonly any[]` instead of properly typed parameters, violating TypeScript coding guidelines. Add explicit return type annotations (void or appropriate types) to extractQueryCacheConfig and validateIsDirectivePlacement methods. In the walkSelections nested function within extractKeyFieldInfos, replace the `readonly any[]` parameter type with the proper GraphQL AST type `readonly SelectionNode[]`. Import SelectionNode from the graphql package if not already imported 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.
Inline comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4965-4966: The filter for determining extraArgs at line 4965 and
the similar filter at line 5005 are currently excluding unmapped arguments when
their name matches a key-field path in allKeyFieldPaths, which can lead to
unsafe incomplete cache-key coverage. Remove the `!allKeyFieldPaths.has(a.name)`
condition from both filter expressions so that all unmapped arguments (those
where !a.isFieldValue is true) are treated as extra arguments regardless of
whether their name happens to match any key-field path.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4290-4491: The new helper methods lack explicit return type
annotations and the walkSelections function uses `readonly any[]` instead of
properly typed parameters, violating TypeScript coding guidelines. Add explicit
return type annotations (void or appropriate types) to extractQueryCacheConfig
and validateIsDirectivePlacement methods. In the walkSelections nested function
within extractKeyFieldInfos, replace the `readonly any[]` parameter type with
the proper GraphQL AST type `readonly SelectionNode[]`. Import SelectionNode
from the graphql package if not already imported at the top of the file.
🪄 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: a033a452-0d2b-463c-a18a-d7f8fe0f042e
⛔ 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
✅ Files skipped from review due to trivial changes (1)
- connect/src/wg/cosmo/node/v1/node_pb.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- composition/src/v1/normalization/utils.ts
- composition/src/directive-definition-data/directive-definition-data.ts
- shared/src/router-config/builder.ts
- composition/src/v1/warnings/params.ts
- proto/wg/cosmo/node/v1/node.proto
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
4290-4491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten TypeScript typing in the new helper set (
any+ implicit returns).The newly added helpers rely on inferred return types, and Line 4408 uses
readonly any[]. This weakens type safety in a critical normalization path.♻️ Suggested typing direction
- extractQueryCacheConfig( + 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: ReadonlyArray<SelectionNode>, + currentTypeName: string, + pathPrefix: string, + ): void => {// If not already present: import type { SelectionNode } from 'graphql';As per coding guidelines,
**/*.{ts,tsx}requires explicit function return annotations and avoidinganyin TypeScript.🤖 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 4290 - 4491, The new helper methods lack explicit return type annotations and the walkSelections function uses `readonly any[]` instead of properly typed parameters, violating TypeScript coding guidelines. Add explicit return type annotations (void or appropriate types) to extractQueryCacheConfig and validateIsDirectivePlacement methods. In the walkSelections nested function within extractKeyFieldInfos, replace the `readonly any[]` parameter type with the proper GraphQL AST type `readonly SelectionNode[]`. Import SelectionNode from the graphql package if not already imported 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.
Inline comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4965-4966: The filter for determining extraArgs at line 4965 and
the similar filter at line 5005 are currently excluding unmapped arguments when
their name matches a key-field path in allKeyFieldPaths, which can lead to
unsafe incomplete cache-key coverage. Remove the `!allKeyFieldPaths.has(a.name)`
condition from both filter expressions so that all unmapped arguments (those
where !a.isFieldValue is true) are treated as extra arguments regardless of
whether their name happens to match any key-field path.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4290-4491: The new helper methods lack explicit return type
annotations and the walkSelections function uses `readonly any[]` instead of
properly typed parameters, violating TypeScript coding guidelines. Add explicit
return type annotations (void or appropriate types) to extractQueryCacheConfig
and validateIsDirectivePlacement methods. In the walkSelections nested function
within extractKeyFieldInfos, replace the `readonly any[]` parameter type with
the proper GraphQL AST type `readonly SelectionNode[]`. Import SelectionNode
from the graphql package if not already imported at the top of the file.
🪄 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: a033a452-0d2b-463c-a18a-d7f8fe0f042e
⛔ 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
✅ Files skipped from review due to trivial changes (1)
- connect/src/wg/cosmo/node/v1/node_pb.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- composition/src/v1/normalization/utils.ts
- composition/src/directive-definition-data/directive-definition-data.ts
- shared/src/router-config/builder.ts
- composition/src/v1/warnings/params.ts
- proto/wg/cosmo/node/v1/node.proto
🛑 Comments failed to post (1)
composition/src/v1/normalization/normalization-factory.ts (1)
4965-4966: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat unmapped arguments as extra regardless of argument name.
Line 4965 and Line 5005 currently exclude non-
@openfed__isarguments fromextraArgswhen the argument name happens to match any key-field path. That can let query-cache extraction accept incomplete cache-key coverage and produce unsafe mappings.💡 Suggested 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: 5005-5006
🤖 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 4965 - 4966, The filter for determining extraArgs at line 4965 and the similar filter at line 5005 are currently excluding unmapped arguments when their name matches a key-field path in allKeyFieldPaths, which can lead to unsafe incomplete cache-key coverage. Remove the `!allKeyFieldPaths.has(a.name)` condition from both filter expressions so that all unmapped arguments (those where !a.isFieldValue is true) are treated as extra arguments regardless of whether their name happens to match any key-field path.
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
1 participant
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__requestScoped— request-scoped L1 field caching, keyed by{subgraphName}.{key}; fields sharing a key share an L1 entry. AddsRequestScopedFieldConfigurationproto (field 4) + bindings, theRequestScopedFieldConfigtype, normalization, a single-field warning, and tests.More documentation on this: https://app.notion.com/p/wundergraph/Directives-37bb19e0a0ec80b9b505d896beed5897
Summary by CodeRabbit
@openfed__requestScoped(key: String!)to configure per-field request-scoped caching with shared L1 coordination, including new router configuration and engine wiring.@openfed__queryCacheplus@openfed__iskey-mapping so query cache keys can be derived from entity@keyfields and nested inputs.