Skip to content

Add nullability annotations in DataFetchingEnvironment & sub-classes #3582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Salzian
Copy link
Contributor

@Salzian Salzian commented Apr 29, 2024

While using Netflix' DGS with Kotlin, we ran into an issue where it was not immediately clear that DataFetchingEnvironment.getLocalContext can return null. After reading the documentation and the rest of the DataFetchingEnvironment interface, I noticed the lack of nullability annotations for Kotlin interoperability. However, I also saw that other classes in this project already use the Jetbrains annotations.

In this PR I added nullability annotations to all methods of the DataFetchingEnvironment interface that had mentions about nullability in their documentation. For patching the whole class I currently lack the overview of the bigger picture of this interface.

I followed through on sub-classes that override these methods.

Copy link
Member

@bbakerman bbakerman left a comment

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 added this pull request to the merge queue May 21, 2024
Merged via the queue into graphql-java:master with commit f3253b4 May 21, 2024
1 check passed
@Salzian Salzian deleted the add-nullability-annotations-in-data-fetching-environment branch May 21, 2024 09:12
@juriad
Copy link

juriad commented May 27, 2024

IMHO this should not have been included in a minor version. Adding @Nullable annotation to a method is a breaking change for Kotlin code. We are getting errors like:

> Task :dmm-dq:compileKotlin FAILED
e: file:///builds/engineering/.../validation/onthefly/OnTheFlyValidationGqlOperation.kt:32:65 Only safe (?.) or non-null asserted (!!.) calls are allowed on a nullable receiver of type '@Nullable() com...graphql.types.node.NodeWrapperBase?'.

caused by this line:

catalogItemId = env.getSource<NodeWrapperBase>().gid,

@hpuac
Copy link

hpuac commented May 27, 2024

@juriad Lovely, the compiler now protects you against a possible NPE that you did not check in your code before, amazing!
I agree with the minor version argument though, including it in the next major release would have been fine as well 🙂

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.

5 participants