Use alias in QP when querying conflicting fields #2294
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
Note that the patch is currently on top of the patch for #2277. That's because the "post-fetch" rewriting done here piggy-back on the query plan changes made by the Note that problem/solution still very much follow what I describe in this comment. The one question I asked there was if we wanted to only add aliases when strictly necessary, and I answer this positively here, not only for the reason I mentioned in the comment (this avoid modifying most existing plan) but also because it is likely more efficient since adding aliases more often would 1) result in slightly larger fetch queries in general and 2) would imply post-fetch rewritings much more often, and while it's super expensive, it has a cost nonetheless. |
| @@ -3516,4 +3516,1113 @@ describe('executeQueryPlan', () => { | |||
| `); | |||
| }); | |||
| }); | |||
There was a problem hiding this comment.
You talk in the comments about potentially having a naming collision when it comes to aliasing and potentially needing to regenerate the name? Is there a test for that case?
There was a problem hiding this comment.
Good call. I had not added a test for that, so pushed one (which obviously uncovered a small issue in some cases).
In a few situations, the query planner was generating queries where the same response name was queried at the same "level" with incompatible types, resulting in invalid queries (the queries were failing the [`FieldsInSetCanMerge`](https://spec.graphql.org/draft/#FieldsInSetCanMerge())) validation for the GraphQL sepc). This commit detects this case, and when it would happen, aliases one of the occurence in the fetch to make the query valid. Once receiving the fetch result, the aliased value is rewritten to it's original response name. Fixes apollographql#1257
pcmanus
force-pushed
the
f1257-conflicting-fields
branch
from
ff621c4
to
bffe4e6
Compare
* Implements @interfaceObject and @key on interfaces (#2279) Add support for the newly added `@interfaceObject` directive and for `@key` on interfaces, in order to allow using interfaces as abstraction across subgraphs. See #2277 for details. Fixes #2277. * Preserves source of union members and enum values in supergraph (#2288) While composition allows unions and enums to differ between subgraphs (at least under certain conditions), the supergraph currently was not preserving which subgraph defined which union member/enum value, and in particular the query planner made the (sometimes incorrect) assumption that unions and enums were defined the same in all the subgraphs in which they are defined. As shown in #2256, this was leading to genuine issues in the case of unions, where the query planner was generating invalid subgraph queries. I am not sure this created similar issues for enum values due to a combination of how the merging rule for enum work and the fact that values of enum are not types, but it is nonetheless a bad idea to run the query planner with incorrect information on the subgraph it generates queries for, and this can get in the way of other toolings that wants to use the supergraph (for instance, this gets in the way of #2250). To fix this, this patch ensures the supergraph preserve the information regarding which subgraph defines which union member and enum values in the supergraph by adding 2 new dedicated "join spec" directives. Fixes #2256. * Use alias in QP when querying conflicting fields (#2294) * Use alias in QP when querying conflicting fields In a few situations, the query planner was generating queries where the same response name was queried at the same "level" with incompatible types, resulting in invalid queries (the queries were failing the [`FieldsInSetCanMerge`](https://spec.graphql.org/draft/#FieldsInSetCanMerge())) validation for the GraphQL sepc). This commit detects this case, and when it would happen, aliases one of the occurence in the fetch to make the query valid. Once receiving the fetch result, the aliased value is rewritten to it's original response name. Fixes #1257 * Review feedback: add test for alias conflicts and fix related code * Regen error doc * Release - @apollo/composition@2.3.0-alpha.0 - apollo-federation-integration-testsuite@2.3.0-alpha.0 - @apollo/gateway@2.3.0-alpha.0 - @apollo/federation-internals@2.3.0-alpha.0 - @apollo/query-graphs@2.3.0-alpha.0 - @apollo/query-planner@2.3.0-alpha.0 - @apollo/subgraph@2.3.0-alpha.0 Co-authored-by: Sylvain Lebresne <sylvain@apollographql.com>
In a few situations, the query planner was generating queries where the same response name was queried at the same "level" with incompatible types, resulting in invalid queries (the queries were failing the
FieldsInSetCanMerge)validation for the GraphQL spec).This commit detects this case, and when it would happen, aliases oneof the occurence in the fetch to make the query valid. Once receivingthe fetch result, the aliased value is rewritten to it's originalresponse name.
Fixes #1257