Skip to content

fix: Fix context passed to GraphQL::Schema.federation_sdl #95

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

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

jturkel
Copy link
Contributor

@jturkel jturkel commented Aug 28, 2020

This fixes a problem where ApolloFederation::ServiceField#_service passed a context of type GraphQL::Query::Context rather than Hash which violates the contract of GraphQL::Language::DocumentFromSchemaDefinition#initialize. The API of GraphQL::Query::Context and Hash are similar enough that this didn't cause any problems until graphql-ruby 1.9.17 with the introduction of context scoping. After that change, calling to_h, fetch or dig on the context from anywhere in the resolution of ApolloFederation::ServiceField#_service (e.g. schema visibility checks) would result in a NoMethodError: undefined methodmerge' for #<Query::Context ...>`.

@jturkel jturkel changed the title Fix context passed to GraphQL::Schema.federation_sdl fix: Fix context passed to GraphQL::Schema.federation_sdl Aug 28, 2020
@jturkel jturkel force-pushed the feature/sdl-context branch from 1b2a9a6 to c368a94 Compare August 28, 2020 15:02
Copy link
Contributor

@rylanc rylanc left a comment

Choose a reason for hiding this comment

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

This looks great! Could you rebase/merge with master so the graphql-1.11 CI checks are run?

@jturkel jturkel force-pushed the feature/sdl-context branch from c368a94 to 9009cdc Compare September 23, 2020 12:38
@jturkel
Copy link
Contributor Author

jturkel commented Sep 23, 2020

@rylanc - I rebased on top of master and the tests are passing.

@jturkel jturkel force-pushed the feature/sdl-context branch from 9009cdc to 7b75258 Compare September 30, 2020 22:01
@jturkel
Copy link
Contributor Author

jturkel commented Sep 30, 2020

@rylanc - Looks like another merge conflict snuck in so I rebased the PR.

@jturkel jturkel requested a review from rylanc October 29, 2020 15:12
@rylanc rylanc merged commit c13a94e into Gusto:master Oct 29, 2020
@rylanc
Copy link
Contributor

rylanc commented Oct 29, 2020

🎉 This PR is included in version 1.1.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants