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
>>
>>
>