Re: URIBuilder setQuery() method inconsistent with other set methods

[Oleg Kalnichevski <ol...@apache.org>]

On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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

Oleg


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

Re: URIBuilder setQuery() method inconsistent with other set methods

[sebb <se...@gmail.com>]

On 9 July 2012 14:13, Oleg Kalnichevski <ol...@apache.org> wrote:
> On Mon, 2012-07-09 at 00:10 +0100, sebb wrote:
>> On 8 July 2012 21:31, Oleg Kalnichevski <ol...@apache.org> wrote:
>> > On Fri, 2012-07-06 at 18:25 +0100, sebb wrote:
>> >> On 4 July 2012 10:18, Oleg Kalnichevski <ol...@apache.org> wrote:
>> >> > On Tue, 2012-07-03 at 16:39 +0100, sebb wrote:
>> >> >> On 3 July 2012 16:04, Oleg Kalnichevski <ol...@apache.org> wrote:
>> >> >> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
>> >> >> >> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
>> >> >> >> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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.
>>
>
> Please review
>
> http://svn.apache.org/viewvc?rev=1359140&view=rev

That looks fine.

> 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

Re: URIBuilder setQuery() method inconsistent with other set methods

[Oleg Kalnichevski <ol...@apache.org>]

On Mon, 2012-07-09 at 00:10 +0100, sebb wrote:
> On 8 July 2012 21:31, Oleg Kalnichevski <ol...@apache.org> wrote:
> > On Fri, 2012-07-06 at 18:25 +0100, sebb wrote:
> >> On 4 July 2012 10:18, Oleg Kalnichevski <ol...@apache.org> wrote:
> >> > On Tue, 2012-07-03 at 16:39 +0100, sebb wrote:
> >> >> On 3 July 2012 16:04, Oleg Kalnichevski <ol...@apache.org> wrote:
> >> >> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
> >> >> >> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
> >> >> >> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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.
> 

Please review

http://svn.apache.org/viewvc?rev=1359140&view=rev

Oleg



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

Re: URIBuilder setQuery() method inconsistent with other set methods

[sebb <se...@gmail.com>]

On 8 July 2012 21:31, Oleg Kalnichevski <ol...@apache.org> wrote:
> On Fri, 2012-07-06 at 18:25 +0100, sebb wrote:
>> On 4 July 2012 10:18, Oleg Kalnichevski <ol...@apache.org> wrote:
>> > On Tue, 2012-07-03 at 16:39 +0100, sebb wrote:
>> >> On 3 July 2012 16:04, Oleg Kalnichevski <ol...@apache.org> wrote:
>> >> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
>> >> >> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
>> >> >> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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

Re: URIBuilder setQuery() method inconsistent with other set methods

[Oleg Kalnichevski <ol...@apache.org>]

On Fri, 2012-07-06 at 18:25 +0100, sebb wrote:
> On 4 July 2012 10:18, Oleg Kalnichevski <ol...@apache.org> wrote:
> > On Tue, 2012-07-03 at 16:39 +0100, sebb wrote:
> >> On 3 July 2012 16:04, Oleg Kalnichevski <ol...@apache.org> wrote:
> >> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
> >> >> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
> >> >> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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.
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.

Cheers

Oleg 



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

Re: URIBuilder setQuery() method inconsistent with other set methods

[sebb <se...@gmail.com>]

On 4 July 2012 10:18, Oleg Kalnichevski <ol...@apache.org> wrote:
> On Tue, 2012-07-03 at 16:39 +0100, sebb wrote:
>> On 3 July 2012 16:04, Oleg Kalnichevski <ol...@apache.org> wrote:
>> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
>> >> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
>> >> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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 '+'.

> 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

Re: URIBuilder setQuery() method inconsistent with other set methods

[Oleg Kalnichevski <ol...@apache.org>]

On Tue, 2012-07-03 at 16:39 +0100, sebb wrote:
> On 3 July 2012 16:04, Oleg Kalnichevski <ol...@apache.org> wrote:
> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
> >> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
> >> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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'?

Oleg



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

Re: URIBuilder setQuery() method inconsistent with other set methods

[sebb <se...@gmail.com>]

On 3 July 2012 16:04, Oleg Kalnichevski <ol...@apache.org> wrote:
> On Tue, 2012-06-26 at 17:55 +0100, sebb wrote:
>> On 26 June 2012 14:33, sebb <se...@gmail.com> wrote:
>> > On 26 June 2012 13:43, Oleg Kalnichevski <ol...@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.

> 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