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: Adds support for defining Interface entities resolvers #263

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

moonflare
Copy link
Contributor

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:

  1. An Interface Entity defined in Subgraph A can be referenced from Subgraph B using the @interfaceObject directive. This work was added in feat: adds support for the @interfaceObject directive #218
  2. An Interface Entity can be defined in your subgraph. This part is missing and this is what this PR adds.

Description

  • Updates the Union validation so we can allow an interface to be a union member
    • The main issue here is the fact that an Union in GraphQL can't be an Interface 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 allow Interfaces (more precisely Modules) as possible types.
  • Adds validation that all implementing types of an Entity Interface also have the @key directive. This is a requirement from the spec.
  • Adds underscore_reference_keys support to interfaces
    • In a previous Pull Request (feat: Optionally underscore _Any keys #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.
  • Adds some docs for the Interface Entities feature support.

cc @nogates

moonflare and others added 4 commits September 14, 2023 15:42
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.
@moonflare
Copy link
Contributor Author

Hey @grxy! Could you please have a look at this PR when time allows? 🙏🏻

@grxy
Copy link
Collaborator

grxy commented Sep 14, 2023

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! 🙏

@nogates
Copy link

nogates commented Oct 3, 2023

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 :)

Copy link

@sethc2 sethc2 left a 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.

Comment on lines +13 to +24
# 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
Copy link

@sethc2 sethc2 Oct 30, 2023

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?

Copy link

@sofie-c sofie-c left a 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?
Copy link

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
Copy link

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 🤷🏼‍♀️

Comment on lines +13 to +16
# 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.
Copy link

Choose a reason for hiding this comment

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

nit:

Suggested change
# 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.

Copy link

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?

@nogates
Copy link

nogates commented Nov 6, 2023

Thanks @sofie-c ! I will have a look this week or early next week :)

@craig-day
Copy link

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?

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.

6 participants