On 10/09/12 22:29, Sergey Beryozkin 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
>
> 2. Update ContainerRequestContext documentation to make it obvious
> global post-match filters run for selected resource and sub-resource
> locator methods
>
> 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.
>
Can we drop a two-parameter variant ?
I think it is wrong to let users replace the base URI after the
underlying container matched the original request URI. Replacing the
path value using a single-variant option can be very handy but letting
the users to hack the base URI seems too open. Note that other options
like redirects (back to the client or locally, using
RequestDispatcherProvider) is possible
> 4. ContainerRequestContext should throw IllegalStateException when
> setters are called as part of the response chain
>
> 5. ContainerResponseFilter: drop @PreMatching support, get the global
> filters executed in all the cases
>
> 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'm seeing the same in ClientRequestContext.
IMHO this opens too many options for users. I'm not even sure it is the
right thing to do, let say the server runtime invokes on a resource
method which returns Book and then let users replace it with
List<Chapter> - I think this is wrong and is prone with problems.
I'm fine with letting users customize/override the response entity,
example, assuming Book is returned, let users update some of Book
properties or replace it with some subclass instance and then reset the
entity using setEntity(Object) - the entity is still 'confined' by the
response type info provided by the resource method.
However letting users completely change the expectation of the runtime
(re the type info of a given entity) is too loose.
If we have a case where say a legacy client is invoking on the method
which now returns a different type, say Book2, then the better solution
is to redirect the client to the legacy method ( which still returns
Book) by using a pre-match request filter and its setRequestUri method, etc.
Thoughts ?
Sergey
> 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 ?
>
> That is it for now
> Thanks, Sergey
--
Sergey Beryozkin
Talend Community Coders
http://coders.talend.com/
Blog: http://sberyozkin.blogspot.com