spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.53k stars 305 forks source link

Schema inspection report skips union types #744

Closed smilyanovr closed 8 months ago

smilyanovr commented 1 year ago

I noticed one more false positive report on SkippedTypes

type Query {
    thing: Thing!
}

type Triangle {
    area: Int!
}

type Square {
    area: Int!
}

union Shape = Triangle | Square

type Thing {
    name: String
    description: String
    shape: Shape
}
@Controller
class ThingController {

    @QueryMapping
    fun thing() = Thing("something", "empty")

    @SchemaMapping(typeName = "Thing", field = "shape")
    fun shape() = Triangle(15)
}

sealed interface Shape
data class Triangle(val area: Int): Shape
data class Square(val area: Int): Shape

data class Thing(
    val name: String,
    val description: String
)

Schema introspection report gives output:

GraphQL schema inspection:
    Unmapped fields: {}
    Unmapped registrations: {}
    Skipped types: [Shape]

The interesting part is that if I move the shape field inside the data class as a field without having a resolver for the field, then the introspection report will not have any skipped types.

Originally posted by @smilyanovr in https://github.com/spring-projects/spring-graphql/issues/739#issuecomment-1602788114

rstoyanchev commented 1 year ago

As mentioned under https://github.com/spring-projects/spring-graphql/issues/739#issuecomment-1611153250, the inspection works by traversing the schema, starting with queries, mutations, and subscriptions, comparing schema return type against controller method return types, and descending recursively.

We stop at shape: Shape because the schema inspection does not support union types currently. We could try to iterate over the types of the union, but the controller method likely returns Object as there are no union types on the Java side (and Kotlin I think), so there is no type information to use, and we also don't know what Java type a given union type corresponds to (TypeResolver goes the other way, from Java type to schema type), so we can't continue checks, and automatically report union types as skipped instead.

In this case, the controller method returns a specific type Triangle, which is surprising. I'm not sure why the shape field is Shape then and not Triangle. Based on this mapping and controller method signature, the application always return Triangle, and never Square which the client could request. Could you clarify the intent or provide more detail to understand the use case.

smilyanovr commented 1 year ago

Let me give more concrete real world example. I was trying to simplify the example in the report.

type Schedule {
    id: ID!
    name: String!
    description: String!
    entity: ScheduleEntity!
    type: ScheduleType!
}

union ScheduleEntity = X | Y

type X {
   # some X type specific fields
}

type Y {
   # some Y specific fields
}

Type Schedule has an enum ScheduleType that gives information on the foreign key relation, i.e. what object should be loaded for entity field.

Resolver is defined as:

@SchemaMapping(field = "entity")
fun getEntity(schedule: Schedule): ScheduleEntity {
     return when (schedule.type) {
          ScheduleType.X ->  // read entity X
          ScheduleType.Y -> // read entity Y
     }
}

sealed interface ScheduleEntity
class X(...): ScheduleEntity
class Y(...): ScheduleEntity

Sorry for the pseudo code. Hope this gives more idea about the actual report from schema introspection.

rstoyanchev commented 1 year ago

I see now, thanks.

So here ScheduleEntity acts as a marker interface? We could probably do something with that. We've been assuming a controller return value of Object for a union, but with a marker interface we could discover the implementation types, then use the registered TypeResolver to find the schema type, and try matching the two.

Some experimenting would be needed to confirm how well this works. For once, if the class hierarchy is deeper it would be a challenge because we don't know what actual type(s) are returned at runtime. We can check leaf classes of the hierarchy for a start. Moreover, the TypeResolver would need to be our ClassNameTypeResolver, which we could enhance to allow checks by type rather than by instance as expected typically for TypeResolver implementations.

smilyanovr commented 1 year ago

Yes, ScheduleEntity acts as a marker interface.

rstoyanchev commented 8 months ago

I've had a look at support for unions and interfaces (#739).

As mentioned before, the schema inspection starts from queries, mutations, and subscriptions, comparing schema output types against the return type of the mapped controller method (or any SelfDescribingDataFetcher), descending recursively on fields that are further mapped to another controller method.

For unions and interfaces, we can't use the declared schema field type and the controller method return type as is. We must first discover all schema and Java type pairs that can be returned, and then work through each pair.

One option is to start with the Java type, discover the hierarchy, and then resolve corresponding schema types. However, there is no easy way to use reflection for that, and the controller might return Object which makes it impossible.

The better option is to start from the schema type, which provides easy access to the interface hierarchy, and likewise for a union, we know the exact types included, and exactly what's declared for use by clients (vs all that exists on the server side). From there we need to map every leaf type to a Java type.

Our ClassNameTypeResolver does the reverse, using a function to derive the schema type which by default uses the Java class name but can be customized. There is also a Map of explicit mappings. We can create a similar component that does the reverse, deriving the Java class for a schema type. We can't derive fully qualified Java class names without any help, but we could do better if given one or more top-level packages to look under, or in any case it would be easy for an application to provide a complete function that maps schema to Java types, provided there is some consistent naming pattern. Or if it's down to explicit mappings, for that we can re-use the mappings from ClassNameTypeResolver but in reverse.

In short, with a little help from the application, we could extend schema inspection to union and interface types. I see no alternatives for this to work completely transparently out of the box, but what's required should be fairly straight forward, and not too different from what's required anyway for Java classname to schema type interface and union mappings with GraphQL Java.

rstoyanchev commented 8 months ago

I've created #924 for union and interface support.