Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserves source of union members and enum values in supergraph #2288

Merged
merged 3 commits into from
Dec 14, 2022

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented Nov 30, 2022

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.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 30, 2022

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.

@pcmanus
Copy link
Contributor Author

pcmanus commented Nov 30, 2022

I'll note that this PR is currently based on top of the #2279 PR. This is because that later PR was already adding the code to introduce a new 0.3 join spec, which this patch needs too. I can try to separated the aforementioned code so this can be merge first if we really really want, but as I do think we should make an effort in getting those 2 PRs in the same release, exactly because they both need new things from the join spec (and, all other things being equal, it's nice to avoid burning new versions too often), so maybe we just review #2279 first and then get to this.

@pcmanus pcmanus self-assigned this Nov 30, 2022
@korinne korinne added this to the 2.3.0 milestone Dec 5, 2022
@clenfest
Copy link
Contributor

clenfest commented Dec 8, 2022

Code LGTM, but can you add some tests to make sure that it's correct? Also for extractSubgraphsFromSupergraphs, ideally there would be tests that exercise both 0.3 and 0.2 functionality to make sure both paths work.

@pcmanus
Copy link
Contributor Author

pcmanus commented Dec 12, 2022

can you add some tests to make sure that it's correct?

Mind being more specific about what you feel is inadequately tested? The patch adds support for correctly keeping the source of union members and enum values, and the patch currently:

  • Adds the test from the description of #2256 which tests that the user-visible consequence of the bug fixed here is solved.
  • Un-skip this test which had been added in a previous patch but was failure as a consequence of this bug.
  • Adds new tests that more specifically validates that both union members and enum values are correctly re-extracted back from the supergraph.

I did just added a modification to the maint test checking a supergraph output so it includes unions and enum values, but the details of the supergraph are ultimately an implementation detail, so I don't want to over-index testing that.

ideally there would be tests that exercise both 0.3 and 0.2 functionality to make sure both paths work.

Well, it's a bit tricky. The 0.3 version is well exercised by virtue of being the current version, but I agree we'd ideally want test ensuring that we can read 0.2 version properly. Except that while we preserve the reading of older versions, we don't preserve the writing of them, so to test this we need to generate supergraphs with older version and ensure they can still be read correctly on newer versions. It's probably doable, but a bit involved if we want this to not be too manual and unmaintainable. In any case, I'd like to tackle that question separately from this particular issue as it's more general. I've created #2297 with a possible initial, but I'm not entirely convinced yet.

Sylvain Lebresne added 3 commits December 14, 2022 11:29
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 apollographql#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 apollographql#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 apollographql#2256.
@pcmanus pcmanus merged commit 858d216 into apollographql:next Dec 14, 2022
jeffjakub added a commit that referenced this pull request Dec 16, 2022
* 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/[email protected]
 - [email protected]
 - @apollo/[email protected]
 - @apollo/[email protected]
 - @apollo/[email protected]
 - @apollo/[email protected]
 - @apollo/[email protected]

Co-authored-by: Sylvain Lebresne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Union types are not rebuilt properly from the supergraph, creating potential runtime issues
3 participants