-
Notifications
You must be signed in to change notification settings - Fork 88
fix: Adds support for defining Interface entities resolvers #263
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
base: main
Are you sure you want to change the base?
Changes from all commits
1dc3562
3b282f1
8d511d6
adf96d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,5 +9,18 @@ class Entity < GraphQL::Schema::Union | |||||||||||||||||
| def self.resolve_type(object, context) | ||||||||||||||||||
| context[object] | ||||||||||||||||||
| end | ||||||||||||||||||
|
|
||||||||||||||||||
| # The main issue here is the fact that an union in GraphQL can't be an interface according | ||||||||||||||||||
| # to the [spec](https://spec.graphql.org/October2021/#sec-Unions.Type-Validation), but at | ||||||||||||||||||
| # the same time, according to the Federation spec, an interface can be an Entity, and an Entity | ||||||||||||||||||
| # is an union. Therefore, we have to extend this validation to allow interfaces as possible types. | ||||||||||||||||||
|
Comment on lines
+13
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||||||||||||||
| def self.assert_valid_union_member(type_defn) | ||||||||||||||||||
| if type_defn.is_a?(Module) && | ||||||||||||||||||
| type_defn.included_modules.include?(ApolloFederation::Interface) | ||||||||||||||||||
| # It's an interface entity, defined as a module | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I don't think this comment is that helpful as it re-iterates the conditional above 🤷🏼♀️ |
||||||||||||||||||
| else | ||||||||||||||||||
| super(type_defn) | ||||||||||||||||||
| end | ||||||||||||||||||
| end | ||||||||||||||||||
|
Comment on lines
+13
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I totally get why we would do this but I was curious if you think we could tweak the implementation to avoid creating a union that is technically invalid according to the GraphQL spec. I was thinking we keep the requirement that all entity interface implementations are entities, and continue to check that when getting the list of possible types for the entities field, but then drop it from the final list of possible types and then in the entities resolver we use the Thinking something like this, though I haven't tested to see how well this plays with GraphQL ruby's maybe_lazies = grouped_references_with_indices.map do |typename, references_with_indices|
references = references_with_indices.map(&:first)
indices = references_with_indices.map(&:last)
# TODO: Use warden or schema?
type = context.warden.get_type(typename)
# pretend we have a has_key_directive method to check if a thing has a key directive on it
if type.nil? || !(type.kind == GraphQL::TypeKinds::OBJECT || (type.kind == GraphQL::TypeKinds::Interface && has_key_directive(type)))
raise "The _entities resolver tried to load an entity for type \"#{typename}\"," \
' but no entity type of that name was found in the schema'
end
# ... EXISTING CODE calling resolve reference(s) ...
final_result[i] = context.schema.after_lazy(result) do |resolved_value|
# force the type to the concrete type
type_to_set = if type.kind == GraphQL::TypeKinds::Interface
type_class.resolve_type(resolved_value, context)
else
type
context[resolved_value] = type_to_set
resolved_value
endDo you think something like that would work? |
||||||||||||||||||
| end | ||||||||||||||||||
| end | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why did we remove this TODO? I actually am inclined to leave it because we probably do want to raise a specific error here as opposed to just
RuntimeError