Affects: 5.2.4

Affected area: Web client decoders (org.springframework.core.codec.Decoder)

Expected Behaviour Decoders throw an instance of a CodecException when unmarshalling of response to POJO fails.

Actual behaviour Jaxb2XmlDecoder throws NoSuchElementException when the xml returned from the downstream is not valid ie. missing close tag. The exception is actually thrown by the XmlEventDecoder which does not look to be consistent in it's exception handling against all other implementations of the Decoder interface.

I've created a sample test in the project below that shows the failure when handling invalid xml responses and another test showing how the same failure with an invalid json body is handled correctly. https://github.com/michaelmcfadyen/spring-xml-decoder-issue

Comment From: rstoyanchev

Thanks for the sample.

That seems to be the error raised by XMLEventReaderImpl which for some reason wraps the XMLStreamException cause. I suppose we could unwrap it and turn into DecodingException.

Comment From: michaelmcfadyensky

is the following assumption correct:

"All implementations of the Decoder interface should wrap all exceptions in a DecodingException"

Comment From: rstoyanchev

Not really, see this from the Javadoc of DecodingException :

 * Indicates an issue with decoding the input stream with a focus on content
 * related issues such as a parse failure. As opposed to more general I/O
 * errors, illegal state, or a {@link CodecException} such as a configuration
 * issue that a {@link Decoder} may choose to raise.

Comment From: michaelmcfadyen

I suppose we could unwrap it and turn into DecodingException

From the code changes in the commit I can see we are not turning the downstream exception into a DecodingException and instead just wrapping it in a RuntimeException. Is this correct?

Comment From: rstoyanchev

The change simply makes the treatment of a wrapped XMLStreamException consistent with the treatment of an XMLStreamException in the existing catch clause right above the change. So yes, it does not get wrapped as a DecodingException.

I'm not entirely sure why that is. @poutsma any idea? By comparison Jaxb2CollectionHttpMessageConverter wraps with HttpMessageNotReadableException and so does SourceHttpMessageConverter.

Comment From: poutsma

In retrospect, the XmlEventDecoder should not have been a decoder, but just a utility class. It behaves a bit differently than the others, because it was designed as a lower level component. Note that it does not get registered by default.

Comment From: rstoyanchev

This is in Jax2XmlDecoder though.

Comment From: poutsma

I don't know why XMLStreamException is treated wrapped in a runtime exception way in Jaxb2XmlDecoder, it was introduced here, as part of a decodeToMono optimisation. Before that, the XMLStreamException would have been an event in the publisher stream, I think.

Comment From: poutsma

I've created https://github.com/spring-projects/spring-framework/issues/24778 to resolve this in the 5.3 timeline.

Comment From: michaelmcfadyensky

why are we creating a new issue? Shouldn't we just reopen this issue as the issue is not resolved.

Comment From: poutsma

As @rstoyanchev explained, the current behavior is consistent with the existing catch clause. That's all we can do in the 5.2 timeframe, because wrapping in a DecodingException would break backward compatibility for users who rely on XMLStreamExceptions to occur.

So I created a new issue to address this in the 5.3 timeframe, where are backward compatibility guarantees are not as strict.

Comment From: michaelmcfadyensky

Is it not misleading to mark this card as closed when the issue raised has not been fixed/the code change that was applied does not resolve the issue? Also, is there a reason why we can't update the milestone on this card to be 5.3 instead of 5.2? I may be misunderstanding the process you guys follow with issues, if so, apologies.

Comment From: poutsma

We did make changes in the 5.2.x branch as a result of this issue, and we'd like to have a resolved issue linked to those changes. That's why I created a new issue for the remaining changes.