Skip to content

Conversation

@tinnou
Copy link
Contributor

@tinnou tinnou commented Mar 8, 2019

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:

    def schema = TestUtil.schema("""
            type Query {
                nothing: String
            }
            
            type Mutation {
                updateUDI(input: UDIInput!): UDIOutput
            }
            
            type UDIOutput {
                device: String
                version: String
            }
            
            input UDIInput {
                device: String 
                version: String
            }
        """)

    def graphQL = GraphQL.newGraphQL(schema).build()


    def "#1440 when fragment type condition is input type it should return validation error - not classCastException"() {
        when:
        def executionInput = ExecutionInput.newExecutionInput()
                .query('''                    
                    mutation UpdateUDI($input: UDIInput!) { 
                        updateUDI(input: $input) { 
                            ...fragOnInputType 
                            __typename 
                        } 
                    }
                    
                    # fragment should only target composite types
                    fragment fragOnInputType on UDIInput { 
                        device
                        version 
                        __typename 
                    } 
                    
                    ''')
                .variables([input: [device: 'device', version: 'version'] ])
                .build()

        def executionResult = graphQL.execute(executionInput)

        then:

        executionResult.data == null
        executionResult.errors.size() == 1
        (executionResult.errors[0] as ValidationError).validationErrorType == ValidationErrorType.InlineFragmentTypeConditionInvalid
    }

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.

…rong input type condition and overlapping fields are being merged.
@tinnou
Copy link
Contributor Author

tinnou commented Mar 8, 2019

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)

@tinnou
Copy link
Contributor Author

tinnou commented Mar 8, 2019

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]
instead of once with a single set of 6 shop ids: ids[shop-1, exshop-1, shop-2, exshop-2, exshop-3, shop-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.

@andimarek
Copy link
Member

@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.

@andimarek
Copy link
Member

@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.

@andimarek
Copy link
Member

@tinnou the test is fixed

Copy link
Member

@andimarek andimarek left a 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
Copy link
Member

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. 🤔

Copy link
Contributor Author

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.

Copy link
Member

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

@andimarek andimarek merged commit 03dc939 into graphql-java:master Mar 11, 2019
@andimarek andimarek added this to the 12.0 milestone Mar 11, 2019
@andimarek
Copy link
Member

thanks

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.

2 participants