← 输入框|github.com|刷新
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 11 - - - - -
Java 17 - - - - -
Java 21 - - - - -
Java 25 - - - - -
jcstress - - - - -

Updated: 2026-10-08 16:32:36 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.

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

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tonyghita @andimarek @jayphelps