feat(composition): entity caching directives, proto config types, and generated bindings (1/6)#2944
Draft
SkArchon wants to merge 3 commits into
Draft
feat(composition): entity caching directives, proto config types, and generated bindings (1/6)#2944SkArchon wants to merge 3 commits into
SkArchon wants to merge 3 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (16)
WalkthroughThis PR adds OpenFed entity caching directives ( ChangesEntity Caching and Request-Scoped Fields
🎯 4 (Complex) | ⏱️ ~60 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. ✨ Finishing Touches📝 Generate docstrings
Comment |
![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: 6
🧹 Nitpick comments (3)
composition/tests/v1/directives/entity-caching.test.ts (1)
683-691: ⚡ Quick winUse dot-notation in
@openfed__is(fields: ...)for nested key paths.This case uses field-set syntax (
"store { id }"). Prefer canonical dot-path syntax ("store.id") so the test matches the declared mapping contract directly.Suggested test SDL tweak
- storeKey: ProductStoreKey! `@openfed__is`(fields: "store { id }") + storeKey: ProductStoreKey! `@openfed__is`(fields: "store.id")As per coding guidelines, “Nested keys use dot-notation paths ... and the
@openfed__isdirective references these dot-notation paths directly.”🤖 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/entity-caching.test.ts` around lines 683 - 691, The test "config: `@openfed__is` can map scalar and composite keys on the same field" uses field-set syntax in the subgraph SDL; update the `@openfed__is`(fields: ...) usage to dot-notation by changing the second directive from fields: "store { id }" to fields: "store.id" so the test SDL in getConfigForType(subgraph(...)) matches the canonical nested-key contract.Source: Coding guidelines
composition/tests/v1/directives/entity-cache-mapping-rules.test.ts (1)
2819-2901: ⚡ Quick winStrengthen rules 40/40b/40c with exact failure assertions.
These tests only assert
errors.length > 0, so unrelated failures can still pass. Pin at least the directive/error family (or exact message) like the rest of this suite.Based on learnings, “Entity caching validation behavior and type-aware mapping rules are the canonical source of truth in test files entity-caching.test.ts and entity-cache-mapping-rules.test.ts.”
🤖 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/entity-cache-mapping-rules.test.ts` around lines 2819 - 2901, The tests for rules 40/40b/40c currently only assert errors.length > 0; tighten them to assert the specific directive/error family so unrelated failures don't pass. In each test (named "rule 40...", "rule 40b...", "rule 40c...") replace the loose expect(errors.length).toBeGreaterThan(0) with an assertion that the errors include the entity-caching validation error from openfed__entityCache/openfed__queryCache (e.g., check that some error.message contains the canonical entity caching validation text or a unique token used elsewhere in entity-caching.test.ts), using normalizeSubgraphFailure's returned errors array to match on the exact message or error code for the rule. Ensure the assertion references the same canonical message used in the entity-caching.test.ts suite so it exactly pins the intended validation failure.Source: Learnings
composition/tests/v1/directives/entity-cache-fuzz.test.ts (1)
27-37: ⚡ Quick winConsolidated: explicit return types are missing across new TS test helpers/factories.
The same typing-rule violation appears in all three new test modules. Please annotate explicit return types consistently (
: Subgraph,: string, and explicit helper return types) to keep contracts compiler-enforced and uniform.As per coding guidelines, “Use explicit type annotations for function parameters and return types in 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/tests/v1/directives/entity-cache-fuzz.test.ts` around lines 27 - 37, The helper functions in the test files lack explicit return type annotations; update getConfigForType and any other test helpers in these modules to declare an explicit return type (rather than relying on inference). Concretely, add a return type to getConfigForType (the function that returns internal!.configurationDataByTypeName.get(...)) using the correct type from the batch normalization result (the Configuration/ConfigurationData type used with BatchNormalizationSuccess and TypeName), and ensure any other helpers in the new test modules also have explicit parameter and return type annotations to match the coding guideline.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/AGENTS.md`:
- Around line 11-25: The fenced code block showing the key mapping pipeline in
AGENTS.md is missing a language identifier; update the triple-backtick that
opens the block to include a language (for example ```text or ```mermaid) so
markdown linting and rendering work correctly. Locate the block that lists the
pipeline symbols (extractKeyFieldSets(), KeyFieldSetData,
keyFieldSetDatasByTypeName, validateKeyFieldSets(), extractPerKeyFieldPaths(),
buildArgumentKeyMappings(), RootFieldCacheConfig.entityKeyMappings) and modify
the opening fence to add the chosen language identifier.
In `@composition/CLAUDE.md`:
- Line 105: The file CLAUDE.md contains fenced code blocks with missing language
identifiers (causing markdownlint MD040); update each triple-backtick fenced
block in CLAUDE.md to include an explicit language tag (e.g., ```bash,
```graphql, ```text) appropriate to the snippet so all code fences (the ```
blocks) have language identifiers and the file passes markdownlint.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 5822-5825: The cache extraction runs too early:
processRootFieldCacheDirectives() (which calls
getOperationTypeNodeForRootTypeName()) is executed before
operationTypeNodeByTypeName is populated, so move the call that triggers cache
extraction (currently validateAndExtractEntityCachingConfigs()) to after the
step that registers/sets up renamed root operations and populates
operationTypeNodeByTypeName (the block around where operationTypeNodeByTypeName
is filled, currently after extractRequestScopedFields()). Ensure
processRootFieldCacheDirectives() is invoked only after
operationTypeNodeByTypeName exists so
`@openfed__queryCache/`@openfed__cacheInvalidate/@openfed__cachePopulate on
renamed root types (e.g., MyQuery) are discovered.
- Around line 4166-4169: The code silently returns null when inputData is
missing or not an INPUT_OBJECT_TYPE_DEFINITION; instead, before returning, emit
a directive validation error so callers know the nested field type is
scalar/unknown (which breaks auto-mapping). Update the branch that checks
inputData/Kind.INPUT_OBJECT_TYPE_DEFINITION in normalization-factory.ts to call
the factory's existing error/reporting helper (e.g., report a directive
validation error via this.report.../this.add.../this.push... error mechanism)
with a clear message mentioning the inputTypeName and the parent field, then
return null; this will ensure buildCompositeIsMapping() and callers see that an
error was recorded rather than assuming null was already reported.
- Around line 5039-5055: The call to invalidateAutoMappingWithExtraArgument
inside the fieldMappings branch is suppressing the auto-mapping warning by
passing false for the "emit warning" flag, which discards mappings but does not
log the required warning; update the invocation in normalization-factory.ts
(inside the block handling fieldMappings and the variable results) to pass the
flag that causes a warning to be emitted (i.e., true or the named parameter that
enables warnings) so that invalidateAutoMappingWithExtraArgument still clears
all mappings and returns the same control flow but also emits the auto-mapping
warning for the nested input-object case.
- Around line 4634-4646: The duplicate-mapping check crashes because
argumentInfos.find((a) => a.isFieldValue === argInfo.name)! assumes an argument
has isFieldValue equal to the field name; for composite `@openfed__is` this can be
undefined. Update the error construction in the loop that uses mappedKeyFields
and argumentInfos so it does not rely on that find: use argInfo.name directly
(or use argumentInfos.find(a => a.name === argInfo.name)?.name ?? argInfo.name)
when composing the displayed argument text, and/or guard the find with a safe
fallback to avoid throwing; keep references to mappedKeyFields, argumentInfos
and the loop that pushes invalidDirectiveError(IS, ...).
---
Nitpick comments:
In `@composition/tests/v1/directives/entity-cache-fuzz.test.ts`:
- Around line 27-37: The helper functions in the test files lack explicit return
type annotations; update getConfigForType and any other test helpers in these
modules to declare an explicit return type (rather than relying on inference).
Concretely, add a return type to getConfigForType (the function that returns
internal!.configurationDataByTypeName.get(...)) using the correct type from the
batch normalization result (the Configuration/ConfigurationData type used with
BatchNormalizationSuccess and TypeName), and ensure any other helpers in the new
test modules also have explicit parameter and return type annotations to match
the coding guideline.
In `@composition/tests/v1/directives/entity-cache-mapping-rules.test.ts`:
- Around line 2819-2901: The tests for rules 40/40b/40c currently only assert
errors.length > 0; tighten them to assert the specific directive/error family so
unrelated failures don't pass. In each test (named "rule 40...", "rule 40b...",
"rule 40c...") replace the loose expect(errors.length).toBeGreaterThan(0) with
an assertion that the errors include the entity-caching validation error from
openfed__entityCache/openfed__queryCache (e.g., check that some error.message
contains the canonical entity caching validation text or a unique token used
elsewhere in entity-caching.test.ts), using normalizeSubgraphFailure's returned
errors array to match on the exact message or error code for the rule. Ensure
the assertion references the same canonical message used in the
entity-caching.test.ts suite so it exactly pins the intended validation failure.
In `@composition/tests/v1/directives/entity-caching.test.ts`:
- Around line 683-691: The test "config: `@openfed__is` can map scalar and
composite keys on the same field" uses field-set syntax in the subgraph SDL;
update the `@openfed__is`(fields: ...) usage to dot-notation by changing the
second directive from fields: "store { id }" to fields: "store.id" so the test
SDL in getConfigForType(subgraph(...)) matches the canonical nested-key
contract.
🪄 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: d8dcf2de-68c4-4808-a4fa-fb729e5c8ba4
⛔ 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 (26)
composition-go/index.global.jscomposition/AGENTS.mdcomposition/CLAUDE.mdcomposition/COMPOSITION_CONVENTIONS.mdcomposition/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/constants/type-nodes.tscomposition/src/v1/normalization/directive-definition-data.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/utils.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/tests/v1/directives/entity-cache-fuzz.test.tscomposition/tests/v1/directives/entity-cache-mapping-rules.test.tscomposition/tests/v1/directives/entity-caching.test.tsconnect/src/wg/cosmo/graphqlmetrics/v1/graphqlmetrics-GraphQLMetricsService_connectquery.tsconnect/src/wg/cosmo/graphqlmetrics/v1/graphqlmetrics_connect.tsconnect/src/wg/cosmo/graphqlmetrics/v1/graphqlmetrics_pb.tsconnect/src/wg/cosmo/node/v1/node_pb.tsgraphqlmetrics/README.mdproto/wg/cosmo/node/v1/node.protoshared/src/router-config/builder.tsshared/src/router-config/graphql-configuration.ts
Comment on lines
+11
to
+25
| ``` | ||
| @key(fields: "...") on entity type | ||
| ↓ | ||
| extractKeyFieldSets() → KeyFieldSetData { documentNode, normalizedFieldSet, isUnresolvable } | ||
| ↓ | ||
| keyFieldSetDatasByTypeName: Map<TypeName, Map<normalizedFieldSet, KeyFieldSetData>> | ||
| ↓ | ||
| validateKeyFieldSets() → keyFieldPathsByTypeNameByFieldSet (per-key dot-notation paths) | ||
| ↓ | ||
| extractPerKeyFieldPaths() → Array<{normalizedFieldSet, isUnresolvable, fieldPaths: Set<string>}> | ||
| ↓ | ||
| buildArgumentKeyMappings() → EntityKeyMappingConfig[] (one per fully-satisfiable key) | ||
| ↓ | ||
| RootFieldCacheConfig.entityKeyMappings → serialized to protobuf → router | ||
| ``` |
Contributor
There was a problem hiding this comment.
Specify language for fenced code block.
The code block illustrating the key mapping pipeline should specify a language identifier (e.g., text or mermaid) for proper rendering and to satisfy markdown linting rules.
📝 Proposed fix
-```
+```text
`@key`(fields: "...") on entity type
↓📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| @key(fields: "...") on entity type | |
| ↓ | |
| extractKeyFieldSets() → KeyFieldSetData { documentNode, normalizedFieldSet, isUnresolvable } | |
| ↓ | |
| keyFieldSetDatasByTypeName: Map<TypeName, Map<normalizedFieldSet, KeyFieldSetData>> | |
| ↓ | |
| validateKeyFieldSets() → keyFieldPathsByTypeNameByFieldSet (per-key dot-notation paths) | |
| ↓ | |
| extractPerKeyFieldPaths() → Array<{normalizedFieldSet, isUnresolvable, fieldPaths: Set<string>}> | |
| ↓ | |
| buildArgumentKeyMappings() → EntityKeyMappingConfig[] (one per fully-satisfiable key) | |
| ↓ | |
| RootFieldCacheConfig.entityKeyMappings → serialized to protobuf → router | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/AGENTS.md` around lines 11 - 25, The fenced code block showing
the key mapping pipeline in AGENTS.md is missing a language identifier; update
the triple-backtick that opens the block to include a language (for example
```text or ```mermaid) so markdown linting and rendering work correctly. Locate
the block that lists the pipeline symbols (extractKeyFieldSets(),
KeyFieldSetData, keyFieldSetDatasByTypeName, validateKeyFieldSets(),
extractPerKeyFieldPaths(), buildArgumentKeyMappings(),
RootFieldCacheConfig.entityKeyMappings) and modify the opening fence to add the
chosen language identifier.
Source: Linters/SAST tools
|
|
||
| ### Directive Hierarchy | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Missing fenced-code language tags across docs (same MD040 root cause).
Add explicit language identifiers (for example bash, graphql, text) to these fences to satisfy markdownlint and keep docs lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/CLAUDE.md` at line 105, The file CLAUDE.md contains fenced code
blocks with missing language identifiers (causing markdownlint MD040); update
each triple-backtick fenced block in CLAUDE.md to include an explicit language
tag (e.g., ```bash, ```graphql, ```text) appropriate to the snippet so all code
fences (the ``` blocks) have language identifiers and the file passes
markdownlint.
Source: Linters/SAST tools
SkArchon
force-pushed
the
milinda/entity-caching-1-composition-proto
branch
from
eaced7f to
41292d0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
composition/tests/v1/directives/entity-cache-fuzz.test.ts (1)
304-310: ⚡ Quick winClarify or remove the bug comment with line number references.
The comment describes a bug and references "lines 4921-4931" which are not in the provided context. This line number reference could be stale, refer to a different file, or become outdated during refactoring. Since the test expects the correct behavior (2 separate mappings), consider either:
- Removing the line number reference and just describing the edge case being tested
- Adding a file path/context to make the reference meaningful
- Replacing the "BUG:" prefix with "EDGE CASE:" if this documents a potential pitfall rather than an actual bug
Suggested revision
- // BUG: Two independent `@key` directives (`@key`(fields: "id") and `@key`(fields: "sku")) - // each produce a single-field EntityKeyMappingConfig. But buildAutoMappings() at lines - // 4921-4931 merges all single-field results into ONE EntityKeyMappingConfig with both - // field mappings. This makes the router treat them as a composite key (AND semantics: - // both "id" AND "sku" must match) instead of independent keys (OR semantics: either - // "id" OR "sku" can be used for a cache hit). The merge is wrong — independent keys - // should remain separate EntityKeyMappingConfig entries. + // EDGE CASE: Two independent `@key` directives (`@key`(fields: "id") and `@key`(fields: "sku")) + // must produce separate EntityKeyMappingConfig entries (OR semantics: either "id" OR "sku" + // can be used for a cache hit). Incorrectly merging them into ONE EntityKeyMappingConfig + // would create a composite key (AND semantics: both must match), breaking cache lookups. + // This test verifies the correct behavior: two separate key mappings. test('12. Two `@key` directives, arguments satisfy both fully — should produce two key mappings', () => {🤖 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/entity-cache-fuzz.test.ts` around lines 304 - 310, Edit the comment in composition/tests/v1/directives/entity-cache-fuzz.test.ts that starts with "BUG:" and references specific line numbers for buildAutoMappings(); remove the stale line number reference and either replace the "BUG:" prefix with "EDGE CASE:" or rewrite the comment to simply describe the edge case being tested (two independent `@key` directives producing separate EntityKeyMappingConfig entries) so it documents intent without relying on fragile line numbers; ensure the comment still mentions buildAutoMappings() and EntityKeyMappingConfig so readers can locate the relevant logic.composition/src/router-configuration/types.ts (1)
89-193: 💤 Low valueConsider using interfaces instead of type aliases for object shapes.
The coding guidelines recommend preferring interfaces over type aliases for object shapes in TypeScript. The new cache configuration types (
RequestScopedFieldConfig,EntityCacheConfig,RootFieldCacheConfig,EntityKeyMappingConfig,FieldMappingConfig,CachePopulateConfig,CacheInvalidateConfig) are all pure object shapes that could useinterfaceinstead oftype.Example refactor
-export type RequestScopedFieldConfig = { +export interface RequestScopedFieldConfig { fieldName: FieldName; typeName: TypeName; l1Key: string; -}; +}Apply similar changes to the other cache configuration types.
As per coding guidelines: "Prefer interfaces over type aliases for object shapes in 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/router-configuration/types.ts` around lines 89 - 193, The review asks to replace object-shaped type aliases with interfaces; update the declarations for RequestScopedFieldConfig, EntityCacheConfig, RootFieldCacheConfig, EntityKeyMappingConfig, FieldMappingConfig, CachePopulateConfig, and CacheInvalidateConfig from "export type X = { ... }" to "export interface X { ... }" preserving all property names, types and optional modifiers (e.g., ?), keeping exports identical so external usage doesn't change, and leave the existing non-object types (ConfigurationData with Sets/Arrays) as-is; ensure there are no remaining "type" definitions for these seven symbols and run type-check to confirm no downstream breakage.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/tests/v1/directives/entity-cache-fuzz.test.ts`:
- Around line 31-37: The helper getConfigForType lacks an explicit return type;
update its signature to include the concrete type returned from
internal.configurationDataByTypeName.get(...) (for example ConfigurationData |
undefined or the exact type stored in configurationDataByTypeName) so TypeScript
explicitly knows the return type; modify the function declaration
getConfigForType(sg: Subgraph, typeName: string): <ReturnType> and ensure
<ReturnType> matches the element type of configurationDataByTypeName (use
TypeName for the key if needed) without changing the function body.
---
Nitpick comments:
In `@composition/src/router-configuration/types.ts`:
- Around line 89-193: The review asks to replace object-shaped type aliases with
interfaces; update the declarations for RequestScopedFieldConfig,
EntityCacheConfig, RootFieldCacheConfig, EntityKeyMappingConfig,
FieldMappingConfig, CachePopulateConfig, and CacheInvalidateConfig from "export
type X = { ... }" to "export interface X { ... }" preserving all property names,
types and optional modifiers (e.g., ?), keeping exports identical so external
usage doesn't change, and leave the existing non-object types (ConfigurationData
with Sets/Arrays) as-is; ensure there are no remaining "type" definitions for
these seven symbols and run type-check to confirm no downstream breakage.
In `@composition/tests/v1/directives/entity-cache-fuzz.test.ts`:
- Around line 304-310: Edit the comment in
composition/tests/v1/directives/entity-cache-fuzz.test.ts that starts with
"BUG:" and references specific line numbers for buildAutoMappings(); remove the
stale line number reference and either replace the "BUG:" prefix with "EDGE
CASE:" or rewrite the comment to simply describe the edge case being tested (two
independent `@key` directives producing separate EntityKeyMappingConfig entries)
so it documents intent without relying on fragile line numbers; ensure the
comment still mentions buildAutoMappings() and EntityKeyMappingConfig so readers
can locate the relevant logic.
🪄 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: 9949845c-df6b-452c-b443-47f939f1b644
⛔ 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-go/index.global.jscomposition/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/constants/type-nodes.tscomposition/src/v1/normalization/directive-definition-data.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/utils.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/tests/v1/directives/entity-cache-fuzz.test.tscomposition/tests/v1/directives/entity-cache-mapping-rules.test.tscomposition/tests/v1/directives/entity-caching.test.tsconnect/src/wg/cosmo/node/v1/node_pb.tsproto/wg/cosmo/node/v1/node.protoshared/src/router-config/builder.tsshared/src/router-config/graphql-configuration.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 (13)
- composition/src/v1/constants/type-nodes.ts
- composition/src/v1/warnings/params.ts
- composition/src/v1/normalization/utils.ts
- composition/src/v1/constants/constants.ts
- shared/src/router-config/graphql-configuration.ts
- composition/src/v1/warnings/warnings.ts
- proto/wg/cosmo/node/v1/node.proto
- composition/tests/v1/directives/entity-cache-mapping-rules.test.ts
- composition/src/v1/normalization/directive-definition-data.ts
- composition/src/v1/constants/directive-definitions.ts
- composition/tests/v1/directives/entity-caching.test.ts
- composition/src/errors/errors.ts
- composition/src/v1/normalization/normalization-factory.ts
SkArchon
changed the title
SkArchon
force-pushed
the
milinda/entity-caching-1-composition-proto
branch
from
41292d0 to
09d98fb
Compare
Router image scan passed✅ No security vulnerabilities found in image: |
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 (1)
composition/src/directive-definition-data/directive-definition-data.ts (1)
1-1124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix CI-blocking formatting issues in this file.
Line 1 onward:
prettier --checkis failing for this file, and the lint job exits non-zero. Please run Prettier and commit the formatting changes so CI can pass.🤖 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/directive-definition-data/directive-definition-data.ts` around lines 1 - 1124, Prettier formatting errors are causing CI to fail for this file; run Prettier to reformat and commit the changes. Execute the formatter (e.g., prettier --write) on composition/src/directive-definition-data/directive-definition-data.ts, verify the file compiles, then stage and commit the updated file; no logic changes are needed—only formatting of the usages of newDirectiveDefinitionData and newDirectiveArgumentData and the directive constant blocks in this file.Source: Pipeline failures
🤖 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/directive-definition-data/directive-definition-data.ts`:
- Around line 1-1124: Prettier formatting errors are causing CI to fail for this
file; run Prettier to reformat and commit the changes. Execute the formatter
(e.g., prettier --write) on
composition/src/directive-definition-data/directive-definition-data.ts, verify
the file compiles, then stage and commit the updated file; no logic changes are
needed—only formatting of the usages of newDirectiveDefinitionData and
newDirectiveArgumentData and the directive constant blocks in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf123e63-9406-4a62-9bf2-93ee28f9c0f4
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (15)
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/constants/type-nodes.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/utils.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/tests/v1/directives/entity-cache-fuzz.test.tscomposition/tests/v1/directives/entity-cache-mapping-rules.test.tscomposition/tests/v1/directives/entity-caching.test.tsconnect/src/wg/cosmo/node/v1/node_pb.ts
💤 Files with no reviewable changes (4)
- connect/src/wg/cosmo/node/v1/node_pb.ts
- composition/tests/v1/directives/entity-caching.test.ts
- composition/src/v1/normalization/normalization-factory.ts
- composition/tests/v1/directives/entity-cache-mapping-rules.test.ts
✅ Files skipped from review due to trivial changes (1)
- composition/src/v1/warnings/params.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- composition/src/utils/string-constants.ts
- composition/src/v1/constants/type-nodes.ts
- composition/src/v1/normalization/utils.ts
- composition/src/v1/warnings/warnings.ts
- composition/src/v1/constants/directive-definitions.ts
- composition/src/router-configuration/types.ts
- composition/src/errors/errors.ts
- composition/src/v1/constants/constants.ts
- composition/tests/v1/directives/entity-cache-fuzz.test.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2944 +/- ##
===========================================
- Coverage 66.33% 47.28% -19.06%
===========================================
Files 258 1107 +849
Lines 27539 151997 +124458
Branches 0 10126 +10126
===========================================
+ Hits 18269 71875 +53606
- Misses 7818 78309 +70491
- Partials 1452 1813 +361
🚀 New features to boost your workflow:
|
… generated bindings Extracted from jensneuse/entity-caching-v2 (PR #2777) — composition + proto layer only. - composition: @openfed__entityCache / @openfed__queryCache / @openfed__requestScoped directive definitions, extraction in normalization-factory, ConfigurationData types, warnings, and tests (incl. fuzz + mapping rules) - proto: additive DataSourceConfiguration fields 17-21 and new messages (EntityCacheConfiguration, RootFieldCacheConfiguration, EntityKeyMapping, EntityCacheFieldMapping, CachePopulateConfiguration, CacheInvalidateConfiguration, RequestScopedFieldConfiguration) - regenerated bindings: router/gen, connect-go, connect (incl. graphqlmetrics regen side-effects), composition-go bundle - shared: router-config serializer for the new proto fields Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SkArchon
force-pushed
the
milinda/entity-caching-1-composition-proto
branch
from
09d98fb to
51f766e
Compare
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.
Part 1 of 4 — split out from #2777 (
jensneuse/entity-caching-v2).Stack: this PR → router (2/4) → playground/studio (3/4) → benchmark/docs/tooling (4/4).
What's included
@openfed__entityCache/@openfed__queryCache/@openfed__requestScopeddirective definitions, extraction innormalization-factory.ts,ConfigurationDatatypes, warnings, and tests (incl. fuzz + mapping-rules suites)DataSourceConfigurationfields 17–21 and new messages (EntityCacheConfiguration,RootFieldCacheConfiguration,EntityKeyMapping,EntityCacheFieldMapping,CachePopulateConfiguration,CacheInvalidateConfiguration,RequestScopedFieldConfiguration) — purely additive, wire-compatible with older routers (DiscardUnknownparsing)router/gen,connect-go,connect(incl. graphqlmetrics regen side-effects),composition-gobundleNotes
src/directive-definition-datamodule (newDirectiveDefinitionDatafactory style), tests adapted to theBatchNormalizerclass API, and thecomposition-gobundle dropped (package was removed on main, chore: remove composition-go #2830). Full composition suite green (1301 tests).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
User-facing Validation & Warnings
Tests
Diff cleanup
Removed from this PR (−1,733 lines): the regenerated
connect/src/wg/cosmo/graphqlmetrics/*TS bindings andgraphqlmetrics/README.mdtweak (generator drift unrelated to entity caching — these files don't exist on main and would reappear for anyone runningmake generate-go; worth fixing in a separate tiny PR), and the composition meta-docs (CLAUDE.md,AGENTS.md,COMPOSITION_CONVENTIONS.md→ moved to the 4/4 PR).