On Aug 15, 2012, at 7:29 PM, Sergey Beryozkin <sberyozkin_at_talend.com> wrote:
> On 15/08/12 17:53, Marek Potociar wrote:
>>
>> On Aug 15, 2012, at 6:29 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>>
>>> On 15/08/12 16:57, Sergey Beryozkin wrote:
>>>> On 15/08/12 16:51, Marek Potociar wrote:
>>>>>
>>>>> On Aug 15, 2012, at 2:19 PM, Sergey Beryozkin<sberyozkin_at_talend.com>
>>>>> wrote:
>>>>>
>>>>>> On 07/08/12 15:22, Sergey Beryozkin wrote:
>>>>>>> Hi Santiago
>>>>>>> On 07/08/12 17:02, Santiago Pericas-Geertsen wrote:
>>>>>>>>
>>>>>>>> On Aug 7, 2012, at 3:49 AM, Sergey Beryozkin wrote:
>>>>>>>>
>>>>>>>>> On 07/08/12 16:15, Bill Burke wrote:
>>>>>>>>>> I don't understand what the problem is. Javadoc is just saying
>>>>>>>>>> that if
>>>>>>>>>> the underlying entity is an InputStream, then the Response copy
>>>>>>>>>> has a
>>>>>>>>>> reference to that InputStream. You probalby should buffer it because
>>>>>>>>>> you
>>>>>>>>>> will get an error if two different Response objects referencing the
>>>>>>>>>> same
>>>>>>>>>> InputStream call readEntity().
>>>>>>>>>
>>>>>>>>> Actually, there are few problems here.
>>>>>>>>> 1. Response entity of the type like String or some other type
>>>>>>>>> different from InputStream is lost during Response.fromResponse;
>>>>>>>>
>>>>>>>> So you think there's a problem in the code, not the Javadoc, right?
>>>>>>>> Here are the first few lines of that method:
>>>>>>>>
>>>>>>>> public static ResponseBuilder fromResponse(Response response) {
>>>>>>>> ResponseBuilder b = status(response.getStatus());
>>>>>>>> if (response.hasEntity()) {
>>>>>>>> b.entity(response.getEntity());
>>>>>>>> }
>>>>>>>> ...
>>>>>>>>
>>>>>>>> It seems OK to me, but there could a problem elsewhere.
>>>>>>>>
>>>>>>>
>>>>>>> It was my fault, I did not do the home work and not implemented
>>>>>>> hasEntity(). So my apologies.
>>>>>>> That said, I'm not sure I understand the hasEntity check, why the
>>>>>>> following does not work:
>>>>>>>
>>>>>>> ResponseBuilder b = status(response.getStatus());
>>>>>>> // idempotent
>>>>>>> b.bufferEntity();
>>>>>>> b.entity(response.getEntity());
>>>>>>>
>>>>>>> ?
>>>>>>>>> 2. The whole idea of the user having to do 'buffer' for
>>>>>>>>> Response.fromResponse to work properly is an anti-pattern. It's not
>>>>>>>>> the responsibility of the calling code.
>>>>>>>>
>>>>>>>> It think this is a consequence of the shallow copying semantics above.
>>>>>>>> Often a trade-off between performance and usability.
>>>>>>>>
>>>>>>>
>>>>>>> I still not like the idea of the user having to make sure the method
>>>>>>> provided by the api works correctly.
>>>>>>>
>>>>>>>
>>>>>>> By the way, what about throwing an exception if bufferEntityt/close is
>>>>>>> called on the server side, or is it impossible to implement properly ?
>>>>>>>
>>>>>>
>>>>>> Can we get some absolute clarity on the use of bufferEntity() and
>>>>>> close() on the server side ? Are those two operations supposed to
>>>>>> work on both ends ?
>>>>>
>>>>> So let me ask you what is exactly unclear about the javadoc of those
>>>>> methods? Isn't the expected behavior obvious?
>>>>>
>>>>
>>>> See a follow-up message
>>
>> Which one? I'm honestly getting lost in your replies to your own messages.
>>
>
> Really, check the depth of few other recent email exchanges.
>
> Well let me repeat here:
>
> Why close() can not lead to an entity being buffered ? I think I got it but would not mind a clarification
>
> Why calling close() is supposed to do when we already have a buffered entity ? Or rather, why calling close on the buffered entity should lead to readEntity methods throwing an exception ?
The answer to all three questions above is: Because close() means close. All resources, including entity input stream are closed/released and buffered data are made available for garbage collection (kindly check the javadoc). Thus readEntity() may not work. Sorry if I sound a bit harsh, but this discussion is becoming bizarre to me as I fail to understand what is your problem here. Why would anyone expect that data can be read from an object once the object has been closed? Are you so confused about all Java APIs that implement Closeable or provide a close() method?
>>>>
>>>>>> Also, regarding Response.fromResponse(): can the requirement to
>>>>>> explicitly call bufferEntity() for this to work reliably can be
>>>>>> removed ?
>>>>>
>>>>> I'd prefer to keep doing shallow copy and let developers to decide
>>>>> whether or not the stream is buffered. We don't want to implicitly
>>>>> buffer potentially large streams.
>>>>>
>>>>
>>>> How on earth am I supposed to know what type of entity is there,
>>>> especially when implementing a response filter on the server end ?
>>
>> A few options:
>>
>> 1. containterResponseContext.getEntity() instanceof InputStream
>> 2. containterResponseContext.hasEntity()&& InputStream.class.isAssignableFrom(containterResponseContext.getEntityClass())
>>
>
> Well, thanks for letting me know how to find out if the entity is InputStream or not. So you think it's OK for developers write this in their code ?
Yes. I don't see anything wrong with #1 especially. Or do you think that we should optimize the API for every esoteric use case someone comes up with?
>
>>>>
>>>> The idea of calling bufferEntity() on the server end does not make sense
>>>> any way, and having to know when actually to call is does not make sense
>>>> at all, sorry
>>>
>>> Let me clarify with a use case. The requirement is to add a new header to an existing Response. One option is to use Response.fromResponse(); The developer does not need to know what type of entity is there.
>>
>> In a filter? If not, why not to use ResponseBuilder? If yes, have you seen the mutable ContainerResponseContext API?
>
> What if user has a function with only Response on the input ?
Then he may probably want to change that signature. Or live with the extra code. Again, how more marginal can we get in this use case discussion?
>>
>>> Now, the docs saying that bufferEntity() is recommended for this method to work when the entity is InputStream does not harm. But I think it just confuses things because the developer may not know what type of entity is there.
>>
>> Response.getEntity() will provide the information.
>>
>>>
>>> IMHO, if we have InputStream pointing to a 100MB file, and we call Response.fromResponse() then lets just do a shallow copy but remove any reference to bufferEntity() from the method's Javadocs.
>>
>> Why? What's wrong with stating that the shallow copy is done and pointing users to bufferEntity() if something special is required. Note that the javadoc is written in a way to satisfy both sides, not just the server side. Therefore it may be a bit vague, but that's a price of the common Response design (which OTOH brings more important advantages, such as the common exception hierarchy).
>>
> OK
>
>> Besides, the primary use case for the method is to produce an abort response in a client request filter.
>
> Is it about bufferEntity() ? Guess what, it's the first time I'm hearing about this primary use case.
I was talking about Response.fromResponse().
>
> Anyway, having bufferEntity & close method available to server developers is confusing. You've spent some time showing me above how to check for InputStream - close() could've been easily implemented that way too, and entity could've been buffered and reset if ever needed.
> Now we have 'vague' docs, as per your statement above
>
No comment.
>>
>>>
>>> If the developer calls Response.fromResponse(oldResponse) and then attempts to read the data from oldResponse and then from a new response then so be it, it's not a JAX-RS runtime issue at all
>>
>> That's what the javadoc says already.
>
> Not quite
Feel free to suggest a better javadoc. But a one that will not be either client or server specific.
Marek
>
> Cheers, Sergey
>
>>
>> Marek
>>
>>>
>>> Sergey
>>>
>>>
>>>
>>>> Sergey
>>>>
>>>>> Marek
>>>>>
>>>>>>
>>>>>> Thanks, Sergey
>>>>>>
>>>>>>
>>>>>>> Cheers, Sergey
>>>>>>>
>>>>>>>> -- Santiago
>>>>>>>>
>>>>>>>>>> On 8/6/2012 1:10 PM, Sergey Beryozkin wrote:
>>>>>>>>>>> In JAX-RS 1.1 I was seeing an easy to understand
>>>>>>>>>>> Response.fromResponse
>>>>>>>>>>> output. For example,
>>>>>>>>>>>
>>>>>>>>>>> Response r = Response.entity("Hello").build();
>>>>>>>>>>> Response r2 = Response.fromResponse(r);
>>>>>>>>>>> assertSame("Hello", r2.getEntity());
>>>>>>>>>>>
>>>>>>>>>>> Note that 'r' contains a String, not 'InputStream'.
>>>>>>>>>>> 'Response.entity("Hello").build()' is executed on the server,
>>>>>>>>>>> where as
>>>>>>>>>>> 'Response.fromResponse(r)' is a follow up operation executed in a
>>>>>>>>>>> response filter.
>>>>>>>>>>>
>>>>>>>>>>> In 2.0 I'm seeing 'Response.fromResponse(r)' losing the original
>>>>>>>>>>> 'String', and after checking the java docs I'm seeing:
>>>>>>>>>>>
>>>>>>>>>>> "Note that if the entity is backed by an un-consumed input
>>>>>>>>>>> stream, the
>>>>>>>>>>> reference to the stream is copied. In such case make sure to buffer
>>>>>>>>>>> the
>>>>>>>>>>> entity stream of the original response instance before passing
>>>>>>>>>>> it to
>>>>>>>>>>> this method."
>>>>>>>>>>>
>>>>>>>>>>> I think it's an API bug. The assumption is made there that the
>>>>>>>>>>> Response
>>>>>>>>>>> entity is kept as InputStream - which is an implementation
>>>>>>>>>>> detail of
>>>>>>>>>>> Response. I've been saying that all this buffer() stuff is out of
>>>>>>>>>>> scope
>>>>>>>>>>> of Response but failed to get anyone convinced.
>>>>>>>>>>>
>>>>>>>>>>> For the very least, the javadocs quoted above should further
>>>>>>>>>>> clarify
>>>>>>>>>>> that it only applies to the client side where it is reasonable to
>>>>>>>>>>> expect
>>>>>>>>>>> Response contains InputStream originally.
>>>>>>>>>>>
>>>>>>>>>>> Any comments ?
>>>>>>>>>>> Sergey
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>