Empty payload should never enforce a MBR to handle the Response

[rmannibucau]

Seems jaxrs 2.1 forces MBR to throw a no content exception when stream is empty. It leads to several issues and should be reverted:

1. It breaks MBR responsability which is only about serialization
2. It breaks specs integration (you cant handle with an exception mapper native exceptions for this case) - and forces a third jaxrs integration through an interceptor instead of letting the rezponsability to either mappers or the ioc itself
3. It breaks backward compatibility (2 being one example) which is a no go for any javaee/jakartaee decision
4. This is rarely desired and all apps will want a payload for that case

Please revert that change before it gets adopted (we are still around jaxrs 2.0 in the industry and libs)

[chkal]

Seems jaxrs 2.1 forces MBR to throw a no content exception when stream is empty.

That's not true. The API docs of MessageBodyReader.readFrom() state:

In case the entity input stream is empty, the reader is expected to either return a Java representation of a zero-length entity or throw a NoContentException in case no zero-length entity representation is defined for the supported Java type.

So it is either NoContentException or a Java representation of a zero-length entity.

[chkal]

@mkarg Ups, looks like you assigned the issue while I was typing. :-)

[mkarg]

@chkal No problem, just go ahead. The more opinions the better. :-)

[rmannibucau]

A default empty representation is a fallback (as optional orElse) which bypasses parsers and fakes processing which is wrong too and has the same drawback as doing something unexpected in place or the default behavior. It breaks most apps btw assuming the parser validated the payload.

So please, if you want that, add an explicit toggle on the method.

Edit: just keep in mind the processing can validate the payload (jsonschema for ex) on the fly and such default can so lead to invalid runtime/processing.

[chkal]

I'm not sure if I understand the problem correctly.

The API doc statement quoted above basically just says:

Either throw a NoContentException or do what every you want to deserialize a zero-length entity.

So could you perhaps explain a bit more why you think that this is problematic?

[mkarg]

@rmannibucau Empty streams are not "wrong" by design, so no, the empty representation is not just a "fallback", but actually is the expression that the MBR decided that it is a "normal" entity: For example, there might be MIME types allowing empty streams as quite normal data (like text/plain where an empty stream simply is an empty string and no problem or exceptional case at all). The decision if this is a normal entity (the empty entity instance case) or something that the application shall handle (the NoContentException) is up to the MBR to decide. The reaction upon that is up to the application (!) to decide. Regarding your request, I doubt that this can get "reverted" easily. This detail is a MUST of the JAX-RS 2.1 specification effective since July 13, 2017. There are existing applications that rely on this detail, which is the reason why I opened the bug report in the Johnzon tracker. If we would remove this detail, these applications would break. Also, this detail is the sole chance an application could register a ReaderInterceptor to actively decide to accept an empty stream: NoContentException is not a failure indicator, it is an indicator that the stream was empty. If we "revert" this detail, those applications must buffer the input stream using their own PushbackReader, which is a bad design decision due to two facts: (a) It puts technological aspects on behalf of the application, while not being an application-level problem, (b) It adds such a high load ontop that youself told me you do not want to have double-buffering.

[mkarg]

@chkal Some background: Romain and I moved this discussion over from the original bug report at Johnzon, as it would be OT there. See https://github.com/apache/johnzon/pull/52.

[mkarg]

So could you perhaps explain a bit more why you think that this is problematic?

Basically his claim is that Johnzon's several MBRs are JAX-RS 2.0 compliant and he wants to claim them JAX-RS 2.1 compliant without changing them, which is impossible due to the fact that those MBRs do throw exceptions other than NoContentException and WebApplicationException (which BTW was disallowed by JAX-RS 2.0 already), and due to the fact that these MBRs neither return an empty instance (which AFAIK seems to be impossible as JSON syntax apparently has no representation for "no bytes at all" but just for "empty braces") nor throw NoContentException. He thinks we can "undo" JAX-RS 2.1, or allow him to ignore this part of the spec, which both is simply impossible as existing JAX-RS 2.1 compliant application will break then. Such a break was the cause why I reported the issue at Johnzon, and even published a simple-but-effective (while inefficient) solution for him. He dislikes the solution as it is not fast enough. So I left it up to him to fix it in a different way now.

[mkarg]

Just checked the history: Actually NoContentException was not added in 2.1, but in 2.0 already. So it is even more years in the spec.

[rmannibucau]

Hmm, does not change much at the end, it broke 1.0 which was forbidden at javaee (at that time ;)) so still a regression at spec level. I can provide examples of such regression if not obvious and why it such a spec change cant be implicit.

[mkarg]

@rmannibucau You need to accept the truth that we will not treat it as a regression and there will not be the outcome that one can call his MBR "JAX-RS 2.1 compliant" without either throwing NoContentException or returning an empty instance (if possible and feasible). So actually I do not understand what you try to reach with your further arguing?

[rmannibucau]

@mkarg as a spec you must guarantee backward compatibility, this is a fact and what makes jakarta valuable. This is not the case here (even review jackson if you doubt https://github.com/FasterXML/jackson-jaxrs-providers/blob/master/base/src/main/java/com/fasterxml/jackson/jaxrs/base/ProviderBase.java#L1031) so this breaks user facing behavior in terms of code or client behavior. As a spec you must fix it - no real choice. Options are IMHO:

1. Make it optional and up to vendors (SHOULD instead of MUST)
2. Make it explicit through a toggle on Application or endpoints or application properties, this still breaks some apps but enable to get back a correct behavior

Note you cant fully revert it since it would break 2.0 apps.

Personally, I think 1. is ok for this particular case but 2. is saner even if it requires some more effort.

Side note: indeed it also breaks other use cases you cant impl anymore passing from 1 to 2 but this is less blocking than the behavior regression which is for me a violation of the full contract of the spec with users.

[mkarg]

@rmannibucau It is backwards compatible. But it is not compliant. This is a difference.

[rmannibucau]

@mkarg ok, explain me how with a JAXRS 1 app using a parser mapper I can migrate on JAXRS 2 without rewriting my app? If you do then I agree it is compatible, if you can't (and guess what, you cant ;)), then it is just not true.

[mkarg]

@rmannibucau In JAX-RS there is no such thing named "parser mapper".

[rmannibucau]

@mkarg replace it by "exception thrown by a MBR when there is an incoming empty stream" ;)

[mkarg]

@rmannibucau JAX-RS 1.1 did not allow an MBR to throw any other exceptions besides WebApplicationException and IOException (see JAX-RS 1.1. JavaDocs):

Throws:
java.io.IOException - if an IO error arises
WebApplicationException - if a specific HTTP error response needs to be produced. Only effective if thrown prior to the response being committed.

So a compliant MBR was forced to throw WebApplicationException. This still works in JAX-RS 2.x.

[rmannibucau]

@mkarg no, this is not true, any RuntimeException is allowed and was handlable by an exception mapper

[mkarg]

@rmannibucau I cannot find anything in neither JavaDocs nor Spec of JAX-RS 1.1 which proofs your claim. It might be the case that you understood it that way, and it might be that products accepted that, but it was definitively not covered by the spec. This means, behavior not covered by the spec might be broken, yes, but not the spec itself.

[rmannibucau]

@mkarg no no no, it is explicit in the spec but even if it wouldnt, it is explicit in java language that a method can throw anything and any method of any spec fallbacks on JLS so it would be built-in even if not written. Now, in this case, it is also explicit through, at least, ExceptionMapper contract which supports Throwable and exception mappers are called for MBR. So it is covered ;).

Spec quotes to complete it (4.4):

Exception mapping providers map a checked or runtime exception to an instance of Response.

and

When a resource class or provider method throws an exception for which there is an exception mapping provider, the matching provider is used to obtain a Response instance

[mkarg]

@rmannibucau Unfortunately this is simply is a common misunderstanding. The spec explains all components separately in different chapters. 4.4 explains what an exception mapper does in case an exception is propagated to it. This is not to be understood as a general agreement that for all times all future specs will not introduce new rules to throw particular exceptions, as it is just to be understood as a "last resort fallback" for exception not handled otherwise (like NoContentExcpetion). Again, this definitively won't get "fixed" or "ignored". All we can do is discuss to change a future spec. For now, the spec is "as-is".

[rmannibucau]

@mkarg if what you are saying is true, it means there is not a single JAX-RS implementation which is compliant to the spec so either the spec must align on implementations or you break again all users ;). Indeed I'm caricaturing a bit but this is really where we are. Does the spec enable to throw a RuntimeException in a MBR? yes, it is explicit through the signature/API. Does the spec enable to handle the exception in an exception mapper? Yes it is explicit in 4.4. So did the NoContentFound change broke the backward compatibility? Obviously. Since it is forbidden it should be mitigated even if you keep the same default (which is fine for me) by a way to disable it.

[mkarg]

@rmannibucau Again we seem to misunderstand each other (English is not my mother's tongue, so maybe my arguments are not perfectly laid out). Certainly you can throw any exception you like from within any JAX-RS component, as it simply is Java. And certainly will an MBR catch it as a last resort (if no other component in the call stack catches it). But the JAX-RS specification does not guarantee that it will never extend in a away that changes the workflow (like the introduction of filters and interceptors did), nor that it requests in later versions to use spec-defined exceptions (like NoContentException instead of product-defined (like Johnzon-specific fail to parse empty stream). This is allowed as it does not replace covered behavior, it only replaces non-covered behavior: In clear words, we only guarantee that interfaces and classes extend instead of shrink or change, but we do not guarantee that things not being part of JAX-RS won't be replaced by idential parts of JAX-RS.

BTW, no EJB 1.x application can run on Java EE 8, you MUST rework that by using JPA. So I cannot see why this change policy should be more strict with JAX-RS than with EJB?

This is my last catch on this issue, as all is said. As much as I love discussions with you, I spent the whole day without any change in the outcome. You are very welcome to propose a change for upcoming major releases of JAX-RS, but we will not agree with you that this is a "bug" in the specification that must lead to a "drop" of this feature.

[rmannibucau]

@mkarg EJB1 / JPA change is another change. It is literally a new technology with the same name. JAX-RS 1 -> 2 is the same technology and only client API justified the major upgrade so nothing comparable.

In this last message there is something wrong: MBR don't catch exceptions, JAXRS is not a stack but a chain which is way different and why mappers are defined handling any exception on all JAXRS components - otherwise you wouldn't be able to impl it.

Changing the workflow is fine while it is enriched and is not when it implies a user code change. Enforcing to change a mapper for an interceptor is not acceptable when it was working and defined before. Keep in mind javadoc is part of the spec and javadoc never mentions unchecked exceptions so it is always allowed until explicitly forbidden and you can't really assume anything else since it is how it is read.

This case is not super vital (it mainly breaks Swagger/OpenAPI specs in user lands when defined with custom errors and clients respecting that spec) but it is key that users are taken into account more than that. Microprofile decided to enable to break users once a year and it already hurts rare adopters so I hope that now jakarta is at eclipse they dont follow the same path and make the rest of javaee pointless. My goal reporting that issue is as much ensuring this is kept in mind or reported to jakarta board for future choices to ensure users can still rely on specs and not only on implementations (like for this feature where johnzon users will have to enable a toggle to not break).