Enable Kotlin compiler to recognise @org.elasticsearch.common.Nullable annotation for null checks.

[vinoov]

This change allows Elasticsearch plugins written in Kotlin to read the nullability
information present in the ES API declarations and warn of unsafe API usage during compile time.
Details here: https://kotlinlang.org/docs/reference/java-interop.html#jsr-305-support.

• Have you signed the contributor license agreement?
• Have you followed the contributor guidelines?
• If submitting code, have you built your formula locally prior to submission with gradle check?
• If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
• If submitting code, have you checked that your submission is for an OS and architecture that we support?
• If you are submitting this code for a class then read our policy for that.

[elasticmachine]

Pinging @elastic/es-core-infra

[alpar-t]

@elasticmachine test this please

[vinoov]

Couldn't reproduce the build failures in CrossClusterSearchUnavailableClusterIT using the "REPRODUCE WITH" command from the build output. Not sure what's up with the packaging-sample build either, but assume that's unrelated to this change.

[alpar-t]

@elasticmachine update branch

[mark-vieira]

Thanks for the PR @vinoov. I've looked at this a bit. This will mostly have a minor effect due to the way we use @Nullable in our codebase. It's mostly used to annotate method arguments. In this scenario the Kotlin benefits are minimal since calling a method whose argument takes a platform type, and one that takes a nullable type are basically indistinguishable. In both cases the Kotlin compiler will disable null checking so there's no real additional safety here. Although in the IDE the parameter type is correctly shown to be a nullable type vs a platform type.

There are a handful of cases where we annotate methods, and in this case there is a discernible difference when calling that code from Kotlin. If a method is annotated with @Nullable then in Kotlin it will return a nullable type, meaning the result must have null-checks associated with it, and can only be assigned to a nullable reference. For example given the following Java code:

    @Nullable
    public static String nullable() {
        return null;
    }

We now get the following behavior when calling from Kotlin:

I'd say this is definitely an improvement in terms of Kotlin support. Its impact is minimal as I say as there are only a few instances where methods are annotated with @Nullable and I'm not even sure to the extent where those APIs are even used by plugins. @rjernst I did a quick search on usages, do you think any of those instances where @Nullable is placed on a method body would benefit plugin authors or is that all internals anyhow?

[vinoov]

@mark-vieira The motivation for this was an NPE I hit by not checking the return value of RestRequest.getXContentType. Looking through the code some other @nullable APIs that plugins might hypothetically use are:

• readOptionalXXX methods in org.elasticsearch.common.io.stream.StreamInput
• some of the methods in org.elasticsearch.search.SearchHits

[vinoov]

This will mostly have a minor effect due to the way we use @Nullable in our codebase. It's mostly used to annotate method arguments. [...] Although in the IDE the parameter type is correctly shown to be a nullable type vs a platform type.

Agreed; for method arguments NonNull is more useful than Nullable. What are your thoughts on either adding a new @NonNullApi annotation (with @TypeQualifierDefault) or @ParametersAreNonNullByDefault to specific packages/classes (for e.g. org.elasticsearch.client.Client)?

[mark-vieira]

Agreed; for method arguments NonNull is more useful than Nullable. What are your thoughts on either adding a new @NonNullApi annotation (with @TypeQualifierDefault) or @ParametersAreNonNullByDefault to specific packages/classes (for e.g. org.elasticsearch.client.Client)?

This would probably require a larger discussion. I'd probably start by opening an issue on the topic.

[mark-vieira]

This has been merged. Thanks for the contribution @vinoov.

[vinoov]

hi @mark-vieira. Will this make it into an upcoming ES 7.x release or do I have to wait for 8.x? I'm not familiar with the github issue tags ES uses. Thanks in advance.

[mark-vieira]

The master branch is the development branch for 8.x so currently it would go in that. I don't see too much issue with backporting this to 7.x though, which at the point would mean it would go out in 7.4.

[vinoov]

Thanks for clarifying @mark-vieira. Do I need to send a patch to backport to 7.x or is it done automatically?

[mark-vieira]

I've created a PR to backport this to the 7.x branch.

#44518

[mark-vieira]

@vinoov this has been backported to the 7.x branch.

[vinoov]

Thanks @mark-vieira .