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