Ignore null HttpMessageConverter in RestTemplate and HttpMessageConverterExtractor

[fodil-a]

Prior to this commit, if a null HttpMessageConverter was configured
in the RestTemplate, this would lead to a NullPointerException once
the list of converters was accessed.

This commit avoids such exceptions by ignoring null converters.

See gh-23123

[fodil-a]

This is an edit from pull request: #23132
I needed to change my fork's branch.

[sbrannen]

Tentatively slated for 5.2 RC1 and considering whether to backport to 5.1.x.

[sbrannen]

@rstoyanchev and @jhoeller, shall we consider this a bug instead of an enhancement and assign it to 5.1.x?

[rstoyanchev]

Having null converter is something we don't even expect and seeing the proposed checks makes it even stranger. Why would there be a null converter in the first place? I think the solution should be to reject these earlier where converters are configured.

[rstoyanchev]

@sbrannen I don't consider it a bug. It's simply a missing validation.

[sbrannen]

@sbrannen I don't consider it a bug. It's simply a missing validation.

Thanks for the feedback.

I actually came to the same conclusion myself.

Having null converter is something we don't even expect and seeing the proposed checks makes it even stranger. Why would there be a null converter in the first place?

I agree that it is a user error if a null converter is present.

I think the solution should be to reject these earlier where converters are configured.

The challenge is that RestTemplate#getMessageConverters() is documented as follows.

Return the list of message body converters.

The returned List is active and may get appended to.

Thus, users can modify the list (e.g., by appending/inserting null) at any time. In other words, RestTemplate returns a modifiable ArrayList of converters, and we probably shouldn't change that at this point.

I would rather perform the non-null validation every time the converters are accessed in RestTemplate and HttpMessageConverterExtractor and throw an IllegalStateException if we encounter a null reference, as opposed to simply ignoring them as in this PR.

@rstoyanchev, what do you think about that?

[rstoyanchev]

users can modify the list (e.g., by appending/inserting null) at any time.

I would definitely add checks in the constructor and setter since that would provide the benefit of an early warning. Beyond that I think it's not worth repeatedly checking for something that should not be happening in the first place. At that point it almost doesn't matter how it fails, NPE or IAE. Either way it fails it needs to be fixed in the application.

[sbrannen]

Superseded by #23151