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

v3.4.x updates federation directive version to 2.3, fails schema checks with Apollo Studio #221

Open
lyricsboy opened this issue Feb 23, 2023 · 17 comments

Comments

@lyricsboy
Copy link

lyricsboy commented Feb 23, 2023

Howdy! We updated this gem to 3.4.1 in one of our subgraphs, and its schema verification is failing in Apollo Studio. Here are some results:

Generated Schema snippet:

extend schema
  @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@inaccessible"])

type Something @federation__extends @federation__key(fields: "id") {
  id: ID! @federation__external
  ...
}

Apollo Studio Check output:

UNKNOWN_FEDERATION_LINK_VERSION
[x-service] Invalid version v2.3 for the federation feature in @link direction on schema
INVALID_GRAPHQL
[x-service] Unknown directive "@federation__external".
INVALID_GRAPHQL
[x-service] Unknown directive "@federation__extends".
INVALID_GRAPHQL
[x-service] Unknown directive "@federation__key".

Our supergraph is configured in Apollo Studio to use Federation version 2.0.

Our subgraphs also specify the Federation version using this gem. It seems like that version specification is being ignored when generating the @link directive.

I'm looking at this code change 👀 c7b987d#diff-48d1e38dfab06335bc60255c48ef179d40143d14e29cfb52673b258346799900R64

Shouldn't that be honoring the federation_version value?

@grxy
Copy link
Collaborator

grxy commented Feb 23, 2023

CC: @moonflare @dariuszkuc

@lyricsboy
Copy link
Author

I opened #222 as a proposed fix for this.

@grxy
Copy link
Collaborator

grxy commented Feb 23, 2023

I think you may be able to fix this by updating your selected version of composition in Apollo Studio. Have you tried that yet? I think your PR could work as well, but would be great if there is a path to unblocking without requiring a code change. Let me know!

@lyricsboy
Copy link
Author

I can try that. We can unblock ourselves by pinning to a prior release of this gem, as well.

Generally, though, is it correct that users of this gem should not be required to use a particular version of Federation other than what they specify in their own setup code? Like, we shouldn't be expected to use 2.3 for composition just because it's hard-coded in this gem, right?

@dariuszkuc
Copy link

In regards to the failure -> new version of a federation spec (v2.3) requires usage of compatible composition version (in studio) and a router/gateway that supports it. General upgrade path should be

router -> studio -> CLI (rover) -> subgraph


In regards to the lib behavior, I'll defer to the owners to decide how they want to proceed but it drills down to

  1. For a code-first solution IMHO it doesn't make sense to support multiple Fed v2 versions and the lib should support just a single (latest) version. If you upgrade to the latest gem you are opting in to use the latest version. This ensures that ALL federation features will work as expected and you won't get composition errors when trying to use new feature with an old Federation spec (e.g. trying to use @composeDirective with a federation spec v2.0).
    NOTE: this is the behavior of some other OSS libs like graphql-kotlin and strawberry-graphql. I am unaware of any code-first library that supports multiple fed v2 spec versions.

  2. Support multiple federation versions with the risk that users may get composition errors (due to the same issue as mentioned above).
    NOTE: this is the expected behavior of SDL first libraries. Some libs (like federation-jvm) also adds validation logic to notify users if they attempt to use new features with old spec version.

Current API allows users to explicitly specify the Federation version to allow users to choose whether to generate v1 or v2 schema. This was fine when lib was only supporting v2.0 spec but now it gets confusing - since API allows specifying floating number (vs just int) the expectation would be to also support the minor versions (i.e. option number 2 above).

Alternative would be to keep inconsistent API or make some breaking change (e.g. use boolean flag to opt-in/opt-out of Fed v2).

@moonflare
Copy link
Contributor

@lyricsboy We experienced the same problem when we first rolled out the update of the gem. We fixed this by updating the version in our variants/graphs directly in Studio.

Variant Build Configuration Screenshot 2023-02-24 at 08 37 43

As for your proposed solution, that's what I also had in mind and how I initially added the support for 2.3 in the gem, but I think @dariuszkuc suggestions make sense. So going forward I think it's less error prone and easier to maintain the gem this way. As @dariuszkuc mentioned, Pothos for example has the same behaviour.

@lyricsboy lyricsboy changed the title v4.3.x updates federation directive version to 2.3, fails schema checks with Apollo Studio v3.4.x updates federation directive version to 2.3, fails schema checks with Apollo Studio Feb 24, 2023
@lyricsboy
Copy link
Author

Thanks for the details. I think I'm understanding better now. A couple of questions come to mind.

General upgrade path should be

router -> studio -> CLI (rover) -> subgraph

If I understand that correctly, we should first update the router, then change the version of federation/composition in Studio for the supergraph, and so on down to the subgraphs.

This library is used by subgraphs to generate their schema. By updating the version of federation in the @link directive in this code, subgraph applications will potentially be the first place where a new federation version appears, leading to the kind of error we have experienced. In our org we aim to update our app's dependencies every week, so new versions of the gem will get used fairly quickly. We have not previously applied the same cadence / rigor to updating the federation version in the supgergraph (thus we are still composing with 2.0).

Given that, it seems like we're going in the reverse order of the recommended update path, with the subgraph being upgraded first. Is this the desired situation?

It feels like giving subgraph app developers control over their schema's federation version makes sense, and is consistent with the way this library is configurable, even though it doesn't flow through to the @link directive. If a subgraph developer uses a directive that is in a newer version of federation, it seems reasonable to expect them to update their configured version at the same time.

Alternatively, if we do not want to allow subgraphs to specify the version of federation directly, I agree that we should not offer a configuration option for it -- otherwise it's confusing. If we go that route, I think it would be nice to have the release notes and semantic version reflect the change in federation version somehow.

@dariuszkuc
Copy link

FYI we just published Graph update guide

@grxy
Copy link
Collaborator

grxy commented Feb 24, 2023

Alternatively, if we do not want to allow subgraphs to specify the version of federation directly, I agree that we should not offer a configuration option for it -- otherwise it's confusing. If we go that route, I think it would be nice to have the release notes and semantic version reflect the change in federation version somehow.

I don't know that the version config option was meant to support minor or patch version when it was originally implemented to support federation 2, so to me it seems acceptable for it to always reference the latest version. We can be more clear about this in a future release.

Speaking of releases, I've been thinking we may want to release a new major version that sets the default federation version to the latest 2.x version with an opt-in for federation 1.x, but I think we could take that a step further and remove version selection support (and federation 1.x support) altogether. Thoughts?

@lyricsboy
Copy link
Author

Looking at graphql-kotlin as an example, they implemented this using an opt-in boolean for Federation 2, and the version of federation is something that they hard code and include in their release notes.

I still think I prefer letting each subgraph (as a consumer of this gem) decide which version of federation they're declaring support for. These are some points that I think about:

  • As a configuration option, the federation version is something a subgraph can be explicit about; it does not update unless the maintainer chooses to update it
  • New releases of this gem could focus mainly on functionality (new directives, changes to the federation spec) without influencing consumers' choice of adoption of that new functionality
  • A boolean option for "Use Federation 2" is only good as long as the condition holds that "I want Federation > 1". What happens when Federation 3 comes into the mix?
  • Rolling a new release of this gem for each new version of the Federation spec is somewhat gatekeep-y; why is it important to intermediate that with the consumers?
  • How do you determine what semantic version of this gem to change for a new Federation version? As this issue highlights, even a minor version of Federation changing in the @link directive can be a "breaking change" in the sense that your supergraph build can fail if it's not up to the latest version.

@jmpage
Copy link
Contributor

jmpage commented Jul 13, 2023

Should bumps of the federation version to v2.1, v2.2, and v2.3 when upgrading apollo-federation-ruby be considered breaking changes in so much as they can cause composition to fail in Apollo Studio if the graph isn't yet configured to support that version and there is no option to opt into a specific 2.x version of federation?

Would you mind if I PRed an update to the changelog which explicitly states at which versions of this gem that federation 2.x+ is bumped to the next version?

@grxy
Copy link
Collaborator

grxy commented Jul 13, 2023

Should bumps of the federation version to v2.1, v2.2, and v2.3 when upgrading apollo-federation-ruby be considered breaking changes in so much as they can cause composition to fail in Apollo Studio if the graph isn't yet configured to support that version and there is no option to opt into a specific 2.x version of federation?

Probably. The other option is to just enable all of the latest features but allow users to configure the specific version of federation being used. I'm not sure of the impact of that, but that could let us skip making a breaking change for what should be a non-breaking update. I think that's what @lyricsboy's PR does, but I haven't had the chance to test out its impacts yet.

Would you mind if I PRed an update to the changelog which explicitly states at which versions of this gem that federation 2.x+ is bumped to the next version?

Yes, that would be most welcome. Thank you!

jmpage added a commit to jmpage/apollo-federation-ruby that referenced this issue Jul 13, 2023
This adds information to v3.4.0 highlighting the breaking change to composition discussed in Gusto#221 and providing guidance on how to proceed.
jmpage added a commit to jmpage/apollo-federation-ruby that referenced this issue Jul 13, 2023
This adds information to v3.4.0 highlighting the breaking change to composition discussed in Gusto#221 and providing guidance on how to proceed.
jmpage added a commit to jmpage/apollo-federation-ruby that referenced this issue Jul 13, 2023
This adds information to v3.4.0 highlighting the breaking change to composition discussed in Gusto#221 and providing guidance on how to proceed.
grxy pushed a commit that referenced this issue Jul 13, 2023
This adds information to v3.4.0 highlighting the breaking change to composition discussed in #221 and providing guidance on how to proceed.
@karmingc
Copy link

karmingc commented Dec 6, 2023

@grxy any updates on #222?

@jmpage
Copy link
Contributor

jmpage commented Dec 6, 2023

@karmingc, per #267:

For now, we've decided our maintenance of this gem is unlikely to extend beyond simple changes, such as bug patches, dependency bumps, and very minor tweaks. More significant changes, such as breaking changes, new features, or alterations to fundamental internal logic will be deprioritized.

There is also a new team which owns this gem, as mentioned in a comment on that issue.

@karmingc
Copy link

karmingc commented Dec 6, 2023

thanks for the reference @jmpage!

@grxy
Copy link
Collaborator

grxy commented Dec 6, 2023

@karmingc If you are able to update the composition version in your router/gateway and in Apollo Studio, you should be able to unblock yourselves on this. I think based on the guidance from some Apollo folks, we will probably only support a single major version of federation with this library, at least for now. PRs are always welcome, of course, but as @jmpage mentioned above, we are only in an as-needed maintenance phase as the team has other priorities at the moment.

@jmpage
Copy link
Contributor

jmpage commented Dec 6, 2023

@karmingc in case it's helpful to you: I have not experienced any issues in a large federated graph from bumping the composition version to the latest one in Apollo Studio and then allowing subgraphs to opt into the current version supported by this library.

Kaytal pushed a commit to lensrentals/apollo-federation-ruby that referenced this issue Oct 1, 2024
This adds information to v3.4.0 highlighting the breaking change to composition discussed in Gusto#221 and providing guidance on how to proceed.
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

No branches or pull requests

6 participants