`streamInput.readOptional...()` assigned to primitive type fields

[sabi0]

I noticed several places in the code where @Nullable result of streamInput.readOptional...() is assigned to a primitive type field.

For instance: https://github.com/elastic/elasticsearch/blob/ac4db1b272660153bac4f1b09a314e0b229602cd/server/src/main/java/org/elasticsearch/search/suggest/phrase/PhraseSuggestionBuilder.java#L131

https://github.com/elastic/elasticsearch/blob/ac4db1b272660153bac4f1b09a314e0b229602cd/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/Classification.java#L223-L225

It is not a problem per se as there should never be null in the stream as per writeTo()s. But the code smells. Do you think it would be worth fixing it?

I guess allocating a new TransportVersion for this would be an overkill? Then the remaining options are:

• suppress the warnings:

  //noinspection DataFlowIssue: can never read null here - see writeTo()
  ownedByAuthenticatedUser = in.readOptionalBoolean();

• change the field types to boxed ones - this will just push the "potential NPE" further, e.g. to getters

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[ldematte]

By fixing it, I suppose changing read/write to the non-optional alternatives (which will require a new TransportVersion). If we are always writing and reading primitive types it may be worth it. Another option is to change the read part to readBoolean + readX (and throw or have a sensible default if we read false). This will need to be decided case by case.

[sabi0]

These are all the cases I found:

[thecoop]

Looking at these examples, none of these could produce a NPE now, they're either holdovers from when the field was nullable many years ago, or marked as optional because the constructor field could be nullable (and a value was given to the field before it got assigned). There's no risk here, so these can be refactored at an appropriate point over time