Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jaxrs-dev] [736] Clarification about Response.readEntity(XXX)

Hi Nicolas,

Thank you so much for writing this note! I think it really helps to clarify some things that have confused me for a long time.

I have one concern. I believe that RESTEasy satisfies everything in your updated javadoc. Given the ambiguity in the current javadoc, though, I wonder if other implementations might behave differently and still pass the TCK.

For example, in the getEntity() javadoc: 

• Else if bufferEntity() was previously invoked successfully, a copy of the reset 
buffered entity input stream is returned.

It turns out that RESTEasy does that, but maybe it's not preordained.

Similarly, in the bufferEntity() javadoc:

"In case the reponse entity is backed by an input stream not consumed at all, all the bytes of that input stream are read (i.e until inputStream.read() == -1) and stored in a local buffer. That input stream is then automatically closed as part of the operation and the method returns true."

Again, RESTEasy behaves that way, but it's possible that the phrase  "unconsumed entity input stream" (now "input stream not consumed at all") in the current javadoc might have been interpreted differently by another implementation.

I think the goal should be to clarify the language without invalidating any existing implementations, so I hope people from other projects will give some feedback.

Again, thanks for moving this along and helping me to be less confused!

-Ron

Hi guys,

Hope everyone is fine during this world wide quarantine :).

This mail is to talk about issue #736 I opened a while ago now.

At the time, we all agreed that following items needs to be clarified:
  •  the cache concept introduced by all readEntity(...) methods javadoc.
  • the javadoc wording with "consumed", "unconsumed", "un-consumed", "fully consumed" to better understand methods behavior.
So I worked on it with Ron Sigal from the RESTEasy project team and we try to find the best way to clarify things. Here is our proposal:

1 - The cache concept between readEntity() and getEntity()

All readEntity(...) methods javadoc introduce a cache concept so that once read, entity is cached for subsequent retrieval via getEntity() method.

Well that's for the theory but in practice Ron spotted out that a TCK test from "com.sun.ts.tests.jaxrs.ee.rs.core.response.JAXRSClient" called "getEntityThrowsIllegalStateExceptionWhenConsumedTest" was in conflict with the javadoc on this cache concept point.
Actually according to this TCK test, if readEntity(...) was previously invoked then the next invocation of getEntity() must throw an IllegalStateException.

Then, given that for any JAX-RS implementations that passes the TCK, the cache concept described in the javadoc does not work anyway, we think that the best option is to remove this cache concept from the javadoc. This would not imply no change from JAX-RS vendors and things will be clearer.
  • The getEntity() javadoc could be reviewed to look like:
Get the response entity Java instance.

Returns null if the response does not contain an entity body. Otherwise, it behaves as follow:

• If a Java object was supplied by a call to Response.accepted(), Response.ok() or 
ResponseBuilder.entity(), then that object is returned.
• Else if bufferEntity() was previously invoked successfully, a copy of the reset 
buffered entity input stream is returned.
• Else if the entity is represented by the original input stream not fully consumed already 
(i.e inputStream.read() != -1), then that input stream is returned.

Returns:
    The response entity or null if response does not contain an entity body 
    (i.e. when hasEntity() returns false).
Throws:
    IllegalStateException - if the entity is represented by the original 
    input stream fully consumed already (i.e inputStream.read() == -1), or if the response 
    has been closed.
  • The readEntity(...) methods javadoc could be reviewed to remove following line:  
A message instance returned from this method will be cached for subsequent retrievals via getEntity().
2 - The javadoc wording
  • The getEntity() javadoc could be reviewed as described above
  • Following changes could be made to the readEntity(...) methods javadoc:
    • "or if the original entity input stream has already been consumed" => "or if the original entity input stream has already been fully consumed (i.e inputStream.read() == -1)"
    • "this method automatically closes the an unconsumed original response entity data stream if open." => "this method automatically fully consumes (i.e inputStream.read() == -1) and closes the original response entity input stream if open."
    • "or if the entity input stream has been fully consumed" => "or if the original entity input stream has been fully consumed (i.e inputStream.read() == -1)"
  • Following changes could be made to the bufferEntity(...) method javadoc:
    • "Buffer the message entity data." => "Buffer the original response entity input stream."
    • "In case the message entity is backed by an unconsumed entity input stream, all the bytes of the original entity input stream are read and stored in a local buffer. The original entity input stream is consumed and automatically closed as part of the operation and the method returns true." => "In case the reponse entity is backed by an input stream not consumed at all, all the bytes of that input stream are read (i.e until inputStream.read() == -1) and stored in a local buffer. That input stream is then automatically closed as part of the operation and the method returns true."
    •  "In case the response entity instance is not backed by an unconsumed input stream an invocation of bufferEntity method is ignored and the method returns false." => "Otherwise an invocation of bufferEntity method is ignored and the method returns false."
  • Following change could be made to the close(...) method javadoc:
    • "The close() method should be invoked on all instances that contain an un-consumed entity input stream" => "The close() method should be invoked on all instances that contain an original entity input stream not fully consumed (i.e until inputStream.read() != -1) "
  • Following change could be made to the ResponseBuilder.fromResponse(...) method javadoc:
    • "Note that if the entity is backed by an un-consumed input stream" => "Note that if the entity is backed by the original input stream not consumed at all"
3 - Adding isClosed() method

Last point is about adding an isClosed() method as suggested by Ron here #706. This would be an additional, rather than changed, behavior but we wonderf if it would be legal to include it in the PR ?
In order JAX-RS vendors have no extra work to do one option would be to provide a default implementation as follow:
	public boolean isClosed() {
		try {
			hasEntity();
			return false;
		} catch (IllegalStateException e) {
			return true;
		}
	}

Well, I tried to sum up the discussion we had on github with Ron and the other guys, so I hope this mail is not too long.

What do you think of the proposal ?

Thanks !

--
Nicolas NESMON


_______________________________________________
jaxrs-dev mailing list
jaxrs-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jaxrs-dev

Back to the top