Fix AnnotationReader returning Input classes as Type classes. #532
Conversation
Codecov ReportBase: 95.67% // Head: 95.77% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #532 +/- ##
============================================
+ Coverage 95.67% 95.77% +0.09%
Complexity 1773 1773
============================================
Files 154 154
Lines 4279 4564 +285
============================================
+ Hits 4094 4371 +277
- Misses 185 193 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Thanks for the PR @mdoelker. So, I think this change is needed, at least according to how it should probably be written. However, I tested your branch on our repo and it's actually failing. It seems this method is being called on some Here is the call stack: I'm not sure why it's calling the In any case, it seems this is the reason this was put in place, which clearly looks like WIP code. I'll try to find some time to dig deeper into it. |
|
So, I've been digging into this and the issue is that Creating a testcase for this is difficult because it seems very dependent on the order in which the schema builder processes types. I believe it's using lexical ordering from the registered type namespaces. However, I'm not 100% on this. I am aware of another issue where there are ordering issues with Working on a fix for this and hopefully I can get something reasonable put together. I don't really like how convoluted it is right now to determine what types you're working with. In reality, these type mappers shouldn't be dealing with these reflection classes and docblocks, and should instead, be using a proxy class we manage with methods that return reliable details about the the type (input, output, nullable, etc.). This would require that we change the |
Can the same reason cause issue #536? |
|
@bladl could absolutely be related. As it's currently written, the order in which types are processed matters. It obviously does a pretty good job, but there are clearly some holes. Making sense of all these type mappers, IMO, is complicated and frustrating. And I blame that on the interface design, coupled with a lack of documentation and sub-optimal parameter names. |
|
@oojacoboo Sounds to me like you are using an input type as a return value somewhere, thus using it as an output type. It makes sense to me that it is then trying to map it over and complains about the missing #[Type] annotation, because that's how we declare output types. The solution would be to either strictly use input types for inputs and output types for outputs OR declaring the class as both an #[Input] AND a #[Type] to make it work. Not sure if that really makes sense, but that's a matter of choice I guess. |
|
@mdoelker Yep, that's basically it. So, it appears the issue is with the factories requiring a I guess it's possible that could be changed to require an |
|
I've created #537 for discussion on factory updates. I see no reason this PR shouldn't be merged, even if it's a breaking change for some. |
This fixes the issue described in #531 that would result in error "Cached type in registry is not the type returned by type mapper" when using Input types in mutations via variables.