Gusto / apollo-federation-ruby

A Ruby implementation of Apollo Federation
MIT License
216 stars 76 forks source link

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

Open lyricsboy opened 1 year ago

lyricsboy commented 1 year ago

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 👀 https://github.com/Gusto/apollo-federation-ruby/commit/c7b987de1d2b32a4a77ceb09718373ffa5a60abb#diff-48d1e38dfab06335bc60255c48ef179d40143d14e29cfb52673b258346799900R64

Shouldn't that be honoring the federation_version value?

grxy commented 1 year ago

CC: @moonflare @dariuszkuc

lyricsboy commented 1 year ago

I opened https://github.com/Gusto/apollo-federation-ruby/pull/222 as a proposed fix for this.

grxy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

FYI we just published Graph update guide

grxy commented 1 year ago

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 commented 1 year ago

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:

jmpage commented 1 year ago

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 commented 1 year ago

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!

karmingc commented 11 months ago

@grxy any updates on https://github.com/Gusto/apollo-federation-ruby/pull/222?

jmpage commented 11 months ago

@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 commented 11 months ago

thanks for the reference @jmpage!

grxy commented 11 months ago

@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 commented 11 months ago

@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.