jsr339-experts@jax-rs-spec.java.net

[jsr339-experts] Re: Update in the client configuration API flow

From: Sergey Beryozkin <sberyozkin_at_talend.com>
Date: Thu, 29 Sep 2011 17:38:19 +0100

Hi
Sorry for a delay...

>>
>>>
>>>>
>>>> - example invocationFlexibility concerns me a bit.
>>>>
>>>> I've checked the source and I see no accept() method at all - Invocation.Builder has a header(...) method, so I can
>>>> use it.
>>>> Target allows for
>>>>
>>>> target.request()
>>>> and
>>>> target.request(acceptMediaTypes)
>>>>
>>>> The former option requires me to do header("Accept", "text/xml") but still lets me do acceptLanguage(...). And if
>>>> Request.RequestBuilder did have accept() then
>>>>
>>>> target.request().buildGet().asRequestBuilder().accept("text/html").invoke()
>>>>
>>>> vs
>>>>
>>>> target.request(acceptMediaTypes).get();
>>>>
>>>>
>>>> seems 'unfair' toward target.request() :-).
>>>
>>> Target.request() is not supposed to be used if you expect to receive a response back. In such case
>>> target.request(type...) should be used. It's ok, if you see the API as being unfair to the Target.request() in this
>>> scenario. We want it to be unfair.
>>>
>>
>> IMHO this attempt to protect the users from forgetting to type accept() is leading to a non-flexible API. Relax it a
>> bit. Having request() and request(types) as Target 'siblings' does indicate to me that they have to be 'equal' in their
>> 'rights', with the latter version offering an option to save on calling the explicit accept() afterwards
>
> Well, it's not about protecting anyone. It's a shortcut. If 80% of client API usecases are dealing with GET, hawing to
> write request().accept(...) seemed unnecessarily long. The shortcut request(acceptedTyped) was a good compromise for
> those who wanted clear SoC as well as for those who wanted accept(...) method on the Target.
>

I'm fine with request(acceptedTypes) - would just rename 'mediaTypes' to
'acceptMediaTypes', agree with Bill that it's not clear otherwise from
looking at the signature what those types are supposed to mean.

Yes it's a shortcut, and request().header("Accept", "text/xml") is
nearly as short the call as request("text/xml");, just a slightly
different path. And please, don't remove headers() from the builder :-).

I see no contradiction in having explicit accept() on the builder.

>>> Also, if you happen to have a more esoteric use case in which you don't know what you are going to invoke or what you
>>> can accept at the time of constructing the invocation, then you have 3 options:
>>>
>>> 1. Rethink the logic& design of your code as chances are that there is a simpler or more elegant solution for your
>>> problem (e.g. maybe you want to use filters or maybe you want to deffer the invocation construction to some later point
>>> in time).
>>> 2. Use generic Invocation.Builder.header(...) method (but only after you failed with #1)
>>> 3. Go one level lower and use the generic Invocation API + Invocation.asRequestBuilder() to perform the "last-minute"
>>> modifications of the request before invoking.
>>>
>>
>> Allowing for last minute modifications within the same chain just seem to me to be very highly impractical. That example
>> with last minute redirect withing the same flow does seem unpractical.Thus IMHO we should not dilute the API with all
>> these extra options. Right now,
>> Target-to-Invocation.Builder offers everything, including header().
>
> I don't think that providing generic invocation interface is "highly impractical". But what is the interface really good
> for if you cannot change anything? Keeping the invocation just for the sake of queue-based execution is not enough.
> Providing the access to the request builder basically gives users the flexibility of filter-based approach without the
> real need to implement a filter, which may be useful in certain cases.
>

I was saying it was highly impractical to assume that someone would ever
want to set up a target and builder within a single chain and then
decide, what about few more esoteric modifications and proceed to
asRequestBuilder(). People would just do whatever setup they need to do
in target and builder *before* and then doing get()/etc, as opposed to
buildGet().doSomeMoreModifications which one can do anyway before
buildGet().

I think the major factor for introducing InvocationHandler was to let
users write *generic* batch handlers - it's very difficult to imagine
what generic handlers would do with asRequestBuilders.


> Again, Invocation is a more advanced and lower-level concept of the API. If one wants to just fiddle with headers there
> is no reason to use Invocation. Invocation.Builder is good enough for such use cases.
>

Sorry if I'm a bit slow. I'm saying that at the moment we can have two
levels of API joined within a single chain - which is wrong IMHO.
Lets a have a single level API, not a two-level API for doing the same
thing.
I thnk Target + Invocation.Builder are OK to go in its own

I'd drop Invocation.asRequestBuilder() - purely to limit the possibility
of joining these two levels and if we really need to let generic
handlers do more than just submit/invoke then lets do some static
Invocation.asRequestBuilder(Invocation)

> As for the single fluent method chain Client -> Target -> I.B -> Invocation -> RequestBuilder, such a usage scenario is
> completely artificial. Sure, the API officially does not break between Invocation and RequestBuilder, but it doesn't
> seem to be a real issue as I doubt that anyone would ever want to use it that way.
>
Possibly - but our API offers this option and we promote anexample for it.

>>
>> Just drop asRequestBuilder, add accept(), and IMHO we will have a reasonable API IMHO... With generic batch Invocation
>> handlers seeing again only submit() or invoke() - there's no way generic handlers can do something reasonable with
>> asRequestBuilder either
>>
>> I can do target.request().header("Accept", "text/xml").get();
>>
>> right now without bothering with asRequestBuilder().
>
> As I tried to explain above, the whole point of Invocation API is to provide extra "filter/handler-like" flexibility if
> needed, not to substitute the header building functionality of the Invocation.Builder.
>
> Btw. since we are on this mail thread, do you have any comments regarding the configuration API updates? :)
>

That is much nicer now...

Sergey

> Marek
>
>>
>> Cheers, Sergey
>>
>>>> Can we have accept() on Invocation.Builder ?
>>>
>>> There's no reason for that. Target.request(acceptedMediaTypes) serves the same purpose in the main (anticipated) API
>>> flow. If you remember, we discussed in the past that combining request().accept(types) into single request(types) will
>>> help us retain SoC between Target and I.B in the API while reducing the number of method invocations to a minimum at the
>>> same.
>>>
>>> Cheers,
>>> Marek
>>>
>>>>
>>>> Cheers, Sergey
>>>>
>>>>
>>>>
>>>> On 20/09/11 15:30, Marek Potociar wrote:
>>>>> Hello experts,
>>>>>
>>>>> Please review the proposed change I have just commited:
>>>>> http://java.net/projects/jax-rs-spec/sources/git/revision/f48ae3d9a98a6194df3abe708347083bf78afc91
>>>>>
>>>>> The change branches off the configuration flow of client-side artifacts from the main request-to-response execution
>>>>> flow. From my recent discussion with Sergey on this list as well as from further feedback I have received privately it
>>>>> seems the mixed mutable and immutable methods on Targets may indeed cause some user confusion.
>>>>>
>>>>> To reduce the confusion we decided to make Target "dirty" immutable - Target will not expose any methods that would
>>>>> return same Target instance, however Target's configuration state can be indirectly modified (in a separate method
>>>>> flow)
>>>>> by accessing its Configuration instance and invoking mutators on that instance. Configuration interface is a
>>>>> rename/replacement for former Configurable<T> interface. Please, look at the samples, they should be rather
>>>>> self-explanatory.
>>>>>
>>>>> We decided to use this solution because it allows for keeping the Target immutable wrt. URI which helps us preserve
>>>>> URI-based identity contract (equals/hashCode) and makes it possible to use Target instances safely with e.g. Java
>>>>> collection classes etc.
>>>>>
>>>>> Kind regards,
>>>>> Marek
>>>>>
>>>>>
>>>>
>>>>
>>
>>


-- 
Sergey Beryozkin
http://sberyozkin.blogspot.com
Talend - http://www.talend.com