users@jax-rs-spec.java.net

[jax-rs-spec users] [jsr339-experts] Re: Re: PLEASE REVIEW: Updated filtering API proposal

From: Sergey Beryozkin <sberyozkin_at_talend.com>
Date: Sun, 22 Apr 2012 17:06:18 +0100

Actually, see better your point about the (im)mutability of
ResponseHeaders...

Sergey

On 22/04/12 17:01, Sergey Beryozkin wrote:
> On 20/04/12 20:10, Marek Potociar wrote:
>>
>> On Apr 19, 2012, at 12:38 PM, Sergey Beryozkin wrote:
>>
>>> Hi Marek
>>> On 05/04/12 16:11, Marek Potociar wrote:
>>>> Hello experts,
>>>>
>>>> Please review the reworked filtering API proposal. To review, please
>>>> checkout the remote branch:
>>>>
>>>> git clone ssh:///<user_name>/_at_git.java.net/jax-rs-spec~git jaxrs-api
>>>> cd jaxrs-api
>>>> git checkout -t new-filter-api origin/new-filter-api
>>>>
>>>> The API code is under src/jax-rs-api. Examples are under src/examples.
>>>> The git commit history will help you to better see the actual changes.
>>>>
>>>> What has changed (quite a lot):
>>>>
>>>> * filtering API has been split into server (container) and client part.
>>>> o client side: ClientRequestFilter, ClientRequestContext,
>>>> ClientResponseFilter, ClientResponseContext
>>>> o server side: ContainerRequestFilter,
>>>> ContainerRequestContext, ContainerResponseFilter,
>>>> ContainerResponseContext, ResourceFilter
>>>> o Response remains a common component (went through some
>>>> modifications though - see bellow)
>>>> * Container filters are by default pre-matching
>>>> o There is a new @PostMatching name-binding annotation to
>>>> support global resource (post-matching) filters and interceptors
>>>> o DynamicBinding interface has been refactored into
>>>> DynamicBinder provider interface and javadoc has been clarified.
>>>> * Request changes have been rolled back - removed RequestHeaders,
>>>> RequestBuilder& Request API methods.
>>>> * Some of the removed methods were added to the filter context
>>>> interfaces
>>>> * ResponseHeaders has been removed, Response API has been updated to
>>>> support common use cases and some of the ResponseHeaders methods
>>>> * Client API has been updated to accommodate the Req/Resp API changes.
>>>> * Examples have been updated to use the new filtering API. (Positive
>>>> fact is that in all cases the use of the new API meant less code
>>>> in the example filter implementation code.)
>>>>
>>>> Other changes:
>>>>
>>>> * MessageProcessingException has been moved from core into root package
>>>> * Client-side Target has been renamed to WebTarget to avoid a name
>>>> clash with java.lang.annotation.Target from Java SE.
>>>>
>>>>
>>>> Looking forward to your feedback. If possible, we would like to put the
>>>> proposal into the next EDR that is planned for 4/20, so please, make
>>>> sure to send your feedback by Friday next week CoB (4/13).
>>>>
>>> I like a lot many of the changes you did, thanks for the effort (as
>>> usual, honestly :-)).
>>> Looking at WebTarget reminded me how I wished we could have an entity
>>> called WebClient whose goal is to consume or post to a given resource
>>> (target) as opposed to the Client explicitly acting as a factory :-),
>>> but I recall you had some pretty convincing arguments at a time
>>> supporting the current approach. I also start liking the explicit
>>> requirement to specify Accept via WebTarget.request...
>>>
>>> I'm not too keen about 'Container' prefixes added to Request/Response
>>> filters handled in scope of the server processing, I'd be happy with
>>> the 'container' in the package highlighting the 'location'.
>>>
>>> PostMatching, ResourceInfo, and as Bill noted, DynamicBinder are nice
>>> additions.
>>>
>>> Response class:
>>> I think it's become simpler, specifically it seems a number of
>>> methods to do with the buffering support is 1 down, and say
>>> getMetadata() docs have been fixed, but I believe some more minor
>>> work needs to be done (perhaps at the JavaDoc level). Example,
>>> Response.getEntity() would return what I set with
>>> ResponseBuilder.entity(). What would Response.readEntity() do when
>>> called on the server side ? Perhaps IllegalStateException ? I can
>>> start another thread for it.
>>
>> It's in the javadoc - IllegalStateException should be thrown.
>>
>>>
>>> ContainerRequestContext& ContainerResponseContext: I agree with you
>>> that HttpHeaders represents the request headers, so it's not reusable
>>> within ContainerResponseContext but may be within
>>> ContainerRequestContext ? I guess you wanted it to be consistent
>>> between the two, meaning ContainerRequestContext&
>>> ContainerResponeContext offer the same interface toward retrieving
>>> the headers.
>>
>> Yes. People writing client and server filters should work with
>> interfaces that look similar IMO. The intention here is to make the
>> API adoption easier for the users.
>>
>>>
>>> I wonder if ResponseHeaders still does make sense. We'd then have
>>> ContainerRequestContext.getHeaders():HttpHeaders,
>>> ContainerResponseContext.getHeaders():ResponseHeaders
>>> Response.getHeaders():ResponseHeaders ?
>>
>> What would it give us? HttpHeaders are immutable so I suppose
>> ResponseHeaders would also be immutable. Changing a header value would
>> still have to be delegated most likely to the context. That would
>> however introduce a side-effect: changing a value in a mutable context
>> would change the content of the "immutable" headers instance. All in
>> all, sounds more confusing than useful. Currently you have all the
>> methods on a single context instance. Which is good, as the context
>> instance is mutable, so you don't have think about what happens if you
>> change something - you simply know you changed that single context
>> instance.
> I think that makes sense.
> I was more about minimizing the duplication a bit or suppose,
> centralizing the representation of response headers in a single
> place/interface.
> However much depends on the policy JAX-RS takes toward possibly updating
> some of its interfaces in the future.
> Example, suppose we'd want to offer an additional and dedicated getter
> to one of the specific response headers.
> We'd need to update ContainerResponseContext & Response but would only
> have to update would-be ResponseHeaders if it existed. The latter though
> depends on the policy, can we update interfaces that users are not
> usually expected to implement or not. I'd prefer a more 'relaxed'
> approach here, but not sure it can be generally be in line with the rest
> of Java/JSRs...
>
> Cheers, Sergey
>
>>
>>>
>>> Overall, nice effort
>> Thank you.
>>
>> Marek
>>> Cheers, Sergey
>>>
>>> P.S I'll open a JIRA to do with ReaderInterctorContext returning a
>>> modifiable Map<String, String>, in all other cases where modifiable
>>> maps are provided it is Map<String, Object>
>>
>> Ok, thanks.
>>
>>>
>>>
>>>> Thank you,
>>>> Marek
>>>
>>>
>>
>


-- 
Sergey Beryozkin
Talend Community Coders
http://coders.talend.com/
Blog: http://sberyozkin.blogspot.com