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

Enhance enumerable graph type determing #262

Merged
merged 9 commits into from
Jun 1, 2024
Merged

Conversation

vcup
Copy link
Contributor

@vcup vcup commented May 11, 2024

Hello,

I've noticed that IReadOnlyList is not recognized as compatible with IEnumerable in schema generation, which may limit the library's functionality in some scenarios.

This PR updates the IsEnumerableGraphType(TypeInfo) method in ReflectionExtensions.cs to use a simple check: type.GetGenericTypeDefinition() == interfaceType.GetGenericTypeDefinition().
It will effect to GraphTypeInfo.cs#166 where marking the GraphTypeInfo.IsListType flag's logic.

Please consider merging this PR.

@tlil tlil self-assigned this May 11, 2024
@vcup
Copy link
Contributor Author

vcup commented May 20, 2024

Hello @tlil , what are you thinks about this PR? have any advice?

@Shane32
Copy link
Member

Shane32 commented May 20, 2024

I'd like to comment that in the main GraphQL.NET package we had an issue where dictionaries and classes that implemented list interfaces were identified as collection types. This was fixed in 5.1 in PR graphql-dotnet/graphql-dotnet#3099 . We restricted types that were identified as collection types to a set list, as follows:

  • IEnumerable
  • IEnumerable<T>
  • IList<T>
  • List<T>
  • ICollection<T>
  • IReadOnlyCollection<T>
  • IReadOnlyList<T>
  • HashSet<T>
  • ISet<T>

Types that derived from these types would not be detected as a list. While the old behavior was retained for a time with a global switch, it has since been removed.

Note that for auto-registering input types, GraphQL.NET must be able to reconstruct the list as well; to support more varied list types, we are adding IListConverterFactory for v8 - see graphql-dotnet/graphql-dotnet#3931

Not sure how much of this applies here, but it seemed like it might be relevant, so I hope it helps.

@vcup
Copy link
Contributor Author

vcup commented May 21, 2024

I think we need recognize derived type as enumerable graph type, because most type are using from 3rd party packages and they will possible use class as Properties insteadof interface.
Maybe we can just avoid recognize the type where implemented IDictionary interface as it.

Copy link
Collaborator

@tlil tlil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response, @vcup. Thanks for putting this together. I've made some minor changes + added a test case for IDictionary. But otherwise, LGTM

@tlil
Copy link
Collaborator

tlil commented Jun 1, 2024

I'd like to comment that in the main GraphQL.NET package we had an issue where dictionaries and classes that implemented list interfaces were identified as collection types. (...) We restricted types that were identified as collection types to a set list, as follows:

Thanks for this added context, @Shane32. Will keep that in mind going forward if similar issues arise from this change.

@tlil tlil merged commit 257800e into graphql-dotnet:master Jun 1, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants