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.
>>
>>>> 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())
>>
>> 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?
> 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).
Besides, the primary use case for the method is to produce an abort response in a client request filter.
>
> 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.
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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>>