Conversation
There was a problem hiding this comment.
Pull request overview
Adds coverage and a planning fix for federated queries involving multiple @boundary types with different field sets, preventing incorrect field expectations across fragments.
Changes:
- Add two new test GraphQL services to reproduce boundary-fragment planning behavior.
- Add an integration test validating correct fragment resolution across boundary types.
- Update query-plan step merging to include
ParentTypein the merge key.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| test_srv_boundary_fragment/service2/gadget_test_server.go | Adds a gadget service test server with Gizmo + Gadget interface and multiple boundary types. |
| test_srv_boundary_fragment/service1/gizmo_test_server.go | Adds a gizmo service test server that provides boundary-only fields per type. |
| server_test.go | Adds a regression test for boundary fragment resolution across multiple types. |
| plan.go | Prevents incorrect plan-step merging by including ParentType in the merge key. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mergedStepsMap := map[string]*QueryPlanStep{} | ||
| for _, step := range childrenStepsResult { | ||
| key := strings.Join(append([]string{step.ServiceURL}, step.InsertionPoint...), "/") | ||
| key := strings.Join(append([]string{step.ServiceURL, step.ParentType}, step.InsertionPoint...), "/") |
There was a problem hiding this comment.
Using strings.Join(..., \"/\") to build the merge key can cause collisions because step.ServiceURL itself contains /. For example, ServiceURL=\"http://a/b\" + ParentType=\"C\" produces the same prefix as ServiceURL=\"http://a\" + ParentType=\"b\" + InsertionPoint starting with \"C\". This can incorrectly merge steps across distinct services/parent types. Prefer a composite key (e.g., a small struct in the map key, or a delimiter that cannot appear in URLs like \"\\x1f\" with explicit escaping) so the segments are unambiguous.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
justinlatimer-vista
requested review from
Copilot and
justinlatimer-vista
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type gizmoWithGadgetResolver struct { | ||
| IDField string | ||
| Gadget *gadgetResolver | ||
| } | ||
|
|
||
| func (u gizmoWithGadgetResolver) ID() graphql.ID { | ||
| return graphql.ID(u.IDField) | ||
| } |
There was a problem hiding this comment.
NewGadgetService() enables graphql.UseFieldResolvers(), which requires resolver methods (not struct fields) for schema fields. Gizmo.gadget will not resolve because gizmoWithGadgetResolver does not implement a Gadget() *gadgetResolver method, and MustParseSchema may fail schema validation. Add a Gadget() resolver method and rename the struct field (e.g., GadgetField) to avoid a method/field name collision and accidental recursion.
| id | ||
| gadget { | ||
| ... on Jetpack { | ||
| name |
There was a problem hiding this comment.
These lines include trailing tab characters after name, which adds noise to diffs and can be flagged by linters/editorconfig. Please remove the trailing whitespace in the query strings.
| name | |
| name |
| id | ||
| gadget { | ||
| ... on Jetpack { | ||
| name |
There was a problem hiding this comment.
These lines include trailing tab characters after name, which adds noise to diffs and can be flagged by linters/editorconfig. Please remove the trailing whitespace in the query strings.
| query := gqlparser.MustLoadQuery(executableSchema.MergedSchema, `{ | ||
| gadgets { | ||
| ... on Jetpack { | ||
| name |
There was a problem hiding this comment.
These lines include trailing tab characters after name, which adds noise to diffs and can be flagged by linters/editorconfig. Please remove the trailing whitespace in the query strings.
2 participants
When there is multiple boundary types, with different fields, queries are failing.
For example when querying with JetPack & InvisibleCar as fragment in the query , with JetPack id, its failing , expecting 'performance' field which is not part of Jetpack.
Should be empty, but was input:8:7: Cannot query field "performance" on type "Jetpack"It seems parentType is not added in URL during planning phase, which results in failure.