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).
> 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() ;-)
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