users@jax-rs-spec.java.net

[jax-rs-spec users] [jsr339-experts] Re: Re: Re: Comments about container filters

From: Marek Potociar <marek.potociar_at_oracle.com>
Date: Wed, 12 Sep 2012 19:52:38 +0200

On Sep 12, 2012, at 5:58 PM, Sergey Beryozkin <sberyozkin_at_talend.com> wrote:

> On 12/09/12 15:21, Marek Potociar wrote:
>>
>> On Sep 12, 2012, at 1:32 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>>
>>> On 12/09/12 12:06, Marek Potociar wrote:
>>>>
>>>> On Sep 10, 2012, at 11:29 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> Instead of creating a new thread every time I have some question or suggestion re container filters I will just keep a single thread, hopefully it wil be easier to track the comments
>>>>>
>>>>> IMHO, here is what needs to be updated (comments from previous threads + new ones) but obviously is up for the discussion
>>>>>
>>>>> 1. ContainerRequestContext does not have to reference Servlet container in its property-related methods. Please review the threads leading to the introduction of these methods - the goal was to let filters exchange the properties - with the current wording they actually provide an interface to HttpServletRequest property-related methods
>>>>
>>>> Please file a Jira issue with suggested wording change.
>>>>
>>>>>
>>>>> 2. Update ContainerRequestContext documentation to make it obvious global post-match filters run for selected resource and sub-resource locator methods
>>>>
>>>> I did update the docs in the last few days. Please review. One note: filters cannot be attached to sub-resource locators. The javadoc never said that IIRC.
>>>>
>>> OK, thanks
>>>
>>>>>
>>>>> 3. Single parameter ContainerRequestContext.setRequestUri: clarify that it is a relative path which is reset, equivalent to the one returned from UriInfo.getPath; do similar clarifications for a 2 parameter overload.
>>>>
>>>> Not sure I understand. The javadoc of setRequestUri methods explicitly describes how the URI is resolved. Can you provide the clarified wording you have in mind?
>>>>
>>>> Btw. I disagree with your other suggestion to drop the 2-URI variant. That's a convenience method for users and we had to add it to Jersey filtering API based on user requests.
>>>>
>>>
>>> Can you expand more on this please ?
>>>
>>> So we have the servlet listens say on http://abc.com/rest.
>>> We have an incoming request, "http://abc.com/rest/1"
>>>
>>> I see why I can replace '/1' with '/2'.
>>>
>>> Replacing "http://abc.com/rest/" with "http://bar.com/rest/"
>>> is an entirely different case, and I'm not sure what are the expectations on the JAX-RS runtime here. I think the case if out-of-band for JAX-RS, but I may be wrong.
>>>
>>> Can you please clarify ?
>>
>> A use case:
>>
>> We have a "normalization" filter that let's you normalize and canonicalize the request URIs (e.g. it replaces multiple consecutive "/" in path with a single one). In order for that one to work properly, you need to also normalize the base URI and set it.
>>
>> Another use case:
>>
>> You may want to shadow some different URI space and the most convenient way how to do that is to use the URIs from the other URI space including it's base URI.
>>
>> In summary, replacing the base URI means that you are going to use a different base URI to resolve the request path that will be ultimately matched by JAX-RS runtime. IOW, you use a custom base URI to extract the proper URI-suffix that should be processed by JAX-RS.
>>
>
> OK, the above helps.
>
>>> I'm sorry for asking for the details !
>>
>> Please...
>>
>>>>>
>>>>> 4. ContainerRequestContext should throw IllegalStateException when setters are called as part of the response chain
>>>>
>>>> Makes sense. Done.
>>>>
>>>>> 5. ContainerResponseFilter: drop @PreMatching support, get the global filters executed in all the cases
>>>>
>>>> Done.
>>>>
>>>
>>> Thanks
>>>
>>>>> 6. ContainerResponseContext.setEntity(Object, Annotation[], MediaType)
>>>>>
>>>>> I wonder if this method is needed, in this form - I'd really limit it to
>>>>> ContainerRequestContext.*setEntity(Object)*: we can set MediaType on response headers directly if really needed.
>>>>>
>>>>> Annotation[]: this is inconsistent with the fact that ContainerRequestContext has getEntityClass and getEntityType methods and obviously we can not determine a correct Type from Object, hence getting Annotation[] passed seems unusual. The whole idea of actually making the runtime to believe that the method returns a class + type different from the pair which can be reflectively introspected does not seem like a good idea. Replacing the actual entity seems OK/reasonable - for a 'complete' replacement we can simply use a name-bound *blocking request* filter
>>>>
>>>> I agree that the method is a powerful tool, but I prefer we keep it in (also based on your follow-up discussion with Bill).
>>>>
>>>
>>> OK. One minor issue remains, we have get getEntityType(), but no Type parameter is available in this method
>>
>> Isn't that something that should be inferred from the entity instance?
>
> Well, I guess there's a reason getEntityType() which returns 'Type' is there. If you expect Type and Class being the same then lets remove getEntityType given we have getEntity()

The reason why getEntityType is there is that when the resource method call returns, the getter provides the information about the static return type of the method. Not sure if we need a similar type setter in filter. But I'm not strongly against or anything. Just that with all the filters I wrote so far I have not run into a use case where I would need such method. Please file a Jira issue if you feel strongly about it.

>
>>
>>>
>>>>> 7. ContainerResponseContext.getHeaders& getStringHeaders - both return mutable maps, the latter method only supports remove operations: can we have getHeaders() only ? A single method dedicated to modifying response headers ?
>>>>
>>>> The getStringHeaders() provide a convenient string-based view of getHeaders(). There are many cases where this is useful. We may updated the javadoc so that getStringHeaders() do not indicate support for removal operations, if you want, but the method should stay.
>>>>
>>> If you can agree to make this view read only then it would simply things IMHO
>>
>> Ok, I don't have any problems with removing the mutability requirement from the javadoc.
>
> That simplifies things a bit

Done.

Marek
>
> Cheers, Sergey
>
>>
>> Marek
>>
>>> Sergey
>>>
>>>> Marek
>>>>
>>>>>
>>>>> That is it for now
>>>>> Thanks, Sergey
>>>>
>>>
>>>
>>
>
>