| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
…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. <==========---> 83% EXECUTING [44s]> :test > 662 tests completed> :test > Executing test ...DataLoaderPerformanceWithChainedInstrumentationTest<==========---> 83% EXECUTING [44s]> :test > 662 tests completed> :test > Executing test ...DataLoaderPerformanceWithChainedInstrumentationTestentation.dataloader.DataLoaderPerformanceWithChainedInstrumentationTest > chainedInstrumentation: 970 ensure data loader is performant for multiple field with lists FAILED
Condition not satisfied:
batchCompareDataFetchers.departmentsForShopsBatchLoaderCounter.get() == 1
| | | |
| 2 2 false
graphql.execution.instrumentation.dataloader.BatchCompareDataFetchers@60f76b0
at graphql.execution.instrumentation.dataloader.DataLoaderPerformanceWithChainedInstrumentationTest.chainedInstrumentation: 970 ensure data loader is performant for multiple field with lists(DataLoaderPerformanceWithChainedInstrumentationTest.groovy:66)
|
Sorry, something went wrong.
|
I was able to reproduce the issue by running the single method DataLoaderPerformanceTest.970 ensure data loader is performant for multiple field with lists. It looks like the departmentsForShopsBatchLoader gets called twice with two sets of 3 shop ids: ids[shop-1, shop-2, shop-3] and ids[exshop-1, exshop-2, exshop-3] There is a random sleep in BatchCompareDataFetchers.sleepSome which I believe to be the culprit. The shops batch loader are dispatched at the same time, but with this random sleep the shop results might actually be resolved at different times. When configuring the sleep time to be 1ms for the shops and 1000ms for the expensive shops, the issue happens consistently. 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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good, but see my comment about the error type
Sorry, something went wrong.
|
|
||
| executionResult.data == null | ||
| executionResult.errors.size() == 1 | ||
| (executionResult.errors[0] as ValidationError).validationErrorType == ValidationErrorType.InlineFragmentTypeConditionInvalid |
There was a problem hiding this comment.
this seems wrong or the naming is off: it is not a InlineFragment. 🤔
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
sure ... do it in one ... thanks
Sorry, something went wrong.
|
thanks |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Description
In cases where a fragment type condition is invalid (say it was an input type instead of a composite type), the FragmentsOnCompositeType rule rightfully collects the validation error. However, the OverlappingFieldsCanBeMerged validation rule is wrongly assuming that the type condition is valid and attempts to cast it as a GraphQLOutputType, throwing a ClassCastException described in issue #1440.
I wrote a test to reproduce the issue:
and this throws
graphql.Issue1440 > #1440 when fragment type condition is input type it should return validation error - not classCastException FAILED java.lang.ClassCastException: graphql.schema.GraphQLInputObjectType cannot be cast to graphql.schema.GraphQLOutputType at graphql.validation.rules.OverlappingFieldsCanBeMerged.collectFieldsForFragmentSpread(OverlappingFieldsCanBeMerged.java:308) at graphql.validation.rules.OverlappingFieldsCanBeMerged.collectFields(OverlappingFieldsCanBeMerged.java:294) at graphql.validation.rules.OverlappingFieldsCanBeMerged.leaveSelectionSet(OverlappingFieldsCanBeMerged.java:58)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.