Fix standard decoders trying to decode body on 404 with decode404 set

[ugrepel]

If decode404 flag is set, according to documentation decoders are supposed to return null
on 404 (not found) return codes. Version 10.6 of OpenFeign changed behavior such that
non-empty bodies of 404 responses are decoded by the standard decoders (GsonDecoder,
JacksonJaxbJsonDecoder, JacksonDecoder, JacksonIteratorDecoder, JAXBDecoder, SAXDecoder),
eventually causing DecodeExceptions if the body does not honor the expected data format.
Only on empty bodies, 404 always was honored with a null return value.

Fixes #1528

[velo]

I would say the documentation is wrong in that case.

A 404 may have an boby that could be decoded.... guess that could be truth for any error codes...

Assuming 404 response is null, is far from decoding 404s as the flag suggests.... is more like dismiss404 or threat404AsNulls

[ugrepel]

The documentation was more or less right until 10.6, where most data structures were returned as null when the response status was 404. The only exceptions where those types that were handled by Util.emptyValueOf, i.e. Collections, boolean and Boolean, Optionals, Streams and byte arrays (but no other arrays), with returning "empty" versions of these, or, in case of Booleans, false.

That is, almost all real data structures were returned as null on a 404 response. A typical REST call returns JSON which could be rendered as a map, but which, with a little bit of comfort Java developers are used to, is mapped into a concrete object such as for example a User containing first name, last name, email address and whatever else. Not as a Map with keys such as "firstName" etc.

This behaviour changed with OpenFeign 10.6, where, for 404 responses, Util.emptyValueOf() is not used anymore, but instead the Decoders relied on an empty body to return null.

I also see the name "decode404" as misleading. The old behaviour would better be described as "decode404asNull", the new one as "ignore404". While "decode404asNull" would just be a rename and an application of my PR, a second flag "ignore404" would require adding a flag to the Response data structure so that the decoders could distinguish between "always return null" and "try to decode the response body as if they were the regular data structure". A bit more work than my PR, but shouldn't be magic either.

On the other hand, I do not see a lot of use cases where the regular data structure would make sense in a 404 case. Something like "GET /userroles/1234" with 1234 being the user ID, and the returned structure holding information about the user (which might exist) and an empty Collection for the roles (which could not be found and caused the 404). This seems like a synthetic example, and smells like bad design.

If a client is interested in the contents of the returned error data structure, handling this is better done in a regular ErrorDecoder, not in the 2xx Decoders marked as "2xx + 404".

Also note that the author of the patch applied to 10.6 added unit tests to ensure the behavior on empty bodies, but not for any real data structure.

Whatever, in our case we stumbled across undecodable 404 error messages (we have a standardized error data structure in XML or, as an alternative, problem.JSON, returned in any error case, and that error data structure happened to suddenly not fit to the expected XML object after we upgraded to 11.0 to fix another problem we had in the prior 10.1. We found this at about the latest possible moment before going live and now have to do an additional emergency delivery to our software release, which is released about every 4 months (waterfall model...). It is an easy fix, just add a wrapper Decoder handling 404 as null and otherwise delegate the work to the real Decoder being used, but there's many places this has to be done, or at least many potential places.

[vitalijr2]

Can we mark decode404 as deprecated but add dismiss404 option?

Dismiss404 means that decoder return empty/null value as the documentation says.

[kdavisk6]

decode404 has been misunderstood for a long time. The intent is to allow users to force 404 results into whatever decoder has been declared on the pipeline instead of routing to the error decoder, as is the default behavior for all requests not in 1xx, 2xx, and 3xx ranges.

Folks over the years have overloaded this intent. I'm OK with what's being discussed, however, we should reconsider this option entirely and come up with a more obvious and explicit way for users to decide, by status, how a response should be decoded.