feat: improve handling for entities with multiple keys#2123
Conversation
WalkthroughThis change updates the version of the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
router/core/factoryresolver.go (1)
652-660: Append logic prevents overwrite — good; consider dedup if upstream can emit duplicatesAppending per type fixes the previous overwrite issue. If upstream config generation can produce identical entries, consider optional dedup to avoid redundant RPCs; otherwise this is fine.
// optional: guard against accidental dups func appendUnique(list []grpcdatasource.EntityRPCConfig, e grpcdatasource.EntityRPCConfig) []grpcdatasource.EntityRPCConfig { for _, v := range list { if v.Key == e.Key && v.RPCConfig.RPC == e.RPCConfig.RPC { return list } } return append(list, e) } // usage: // result.EntityRPCs[entity.TypeName] = appendUnique(result.EntityRPCs[entity.TypeName], cfg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
router-tests/go.mod(1 hunks)router/core/factoryresolver.go(2 hunks)router/go.mod(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
router/go.mod (1)
34-34: Upgrade to graphql-go-tools v2.0.0-rc.219 — LGTMAligns with the entity multi-key handling and the alias/StringValue fix. No issues from this bump on its own.
router/core/factoryresolver.go (1)
630-633: EntityRPCs now supports multiple configs per entity — LGTMInitializing as map[string][]EntityRPCConfig correctly enables multiple @key mappings per type. Matches the intent of “entities with multiple keys.”
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
router-tests/grpc_subgraph_test.go (1)
131-135: Add coverage for null wrappers and repeated aliasesTo guard against regressions:
- Add a case where a StringValue-backed field is null and ensure the aliased field returns null (not omitted).
- Add a case selecting the same field twice with different aliases (and optionally alongside the non-aliased field) to ensure per-alias resolution and serialization are correct.
Example query shape you can adapt to available fixtures:
query { project(id: "X") { original: description alias1: description alias2: description } }Happy to help add/adjust fixtures if none currently return a null wrapper.
router-tests/router_plugin_test.go (2)
254-258: Deduplicate identical test case across filesSince both files are in package integration, consider extracting the query/expected into shared constants to avoid drift:
// in a new file e.g., router-tests/alias_testcases.go (package integration) const QueryProjectWithAliases = `query { project(id: "1") { id name myDescription: description myStartDate: startDate myEndDate: endDate } }` const ExpectedProjectWithAliases = `{"data":{"project":{"id":"1","name":"Cloud Migration Overhaul","myDescription":"Migrate legacy systems to cloud-native architecture","myStartDate":"2021-01-01","myEndDate":"2025-08-20"}}}`Then reference these in both test tables.
254-258: Optional: JSON structural assertion to reduce brittlenessIf you ever see flakiness from serialization ordering changes, switching to a structural assertion helps:
require.JSONEq(t, ExpectedProjectWithAliases, response.Body)Applying suite-wide would be most consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
router-tests/grpc_subgraph_test.go(1 hunks)router-tests/router_plugin_test.go(1 hunks)router/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- router/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router-tests/grpc_subgraph_test.go (2)
131-135: Aliased StringValue fields regression test — LGTMThis directly exercises the aliasing bug described in #2110 and the expected payload matches the dataset. Good addition.
131-135: No action needed: Go toolchain ≥ 1.22 is confirmed
All modules currently specify Go 1.23 (via go.mod), which includes per-iteration range variable semantics introduced in Go 1.22. The use of t.Parallel in your subtests is therefore safe.router-tests/router_plugin_test.go (1)
254-258: Aliased StringValue fields under plugin-enabled router — LGTMMirrors the gRPC subgraph test and ensures parity when plugins are enabled. Looks correct and consistent with dataset.
Noroth
deleted the
ludwig/eng-7007-support-of-multiple-and-nested-keys-for-entities
branch
2 participants
Summary by CodeRabbit
Chores
Refactor
Tests
No changes to user-facing features or documentation.
Checklist
fixes #2110
Corresponding engine release: https://github.com/wundergraph/graphql-go-tools/releases/tag/v2.0.0-rc.219