feat: add support for @composeDirective #253

utay · 2023-06-16T08:37:19Z

Fixes #228

This PR adds support for @composeDirective as well as having multiple @link on the schema which is a prerequisite of the former. There's a breaking change link -> default_link_namespace in the federation function because I felt like it makes more sense like this. Happy to hear your thoughts!

Add support for @composeDirective

utay · 2023-06-16T08:49:06Z

CI fails on graphql-ruby <= 1.11, it looks like schema directives support was added in 1.12:

  1) ApolloFederation::ServiceField with older versions of GraphQL and the interpreter runtime behaves like service field returns the federation SDL with compose directives for the schema
     Failure/Error: super(*args, **kwargs, &block)
     
     ArgumentError:
       unknown keyword: :directives

grxy · 2023-06-20T16:31:53Z

@utay Thanks for putting this together! I will try to take a look this week, but will be OOO for a couple weeks after in the case I'm not able to get to it this week.

grxy

I have a few concerns with how this is currently written:

I'm not understanding the need to support multiple links here. Can we not use @composeDirective with a directive defined in our schema itself?
I don't think we want to support code paths for multiple federation minor versions. I think we need to clean up the API for defining the version, but '2.0', 2.3, 2, etc. essentially imply the latest 2.x version for now.

grxy · 2023-07-11T16:18:25Z

spec/apollo-federation/service_field_v2_spec.rb

+      query_obj = Class.new(base_object) do
+        graphql_name 'Query'
+
+        field :product, product, null: true, directives: { complexity_directive => { fixed: 1 } }


I think this is always going to fail on older GraphQL ruby versions. I think we have a couple options potentially:

Drop support for GraphQL Ruby < 1.12 (or maybe even < 2.0)

Only run these tests on newer versions

If we do this, we should add code to make sure that we warn or raise if composeDirective is used on an older GraphQL Ruby version

I lean towards the first option.

I like the first option as well. Do you plan to drop support for GraphQL Ruby < 1.12 in a separate PR?

@utay Yeah we'd probably want to do it in a separate PR and publish a new major version.

@grxy Any plan to do that in the near future? It was the main blocker here I think

Hey @utay I'll have to defer to the folks who have taken over maintenance on this gem. CC: @sethc2 @slauppy @sofie-c @simoncoffin

grxy · 2023-07-11T16:24:35Z

lib/apollo-federation/schema.rb

@@ -29,6 +37,10 @@ def federation_2?
        Gem::Version.new(federation_version.to_s) >= Gem::Version.new('2.0.0')
      end

+      def federation_2_1?


I don't think we should add specific minor version support. We do have some cleanup to do, but it's going to be easier to maintain this lib if we simply aim to support the latest version of federation instead of incrementally supporting minor versions of 2.x. See this issue for more context.

I see, thanks for the context. I removed this check, mutualized imported directives and fixed tests. I'll wait your call on how we plan to clean up versions

spec/apollo-federation/service_field_v2_spec.rb

grxy · 2023-07-11T16:40:54Z

spec/apollo-federation/service_field_v2_spec.rb

+        federation version: '2.3',
+                   links: [{
+                     url: 'https://specs.example.com/federation/v2.3',
+                     import: [complexity_directive.graphql_name],


This one probably doesn't need importing with an @link since it is being defined inline.

It looks like even if it's being defined inline it needs to be referenced via a custom link. For example, if I try to compose a supergraph with an inline directive that isn't imported with link I get:

UNKNOWN: Directive "@..." in subgraph "..." cannot be composed because it is not a member of a core feature

In the docs they're doing it similarly

grxy · 2023-07-11T16:45:20Z

lib/apollo-federation/schema.rb

-        federation_namespace = ", as: \"#{link_namespace}\"" if link_namespace != DEFAULT_LINK_NAMESPACE
+        schema = ['extend schema']
+
+        all_links.each do |link|


This printing logic is fairly complex. Can we make it easier to read?

Tried to do make it easier to read but not a ruby expert, please tell me if there's a better way :)

utay

@grxy Sorry for the delay here and thank you for your review!

utay · 2023-08-01T09:44:22Z

spec/apollo-federation/service_field_v2_spec.rb

+      query_obj = Class.new(base_object) do
+        graphql_name 'Query'
+
+        field :product, product, null: true, directives: { complexity_directive => { fixed: 1 } }


I like the first option as well. Do you plan to drop support for GraphQL Ruby < 1.12 in a separate PR?

utay · 2023-08-01T09:45:53Z

lib/apollo-federation/schema.rb

@@ -29,6 +37,10 @@ def federation_2?
        Gem::Version.new(federation_version.to_s) >= Gem::Version.new('2.0.0')
      end

+      def federation_2_1?


I see, thanks for the context. I removed this check, mutualized imported directives and fixed tests. I'll wait your call on how we plan to clean up versions

utay · 2023-08-01T10:06:26Z

lib/apollo-federation/schema.rb

-        federation_namespace = ", as: \"#{link_namespace}\"" if link_namespace != DEFAULT_LINK_NAMESPACE
+        schema = ['extend schema']
+
+        all_links.each do |link|


Tried to do make it easier to read but not a ruby expert, please tell me if there's a better way :)

sofie-c

Hi @utay - the team took a look at this yesterday and had some high-level feedback:

Because this change would require dropping support for other versions of GQL-Ruby, this isn't a trivial lift. We've announced in Development / maintenance status of this gem #267 that we're de-prioritizing these types of changes.
We'd also like to see some cleanup to this PR before it's in a merge-able state, just around the printing, hard-coding the default link url, etc.

Thank you for taking the time to open this PR and add new functionality. We'd like to eventually support the @composeDirective but it's something we're de-prioritizing right now.

utay added 4 commits June 14, 2023 23:05

Add support for @composeDirective

c8f5259

Use schema.join

2f19d5e

Lint, add links test

66d060d

Merge pull request #1 from sorare/yu/compose-directive

15cfca6

Add support for @composeDirective

utay changed the title ~~Add support for @composeDirective~~ feat: add support for @composeDirective Jun 16, 2023

Add required to argument

ede329b

grxy requested changes Jul 11, 2023

View reviewed changes

utay added 2 commits August 1, 2023 11:51

Drop minor version check

96ec2ca

Make federation_2_prefix easier to read

d373b7a

utay commented Aug 1, 2023

View reviewed changes

utay requested a review from grxy August 1, 2023 10:08

Lint

0701bff

grxy removed their request for review October 16, 2023 19:34

sofie-c reviewed Nov 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for @composeDirective #253

feat: add support for @composeDirective #253

utay commented Jun 16, 2023

utay commented Jun 16, 2023

grxy commented Jun 20, 2023

grxy left a comment

grxy Jul 11, 2023

utay Aug 1, 2023

grxy Aug 15, 2023

utay Nov 14, 2023

grxy Nov 14, 2023

grxy Jul 11, 2023

utay Aug 1, 2023

grxy Jul 11, 2023

utay Aug 1, 2023

grxy Jul 11, 2023

utay Aug 1, 2023

utay left a comment

utay Aug 1, 2023

utay Aug 1, 2023

utay Aug 1, 2023

sofie-c left a comment

feat: add support for @composeDirective #253

Are you sure you want to change the base?

feat: add support for @composeDirective #253

Conversation

utay commented Jun 16, 2023

utay commented Jun 16, 2023

grxy commented Jun 20, 2023

grxy 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

Choose a reason for hiding this comment

utay 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

sofie-c left a comment

Choose a reason for hiding this comment