⚡ Sangsakala Proxy 🏠 Home
Skip to content

Fix non-null union field implements nullable union interface field - #4467

Open
tonyghita wants to merge 9 commits into
graphql-java:masterfrom
tonyghita:fix-interface-union-nullability
Open

Fix non-null union field implements nullable union interface field#4467
tonyghita wants to merge 9 commits into
graphql-java:masterfrom
tonyghita:fix-interface-union-nullability

Conversation

@tonyghita

@tonyghita tonyghita commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown

According to the spec IsValidImplementationFieldType(), concrete type fields are able to strengthen the nullability guarantees made by an interface type field.

interface I { f: F }
type T implements I { f: F! }

This mostly happens, except when the field is a Union type.

type A { a: ID }
union U = A
interface I { u: U }

type B implements I {
  u: U! # valid, but treated as invalid
}

schema { query: B }

This change moves the branch for isNonNull up so that non-null unions fall-through hit the unwrap case first. This is closer to how the specification is written.

@tonyghita

tonyghita commented Aug 28, 2026

Copy link
Copy Markdown
Author

Alternatively we could always unwrap non-null first?

@andimarek

andimarek commented Aug 28, 2026

Copy link
Copy Markdown
Member

Have not looked into it further, but what about nullability inside list and list of lists?

@jayphelps jayphelps mentioned this pull request Aug 29, 2026
Merged
@tonyghita

tonyghita commented Aug 31, 2026

Copy link
Copy Markdown
Author

@andimarek I added a more comprehensive test across different type combinations in 2e21d2a. I only found the case when a union field strengthens nullability to fail.

I moved the branch for unwrapping non-null types up so that the control flow is closer to the spec, rather than the extra instanceOf check on objectType.

@github-actions

github-actions Bot commented Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 17 5837 (-394 🔴) 5828 (-347 🔴) 1 (+1 🔴) 0 (±0) 8 (-48)
Java 21 5837 (-394 🔴) 5828 (-347 🔴) 1 (+1 🔴) 0 (±0) 8 (-48)
Java 25 5837 (-394 🔴) 5828 (-347 🔴) 1 (+1 🔴) 0 (±0) 8 (-48)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 17543 (-1182 🔴) 17516 (-1041 🔴) 3 (+3 🔴) 0 (±0) 24 (-144)

Updated: 2026-11-04 06:24:09 UTC

@tonyghita

tonyghita commented Sep 8, 2026

Copy link
Copy Markdown
Author

It seems like moving the branches around caused some changes in test branch coverage metrics.

graphql.schema.validation.TypesImplementInterfaces Branch: 90.4% (was 94.2%, delta -3.8%, missed: 5 was 3)

I added some tests for the missing branches as well to get things unblocked in 38e843e.

@tonyghita

tonyghita commented Oct 13, 2026

Copy link
Copy Markdown
Author

@andimarek gentle nudge to take a look over the chagnes

@jayphelps

jayphelps commented Oct 26, 2026

Copy link
Copy Markdown

To add confidence that this is inline with the spec we landed additional tests in the reference implementation of GraphQL to verify this behavior is valid as well.

bbakerman
bbakerman approved these changes Nov 4, 2026
View reviewed changes

@bbakerman bbakerman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this

bbakerman
bbakerman reviewed Nov 4, 2026
View reviewed changes
Comment thread src/test/groovy/graphql/schema/validation/TypesImplementInterfacesTest.groovy
"interface type 'SecondInterface' cannot implement 'FirstInterface' because that would result on a circular reference"
] as Set
}

@bbakerman bbakerman Nov 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra tests. More tests good!

This branch has not been deployed

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

Reviewers

@bbakerman bbakerman bbakerman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tonyghita @andimarek @jayphelps @bbakerman