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 non-null union field implements nullable union interface field by tonyghita · Pull Request #4467 · graphql-java/graphql-java

Conversation

@tonyghita

@tonyghita tonyghita commented Aug 28, 2026 •
edited
Loading

Uh oh!

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jayphelps reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@tonyghita
Fix non-null union field implements nullable union interface field
f6e7ca7
@tonyghita

tonyghita commented Aug 28, 2026

Copy link
Copy Markdown
Author

Alternatively we could always unwrap non-null first?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jayphelps jayphelps mentioned this pull request Aug 29, 2026
test: accept non-null union implementing a nullable union graphql/graphql-js#4851
Merged
tonyghita added 2 commits August 31, 2026 08:33
@tonyghita
Add test coverage for all nullabilty strengthening cases
2e21d2a
@tonyghita
Move unwrap non-null first so we're closer to spec
ed84963
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tonyghita
Merge branch 'master' into fix-interface-union-nullability
cca8408
@github-actions

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

Uh oh!

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

Copy link
Copy Markdown
Contributor

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 17 - - - - -
Java 21 - - - - -
Java 25 - - - - -
jcstress - - - - -

Updated: 2026-11-10 18:42:00 UTC
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tonyghita
Add test coverage for TypesImplementInterfaces branches
38e843e
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tonyghita added 3 commits October 1, 2026 16:26
@tonyghita
Merge branch 'master' into fix-interface-union-nullability
19c01c2
@tonyghita
Merge branch 'master' into fix-interface-union-nullability
5ed8988
@tonyghita
Merge branch 'master' into fix-interface-union-nullability
371322c
@tonyghita

tonyghita commented Oct 13, 2026

Copy link
Copy Markdown
Author

@andimarek gentle nudge to take a look over the chagnes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tonyghita
Merge branch 'master' into fix-interface-union-nullability
73583b8
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bbakerman
bbakerman approved these changes Nov 4, 2026
View reviewed changes

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

Thanks for this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

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.

Thanks for the extra tests. More tests good!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bbakerman

bbakerman commented Nov 5, 2026

Copy link
Copy Markdown
Member

Looks like there are test failures
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bbakerman
Merge branch 'master' into fix-interface-union-nullability
081c438

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

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

@tonyghita @andimarek @jayphelps @bbakerman
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 01402503 — 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.