Hi,
On 26/09/11 11:30, Marek Potociar wrote:
>
>
> On 09/25/2011 07:00 PM, Sergey Beryozkin wrote:
>> Hi Marek
>>
>> here are some comments:
>>
>> - a form(...) entity qualifier in the example showing post(form(new Form(...)))
>> seems redundant given that Form is a dedicated class capturing a form submission
>
> Form is a concept used both on the server as well as the client side. As such it cannot extend client-side Entity. One
> solution would be to create a special overload for post() to accept Form directly. But I'm not sure it's worth the
> effort. Perhaps renaming Entity.form(Form) to Entity.entity(Form) (or Entity.encoded(Form) etc.) would do the trick?
yea, may be lets leave it the way it is
>
>>
>> - 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
> 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().
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().
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