-
Notifications
You must be signed in to change notification settings - Fork 77
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: Adds support for defining Interface entities resolvers #263
base: main
Are you sure you want to change the base?
Conversation
Context: In Federation v2.3 there was added support for interfaces to be Entities. There are two ways to consume this new feature. 1. Extend an Interface Entity with the `@interfaceObject` directive, when the Entity is defined by a different subgraph. 2. Implement the Interface Entity with its implementing Type Entitites. There was previos work into getting the first part covered in this gem, but we were still lacking support for the 2nd part. This work address that. 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 Apollo Federation [spec](https://www.apollographql.com/docs/federation/federated-types/interfaces), an interface can be an Entity, and an Entity is an Union. Therefore, we have to extend the validation (`assert_valid_union_member`) for the Entity union to allow `Interfaces` (more exactly `Modules`) as possible types.
In a previous Pull Request (Gusto#248) there was added support for underscoring the keys and while this covered the `Object` class, it didn't also cover the `Interface` module. This commit adds the same `underscore_reference_keys` method to the `Interface` module.
Every entity that implements an interface entity must include all `@keys` from the interface definition. Therefore, we add this extra validation to ensure that.
Hey @grxy! Could you please have a look at this PR when time allows? 🙏🏻 |
Hey there @moonflare. We are in the midst of a team shuffle so reviewing this will be delayed. Thanks for the PR and for your patience! 🙏 |
Hey @grxy ! Sorry to ask, but do you when you will have time to look at this? We are going to need this pretty soon and we would love to see the code here :) |
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.
Hey sorry we've been so long getting to this. Our team had a big shuffle, and Grex switched to a different team.
I had one comment that I'm curious to know if you think could work as a way to avoid adding the interface to the list of entity types.
# 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. | ||
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 | ||
else | ||
super(type_defn) | ||
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.
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 resolve_type
method on an interface and just force that resolution to happen.
Thinking something like this, though I haven't tested to see how well this plays with GraphQL ruby's after_lazy
.
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
end
Do you think something like that would work?
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.
Thanks for your patience as we restructured the team and took a while to get to this PR. I gave this an initial look but am still trying to understand all the context around the change.
Left some initial nits / question. I think we as a team want to discuss some more around Seth's question and make sure we are aligned with the proposed solution in this PR.
@@ -40,13 +40,8 @@ def _entities(representations:) | |||
|
|||
# TODO: Use warden or schema? | |||
type = context.warden.get_type(typename) | |||
if type.nil? || type.kind != GraphQL::TypeKinds::OBJECT | |||
# TODO: Raise a specific error class? |
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
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 comment
The 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 🤷🏼♀️
# 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. |
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:
# 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. | |
# The main issue here is the fact that a 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 a union. Therefore, we have to extend this validation to allow interfaces as possible types. |
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.
I'm not a fan that this behavior is tested in a separate file. In my initial review of this PR, I missed these tests because I was looking for them in entities_field_spec
. Since all the logic lives in entities_field.rb
I think I'd like to see all of the test cases describing its behavior in one file. Can you elaborate on your decision to make this a separate file?
Thanks @sofie-c ! I will have a look this week or early next week :) |
Hey @sofie-c and @sethc2 after some moving around on our side as well, I'm going to pick this work up from @moonflare. Do we still want to keep things in this PR, or would you be OK with me opening up a new PR based on the latest main? |
Context
As part of the Federation v2.3 spec, the Apollo team added a new feature, to support defining and referencing Entities that are interfaces. You can read more about this here, and check the initial implementation here.
This feature consists of two parts:
@interfaceObject
directive. This work was added in feat: adds support for the@interfaceObject
directive #218Description
Union
inGraphQL
can't be anInterface
according to the spec, but at the same time, according to the Apollo Federation spec, an interface can be an Entity, and an Entity is an Union. Therefore, we have to extend the validation (assert_valid_union_member
) for the Entity union to allowInterfaces
(more preciselyModules
) as possible types.@key
directive. This is a requirement from the spec.underscore_reference_keys
support to interfacesObject
class, it didn't also cover theInterface
module. This commit adds the sameunderscore_reference_keys
method to theInterface
module.Interface Entities
feature support.cc @nogates