Clarification about Response.readEntity(XXX)

[NicoNes]

This PR is my proposal to fix issue 736.
It was about:

• removing the cache concept introduces in Response.readEntity(...) and Response.getEntity() methods.
  As spotted by @ronsigal , that cache concept appears to be inconsistent with a TCK test from "com.sun.ts.tests.jaxrs.ee.rs.core.response.JAXRSClient" called "getEntityThrowsIllegalStateExceptionWhenConsumedTest" . Actually according to this TCK test, if readEntity(...) was previously invoked then the next invocation of getEntity() must throw an IllegalStateException.
• clarifying the javadoc wording with "consumed", "unconsumed", "un-consumed", "fully consumed" to better understand methods behavior.

Let me know what you think.

Thanks

[spericas]

@NicoNes Issue #736 is long and difficult. Would you care to summarize here the final set of rules via examples that combine the various entity methods? I know this is what the PR is, but a summary here would be useful to make sure we are all (finally) on the same page.

[NicoNes]

@spericas sure you're right a summary will help.

Here it is, it is a bit long but I tryied to explain all cases I had in mind:

• getEntity()

  • Response.ok(new MyObject()).build() then response.getEntity() -> the given MyObject instance
  • Response.accepted(new MyObject()).build() then response.getEntity() -> the given MyObject instance
  • Response.ok().entity(new MyObject()).build() then response.getEntity() -> the given MyObject instance
  • response.getEntity() -> the response body input stream
  • response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.getEntity() -> the same responseBodyInputStream partially consumed and still usable.
  • response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.getEntity() -> IllegalStateException because responseBodyInputStream is fully consumed already and not usable anymore.
  • response.readEntity(Foo.class) -> Foo, then response.getEntity() -> IllegalStateException because previous invocation of readEntity(Foo.class) closed the responseBodyInputStream that is not usable anymore.
  • response.bufferEntity() -> true then response.getEntity() -> a copy of the reset buffered entity input stream
  • response.close() then response.getEntity() -> IllegalStateException

• bufferEntity()

  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream not consumed at all, reponse.bufferEntity() -> true
  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}), reponse.bufferEntity() -> false
  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()), response.bufferEntity() -> false

• readEntity()

  • response.readEntity(Foo.class) -> Foo
  • response.readEntity(Foo.class) -> Foo then response.readEntity(Foo.class) -> IllegalStateException
  • response.bufferEntity() -> true then response.readEntity(Foo.class) -> Foo and response.readEntity(Foo.class) -> Foo.
  • response.bufferEntity() -> true then response.readEntity(InputStream.class) -> the reset buffered InputStream and response.readEntity(InputStream.class) -> the reset buffered InputStream.
  • response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.readEntity(InputStream.class) -> the same responseBodyInputStream partially consumed and still usable.
  • response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.readEntity(InputStream.class) -> IllegalStateException because responseBodyInputStream is fully consumed already and response has not been buffered first.
  • response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream not consumed at all soresponse.readEntity(Foo.class) -> Foo.
  • response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.readEntity(Foo.class) -> ProcessingException or partial result (e.g response.readEntity(String.class)).
  • response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.readEntity(Foo.class) -> IllegalStateException because responseBodyInputStream is fully consumed already and response has not been buffered first.

Please note that contrary to what was written in the original doc response.read(Foo.class) should close the reponseBodyInputStream not the response itself so that :
response.read(Foo.class) -> Foo then response.read(Foo.class) -> IllegalStateException but response.hasEntity() -> true instead of IllegalStateException.

Hope it's clearer.

Nicolas

[spericas]

I understand that this PR is in essence a "clarification". However, given its breadth, should we push it into 3.1 at this stage? I suspect that if we write some TCKs for this, some implementations would struggle.

I'm generally in favor of merging this, but wanted to bring this to everyone's attention.

[spericas]

@spericas sure you're right a summary will help.

I should be more careful what I ask for ;)

Here it is, it is a bit long but I tryied to explain all cases I had in mind:

• getEntity()

  • Response.ok(new MyObject()).build() then response.getEntity() -> the given MyObject instance
  • Response.accepted(new MyObject()).build() then response.getEntity() -> the given MyObject instance
  • Response.ok().entity(new MyObject()).build() then response.getEntity() -> the given MyObject instance
  • response.getEntity() -> the response body input stream

OK.

• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.getEntity() -> the same responseBodyInputStream partially consumed and still usable.
• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.getEntity() -> IllegalStateException because responseBodyInputStream is fully consumed already and not usable anymore.

Looks like these two could be combined, but it's fine. The use of responseBodyInputStream as a token is a bit confusing in the text above.

• response.readEntity(Foo.class) -> Foo, then response.getEntity() -> IllegalStateException because previous invocation of readEntity(Foo.class) closed the responseBodyInputStream that is not usable anymore.
• response.bufferEntity() -> true then response.getEntity() -> a copy of the reset buffered entity input stream
• response.close() then response.getEntity() -> IllegalStateException

OK.

• bufferEntity()

  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream not consumed at all, reponse.bufferEntity() -> true
  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}), reponse.bufferEntity() -> false
  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()), response.bufferEntity() -> false

So, the last two are essentially one, where the stream is fully or partially consumed.

• readEntity()

  • response.readEntity(Foo.class) -> Foo
  • response.readEntity(Foo.class) -> Foo then response.readEntity(Foo.class) -> IllegalStateException
  • response.bufferEntity() -> true then response.readEntity(Foo.class) -> Foo and response.readEntity(Foo.class) -> Foo.
  • response.bufferEntity() -> true then response.readEntity(InputStream.class) -> the reset buffered InputStream and response.readEntity(InputStream.class) -> the reset buffered InputStream.

OK.

• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.readEntity(InputStream.class) -> the same responseBodyInputStream partially consumed and still usable.
• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.readEntity(InputStream.class) -> IllegalStateException because responseBodyInputStream is fully consumed already and response has not been buffered first.

These last two appeared repeated from before?

• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream not consumed at all soresponse.readEntity(Foo.class) -> Foo.
• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.readEntity(Foo.class) -> ProcessingException or partial result (e.g response.readEntity(String.class)).
• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.readEntity(Foo.class) -> IllegalStateException because responseBodyInputStream is fully consumed already and response has not been buffered first.

Are these last ones related to buffering at all?

Please note that contrary to what was written in the original doc response.read(Foo.class) should close the reponseBodyInputStream not the response itself so that :
response.read(Foo.class) -> Foo then response.read(Foo.class) -> IllegalStateException but response.hasEntity() -> true instead of IllegalStateException.

Got it.

[NicoNes]

@spericas

I suspect that if we write some TCKs for this, some implementations would struggle.

I'm generally in favor of merging this, but wanted to bring this to everyone's attention.

Well I agree with you. At the time I tried with Jersey and Resteasy and they both require some fixes to be fully compliant with the clarification introduced by this PR.

• bufferEntity()
  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream not consumed at all, reponse.bufferEntity() -> true
  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}), reponse.bufferEntity() -> false
  • response.getEntity() -> responseBodyInputStream then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()), response.bufferEntity() -> false

So, the last two are essentially one, where the stream is fully or partially consumed.

Yes they could be merged in one.

• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.readEntity(InputStream.class) -> the same responseBodyInputStream partially consumed and still usable.
• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.readEntity(InputStream.class) -> IllegalStateException because responseBodyInputStream is fully consumed already and response has not been buffered first.

These last two appeared repeated from before?

They are almost the same as the ones from the "- getEntity" section.
The only difference is that they focus on the behavior of the readEntity(...) method contrary to the ones from the "- getEntity" section that are focused more on the behavior of the getEntity() method .
Anyway, both getEntity() and readEntity(InputStream.class) methods behave the same when the response inputstream has been buffered or not and is partially or fully consumed.

• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream not consumed at all soresponse.readEntity(Foo.class) -> Foo.
• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream partially consumed (e.g. responseBodyInputStream.read()) soresponse.readEntity(Foo.class) -> ProcessingException or partial result (e.g response.readEntity(String.class)).
• response.getEntity() or response.readEntity(InputStream.class) -> responseBodyInputStream, then responseBodyInputStream fully consumed (e.g. while (responseBodyInputStream.read()!=-1){}) so response.readEntity(Foo.class) -> IllegalStateException because responseBodyInputStream is fully consumed already and response has not been buffered first.

Are these last ones related to buffering at all?

What I tried to highlight is that if you try to deserialize the underlying input stream into an entity (other than InputStream) using readEntity(...), it will work only if the remaining bytes form the underlying input stream can be mapped into that entity.

[spericas]

@NicoNes I have set the target to 4.0 (tentatively) until I hear from others. As I said above, this is sort of a broad clarification that is probably better for that release.

[mkarg]

Now that working on 4.0 is in progress, we should continue this discussion thread.

[NicoNes]

Hi guys,

Any update on this one ?

Thanks !

[spericas]

@NicoNes Would you be interested in creating a PR targeted to the release-4.0 branch for this?

[NicoNes]

@spericas Sure, I'll do it.