On Mar 25, 2013, at 8:32 PM, Sergey Beryozkin <sberyozkin_at_talend.com> wrote:
> On 25/03/13 21:53, Marek Potociar wrote:
>>
>> On Mar 25, 2013, at 2:07 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>>
>>> Hi Bill
>>> On 25/03/13 16:01, Bill Burke wrote:
>>>>
>>>>
>>>> On 3/25/2013 8:50 AM, Sergey Beryozkin wrote:
>>>>> On 25/03/13 15:16, Sergey Beryozkin wrote:
>>>>>> On 25/03/13 13:42, Marek Potociar wrote:
>>>>>>>
>>>>>>> On Mar 25, 2013, at 9:18 AM, Sergey Beryozkin<sberyozkin_at_talend.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>> On 15/03/13 15:48, Marek Potociar wrote:
>>>>>>>>>
>>>>>>>>> On Mar 15, 2013, at 1:28 PM, Sergey Beryozkin<sberyozkin_at_talend.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> On 15/03/13 11:29, Marek Potociar wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Mar 13, 2013, at 11:14 PM, Sergey
>>>>>>>>>>> Beryozkin<sberyozkin_at_talend.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On 13/03/13 14:27, Sergey Beryozkin wrote:
>>>>>>>>>>>>> On 13/03/13 14:25, Marek Potociar wrote:
>>>>>>>>>>>>>> I'd say that if there is no entity produced, no writers are
>>>>>>>>>>>>>> invoked
>>>>>>>>>>>>>> and thus no writer interceptors are invoked. That's analogous to
>>>>>>>>>>>>>> processing empty inbound messages where the user does not try to
>>>>>>>>>>>>>> inject or read the entity.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> That sounds OK, agreed
>>>>>>>>>>>>>
>>>>>>>>>>>> What should ContainerResponseContext return in case of the null
>>>>>>>>>>>> response body and the resource method actually invoked, for
>>>>>>>>>>>> example,
>>>>>>>>>>>>
>>>>>>>>>>>> public Book getBook() {
>>>>>>>>>>>> throw new SomeException();
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> Mapper: maps it to Response with a null entity.
>>>>>>>>>>>>
>>>>>>>>>>>> The filters are processing this response, should they return
>>>>>>>>>>>> 'null' for ContainerResponseContext for
>>>>>>>>>>>> getEntityClass/Type/Annotations, or the actual Book type info ?
>>>>>>>>>>>
>>>>>>>>>>> Annotations should be always set from method.
>>>>>>>>>>> Type and Class should be only set for non-null results.
>>>>>>>>>>>
>>>>>>>>>> Yes, it helps, thanks. This is actually how I implement it now,
>>>>>>>>>> though the user can reset the method-level annotations from the
>>>>>>>>>> response filter
>>>>>>>>>
>>>>>>>>> That's fine,
>>>>>>>>>
>>>>>>>>
>>>>>>>> I've spotted that ResponseBuilder has also has a way to set the
>>>>>>>> annotations, thus effectively overriding the method level annotations
>>>>>>>> too.
>>>>>>>
>>>>>>> It's not overriding, it's adding. We discussed this earlier in this
>>>>>>> forum. IIRC it was Bill who asked.
>>>>>>>
>>>>>>
>>>>>> OK. So ClientResponseFilter is 'overriding'/resetting, see you earlier
>>>>>> confirmation above, and ResponseBuilder is 'adding', is that right ?
>>>>>>
>>>>>>>>
>>>>>>>> FYI, I'd actually prefer to remove the relevant ResponseBuilder
>>>>>>>> method, ResponseBuilder#entity(Object entity, Annotation[]
>>>>>>>> annotations) for the following reasons:
>>>>>>>>
>>>>>>>> - ResponseBuilder is all about building the response representation
>>>>>>>> - ContainerResponseFilter would be the perfect place to override the
>>>>>>>> annotations whenever it is really needed
>>>>>>>> - All the data one passes to ResponseBuilder is next available at
>>>>>>>> Response level but not the annotations - this will cause the issues
>>>>>>>> in cases where Response copy is made in the presence of multiple
>>>>>>>> JAX-RS implementations in a given container - this is actually not
>>>>>>>> that rare at all....
>>>>>>>>
>>>>>>>> Do you agree it can make sense to tackle it ? I can open a JIRA for
>>>>>>>> that, IMHO this kind of change is minor enough to do even at this
>>>>>>>> stage
>>>>>>>
>>>>>>> No, I disagree. E.g. request filters on client may need to build an
>>>>>>> abort response and may need this functionality.
>>>>>> The client response is routed via the response filters, so rather than
>>>>>> have this foreign method on Response, why doin't we add it to
>>>>>> ClientResponseFilter ?
>>>>>>
>>>>>
>>>>> Well, if we want to have both "reset" (in ResponseBuilder) + "override"
>>>>> (in response interceptors) then we do have to keep it.
>>>>>
>>>>> That said, the code which 'overrides' can have the original + new
>>>>> annotations added if needed...
>>>>>
>>>>
>>>> IMO, if the JAX-RS method is returning a Response, then the JAX-RS
>>>> method metadata is the default and any piece of it can be overriden by
>>>> the user within the Response, including annotations. As an analogy, if
>>>> the JAX-RS method has a @Produces, and the Response is set with a
>>>> different media type, the overriden media type is honored.
>>>>
>>>> Also, +1 to no writers invoked if there is no response entity.
>>>>
>>>
>>> Sorry I may've moved the thread in some different direction, though indeed, all agree that no writers are to be invoked in the case of a null entity,
>>>
>>> What confuses me even more now is that you seem to mean "override", when referring to the annotations, while Marek thinks it is actually 'add'.
>>>
>>> We definitely need to have a common position on that :-)
>>
>> It MUST be add IMO. That's how it worked in JAX-RS 1.x RI.
>
> ResponseBuilder did not have a method for adding the annotation in 1.1 ?
No, it did not. Btw. if you looked at the ResponseBuilder javadoc and checked the "Since" information, you would not have to ask ;)
http://jax-rs-spec.java.net/nonav/2.0-SNAPSHOT/apidocs/javax/ws/rs/core/Response.ResponseBuilder.html#entity(java.lang.Object, java.lang.annotation.Annotation[])
>
>> Also, I think it would be wrong to ask users to specify their method level annotations in the response builder, whenever they return a Response from a resource method. Annotation instances are not particularly easy to create...
>>
> Definitely not easy - why they users would *have* to do though ? It is optional, if the user does not add them then method level annotations are used.
> May be we are talking about slightly different things ?
I cannot recall the exact scenario, but I certainly have ran into a case where not adding the method level annotations broke some use case or even 1.x RI tests. My hunch is that it had something to do with not having the HttpMethod annotation available in a MBW or somewhere in the filter or interceptor. IMO the use case for rb.entity(Object, Annotation[]) is to e.g. provide support for additional annotations that vary based on entity content (validation, extra entity metadata for providers etc.) These typically do not interfere with static annotations placed on a resource method.
>
>> Filters can then choose to add or override the response annotations as necessary.
>>
> How will the filter response context implementation know what the user wants ? It is either add or override.
I don't get your question. Filters either know what they do or should not touch the annotations at all. In general, I'd say that filters, too, will more often add new annotations or remove individual ones, if needed and will rarely replace all the annotations. But I do not know enough to be able to completely exclude such use case. Having the option in the API for low-level filters certainly does not hurt anyone.
Marek
>
> Sergey
>
>> Marek
>>>
>>> Given that other Response relevant methods do override (as your example of @Produces), it probably makes sense to have the same for annotations, 'override'. Marek - would you agree this time ?
>>>
>>> Thanks,
>>>
>>> Sergey
>>
>