Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throwing NoContentException when InputStream is empty #4275

Merged
merged 5 commits into from
Oct 2, 2019

Conversation

mkarg
Copy link
Member

@mkarg mkarg commented Sep 26, 2019

Since JAX-RS 2.1 an MBR must react upon an empty stream either with an empty instance or with NoContentException, but Jersey's JSON-B MBR does neither - it simply throws ProcessingException:

"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 javax.ws.rs.core.NoContentException in case no zero-length entity representation is defined for the supported Java type."

@mkarg
Copy link
Member Author

mkarg commented Sep 26, 2019

This PR proposes a solution which reconstitutes compliance with JAX-RS 2.1 with a two-step approach to detect an empty input stream:

  • First it tries to use mark/reset as this is very fast, but not guaranteed to be supported by all kinds of input streams.
  • If this is not possible, then it will actively buffer the first byte, which is rather slow, but will work with all kinds of input stream.

As long as we allow this MBR to be used with other JAX-RS implementations (not just Jersey), we need this PR, and we need this two-step approach. An alternative solution could be to clearly restrict this MBR to work on Jersey only (e. g. by documentation or even technical checks), in which case we could leave out one of the two steps as we then know for sure the type of input stream Jersey will run this MBR with.

@mkarg
Copy link
Member Author

mkarg commented Sep 26, 2019

Please see also jakartaee/rest#795.

@mkarg
Copy link
Member Author

mkarg commented Sep 26, 2019

@spericas @chkal I assume this is the code location where you want the topic discussed in jakartaee/rest#795 to be handled (rather than the JAX-RS spec). :-)

@jansupol
Copy link
Contributor

Is it possible to add a test checking the behaviour for an empty stream?

@mkarg
Copy link
Member Author

mkarg commented Sep 28, 2019

Is it possible to add a test checking the behaviour for an empty stream?

Added unit test covering this issue with 4c6b2ac.

@mkarg mkarg force-pushed the JERSEY-NOCONTENTEXCEPTION branch 2 times, most recently from b909908 to 69c1533 Compare September 28, 2019 17:40
mkarg and others added 4 commits September 30, 2019 20:02
Since JAX-RS 2.1 an MBR must react upon an empty stream either with an
empty instance or with NoContentException, but Jersey's JSON-B MBR does
neither - it simply throws ProcessingException:

"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 javax.ws.rs.core.NoContentException
in case no zero-length entity representation is defined for the supported Java type."

Signed-off-by: Markus KARG <[email protected]>
@senivam
Copy link
Contributor

senivam commented Oct 1, 2019

just another thought... previous comment was wrong :)
modify main method this way:

@Override
    public Object readFrom(Class<Object> type, Type genericType,
                           Annotation[] annotations,
                           MediaType mediaType,
                           MultivaluedMap<String, String> httpHeaders,
                           InputStream entityStream) throws IOException, WebApplicationException {

        final EntityInputStream streamWithMark =  new EntityInputStream(entityStream);
        entityStream = streamWithMark;
        if (streamWithMark.isEmpty()) {
            throw new NoContentException(LocalizationMessages.ERROR_JSONB_EMPTYSTREAM());
        }

        Jsonb jsonb = getJsonb(type);

        try {
            return jsonb.fromJson(entityStream, genericType);
        } catch (JsonbException e) {
            throw new ProcessingException(LocalizationMessages.ERROR_JSONB_DESERIALIZATION(), e);
        }
    }

@mkarg
Copy link
Member Author

mkarg commented Oct 1, 2019

just another thought... previous comment was wrong :)

Cool optimization! 😎 If you are sure it is as safe and as fast I would be happy to accept your PR ontop! 👍

@senivam
Copy link
Contributor

senivam commented Oct 1, 2019

@mkarg, OK, I've submitted a PR to your branch :)

@jansupol jansupol merged commit 1c3b8d5 into eclipse-ee4j:master Oct 2, 2019
@jansupol jansupol added this to the 2.29.2 milestone Oct 2, 2019
@mkarg mkarg deleted the JERSEY-NOCONTENTEXCEPTION branch October 2, 2019 16:38
@jansupol jansupol modified the milestones: 2.29.2, 2.30 Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants