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

fix: support for graphql-ruby v1.13 #147

Merged
merged 2 commits into from
Feb 4, 2022

Conversation

1060ki
Copy link
Contributor

@1060ki 1060ki commented Dec 24, 2021

  • Added support for GraphQL-ruby v1.13.

The following error occurs since graphql-ruby v1.13.1.

  1) ApolloFederation::EntitiesField with the interpreter runtime behaves like entities field when a type with the key directive exists when a Query object is provided sets the Query as the owner to the _entities field
     Failure/Error:
       def to_graphql
         field_defn = super # Returns a GraphQL::Field
         field_defn.metadata[:federation_directives] = @federation_directives
         field_defn
       end

     ArgumentError:
       wrong number of arguments (given 1, expected 0)
     Shared Example Group: "entities field" called from ./spec/apollo-federation/entities_field_spec.rb:349
     # ./lib/apollo-federation/has_directives.rb:12:in `to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/object.rb:137:in `block in to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/object.rb:136:in `each'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/object.rb:136:in `to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/member/accepts_definition.rb:129:in `to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/member/cached_graphql_definition.rb:52:in `to_graphql'
     # ./lib/apollo-federation/has_directives.rb:13:in `to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/member/cached_graphql_definition.rb:28:in `deprecated_to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/member/cached_graphql_definition.rb:21:in `graphql_definition'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema.rb:947:in `to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/member/accepts_definition.rb:129:in `to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema/member/cached_graphql_definition.rb:52:in `to_graphql'
     # ./.bundle/ruby/2.7.0/gems/graphql-1.13.1/lib/graphql/schema.rb:910:in `graphql_definition'
     # ./spec/apollo-federation/entities_field_spec.rb:74:in `block (5 levels) in <top (required)>'

This is because to_graphql is passed parameter argument.
ref: rmosolgo/graphql-ruby@f449669
So I also fixed to_graphql of ApolloFederation::HasDirectives so that it can accept any parameter argument.

@1060ki 1060ki changed the title support for graphql-ruby v1.13 fix: support for graphql-ruby v1.13 Dec 24, 2021
@1060ki 1060ki closed this Dec 24, 2021
@1060ki 1060ki reopened this Dec 25, 2021
@daemonsy daemonsy mentioned this pull request Jan 20, 2022
@Austio
Copy link

Austio commented Jan 21, 2022

@1060ki Thanks so much for the PR! Could you add more details in the description for why the changes are needed here (also if you could what changed in graphql-ruby to require)

@1060ki
Copy link
Contributor Author

1060ki commented Jan 27, 2022

@Austio Thanks for the reply.I have added the description.
Best regards.

@1060ki 1060ki force-pushed the support-graphql-1-13 branch from 47ac5bc to ff1630b Compare January 27, 2022 09:50
@daemonsy
Copy link
Contributor

daemonsy commented Feb 1, 2022

Hi @1060ki thanks for the PR to upgrade this to 1.13 and taking a while to respond.

I have a PR that works on bringing this library up to 1.13.x, but I'm inclined to take yours for the upgrade if you're still keen on working on it.

That way, I can focus more on doing yak shaving, removing the blockers that make it harder to ship the upgrade.

I'll leave a few notes in the code review.

@@ -9,8 +9,8 @@ def add_directive(name:, arguments: nil)
@federation_directives << { name: name, arguments: arguments }
end

def to_graphql
field_defn = super # Returns a GraphQL::Field
def to_graphql(**args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for linking to this. I know the tests were failing but that was in the 1.9.x test suite We removed support for 1.9.x and now tests can pass without adding args.

But the key question IMO is what are the args for? From reading the commit It seems like args were actually added to support passing silence_deprecation_warnings: true to to_graphql.

  1. Should we be using to_graphql(**args, **kwargs)? This captures both positional and keyword args to the function?
  2. With our library patching to_graphql, does the silencing of args still work as expected?

Copy link
Contributor

@daemonsy daemonsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other considerations

@daemonsy
Copy link
Contributor

daemonsy commented Feb 4, 2022

Hi @1060ki , I'd like to get the GraphQL 1.13 support out earlier, will be merging your commit into my branch. Happy Friday!

@daemonsy daemonsy changed the base branch from main to graphql-1.13 February 4, 2022 14:23
@daemonsy daemonsy merged commit dc81c5b into Gusto:graphql-1.13 Feb 4, 2022
daemonsy added a commit that referenced this pull request Feb 4, 2022
* Stop using the legacy graphql_definition method

* Add GraphQL 1.1.3 to the appraisal matrix

* Reconcile gemfiles with main

* support for graphql-ruby v1.13 (#147)

* GraphQL Ruby deals with arity, we don't have to pass args

Co-authored-by: Tomoki Ichikawa <[email protected]>
rylanc pushed a commit that referenced this pull request Feb 4, 2022
# [2.2.0](v2.1.0...v2.2.0) (2022-02-04)

### Features

* Get Apollo Federation to work with GraphQL 1.13.x ([#160](#160)) ([800001b](800001b)), closes [#147](#147)
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.

3 participants