feat: Support Federation v2 #196

col · 2022-04-27T04:13:34Z

This PR adds support for the @sharable, @inaccessible and @OverRide directives. I believe this is all that is required to support Apollo Federation v2.0.

Issue: #153

Original PR #195 was closed due to branch rename.

Oops! I didn't realise `Hash#except` was only available from Ruby 3.0

…hack!) See: https://www.apollographql.com/docs/federation/federation-2/moving-to-federation-2/#opt-in-to-federation-2 This is a horrible hack. Need to figure out if there is a clean way of including this in the document and the printer output.

rmosolgo · 2022-04-28T12:16:07Z

👍 Seems like it should do the job! The only new feature I read about but didn't see in the tests was @key(..., resolvable: false), but it looks like the existing add_directive(...) method should cover it.

daemonsy · 2022-05-04T14:24:27Z

Hey @col thanks for submitting this. 👏. I personally haven't caught up with the changes in Federation V2. We should be able to put some focus on this in mid month.

Meanwhile, @lennyburdette if you're around, be nice to get some thoughts from you on this PR.

lennyburdette · 2022-05-04T17:19:51Z

This is great, thanks for putting this together!

I'd like to make one suggestion: can there be a way to explicitly opt-in to Federation 2 (so that it adds the @link directive) without using one of the federation 2 directives? (shareable, inaccessible, override). Maybe federation_version: 2 in the GraphQLSchema subclass?

A common workflow for converting a subgraph to Federation 2 looks like this:

Add the @link directive to opt into new Federation features.
Run composition (either rover supergraph compose or rover subgraph check).
Use the composition errors and hints to figure out which fields need to marked as @shareable.

The rules around shareable types and fields and the @shareable directive don't come into play until you opt into Federation 2 with @link. It can be difficult to tell which fields need to be marked shareable, and we'd prefer people let the composer dictate that rather than having to guess (and probably mark too many fields as shareable!)

Otherwise, I think the code is sound and I'm excited to see this land!

This is more explicit and extendable in the future if we ever need any other federation options at the schema level

col · 2022-05-05T01:28:48Z

Good suggestion @lennyburdette! I was never very happy with my initial approach of searching for usages of the v2 directives.

I've opted for the version to be a keyword arg so that we can add other options in the future if required.
Example:

class MySchema
  include ApolloFederation::Schema
  federation version: 2
end

lyricsboy

Looking forward to seeing this land!

lyricsboy · 2022-05-16T20:35:48Z

lib/apollo-federation/schema.rb

+      FEDERATION_2_PREFIX = <<~SCHEMA
+        extend schema
+          @link(url: "https://specs.apollo.dev/federation/v2.0",
+          import: ["@key", "@extends", "@external", "@requires", "@provides", "@shareable", "@inaccessible", "@override"])


This looks like it's importing all of the directives unconditionally. Is that acceptable, or should it be more dynamic to only import the directives that are actually used in the schema?

Apollo docs say "Modify the import array to include whichever federated directives your subgraph schema uses."

Is @extends a valid directive in this context? I don't see it in the docs.

One subtle feature of the @link spec is that you don't have to import these directives. If you don't supply the import: argument, the directives are still available in a namespaced form:

extend schema @link(url: "https://specs.apollo.dev/federation/v2.0") type Foo @federation__key(fields: "bar") @federation__shareable { bar: ID! baz: String @federation__external quux: String @federation__requires(fields: "bax") blah: String @federation__inaccesible }

This might be the "right" approach for a code-first library. The key ruby method could output @federation__key.

And yes, I believe the @extends directive was dropped entirely in Fed 2.

I think @extends is still optionally supported. We either need to support @extends or have proper support for type extensions (eg. extend type).
See: https://www.apollographql.com/docs/federation/federation-spec/#federation-schema-specification

I don't have an example problem scenario, no. I honestly don't know what impact importing all of the directives might have. I suppose there's a possibility that a Graph has already defined directives with the same names, and those could be in conflict with the Apollo Federation directives -- thus an author might need to use the namespaced directives to work around that. Is that a realistic scenario?

The docs for the @link directive cover the namespacing feature, which is actually configurable using as:. I'm inclined to agree with @lennyburdette on the preference to use the namespaced directives in this implementation, and thus avoid the need to specify the directives in the @link declaration.

Thanks for pointing out the spec—that's exactly where the import: behavior is documented. Here's what the federation team says about using import or not:

you can absolutely skip any import and use @federation__key and I can see many code-first libraries doing just that since it’s probably a bit simpler (no worries about name conflicts, etc…).
...
if it matters to you that the SDL you generate is as readable as possible, then using the import makes sense.

The only benefit to not using import and emitting the namespaced directives is that it would allow graph authors to use the shorter directive names for their own purposes. But you could also support that with a DSL for emitting import: [{ name: "@tag", as: "@mytag" }] so you're not painted into a corner if you choose the short names today.

The way this is done currently, @lennyburdette , does it pass the @link test in https://www.apollographql.com/docs/federation/other-servers/?

Yeah this should be sufficient. The test is rudimentary right now: it just searches the _Service.sdl for the presence of "@link": https://github.com/apollographql/apollo-federation-subgraph-compatibility/blob/main/src/tests/link.test.ts

daemonsy · 2022-05-20T18:24:44Z

Hi @col @lyricsboy @lennyburdette thanks for all the work and comments on the PR so far.

@geshwho and I paired over reviewing the 2.0 spec and the PR. Overall the PR looks great! Jotting down a few notes:

Code considerations

In this PR, do we want to improve @link support so directives can be renamed?
Let's work separately on updating https://github.com/apollographql/apollo-federation-subgraph-compatibility to do V2 tests

Proposed ops for shipping

When reviews are all good, we will ship this a major version bump (technically it's non breaking)
Help internal Gusto apps adopt the latest gem version whilst still using V1. This helps to ensure that we didn't break anything in V1 backwards compatibility. FYI @grxy we can discuss separately switching to V2.
Mark on the README that beta 2.0 support is available, encourage folks to try it out.
Remove beta when we're confident. Ideally, by that time, Gusto is already on V2.

daemonsy

This looks great! Thanks for adding 2.0 support to the library. Just some general code comments in this review.

I'll take the feature branch out for a spin with our main app later today.

spec/apollo-federation/service_field_spec.rb

daemonsy · 2022-05-20T18:08:36Z

README.md

+```ruby
+class MySchema < GraphQL::Schema
+  include ApolloFederation::Schema
+  federation version: 2.0


Might this be the place to support import?

version: should probably be a string. we don't foresee a need for 2.0.1 but it's possible

daemonsy · 2022-05-20T18:09:46Z

lib/apollo-federation/schema.rb

+      FEDERATION_2_PREFIX = <<~SCHEMA
+        extend schema
+          @link(url: "https://specs.apollo.dev/federation/v2.0",
+          import: ["@key", "@extends", "@external", "@requires", "@provides", "@shareable", "@inaccessible", "@override"])


The way this is done currently, @lennyburdette , does it pass the @link test in https://www.apollographql.com/docs/federation/other-servers/?

lennyburdette · 2022-05-26T21:17:41Z

FYI I spoke with the maintainer of graphql-kotlin (another code-first graphql library) and I think we're settled on emitting only namespaced directives (@federation__key) and not providing an API to specify imports or renames. I'm pretty sure this will be the path forward for most code-first libraries.

daemonsy · 2022-05-26T22:19:52Z

Thanks @lennyburdette !

daemonsy · 2022-06-02T18:21:45Z

Hi @col are you still actively working on this PR? No worries if you're not able to, we can merge the existing work and do some of the action items separately.

col · 2022-06-07T03:27:22Z

Hi @col are you still actively working on this PR? No worries if you're not able to, we can merge the existing work and do some of the action items separately.

I'm happy to incorporate some of the feedback and update the PR over the next day or two.

…ration version >= 2.0

This approach feel a little easier to understand

col · 2022-06-07T07:48:07Z

@daemonsy (CC: @lennyburdette @lyricsboy) I've updated the PR with your feedback. Please take another look.

Changes:

Changed the federation version to support semantic versioning
Remove the explicit import of the federation directives
Output the directives using the federation__ namespace prefix when using >= 2.0
Split the service field specs into v1 and v2

daemonsy · 2022-06-08T22:25:58Z

Hey @col it's looking good to me. I am going to run some sanity check on the PR using our codebase.

@lennyburdette how does it look to you?

lennyburdette · 2022-06-08T22:46:39Z

looks great!

sethc2

Looks good to me. Just a couple grammar nits.

Thanks for putting this together!

spec/apollo-federation/schema_spec.rb

pmrotule · 2022-06-20T13:25:14Z

@daemonsy Any update? Can't wait to use this feature 😅 👏

daemonsy · 2022-06-21T21:01:36Z

Thanks for working on this @col and your patience in getting the PR through 👏

# [3.1.0](v3.0.0...v3.1.0) (2022-06-21) ### Features * Support Federation v2 ([#196](#196)) ([238736c](238736c)), closes [/www.apollographql.com/docs/federation/federation-2/moving-to-federation-2/#opt-in-to-federation-2](https://github.com//www.apollographql.com/docs/federation/federation-2/moving-to-federation-2//issues/opt-in-to-federation-2)

rylanc · 2022-06-21T21:04:05Z

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

v3.1.0
GitHub release

Your semantic-release bot 📦🚀

col added 7 commits April 26, 2022 21:53

Add support for the @Shareable directive

81b0182

Fix rubocop offense

17ca1a6

Add support for the @inaccessible directive

426bb70

Add support for the @OverRide directive

4df5fe0

Refactor to appease rubocop

9ceb4d6

Replace Hash#except with Hash#delete_if

8bd6ea2

Oops! I didn't realise `Hash#except` was only available from Ruby 3.0

Update README to include v2.0 directives

fd9771e

This was referenced Apr 27, 2022

Add support for the @shareable, @inaccessible and @override directives #195

Closed

Apollo Federation 2.0 support #153

Open

col added 2 commits April 27, 2022 15:37

Fix incorrect handling of keyword args in field

b767409

col added 2 commits May 5, 2022 09:18

Enable federation 2 via a federation_2 class method on Schema

9deeada

Change federation_2 to federation version: 2

614a2d2

This is more explicit and extendable in the future if we ever need any other federation options at the schema level

col added 3 commits May 5, 2022 09:40

Add a unit test for Schema.federation_version

45ff796

Update README with how to opt in to Federation v2

73c75ae

Merge branch 'main' into federation_v2

4ab5b02

lyricsboy reviewed May 16, 2022

View reviewed changes

daemonsy reviewed May 20, 2022

View reviewed changes

col added 3 commits June 7, 2022 13:24

Update federation version to support semantic versioning

2030820

Add federation__ namespace prefix to all directives when using fede…

1d8c438

…ration version >= 2.0

Merge branch 'main' into federation_v2

a1484f1

Refactor the merge_directives method

80337ca

This approach feel a little easier to understand

sethc2 requested changes Jun 9, 2022

View reviewed changes

spec/apollo-federation/schema_spec.rb Outdated Show resolved Hide resolved

spec/apollo-federation/schema_spec.rb Outdated Show resolved Hide resolved

Fix grammar issues in test names

896438d

daemonsy changed the title ~~Support Federation v2~~ feat: Support Federation v2 Jun 21, 2022

daemonsy merged commit 238736c into Gusto:main Jun 21, 2022

rylanc added the released label Jun 21, 2022

geshwho mentioned this pull request Jul 12, 2022

unable to configure federation directive namespace #201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support Federation v2 #196

feat: Support Federation v2 #196

col commented Apr 27, 2022 •

edited

Loading

rmosolgo commented Apr 28, 2022

daemonsy commented May 4, 2022

lennyburdette commented May 4, 2022 •

edited

Loading

col commented May 5, 2022

lyricsboy left a comment

lyricsboy May 16, 2022

lyricsboy May 16, 2022

lennyburdette May 16, 2022

lennyburdette May 16, 2022

col May 17, 2022

lyricsboy May 17, 2022

lyricsboy May 17, 2022

lennyburdette May 17, 2022

daemonsy May 20, 2022

lennyburdette May 20, 2022

daemonsy commented May 20, 2022 •

edited

Loading

daemonsy left a comment •

edited

Loading

daemonsy May 20, 2022

lennyburdette May 26, 2022

daemonsy May 20, 2022

lennyburdette commented May 26, 2022

daemonsy commented May 26, 2022

daemonsy commented Jun 2, 2022

col commented Jun 7, 2022

col commented Jun 7, 2022

daemonsy commented Jun 8, 2022

lennyburdette commented Jun 8, 2022

sethc2 left a comment

pmrotule commented Jun 20, 2022

daemonsy commented Jun 21, 2022

rylanc commented Jun 21, 2022

feat: Support Federation v2 #196

feat: Support Federation v2 #196

Conversation

col commented Apr 27, 2022 • edited Loading

rmosolgo commented Apr 28, 2022

daemonsy commented May 4, 2022

lennyburdette commented May 4, 2022 • edited Loading

col commented May 5, 2022

lyricsboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daemonsy commented May 20, 2022 • edited Loading

Code considerations

Proposed ops for shipping

daemonsy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lennyburdette commented May 26, 2022

daemonsy commented May 26, 2022

daemonsy commented Jun 2, 2022

col commented Jun 7, 2022

col commented Jun 7, 2022

daemonsy commented Jun 8, 2022

lennyburdette commented Jun 8, 2022

sethc2 left a comment

Choose a reason for hiding this comment

pmrotule commented Jun 20, 2022

daemonsy commented Jun 21, 2022

rylanc commented Jun 21, 2022

col commented Apr 27, 2022 •

edited

Loading

lennyburdette commented May 4, 2022 •

edited

Loading

daemonsy commented May 20, 2022 •

edited

Loading

daemonsy left a comment •

edited

Loading