Hi Sergey,
On 08/23/2011 02:00 PM, Sergey Beryozkin wrote:
> Hi Marek
>
> Comments inline
> ________________________________________
> From: Marek Potociar [marek.potociar_at_oracle.com]
> Sent: 23 August 2011 12:19
> To: jsr339-experts_at_jax-rs-spec.java.net
> Cc: Sergey Beryozkin
> Subject: Re: [jsr339-experts] Re: client revisions
>
> On 08/22/2011 05:49 PM, Sergey Beryozkin wrote:
>> I was actually looking forward to Bill's revision being approved too - thought there was some agreement about
>> finalizing the chain with get()/etc in most cases and generally I thought it was step forward toward the simplification of the client api.
>
> While we agree with Bill in the end result (i.e. what the end-user sees in code), we seem to differ in the solution.
> Bill prefers API simplicity over consistency. I prefer API consistency over simplicity, esp. in case of a fluent API.
> When it comes to the ease of use, both versions are comparably easy to use.
>
> S.B: I'm browsing the example you linked to in the response to Markus - that actually looks ok to me - sorry I did not look first.
> At the moment I'm seeing that get() and post() and such is at the end of the chain which makes me happy :-) but will follow with more
> comments - Bill's comments re multiple factories and the complexity of the transitions did sound serious enough.
There are *NO* additional factories. There are new interfaces to support the fluent context flow, to prevent you from
doing things like:
client.target("
http://foo.com/").method("PUT).entity(e1).path("
http://bar.com").post(e2);
I hope we can eventually agree that at each point in the time the current context of the fluent API should reflect the
past invocations to support a method chaining that would represent a semantically correct DSL. IOW, the proposed flow is
this - bootstrap client, specify path, set headers, (invoke method [with content] | build invocation command [with
content]). Each part of this DSL requires it's own context representation. That's where the extra interfaces come from.
>>
>> More comments soon, just back form holidays - particularly keen to see how the idea of Active links has been implemented -
>
> There's nothing fancy to it - I just renamed client.Link to client.Target to distinguish it as an active link. Then I
> introduced a Link POJO header as well as client.target(Link) method. This is still a fresh area however, so there should
> be a room for suggestions and improvements.
>
> S.B: I think I'm +1 on that - it does look OK to me too. I think it makes things cleaner.
> I'd only comment that a Link POJO should be doc-ed to say that it can capture
> links not only from headers but from attributes, etc.
I haven't thought about using Link in place of structural links, but it should certainly be possible. Btw. in case you
have not seen it, there is a simple example of how the links can be used on the client-side:
http://java.net/projects/jax-rs-spec/sources/git/content/src/examples/src/main/java/jaxrs/examples/client/links/LinkUsageExample.java
Cheers,
Marek
>
> Thanks, Sergey
>
>
> Marek
>
>>
>> thanks, Sergey
>> ________________________________________
>> From: Bill Burke [bburke_at_redhat.com]
>> Sent: 22 August 2011 15:21
>> To: jsr339-experts_at_jax-rs-spec.java.net
>> Subject: [jsr339-experts] Re: client revisions
>>
>> First the good things: I like the addition of the Feature interface.
>> It should be allowed to be annotated with @Provider and scanned and
>> automatically registered too. You did a good job of refactoring my
>> interceptor proposal. I like the class names and hierarchy there.
>>
>> Now the bad...
>>
>> At first glance the class hierarchy is ridiculous. The Javadoc will not
>> be very useful to first-time users as the class hierarchy is hard to
>> understand even for somebody that has been working on JAX-RS 2.0 (i.e.
>> me). I dread the day when I have to update my O'Reilly book to explain it.
>>
>> I want you to do an exercise:
>>
>> 1. Count how many Builder interfaces you have
>> 2. Calculate the average of how many interfaces an interface implements
>> 3. Load this revision into a graphical class hierarchy browser and view
>> all the criss-crossing lines.
>>
>> Then maybe you'll see how complicated you've made this API in order to
>> make it "pure". IMO, ditch this, revert to the previous version, and
>> let's move on from there.
>>
>> I also think there are a lot of problems with it:
>>
>> * How do you set the Content-Type of a Target? What I'm getting at is,
>> what's the point of having Target implement the SyncInvoker interface?
>>
>> target.put(entity) makes no sense as there's no way of setting the
>> content-type of the entity.
>>
>> * The only way to go from a Target to an Invocation/Invocation.Builder
>> is to set a header on Target. What if you don't want/need to set a
>> header? i.e. a get or delete request?
>>
>> * Client.Builder and ClientFactoryBuilder and ClientFactory can be
>> combined. There's no need to have a factory of a factory of a factory.
>> Please see my earlier proposal (4-5 weeks ago) on a better way to do this.
>>
>>
>> All and all, I'm really disappointed with this revision. I think the
>> previous revision plus the additional changes I proposed (in the GIT i
>> linked) was far superior had a greatly simplified class hierarchy and
>> was just as simple to use.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> On 8/17/11 11:33 AM, Marek Potociar wrote:
>>> Sorry for a delayed response. I have reviewed the proposed changes and based on the proposal bellow as well as based on
>>> the feedback from Sergey, Guilherme and Markus I have the updated the proposed client API:
>>>
>>> http://java.net/projects/jax-rs-spec/sources/git/show/src
>>>
>>> The main changes include:
>>>
>>> CORE HTTP REQUEST& RESPONSE API
>>>
>>> - Introduced Headers, RequestHeaders and ResponseHeaders + inner Builder classes to hold the header-related APIs reused
>>> in client-side fluent API as well as core HttpRequest and HttpResponse classes
>>>
>>> - Introduced new core exception to decouple core req/resp classes from the client API
>>>
>>> - Cleaned up and extended the HttpRequest/Response API ...somewhat :)
>>>
>>> CLIENT BOOTSTRAPPING AND CONFIGURATION
>>>
>>> - Added typed Feature support - see Feature and Configurable interfaces
>>>
>>> - Removed redundant ClientConfiguration interface as well as DefaultClientConfiguration impl class
>>>
>>> FLUENT CLIENT API
>>>
>>> - Link renamed to Target
>>> This is to distinguish the "active link" from the plain link header POJO (Link header POJO still to be introduced).
>>>
>>> - Streamlined the fluent client API: client -> target -> invocation builder [ -> invocation] -> response
>>> -- Invocation API was reduced to just support the command pattern for request processing
>>> -- new Sync/AsyncInvoker interfaces were introuced to support HTTP method at the end of the fluent API method chain
>>> -- Invocation.Builder was introduced to enforce the consistency in the fluent method invocation chain (e.g. the API does
>>> not allow you to set a POST method and then invoke a GET). Such "advanced" features were removed from the fluent API,
>>> but the flexibility for filters and handlers is retained since those work with mutable HttpRequest/Response interfaces.
>>>
>>> Now, I know that with this fluent client API change we somewhat closed the loop with the client API proposals. The API
>>> resembles the original proposal once again and the class hierarchy is more complex compared to the previous version.
>>> Yet, this is not necessarily a bad thing IMHO. The problem with the older version of the API is that it was trying to do
>>> too many things: it was supposed to be fluent, consistent and easy-to use, but also remain simple and lean (in terms of
>>> class hierarchy) as well as super-flexible at the same time. The main point in providing a fluent API is however to be
>>> primarily consistent and easy to use by the end users. Reduced flexibility and slightly increased complexity of the
>>> class hierarchy is a reasonable price to pay. It is the usability of the fluent API that is important, not how the
>>> javadoc looks like or how many (hidden) classes are there. And, again, please notice that the original flexibility
>>> requirement is still preserved in the places where it makes most sense - filters and handlers are still using the
>>> HttpRequest/Response interfaces.
>>>
>>> With that, I am looking forward to your feedback,
>>> Marek
>>>
>>> On 07/05/2011 05:30 PM, Bill Burke wrote:
>>>> I made some changes to client API to simplifiy it a little.
>>>>
>>>> https://github.com/patriot1burke/redhat-jaxrs-2.0-proposals-rev2
>>>>
>>>> * removed queue from Invocation interface
>>>> * Added an AsyncInvocation interface
>>>> * Added get/post/delete/etc. methods to AsyncInvocation and Invocation
>>>> * removed invoke/queue methods from Client as they are redundant and I don't get why they are there.
>>>> * removed HttpRequest.Builder as its not needed anymore
>>>> * Link no longer extends HttpRequest.Builder
>>>> * added request/async methods to Link
>>>> * Gutted and simplified ClientFactory. No need for all those extra abstractions.
>>>>
>>>> You now are forced to put a get(), Post() or whatever at end of method call unless you are doing a custom HTTP method
>>>> like PATCH. It now looks like:
>>>>
>>>> Client c = ...;
>>>>
>>>> Invocation invocation = c.request();
>>>> HttpResponse response = invocation.header("foo", "bar").get();
>>>> String str = invocation.header("foo", "bar").get(String.class);
>>>>
>>>> AsyncInvocation async = c.async();
>>>> Future<HttpResponse> f = async.header("foo", "bar").get();
>>>> Future<String> f = async.header("foo", "bar").get(String.class);
>>>>
>>>> Generic invocations are:
>>>>
>>>> Invocation invocation = c.request();
>>>> HttpResponse response = invocation.header("foo", "bar").method("PATCH").invoke();
>>>>
>>>>
>>>> Alternatively, you could combine Invocation/AsyncInvocation, but you'd have to rename get() to getAsync(), etc. I think
>>>> originally it wasn't done this way because of the explosion of methods within Invocation.
>>>>
>>>>
>>>>
>>
>> --
>> Bill Burke
>> JBoss, a division of Red Hat
>> http://bill.burkecentral.com