graphql-java-kickstart / graphql-java-tools

A schema-first tool for graphql-java inspired by graphql-tools for JS
https://www.graphql-java-kickstart.com/tools/
MIT License
810 stars 174 forks source link

FieldResolverError with resolver for superclass or interface #497

Open florensie opened 3 years ago

florensie commented 3 years ago

Description

Consider the following dataclass hierarchy:

abstract class Base {
}

class Thing extends Base {
}

This is a custom resolver for the abstract class Base:

class BaseResolver implements GraphQLResolver<Base> {
    public boolean isActive(Base base) {
       return true;
   }
}

Root query resolver with datafetcher that returns our concrete class Thing:

class RootQueryResolver implements GraphQLQueryResolver {
    public Thing getThing() {
        return new Thing();
    }
}

Expected behavior

The following schema should work fine and return true for queries on the active field:

type Query {
    thing: Thing!
}

type Thing {
    active: Boolean!
}

Actual behavior

graphql.kickstart.tools.resolver.FieldResolverError: No method or field found as defined in schema [...]

Steps to reproduce the bug

  1. Create a resolver for a supertype (class or interface) with any datafetcher
  2. Have a query return a subclass of that type
  3. Try building the schema
florensie commented 3 years ago

I have a working solution here but it probably needs some more work before it can be turned into a PR.

KammererTob commented 3 years ago

I am not saying that this is not a bug (or maybe should work), but i think you can work around that by doing this:

abstract class BaseResolver  {
    public boolean isActive(Base base) {
       return true;
   }
}

class ThinkResolver extends BaseResolver implements GraphQLResolver<Thing> {}

Although Thing should probably not be abstract in this case, since abstract classes can never be instantiated.

florensie commented 3 years ago

Right, but that kinda defeats the purpose of polymorphism here. If you have a ThingA, ThingB, ThingC, etc. you'll still have to create a resolver for each of those subclasses, just not the implementation. So it's only really half of a solution, since in some cases you don't exactly know what the subclasses are at compile time, making this very fragile.

Also yeah Thing shouldn't be abstract in my example, I've fixed it.

KammererTob commented 3 years ago

since in some cases you don't exactly know what the subclasses are at compile time

How so?

Regarding the issue: I guess it make make sense to check for resolvers up the ancestor chain starting at the most concrete resolver (since it might override properties). Although i am not sure if this is really worth the effort. Most of the time the concrete classes will not be empty and you would need a resolver for those anyways, and this resolver could then just extend the BaseResolver. But maybe i am just not seeing the downside.

florensie commented 3 years ago

In situations with dynamic linking or as part of a library.

KammererTob commented 3 years ago

In situations with dynamic linking or as part of a library.

I see. Although those could probably still just write their own resolvers (extending your base resolver) when extending your base class, right? But i see how this could be more beneficial now. Thanks for explaining your use-case.

pelletier197 commented 3 years ago

I'm coming a little late on this, but I faced the same issue. Actually, @KammererTob's proposition didn't work for me right out of the box, but I was able to make it work in a pretty similar way if anyone needs it

abstract class BaseResolver<T :  Base> : GraphQLResolver<T>  {
    fun isActive(base: T) : Boolean {
       return true
   }
}

class ThinkResolver : BaseResolver<Thing> 

That being said, I agree with @florensie that it would be great to be able to provide only the GraphQLResolver<T> for an interface / abstract class and have if work automatically with subclasses (or pick the most specific resolver if there is more than one).