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

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

Closed
njuro opened this issue Sep 3, 2024 · 7 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@njuro
Copy link

njuro commented Sep 3, 2024

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?

@njuro njuro changed the title Allow edge type with different name than connection type Allow node type with different name than Connection type Sep 3, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 3, 2024
@bclozel
Copy link
Member

bclozel commented Sep 3, 2024

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?

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Sep 3, 2024
@njuro
Copy link
Author

njuro commented Sep 3, 2024

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?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 3, 2024
@bclozel
Copy link
Member

bclozel commented Sep 3, 2024

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?

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 3, 2024
@njuro
Copy link
Author

njuro commented Sep 3, 2024

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

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 3, 2024
@bclozel
Copy link
Member

bclozel commented Sep 4, 2024

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?

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 4, 2024
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 5, 2024

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.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Sep 5, 2024
@rstoyanchev rstoyanchev added this to the 1.3.3 milestone Sep 5, 2024
@rstoyanchev rstoyanchev changed the title Allow node type with different name than Connection type Schema inspection should check actual paginated type name rather than deriving it from the Connection type name Sep 5, 2024
@rstoyanchev rstoyanchev self-assigned this Sep 5, 2024
@njuro
Copy link
Author

njuro commented Sep 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants