Hi Marek
Please see comments with S.B
________________________________________
From: Marek Potociar [marek.potociar_at_oracle.com]
Sent: 23 August 2011 21:54
To: jsr339-experts_at_jax-rs-spec.java.net
Cc: Sergey Beryozkin; Bill Burke
Subject: Re: [jsr339-experts] Re: client revisions
Hi Sergey,
thanks for chiming in on this controversial topic.
On 08/23/2011 10:27 PM, Sergey Beryozkin wrote:
> If we can avoid re-introducing request() as a necessary code step then it would be nice.
Can you provide more justification for your preference if possible? I am asking because so far, only Markus is the clear
supporter. (I'm personally 50:50. I see some advantage of not having it, but I still included the Target.request() in my
initial API update as DSL looked good with it).
S.B: My primitive justification is that .request() is just one more step and thus I'm satisfied with the fact that the current revision is capable of
creating simple intuitive expressions (I think I'm with Markus on this one) without having to do .request() while also allowing for more sophisticated
ways of building the requests.
I will be commenting more soon, just need to properly review...
> I'm wondering if it's similar to Target.prepare() ?
It is similar, and so is the Invocation.TargetedBuilder.async() method. We would need to add buildGet(), buildPost(),
etc. methods to the target to get rid of the prepare(). Not sure it's a win. Alternatively, to get rid of prepare(), we
could reintroduce request() ;-)
S.B: I;d be for avoiding buildGet() and such. One does not have to use prepare() unless specifically needed and I'm OK with that.
But I will be commenting more shortly...
Thanks, Sergey
Marek
>
> Cheers, Sergey
>
> ________________________________________
> From: Marek Potociar [marek.potociar_at_oracle.com]
> Sent: 23 August 2011 21:25
> To: jsr339-experts_at_jax-rs-spec.java.net
> Cc: Bill Burke
> Subject: [jsr339-experts] Re: client revisions
>
> On 08/23/2011 08:28 PM, Bill Burke wrote:
>> On 8/23/11 12:33 PM, Marek Potociar wrote:
>>>>
>>>> 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.
>>
>> Not really. One of the first things I look at is the Javadoc *as a user*.
>>
>>
>>> 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.
>>>
>>
>> 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.
>
> - the API was suddenly not consistent at all; following completely broken flow would be suddenly possible:
>
> invocation.method("PUT").entity(e).get();
>
>>
>>>>
>>>> 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.
>>>
>>
>> All this just so that you don't have to invoke a request() method (which is fluent-compliant BTW) on Target. IMO, which
>> I've stated in the past, a request() method makes a clear separation of concerns.
>>
>> Target.header(foo,bar) makes as much sense as URL.header(foo, bar). Target is a resource, not a request.
>
> I had it there in my first update, but then Markus commented that from the user perspective that the extra request()
> method seemed redundant to him. I think he's got the point from the fluent API perspective, so I removed the extra
> method. I can see the pros/cons of both approaches but I don't have any strong feelings. I don't have problem
> re-introducing the extra SoC method again if I here more objections in EG.
>
>>
>>>>
>>>> * 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 only possible reduction I see is merging Invocation.TargetedBuilder and Invocation.PreparedBuilder. Thus when
> buidling an Invocation command with a generic invoke()/submit() interface, instead of:
>
> target.prepare().get();
> target.accept("text/plain").prepare().get();
>
> You would write:
>
> target.prepareGet();
> target.accept("text/plain").prepareGet();
>
> Or:
>
> target.buildGet();
> target.accept("text/plain").buildGet();
>
> But Client, Target, (at least a single) Invocation.Builder (implementing RequestHeaders.Builder and SyncInvoker),
> Invocation, SyncInvoker and AsyncInvoker would still be there.
>
>>>>
>>>> * 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. Also, to support
> multiple default providers without any factories, Client would need to be an abstract class, which was, IIRC your very
> first objection.
>
>>>> 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.
>
> Marek