-
Notifications
You must be signed in to change notification settings - Fork 122
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
Clarification about Response.readEntity(XXX) #736
Comments
IMO your understanding is correct and both Jersey and RESTEasy doesn't behave correctly. But it would be interesting to hear other thoughts about this as well. |
Javadoc to
That's what Jersey does. It does not return the cached entity, as the entity is never cached. I am concerned about automatic caching being a performance issue unless the cached entity means a simple reference to a Java entity returned by The difference between caching and buffering is not clear, which is why the implementations do not honor the javadoc to the letter, IMO. |
Hi @jansupol ,
Actually javadoc for
IMO it means that if the entity has been read as a Then the doc says:
IMO it means that if the entity has not been read at all using any of Then the doc says:
It means for me that, if the entity has not been read as any XXX java type but only used as an input stream (using So in my understanding caching simply means storing and re-using the reference to a Java entity returned by
Caching is the default behavior and it means: storing the Java entity returned by Buffering is optional as you said and is a way to allow multiple That's my opinion and understanding WDYT ? |
Its not the point of this issue, but following is not totally right
In following case I was expecting an IllegalState to be thrown since the entity was previously fully consumed as an input stream but instead
Am I wrong ? (Using jersey 2.24) |
@NicoNes Caching term was not clear even back in the JAX-RS 2.0 days. If caching the entity means keeping a reference to the entity, that would be easy to implement. I'd like to see some javadoc warning that if the user would retrieve the entity once, modify it, then the entity would be retrieved once again then the user should not be surprised that it's altered, though. The behavior of the example is as you describe. Strictly speaking, there is no notion of consumption in the InputStream javadoc (Is it consumed when it is not read but closed? Is it consumed when it is read but not closed?). BTW, your analysis contradicts itself, too:
Here the consumed means used by
Here consumed means to check the InputStream can still be read. The javadoc wording is not so clear. |
I agree that javadoc wording (un-consumed, fully consumed, consumed) are a bit confusing. So let's try to make that clear to get a precise idea of what those methods are supposed to do. getEntity(): I do not want to talk about I suggest to change the javadoc as follows:
This way this method will always returns something usable/practical (an input stream fully read or closed is no longer usefull):
Before talking about readEntity() maybe I can get your thoughts on that guys. Is this acceptable ? |
I agree with @NicoNes interpretation of the Javadoc. I think it's important to convert this into actual use cases: (1) getEntity() -> InputStream (2) getEntity() -> InputStream, InputStream consumed, getEntity() -> IllegalStateException (3) readEntity(Foo.class) -> Foo, getEntity() -> Foo (4) readEntity(Foo.class) -> Foo, readEntity(Bar.class) -> IllegalStateException (5) bufferEntity(), readEntity(Foo.class) -> Foo, getEntity() -> Foo, readEntity(Bar.class) -> Bar, getEntity() -> Bar The one caveat to the discussion above (see 2) is that, when an implementation is trying to determine if an input stream is consumed, it should probably use |
About case 4, I think it's important to be clear that any subsequent invocation of (4) readEntity(Foo.class) -> Foo, readEntity(XXX.class) -> IllegalStateException
Yes probably or any other internal mechanism sure 👍 |
@NicoNes Are you interested in submitting a PR that clarifies the |
@spericas Yes sure I will do it 👍 |
Hey everyone, I've started thinking about this stuff again, and I have a couple of comments.
I think that should be changed in favor of the behavior we all (I think) agree on.
Why? 1) If you're working explicitly with an InputStream, then you have to be looking for end of file anyway. 2) If you mix together reading the backing InputStream and calling readEntity(), then 2a) you get an Exception because the remaining bytes can't be turned into an instance of the Class you want, or 2b) you'll get a partial object like a shortened String, or 2c) you'll get an "empty" version of your desired class, if one exists, or an Exception otherwise. Currently, the semantics would require an IllegalStateException in case 2c, but I think 2c is just a particular case of 2b, which doesn't require an IllegalStateException. I think we should say that mixing the two approaches to getting the contents is a bad idea, and you better know what you're doing if you want to do it. Beyond that, I think the semantics and the code would be simpler if we don't have to throw the IllegalStateException. WDYT? |
Hey @ronsigal, I agree with you on point 1. About point 2, I agree that things would be clearer without this specific But before talking about it in details maybe we should clarify the current expected behaviour for this current edition. It will help us later to remove the Based on what we started a while ago, I've just finish writting a version of what the current doc could be to better clarify I sent it to you first for review. Thanks |
Hey guys, Forget about my last comment. I fully agree with @ronsigal. I was reading again the version I sent to him and I figured out that few cases, the ones dealing with I think that Ron is right. The reason why it is so difficult for us to explain things clearly is because in these cases we try to handle
So based on Ron analisys, I thought about a new version that behave like that: Below resetable means that user can invoke (1) getEntity() -> InputStream (not resetable) (2) bufferEntity(), getEntity() -> InputStream (resetable) (3) readEntity(Foo.class) -> Foo, getEntity() -> Foo (4) readEntity(Foo.class) -> Foo, readEntity(Bar.class) -> IllegalStateException (5) readEntity(Foo.class) -> Foo, readEntity(InputStream.class) -> InputStream (not resetable) (6) bufferEntity(), readEntity(Foo.class) -> Foo, getEntity() -> Foo, readEntity(Bar.class) -> Bar, getEntity() -> Bar, readEntity(InputStream.class) -> InputStream (resetable), getEntity() -> InputStream (resetable) WDYT ? |
@NicoNes Sorry, it's been a while. Could you clarify in (5) the state of |
Hey @spericas , Good catch. In (5), the (5) readEntity(Foo.class) -> Foo, readEntity(InputStream.class) -> IllegalStateException The general rule must be that unless (7) readEntity(InputStream.class) -> InputStream, readEntity(InputStream.class) -> IllegalStateException To hilight what Ron was saying about preventing user from using both (8) getEntity() -> InputStream, inputstream.read(), readEntity(...) -> IllegalStateException WDYT ? |
Right.
Makes sense.
I am not sure we can always detect this condition. It's sometimes difficult to prevent users from shooting themselves on the foot. |
Sure. I think we agree that Now if partially consumed it is less obvious. So to keep it simple maybee we should just keep what is already stated by the doc about throwing a Does it sound acceptable to you ? |
Hi @NicoNes, On Sunday I saw your comment about working on clarifying things first, and then changing the behavior in a later edition, but I wasn't in working mode, so I didn't respond at the time. Now, I've just come by to say that I agree with YOU. Lolll.
-Ron |
Hey @ronsigal , I share your analysis and it makes me wonder too if caching support is useful.
As you said, since all implementations that passes TCK does not support caching, I think that eliminate this concept is the best things to do.
Well, I'm not sure. I personnally use one or the other but not both (it does not work anyway). So based on what you said, for stage 1
I don't think that adding the cache concept back will be a good thing. So
This way if user uses both
If we all agree to remove caching concept and about the behaviour of Does it sound good to you ? |
@NicoNes, yes, I mostly agree. Stage 1. +1. I like that you added "(i.e inputStream.read() != -1)". I was always confused by "consumed" and "fully consumed" here and there. I think it would be reasonable to add that explanation every place "consumed" appears, at least for Stage 1. In Stage 2, the concept of "consumed" goes away, I think. This is a minor point, but I read
as "Get the response entity Java instance. ... Otherwise, it behaves as follows." I would do something like
Stage 2. +1 Point 3. I don't understand this part. My feeling is everything would be much clearer if getEntity() is considered to be the getter that matches ok(), etc., and that it should live in a different world than a received entity stream. This would be a change in behavior, since getEntity() can currently return a received entity stream. So, I would have, for example, ok() in Response and bufferEntity() and readEntity() in a subclass. One concern. Removing the bit about caching for getEntity() makes the updated version look like a behavior change. Well, technically, I guess it is a behavior change if the semantics is defined by the javadoc and not by the TCK. But, in reality, it shouldn't be a behavior change for any JAX-RS implementation. I think there should be someplace where that is made clear. |
About point 3 I think that I get you mean. Tell me if I'm wrong. It would be like what is already done with
Am I right? My feeling is that I like the current the semantic (without the cache concept) of
What I meant in point 3 about how If |
Hey @NicoNes, re; "Am I right" Yes re: "the current semantic (without the cache concept) of getEntity() and readEntity()" I guess I don't have strong feelings about the issue. I would say In favor of my version of the semantics:
In favor of your version:
I think we need more input. Maybe a note on the eclipse-ee4j/jaxrs-api mailing list? Btw, I'm glad you mentioned hasEntity(). That javadoc should probably be clarified also. |
As for bufferEntity() and getEntity(), if we keep the ability of getEntity() to retrieve a backing InputStream, then I think your suggestion makes sense. Otherwise, it's moot. |
Hey @ronsigal
Yes that's it. At least for stage 1, since we think it's better to not introduce change behavior, my suggestion is just to clarify relationship between So let me try to sum up everything for every one on how we would like to fix the initial issue without introduce no behavior change for this version. The goal is to remove the cache concept from both
@ronsigal Once this issue will be closed, I suggest we open and another issue to propose the stage 2 so that we treat them separately. WDYT ? If it's Ok for you, yes we could submit it to the mailing list (or mention them here) to get more input about it. Thanks !! |
Hi @NicoNes,
+1
|
Hey @ronsigal ,
Ok, I will add a line about it 👍
Sure. What is the email address ? |
I was just reminded that my original motivation for raising #706 "Clarify javax.ws.rs.core.Response javadoc wrt extracting entities." was to ask for a Response.isClosed() method to make it possible to avoid getting an exception when calling, for example, hasEntity(). This would be an additional, rather than changed, behavior. I wonder if it would be legal to include it in Stage 1. |
Hey @ronsigal , Thanks for the address. I'll suscribe to the mailing list and send the mail to the group.
You're right we almost forget your initial need. Maybe for stage 1 the quick workaround to add this
It's a bit weird I agree, but it works..anyway it's just a proposal. |
Hey @NicoNes, your isClosed() makes sense, and it makes life a little easier for using all of those methods that throw an exception when the response is closed. +1 |
@NicoNes This is a kind reminder that this issue is assigned to you and is planned for the next release of JAX-RS. :-) |
Are we still targeting this clarification for 3.1? |
According to the javadoc of:
those methods are supposed to close the original entity input stream (unless the supplied type is input stream) and then cache the result for subsequent retrievals via
Response.getEntity()
.So it clearly means that those methods MUST not close() the response itself, else subsequent retrievals via
Response.getEntity()
will always end up withIllegalStateException
being thrown.Instead, only the underlying input stream should be closed.
Subsequent call to one of those methods MUST throw an IllegalStateException if the original entity input stream has already been fully consumed without buffering the entity data prior consuming.
In other words:
Is my understanding right for those 2 points ? because it seems that many implementation (https://issues.jboss.org/browse/RESTEASY-2112) including the RI (Jersey) does not behave as exepected for point 1.
If my understanding is wrong what is the intented behavior of those methods ?
The text was updated successfully, but these errors were encountered: