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 ?
>>>
>>>>> 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 ?
>>>
>>> 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 ?
>
>> 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.
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
>
>>
>> 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
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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>