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

Circularity between object and interface type definitions #205

Closed
dminkovsky opened this issue Sep 12, 2016 · 3 comments

Comments

@dminkovsky
Copy link
Contributor

commented Sep 12, 2016

The following circularity is problematic for users:

  • Interface types require a type resolver, and type resolvers must be able to reference the object types they need to resolve.
  • However, object types need to be able to reference the interfaces that they implement.

For example:

public class Schema {

    public static GraphQLInterfaceType AnimalType = newInterface()
      .name("Animal")
      .field(newFieldDefinition().name("name").type(GraphQLString).build())
      .typeResolver(new TypeResolver() {
          @Override
          public GraphQLObjectType getType(Object object) {
              return BirdType;
          }
      })
      .build();

    public static GraphQLObjectType BirdType = newObject()
      .name("Bird")
      .withInterface(AnimalType)
      .field(newFieldDefinition().name("name").type(GraphQLString).build())
      .build();
}

In the above example, the AnimalType type resolver can forward reference BirdType because it is implemented as an anonymous class.

However, this does not work if the type resolver is implemented as a lambda:

    public static GraphQLInterfaceType AnimalType = newInterface()
      .name("Animal")
      .field(newFieldDefinition().name("name").type(GraphQLString).build())
      .typeResolver(obj -> BirdType)
      .build();

In this case, BirdType must be referenced by its fully qualified name: Schema.BirdType:

      .typeResolver(obj -> Schema.BirdType)

The situation is further complicated when schema types are not defined as class members. For example, there is no direct way to resolve the circularity above if the same schema is defined within a method body.

public class Schema {

    public static void main(String[] args) {

        GraphQLObjectType BirdType = newObject()
          .name("Bird")
          .withInterface(AnimalType)  // AnimalType is not defined
          .field(newFieldDefinition().name("name").type(GraphQLString).build())
          .build();

        GraphQLInterfaceType AnimalType = newInterface()
          .name("Animal")
          .field(newFieldDefinition().name("name").type(GraphQLString).build())
          .typeResolver(new TypeResolver() {
              @Override
              public GraphQLObjectType getType(Object object) {
                  return BirdType;
              }
          })
          .build();
    }
}

There is no idiomatic way to solve this issue. Looking at the tests, types are defined as class members, leading the user to conclude that that is the only way to define a schema where interface/object relationships must be defined.

This is fine, but not good. To address this issue:

  • A type resolver must be declared in the interface example in the README. The example must be written in such a way that if the user follows this example, they are lead to writing code that compiles and defines a valid schema.
  • The type resolver API could be improved. For example, as @idubrov suggests, the type resolver could accept the GraphQL schema as a parameter.
@bbakerman

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

Is this related to #122 ?

@kaqqao

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

It was one of my motivations behind #122, yes. The solution I proposed there is to allow TypeResolver access to the schema, so that it can get to the GraphQLType by name or other means.

bbakerman added a commit to bbakerman/graphql-java that referenced this issue Apr 28, 2017

Adding extra data to type resolver
Merge branch '122TypeResolver' of git://github.com/kaqqao/graphql-java into kaqqao-122TypeResolver

# Conflicts:
#	src/main/java/graphql/execution/ExecutionStrategy.java
#	src/test/groovy/graphql/RelaySchema.java
#	src/test/groovy/graphql/validation/SpecValidationSchema.java
#	src/test/groovy/graphql/validation/rules/Harness.java

This addresses graphql-java#122 and graphql-java#205.

We had clashing parameters in place.  I fix this up and removed one set of them from the original PR

bbakerman added a commit to bbakerman/graphql-java that referenced this issue Apr 28, 2017

Adding extra data to type resolver
Damn you IDEA - you often miss changed files

Merge branch '122TypeResolver' of git://github.com/kaqqao/graphql-java into kaqqao-122TypeResolver

# Conflicts:
#	src/main/java/graphql/execution/ExecutionStrategy.java
#	src/test/groovy/graphql/RelaySchema.java
#	src/test/groovy/graphql/validation/SpecValidationSchema.java
#	src/test/groovy/graphql/validation/rules/Harness.java

This addresses graphql-java#122 and graphql-java#205.

We had clashing parameters in place.  I fix this up and removed one set of them from the original PR
@andimarek

This comment has been minimized.

Copy link
Member

commented May 13, 2017

this is done with #122 merged

@andimarek andimarek closed this May 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.