On 08/24/2011 12:04 AM, Bill Burke wrote:
> On 8/23/11 4:25 PM, Marek Potociar wrote:
>>> What exact compromises did the previous revision make (especially with my added additions)?
>>
>> - users would have to decide about sync/async in the very begining of the invocation building; that's similar to Jersey
>> API and we had customers complaining about this approach that resulted in a need to create overloaded methods that would
>> take WebResource or AsyncWebResource respectively.
>>
>
> Then you combine AsyncInvocation with Invocation:
>
> interface Invocation extends HttpRequest {
>
> HttpResponse get();
> Future<HttpResponse> getAsync();
>
> Future submit();
> HttpResponse invoke();
>
> }
>
> BTW, you have the same issue with the current revision, don't you?
No, I don't. User can decide about sync/async only immediately before the invocation is executed. Once the decision is
made, there is no way back to modifying headers etc.
>
>
>> - the API was suddenly not consistent at all; following completely broken flow would be suddenly possible:
>>
>> invocation.method("PUT").entity(e).get();
>>
>
> "Completely broken" is a very large exaggeration. While possible, the possibility of anybody ever doing that is not
> very probable. Also, when you think about it, this isn't broken at all as its not much different than calling type()
> twice. What else was "completely broken", just this?
In anticipation of your reaction, I wouldn't dare adding extra interface to shield the type from overriding. :)
I was considering moving it to the invocation method, but it would produce too long method signatures. Admittedly it is
a trade-off. If you want me to fix it, I'll add one more builder IF.
>
> Also, you completely blew off my complaint that the current revision was "broken" because it allowed you to call put()
> or post() without setting a representation type. How is that consistent?
Did I really ignored that? I told you, that according to the spec, it's not a MUST, but SHOULD. Does it mean you request
the API to treat SHOULDs as MUSTs?
> Finally, this doesn't look very "fluent" with the current API as the type and the entity are completely disjointed:
>
> target.type("application/xml").build("PATCH", customer).invoke()
As I said, we can either add the type to the invocation method or introduce an extra builder interface. Either way, we
would improve the consistency instead of loosing it.
>
> This is much more clear and easier to read (with the previous revision);
>
> target.request().type("application/xml").entity(customer).method("PATCH").invoke();
I suppose you are talking about your proposal, since there I am not aware of any revision in the source repository where
the above code would compile. It's true, that your revision supports the above. It supports almost any combination since
it's super-flexible. It also supports:
link.request().type("application/xml").entity(customer).invoke(); // what method I am calling?
>
> You could also make the case that this is more fluent to read as it models what an HTTP invocation looks like:
>
> client.put().resource(targetUrl).type("application/xml").entity(customer);
>
> Which goes to show that "fluency" is all a matter of perspective and opinion.
I disagree. The DSL we choose is the matter of perspective and opinion. The fluency and consistency are the attributes
you need to bake in when designing the API for the DSL.
>
>>>
>>>>>
>>>>> * 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.
>>>>
>>>
>>> I want to remove Target implementing SyncInvoker (and RequestHeaders, etc.). Once you do that, you can pretty much
>>> eliminate most of the additional abstractions you added to the latest revision.
>>
>> Well, I did play with the code *a lot* and knowing you are in our EG, I would not dare including any redundant
>> interfaces in the proposal. I don't think we can remove any of the new builders even if we re-introduce the
>> target.request() method. To keep the flow semantically correct, we need to distinguish between the following flow
>> context transitions:
>>
>> client -> path -> set headers -> [ enable asynchrony -> ] invoke
>> client -> path -> set headers -> prepare invocation -> invoke
>>
>
> The previous revision supported this, especially if you combine AsyncInvocation with Invocation.
>
> client.resource("http://...").request().type("text/plain").put();
> client.resource("http://...").request().type("text/plain").putAsync();
>
> // Batch processing
> Invocation invocation = client.resource("http://...").request().type("text/plain").method("PUT");
>
> batchProcessor.add(invocation);
>
>
> No builders required. You only need Target, Invocation, HttpRequest, HttpResponse, and Client.
>
We move in circles here. I don't dispute that your revision supports this DSL. Due to its extreme flexibility it
supports many DSL variants, even the ones that are semantically incorrect or incomplete.
Let's focus on the real issue. My objection is that your API proposal is not consistent. Your objection is that my API
proposal is too complex. I say that with fluent API the consistency is more important than simplicity. Assuming that
according to RFC2616, every HTTP request MUST consist of at least target URI and a HTTP method, your API proposal does
not guarantee the request consistency. Furthermore, your API proposal does not guarantee the consistency of the fluent
invocation context in the major aspects of the request (URI, method), IOW you allow the users to redefine those aspects
multiple times as part of the single invocation chain. That's a fundamental problem which cannot be solved without
introducing more intermediate builder interfaces.
>
>>>>>
>>>>> * 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?
>>>>
>>>
>>> I think this suffers a little bit from over-engineering.
>>>
>>> What is wrong with a simple constructor? You're already referencing the vendor specific class.
>>>
>>> ThrottledClient tc = new ThrottledClient();
>>> tc.requestQueueCapacity(...)
>>> .timeout(...)
>>> .cacheSize(...)
>>>
>>> Or even:
>>>
>>> ThrottledClient tc = ThrottledClient.create().requestCapacity(...).timeout().cacheSize();
>>>
>>> Its not the specs domain to define how an implementor provides implementation specific features/APIs.
>>
>> The idea would be to support injection which is hardly possible with direct constructor invocation.
>
> Please explain what you're talking about by injection and what it would look like?
For example, you could inject the custom client factory classes e.g. leveraging the JSR330 integration - this is
something I only have a rough idea about at the moment. You could also inject some of the JAX-RS injectable stuff,
particularly from the ext package, e.g. RuntimeDelegate, Providers, ExceptionMapper... again, just a rough idea at the
moment.
>
> FYI, JPA does not give you the flexibility to inject a vendor specific implementation of a specific derivation of the
> EntityManager interface. Unless of course you wrote your own injector.
>
>> Also, to support
>> multiple default providers without any factories, Client would need to be an abstract class, which was, IIRC your very
>> first objection.
>>
> JAX-RS 1.1 does not support multiple default providers running at the same time, at least from a RuntimeDelegate
> perspective.
True, and we have it reported as a MAJOR bug that needs to be fixed in JAX-RS 2.0:
http://java.net/jira/browse/JAX_RS_SPEC-110
>
> To clarify the change, ClientFactory ends up looking a lot like RuntimeDelegate.
>
> public abstract class ClientFactory {
> public abstract Client create(Configuration config);
> public abstract Client create();
>
> public static void setInstance(ClientFactory defaultFactory)
> {
> ...
> }
>
> public static ClientFactory getInstance() {...}
> }
>
> If you want a specific vendor implementation, then just allocate it yourself:
>
> ResteasyClientFactory resteasy = new ResteasyClientFactory();
>
> or
>
> ResteasyClient client = new ResteasyClient();
>
First of all, the ClientFactory IS a client-side version of RuntimeDelegate. The idea was to let users define their own
"light-weight" client extensions independent of the underlying JAX-RS API provider. E.g. to replace the underlying
communication framework etc. with the extension attributes exposed in the type-safe way. Such extensions would still
leverage most of the functionality from the underlying JAX-RS provider, including the potential injection support.
>>>>> The thing is, I doubt anybody will care if its 100% fluent. The previous version was already pretty much 99% fluent.
>>>>> Even that 1% difference was up for debate, IMO on whether it was "fluent" or not.
>>>>
>>>> The problem was with the verb not being at the end. It sure had to bother you too as you tried to address it in your
>>>> proposal.
>>>>
>>>
>>> Yeah, and I solved the verb problem pretty simply, not by adding mounds of abstractions.
>>
>> I wonder if you really don't see that there actually was a compromise you had to make too? You opted for simplicity in
>> class hierarchy and sacrificed fluent API consistency. A wrong choice when it comes to a fluent API design, if you ask
>> me.
>>
>
> No, I don't see it. Especially when you only gave one example of its deficiency. And even that deficiency was debatable.
>
I reiterated multiple times over the consistency issue. I tried to explain why it is a fundamental problem when it comes
to the fluent API. I am done repeating myself. Take it or leave it, it's your choice.
Marek