On 08/22/2011 04:21 PM, Bill Burke wrote:
> 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.
Thanks. Interceptors however are Santiago's work. The credit goes to him.
>
> 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).
That's the natural cost of a fluent API[*]. I would argue that the purpose of a fluent API is to be easy to use by *end
users*, resemble natural language flow and provide consistent context continuation wrt. given context state so that e.g.
IDE code completion can benefit from that.
Such API is best described by examples, not as a javadoc merely describing the class hierarchy and list of their
methods. See here for a good example:
http://google-guice.googlecode.com/svn/trunk/javadoc/com/google/inject/Binder.html
[*] We could slightly reduce the number of interfaces by repeating the interface method definitions in each implementing
class. But it's really not worth considering at least until the API is truly stable and fully javadoced.
> I dread the day when I
> have to update my O'Reilly book to explain it.
You should be describing a fluent API in terms of the API example usages rather than javadoc. Maybe I can help you with
the chapter? :)
>
> 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.
That's something which would provide the implementor's point of view. Yet the purpose of the API is not to make it
simple for a provider (or a book author), but for the API consumer. I want you to do a different exercise. Have a look
at the content of BasicExamples.commonFluentUseCases() method and calculate how many of those classes are actually
visible to the end user when the API is used:
http://java.net/projects/jax-rs-spec/sources/git/content/src/examples/src/main/java/jaxrs/examples/client/BasicExamples.java
>
> 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.
Again, I am not ignorant of the increased complexity in the class hierarchy. But, again, that's not the goal of a fluent
API. The goal is to be easy to use, DSL-like and semantically consistent. What we had before was simpler, but at the
expense of compromises in the actual goals. Class hierarchy complexity can be dealt with by providing usage examples.
What we really should be focusing on are the method names to make the DSL part of the API natural.
>
> 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?
Content type SHOULD be included when entity is not empty, but it is not a mandatory header. You can include it via:
client.target(...).type("text/plain").put(entity);
Or you can just omit it (not recommended):
client.target(...).put(entity); // content type defaults to "application/octet-stream"
Other option would be to add an extra parameter for content-type into the invoker methods that consume entity (PUT, POST
...). But we discussed this with Santiago and we feel that the extra parameter would make the method signatures somewhat
complex.
Yet another option would be to provide some default content type resolution support for most common entity types.
>
> target.put(entity) makes no sense as there's no way of setting the content-type of the entity.
See above. Such code most certainly makes sense.
>
> * 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?
We can always add a new method to Target, but before we do it, I am interested in seeing a particular use case. Maybe it
can be solved without the extra method.
>
> * 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.
ClientFactory is a common entry point. As such it has to be an abstract class.
ClientBuilderFactory is the implementation provider hook.
Client.Builder is the extensible fluent client bootstrapping base class.
The way those classes currently work together let's you write code like this (notice the type-safe requestQueueCapacity
config option):
ClientFactory.newClientBy(ThrottledClient.Builder.Factory.class).requestQueueCapacity(10).build();
This is a line from the BasicExamples.clientBootstrapping(). How would you support this type-safe bootstrapping
configuration with the reduced API?
>
> 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.
>
As I keep repeating, the class hierarchy simplicity is not the main goal here. We need to provide consistent context
flow otherwise our fluent invocation API will be a joke and I would not sign off on it.
Btw. all the original flexibility and simplicity is retained in the filters and handlers that still use
HttpRequest/Response mutable classes. That's all right, because filters and handlers are naturally supposed to work with
super-flexible and simple API.
Marek
>
>
> 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.
>>>
>>>
>>>
>