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

Scan for resolvers that have a supertype of the data class #638

Conversation

florensie
Copy link

Fixes #497

Checklist

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

Description

Made necessary changes to class scanning logic to support using supertypes in resolvers.

Copy link
Collaborator

@oryan-block oryan-block left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 👍
I have some questions.

val resolverInfoList = this.resolverInfos.filter { it.dataClassType == item.clazz }
val resolverInfo: ResolverInfo = (if (resolverInfoList.size > 1) {
MultiResolverInfo(resolverInfoList)
val resolverInfoList = this.resolverInfos.filter { it.dataClassType.unwrap().isAssignableFrom(item.clazz.unwrap()) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets extract item.clazz.unwrap() to a variable.

* Checks if all `ResolverInfo` instances are related to the same data type
*/
private fun findDataClass(): Class<out Any> {
val dataClass = resolverInfoList.asSequence().map { it.dataClassType }.distinct().singleOrNull()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we fix this to find the correct type instead of deleting it?

@@ -394,6 +395,10 @@ class ItemResolver : GraphQLResolver<Item> {
}
}

class ItemInterfaceResolver : GraphQLResolver<ItemInterface> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind creating a separate test class with an example closer to the original issue instead of adding to this one?

} else {
if (item.clazz == Object::class.java) {
getResolverInfoFromTypeDictionary(item.type.name)
} else {
resolverInfosByDataClass[item.clazz] ?: DataClassResolverInfo(item.clazz)
if (resolverInfoList.size == 1) {
MultiResolverInfo(resolverInfoList, item.clazz.unwrap())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why multi if there's only one?

@@ -127,7 +127,8 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) {
private fun verifyMethodArguments(method: Method, requiredCount: Int, search: Search): Boolean {
val appropriateFirstParameter = if (search.requiredFirstParameterType != null) {
method.genericParameterTypes.firstOrNull()?.let {
it == search.requiredFirstParameterType || method.declaringClass.typeParameters.contains(it)
it == search.requiredFirstParameterType || (it is Class<*> && it.unwrap().isAssignableFrom(search.requiredFirstParameterType))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the cast to Class here?

@florensie
Copy link
Author

I'm going to redo this some other time.

@florensie florensie closed this Jun 15, 2022
@swollner
Copy link

Hi is there any progress on this?

@florensie
Copy link
Author

@swollner None beyond this PR. Best to keep discussion to #497, feel free to use this PR as a reference/base if you wish to contribute yourself.

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

Successfully merging this pull request may close these issues.

FieldResolverError with resolver for superclass or interface
3 participants