-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix #1440 ClassCastException - fragment with wrong input type condition #1449
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
Fix #1440 ClassCastException - fragment with wrong input type condition #1449
Conversation
…rong input type condition and overlapping fields are being merged.
|
Hmm, looks like there is a flaky test. Have you seen this before? I'm unable to reproduce on my local so far. I'll try further. |
|
I was able to reproduce the issue by running the single method It looks like the There is a random sleep in My intuition is the random sleep was introduced for the deferred scenarios and is now randomly impacting the non-deferred test case. I can put together a PR to separate these use cases if you think I'm on the right path. |
|
@tinnou thanks a lot for looking into the flaky test. I am not sure if you are on the right path ... will also have a look. |
|
@tinnou you were right: it is very likely caused by theses sleeps combined with the general behaviour of the field level tracking approach. We will fix that test. |
|
@tinnou the test is fixed |
andimarek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but see my comment about the error type
|
|
||
| executionResult.data == null | ||
| executionResult.errors.size() == 1 | ||
| (executionResult.errors[0] as ValidationError).validationErrorType == ValidationErrorType.InlineFragmentTypeConditionInvalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems wrong or the naming is off: it is not a InlineFragment. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh great catch. The FragmentsOnCompositeType is indeed using the wrong enum value. I would do another PR to fix it but since it's fairly small, I'll fix in this one if that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure ... do it in one ... thanks
|
thanks |
Description
In cases where a fragment type condition is invalid (say it was an input type instead of a composite type), the
FragmentsOnCompositeTyperule rightfully collects the validation error. However, theOverlappingFieldsCanBeMergedvalidation rule is wrongly assuming that the type condition is valid and attempts to cast it as aGraphQLOutputType, throwing aClassCastExceptiondescribed in issue #1440.I wrote a test to reproduce the issue:
and this throws
Fix
The fix consists of removing the un-necessary casts as they appear to not be needed.
I also added an additional test to cover for inline fragments as well.
I will do a second PR for the stable branch.