spring-projects / spring-graphql

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

Schema inspection should check actual paginated type name rather than deriving it from the Connection type name #1053

Closed njuro closed 2 months ago

njuro commented 2 months ago

Hello, we have following connection types defined in our GraphQL schema:


type Member { ... }

type EntityOwnerEdge {
    cursor: String!
    node: Member
}

type EntityOwnerConnection {
    edges: [EntityOwnerEdge!]!
    pageInfo: PageInfo!
}

these conforms to the Relay Cursor Connections specification. However, using this schema with Spring GraphQL results in following error, produced by SchemaMappingInspector:

No node type for EntityOwnerConnection.

Upon inspecting the code it is obvious, that the inspector expects every connection type FooConnection to have edge types with node field of type Foo. But the Relay Connection spec doesn't say anything about the names of the node types (nor edge types), only about the name of connection types. Imho it can be merely considered a good practice, but it shouldn't be something that is enforced through hard assert. This expectation also isn't explictly stated anywhere in Spring GraphQL documentation. Am I missing something?

bclozel commented 2 months ago

Hello @njuro, thanks for reaching out. You are pointing at the schema inspector feature, which checks for the presence/absence of type definitions and corresponding data fetchers.

Is your application manually registering such types and data fetchers? Your code snippet is showing the schema part, but I'm wondering what's actually done by the application for this. As far as I remember, our ConnectionTypeDefinitionConfigurer will only automatically register infrastructure for connection types that follow this convention, so in that sense maybe the warning is correct because the relevant data fetchers are not registered?

Ideally, a minimal sample application should help us discuss this issue. Can you provide one?

njuro commented 2 months ago

Hi @bclozel thanks for the swift reply!

We are actually using Netflix DGS framework and right now are trying to migrato to the new starter (com.netflix.graphql.dgs:graphql-dgs-spring-graphql-starter) which integrates with Spring GraphQL. The DGS calls the schema inspector here. Are you suggesting the problem may be with the way DGS is using the inspector? Should I open an issue with them?

bclozel commented 2 months ago

From what I can see, DGS is also following the type+connection/edge/node convention. So, back to my question: how are those types supported in the first place in your application?

njuro commented 2 months ago

We are writing the schema manually and then generating the objects with dgs-codegen plugin. However, we are not using the @connection directive, we define the connection, edge and node types by hand (since we have federated architecture and the docs warn that the directive may not work in these cases). It wasn't a problem until now and from what I can see, the DGS docs also don't indicate that you have to follow this naming convention, it just provides a tool to autogenerate these types for you if you want (which we don't).

bclozel commented 2 months ago

If typed and data fetchers are present in the graphql engine at the time of schema introspection, then this should not be raised as a warning.

I'm probably missing something though. Could you share a minimal sample application so E can have a look, please?

rstoyanchev commented 2 months ago

I think I see the problem. At the moment, the schema inspection derives the paginated type from the ~Connection type name and expects those to be aligned, which in this case they are not. We can improve this by getting the actual type name of the node field in the ~Edge type.

njuro commented 2 months ago

Thank you @rstoyanchev, that's exactly the problem! Sorry I didn't make myself clear at first.