On 09/26/2011 04:15 PM, Sergey Beryozkin wrote:
> 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
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.
>> 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.
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.
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.
>
> 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? :)
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
>>>>
>>>>
>>>
>>>
>
>