GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix #1440 ClassCastException - fragment with wrong input type condition by tinnou · Pull Request #1449 · graphql-java/graphql-java

Conversation

@tinnou

@tinnou tinnou commented Mar 8, 2019

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tinnou
Fix graphql-java#1440 ClassCastException when using a fragment with w…
7721f62
…rong input type condition and overlapping fields are being merged.
@tinnou

tinnou commented Mar 8, 2019

Copy link
Copy Markdown
Contributor Author

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tinnou tinnou mentioned this pull request Mar 8, 2019
Fix #1440 ClassCastException when using a fragment with wrong input type condition and overlapping fields are being merged. #1450
Merged
@tinnou

tinnou commented Mar 8, 2019

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@andimarek

andimarek commented Mar 9, 2019

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@andimarek

andimarek commented Mar 10, 2019

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@andimarek

andimarek commented Mar 11, 2019

Copy link
Copy Markdown
Member

@tinnou the test is fixed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

andimarek
andimarek approved these changes Mar 11, 2019
View reviewed changes

@andimarek andimarek left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/test/groovy/graphql/Issue1440.groovy Outdated

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

@andimarek andimarek Mar 11, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tinnou tinnou Mar 11, 2019

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@andimarek andimarek Mar 11, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tinnou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@tinnou
Fix Enum value for FragmentsOnCompositeType
1cb8375
@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

andimarek commented Mar 11, 2019

Copy link
Copy Markdown
Member

thanks

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andimarek andimarek andimarek approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

12.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@tinnou @andimarek
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.