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.
>
> 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.
>
> 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.
> 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).
> 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.
Marek
>
> That is it for now
> Thanks, Sergey