hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: URIBuilder setQuery() method inconsistent with other set methods
Date Sun, 08 Jul 2012 23:10:21 GMT
On 8 July 2012 21:31, Oleg Kalnichevski <olegk@apache.org> wrote:
> On Fri, 2012-07-06 at 18:25 +0100, sebb wrote:
>> On 4 July 2012 10:18, Oleg Kalnichevski <olegk@apache.org> wrote:
>> > On Tue, 2012-07-03 at 16:39 +0100, sebb wrote:
>> >> On 3 July 2012 16:04, Oleg Kalnichevski <olegk@apache.org> wrote:
>> >> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
>> >> >> On 26 June 2012 14:33, sebb <sebbaz@gmail.com> wrote:
>> >> >> > On 26 June 2012 13:43, Oleg Kalnichevski <olegk@apache.org>
wrote:
>> >> >> >> On Tue, 2012-06-26 at 12:13 +0100, sebb wrote:
>> >> >> >>> The setQuery(String) method currently expects an escaped,
ASCII-only
>> >> >> >>> string - basically encoded form data - whereas all
the other set
>> >> >> >>> methods expect unescaped input.
>> >> >> >>> This is a bit confusing.
>> >> >> >>>
>> >> >> >>
>> >> >> >> Yes, I am aware of it. I was going to deprecate this method
in 4.3 and
>> >> >> >> replace it with something like #parseQuery.
>> >> >> >>
>> >> >> >>> AFAIK, a query string does not have to consist of
name value pairs
>> >> >> >>> (i.e. form data), it can be any arbitrary string.
>> >> >> >>> There's currently no way for the end-user to provide
such a query.
>> >> >> >>> They would have to use the URI class.
>> >> >> >>
>> >> >> >> In this case it is basically a lone parameter without
a value.
>> >> >> >
>> >> >> > Yes and no; depends what is allowed for a parameter name.
>> >> >>
>> >> >> The URI class allows spaces in the query segment.
>> >> >> AFAICT this is currently impossible to replicate using URIBuilder,
>> >> >> because it converts space to '+'.
>> >> >>
>> >> >> > Also, '+' only means space in the context of form data; otherwise
it
>> >> >> > is just another safe character in the query.
>> >> >> >
>> >> >> > Presumably applications that use arbitrary query strings decode
the
>> >> >> > query differently from ones that expect form data.
>> >> >> >
>> >> >> >>> Maybe that is OK, in which case we just need to clarify
the URIBuilder
>> >> >> >>> Javadoc to state that it is only intended for use
with form data
>> >> >> >>> queries.
>> >> >> >>>
>> >> >> >>> Perhaps there should be a setQuery(NVP) method which
handles form data directly?
>> >> >> >>>
>> >> >> >>
>> >> >> >> Makes sense. The question is whether we should add this
method in 4.2.1
>> >> >> >> or wait until 4.3.
>> >> >> >
>> >> >> > It could wait.
>> >> >> >
>> >> >> > As could providing support for non-form data query strings;
the URI
>> >> >> > class can be used meanwhile.
>> >> >>
>> >> >
>> >> > Sebastian
>> >> >
>> >> > I deprecated #setQuery method and added new method to set arbitrary
>> >> > custom query component. Please review.
>> >> >
>> >> > http://svn.apache.org/viewvc?rev=1356775&view=rev
>> >>
>> >> Mostly OK.
>> >>
>> >> I'm not quite sure why query and queryParams are both used to build
>> >> the same URI query
>> >> [Equally, why setCustomQuery does not set queryParams=null.]
>> >>
>> >> Is there a use case that requires both a custom query and query params?
>> >> If so, then there should be a test case for it.
>> >>
>> >
>> > URI uri = new URIBuilder()
>> >  .setCustomQuery("this")
>> >  .addParameter("that", null).build();
>> > System.out.println(uri);
>> >
>> > What do you see as an expected result of this operation?
>> > '?this&that' or '?that'?
>>
>> I'm not sure the sequence makes sense, so perhaps the code should
>> throw a runtime error.
>>
>> A custom query string is not form data, so space should be encoded as
>> '%20' rather than '+'.
>>
>
> I do not know if it makes any sense or not but it is certainly legal.

Well yes, the result of encoding form data has to be valid as a query string.

> Throwing a runtime exception does not sound quite right to me but I can
> make custom query and form parameters mutually exclusive if you think
> that makes more sense.

Yes, I think they should be mutually exclusive.
Does not have to throw an error so long as the Javadoc makes the
behaviour clear.

I think it will just be confusing to allow them both, and I cannot see
any use case that requires that functionality.
Also more tests would be needed to ensure that the additional combinations work.

> Cheers
>
> Oleg
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org


Mime
View raw message