Clarify javax.ws.rs.core.Response javadoc wrt extracting entities.

[ronsigal]

A number of javax.ws.rs.core.Response methods, e.g., getEntity() and hasEntity(), include a line in the javadoc like

* @throws IllegalStateException in case the response has been {@link #close() closed}.

However, in the absence of a method like isClosed(), we end up writing code like

try {
   if (response.getEntity() != null) return response;
}
catch(IllegalStateException ise) {
   // IllegalStateException from ClientResponse.getEntity() means the response is closed and got no entity
}

instead of

if (!response.isClosed() && response.getEntity() != null) {
   return response;
}

The implementation of isClosed() should be simple, and it leads to nicer code.

An example of a potential use of Response.isClosed() arises in RESTEasy when:

1. A resource method uses a Client to contact another server and gets an exception back.

   2. A builtin ExceptionMapper tries to unwrap the Exception:

   protected Response unwrapException(HttpRequest request, Throwable e, RESTEasyTracingLogger logger)
   {
      Response jaxrsResponse = null;
      Throwable unwrappedException = e.getCause();

      /*
       *                If the response property of the exception does not
       *                contain an entity and an exception mapping provider
       *                (see section 4.4) is available for
       *                WebApplicationException an implementation MUST use the
       *                provider to create a new Response instance, otherwise
       *                the response property is used directly.
       */

      if (unwrappedException instanceof WebApplicationException) {
         WebApplicationException wae = (WebApplicationException) unwrappedException;
         Response response = wae.getResponse();
         if (response != null) {
            try { // Here's where we guard against IllegalStateExceptioin
               if (response.getEntity() != null) return response;
            }
            catch(IllegalStateException ise) {
               // IllegalStateException from ClientResponse.getEntity() means the response is closed and got no entity
            }
         }
      }

...

Before we wrapped the call to Response.getEntity() in try/catch, we were getting an IllegalStateException, which isn't very helpful.

[spericas]

As I stated before, this addition seems reasonable to me. You may want to consider creating a PR to let people comment on it.

[arjantijms]

+1 seems quite reasonable indeed.

[mkarg]

@ronsigal Will you provide a PR?

[mkarg]

@ronsigal Ron? Hello?

[ronsigal]

Hi @mkarg,

Sorry. Yes, I would be happy to create a PR.

-Ron

[mkarg]

@ronsigal Great, go ahead! We'd love to review it. :-)

[ronsigal]

Looking at the javadoc again, I see that Response.getEntity() throws IllegalStateException in two cases:

   * @throws IllegalStateException if the entity was previously fully consumed
   *                               as an {@link InputStream input stream}, or 
   *                               if the response has been {@link #close() closed}.

So using !response.isClosed() as a "guard" in

    if (!response.isClosed() && response.getEntity() != null) {
       return response;
    }

isn't quite enough.

As far as I can tell, the phrase "the entity was previously fully consumed as an {@link InputStream input stream}" seems to be equated with something like "a call to Response.readEntity()" has returned an entity". For example, the only related test in the TCK that I can find is

  /*
   * @testName: getEntityThrowsIllegalStateExceptionWhenConsumedTest
   * 
   * @assertion_ids: JAXRS:JAVADOC:123;
   * 
   * @test_Strategy: if the entity was previously fully consumed as an
   * InputStream input stream, or if the response has been #close() closed.
   */
  public void getEntityThrowsIllegalStateExceptionWhenConsumedTest()
      throws Fault {
    Response response = invokeGet("entity");
    response.readEntity(String.class);
    try {
      Object entity = response.getEntity();
      fault("No exception has been thrown entity=", entity);
    } catch (IllegalStateException e) {
      logMsg("#getEntity throws IllegalStateException as expected", e);
    }
  }

in com.sun.ts.tests.jaxrs.ee.rs.core.response.JAXRSClient. But what if an InputStream is returned and read outside of Response?

Also, the javadoc for Response.readEntity() says, "A message instance returned from this method will be cached for subsequent retrievals via {@link #getEntity()}." But if the call to readEntity() consumes the InputStream, then getEntity() should throw an IllegalStateException, which the TCK test shown above enforces. Note that in RESTEasy, if Response.bufferEntity() is called before readEntity(), then a subsequent call to getEntity() succeeds.

Maybe it's me, but this all seems a bit muddled. That is, I think that the underlying logic is probably sensible, but more precise language is needed.

[mkarg]

@ronsigal You might be right was all you say, but that aspect is a different issue, so can you please either rename the thread or create a second one? Thanks. :-)

[ronsigal]

Hi @mkarg. Ok, sure, I think I would create a separate issue in which I suggest some new wording in the Response javadoc. If I can figure out some sensible wording. I'll have to think a little more.

[ronsigal]

By the way, @mkarg, would an issue like the current one normally include an update to the TCK? In this case, testing the behavior of Response.isClosed().

[mkarg]

@ronsigal Again, we can deal with it in this issue once you simply modify the title to reflect the actual target of your thread.

[mkarg]

@ronsigal TCK must be updated once the behavior is actually changed.

[ronsigal]

Ok, @mkarg, that's good too. :)

[ronsigal]

Umm, @mkarg, re: "TCK must be updated once the behavior is actually changed."

Does that imply a separate issue?

[mkarg]

@ronsigal No. We can cover TCK and API changes and Spec changes in the same issue, as long as the title and discussion of the issue clearly reflect the common intention. :-)

[ronsigal]

Ok, @mkarg, thanks. Will do.