Skip to content

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

Open
tonyghita wants to merge 5 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 5 commits into
graphql-java:masterfrom
tonyghita:fix-interface-union-nullability

Conversation

@tonyghita

@tonyghita tonyghita commented Aug 28, 2026

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

Copy link
Copy Markdown
Author

Alternatively we could always unwrap non-null first?

@andimarek

Copy link
Copy Markdown
Member

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

@tonyghita

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

Copy link
Copy Markdown
Contributor

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 11 5951 (+11 🟢) 5895 (+11 🟢) 0 (±0) 0 (±0) 56 (±0)
Java 17 5951 (+11 🟢) 5894 (+11 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 21 5951 (+11 🟢) 5894 (+11 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 25 5951 (+11 🟢) 5894 (+11 🟢) 0 (±0) 0 (±0) 57 (±0)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 23836 (+44 🟢) 23609 (+44 🟢) 0 (±0) 0 (±0) 227 (±0)

Code Coverage (Java 25)

Metric Covered Missed Coverage vs Master
Lines 30071 3121 90.6% ±0.0%
Branches 8786 1520 85.3% ±0.0%
Methods 8033 1206 86.9% ±0.0%

Changed Class Coverage (1 class)

Class Line Branch Method
g.s.v.TypesImplementInterfaces ±0.0% -3.8% 🔴 ±0.0%
TypesImplementInterfaces — method details
Method Line Branch
isCompatible 100.0% 90.0% (-10.0% 🔴)

Full HTML report: build artifact jacoco-html-report

Updated: 2026-09-04 23:02:15 UTC

@tonyghita

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.

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