On 07/09/12 14:26, Marek Potociar wrote:
>
> On Sep 6, 2012, at 10:05 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>
>> On 06/09/12 18:24, Marek Potociar wrote:
>>>
>>> On Sep 6, 2012, at 6:10 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>>>
>>>> On 06/09/12 15:09, Marek Potociar wrote:
>>>>> Hello experts,
>>>>>
>>>>> I know it's a long email, but please bear with me. I'd like to propose
>>>>> some changes to the WebTarget and UriBuilder API. Following email
>>>>> provides the rationale as well as the proposal.
>>>>>
>>>>> Currently we have following methods in the WebTarget:
>>>>>
>>>>> pathParam(String name, Object value)
>>>>> pathParams(Map<String, Object> parameters)
>>>>>
>>>>> Their purpose is to set values to path template variables. E.g.:
>>>>>
>>>>> client.target("{a}/{b}").pathParam("a", "x") --> "x/{b}"
>>>>>
>>>>> Also, we have a couple of requests in Jira to provide similar
>>>>> functionality in UriBuilder as well as in general resolve the
>>>>> inconsistencies between WebTarget and UriBuilder URI building API:
>>>>>
>>>>> http://java.net/jira/browse/JAX_RS_SPEC-211
>>>>> http://java.net/jira/browse/JAX_RS_SPEC-163
>>>>>
>>>>> Also there is a Jersey issue filed around inconsistencies/confusion of
>>>>> what WebTarget.getUriBuilder() should return:
>>>>>
>>>>> http://java.net/jira/browse/JERSEY-1329
>>>>>
>>>>> So while looking at the WebTarget/UriBuilder consolidation from the
>>>>> larger perspective provided by all these issues, I'd like to propose
>>>>> some changes in these APIs.
>>>>>
>>>>> First of all,WebTarget.pathParam(s) are misleading method names, since
>>>>> the actual intended purpose of the methods was to resolve a template
>>>>> variable to a value. IOW the expected behavior is:
>>>>>
>>>>> client.target("{a}").path("{b}").queryParam("a", "{a}").pathParam("a",
>>>>> "x") --> "x/{b}?a=x"
>>>>>
>>>>> Now I think you're with me, when I say that the name of the pathParam
>>>>> method is misleading. Another issue, perhaps even more interesting is
>>>>> finding the right answer for a question on what should
>>>>> WebTarget.getUriBuilder() return - how should the returned UriBuilder be
>>>>> initialized. For example:
>>>>>
>>>>> client.target("{a}/{b}").pathParam("a", "x").getUriBuilder()
>>>>>
>>>>> In this case a few answers are possible for the internal state of the
>>>>> returned UriBuilder:
>>>>>
>>>>> 1. URI template: "{a}/{b}", internal template values: N/A
>>>>> 2. URI template: "{a}/{b}", internal template values: "a" -> "x"
>>>>> 3. URI template: "x/{b}", internal template values: N/A
>>>>>
>>>>>
>>>>> Now after a careful consideration, I'd argue that we should take
>>>>> approach that corresponds to the answer #3 (even though it may not seem
>>>>> to be the most intuitive answer). IOW, I could not find any practical
>>>>> use case that would support the other 2 options and could not be
>>>>> solvable by 3, but I could find use cases that would only be solvable by
>>>>> #3. One other indication might be that it's hard to find a use case
>>>>> where following makes sense and could be considered as a good practice:
>>>>>
>>>>> client.target("{a}/{b}").pathParam("a", "x")...pathParam("a", "y")
>>>>>
>>>>> So my conclusion is that typically you only set a template value only
>>>>> once and once you set it you do want to overwrite it later. E.g.:
>>>>>
>>>>> authors = client.target("bookstore/authors/{name}");
>>>>> author = authors.pathParam("name", "Dan Ariely");
>>>>>
>>>>> ... /* somewhere in a different part of app */ ...
>>>>>
>>>>> books = author.path("books/{name}");
>>>>> book = books.pathParam("name", "Predictably Irrational");
>>>>>
>>>>> In the code above the issue with overwriting template values set before
>>>>> is obvious. You certainly do not want to overwrite the author's name
>>>>> with the name of the book.
>>>>>
>>>>> So finally, here comes my proposal:
>>>>>
>>>>> * remove WebTarget.pathParam(s) methods
>>>>> * introduce new WebTarget.resolveTemplate(s) methods, including method
>>>>> versions that provide flag for "/" encoding in path
>>>>> * introduce new UriBuilder.resolveTemplate(s) methods, including
>>>>> method versions that provide flag for "/" encoding in path
>>>>>
>>>>>
>>>>> The new resolveTemplate(s) methods would *irreversibly* resolve the
>>>>> template values in WebTarget or UriBuilder for given template variables,
>>>>> e.g.:
>>>>>
>>>>> client.target("{a}/{b}").resolveTemplate("a", "x").resolveTemplate("a",
>>>>> "y") --> "x/{b}"
>>>>> // resolveTemplate("a", "y") above is either ignored or throws
>>>>> IllegalStateException
>>>>>
>>>>
>>>> Can having 'irreversible' effect help users who work directly with UriBuilder ?
>>>
>>> Yes. Similar example as above could apply to UriBuilder as well.
>>>
>>>> Can that be introduced only at WebTarget level ?
>>>
>>> That would be inconsistent. What would be the benefit of not introducing the methods in UriBuilder? See the issues filed in Jira that ask for them.
>>>
>>
>> I do not see the benefit of introducing it in UriBuilder. It has so many methods and they can do all the replacing as needed.
>
> No, they cannot. You are not e.g. able to resolve just some URI templates and return the resulting URI template. If you use UriBuilder.build(), the unresolved templates will be encoded into the URI.
>
> Also see http://java.net/jira/browse/JAX_RS_SPEC-163.
>
>>
>> By the way,
>>
>> http://java.net/jira/browse/JERSEY-1329
>>
>> has nothing to do with these proposed methods - this is an issue of the wrongly initialized UriBuilder.
>
> It does - for one, these methods on UriBuilder will let you more easily implement the getUriBuilder() support. But that was not my main point. The issue helped me to realize the inconsistencies in these 2 APIs.
>
>> You are asking what would be the benefit of not introducing them in UriBuilder ? Keeping the complex UriBuilder interface to the existing 'minimum', avoiding adding 4+ methods which are assumed without serious justification, are required.
>
> In such a large API and with current IDEs that help you do code completion on your every step, that reason sounds a bit weak compared to the fact that users are asking for these methods and
>
And the users have to read the docs first in order to understand what
UriBuilder is. Sometimes I think only two people in the world know how
it works :-), I'd like to believe I do too, but having the interface
expanded further just does not excite me at all :-)
I'm sorry but I haven't seen a single user issue where a request to
enhance UriBuilder to support partial resolutions is submitted.
I can imagine why one may need to do it at WebTarget level.
But why at UriBuilder level ? I'm sure we can imagine why, but
practically, no idea.
So then we'd need to add IllegalStateExceptions to other build methods,
in case some of the vars have already been resolved, etc...
>>
>> By the way, agree with Markus. 2 days before PR, no time to properly discuss or have any reasonable analysis...
>
> I know it's a short time, I apologize. Yet, with the quite extensive analysis of this relatively narrow-scoped problem in my previous email and after answering your initial questions, do you really feel that we need to spend more time analyzing and discussing this?
>
> See, the thing with the PR release is that I'll need to do a versioned JAX-RS API release which I will need to quickly uptake in RI. So if possible, I'd like to make sure this relatively minor change can get in before the release so that I won't have to do respins later on which will already have to follow a more rigorous PR process. But I understand that you may feel pressed by the short dates, so if you or anyone else in this EG really feel uncomfortable about this, I am fine with giving this discussion more time.
>
I'm not going to say that we need the extra time specifically for this
issue to be sorted out.
IMHO we need to be very very conservative with regard to UriBuilder any
further, but it is only me
Sergey
> Marek
>
>>
>> Sergey
>>
>>> Marek
>>>
>>>>
>>>> Thanks, Sergey
>>>>
>>>>> This proposal should make things consistent. Please provide your
>>>>> feedback ASAP. I'd like to make this change before the PR release
>>>>> planned for next Monday.
>>>>>
>>>>> Thank you,
>>>>> Marek
>>>>>
>>>
>