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?
> - 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?
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?
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()
This is much more clear and easier to read (with the previous revision);
target.request().type("application/xml").entity(customer).method("PATCH").invoke();
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.
>>
>>>>
>>>> * 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.
>>>>
>>>> * 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?
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.
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();
>>>> 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.
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com