Mailing-List: contact dev-help@hc.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "HttpComponents Project" <dev@hc.apache.org>
Received-SPF: neutral (athena.apache.org: local policy)
Message-ID: <1341839598.11765.8.camel@ubuntu>
Subject: Re: URIBuilder setQuery() method inconsistent with other set methods
From: Oleg Kalnichevski <olegk@apache.org>
To: HttpComponents Project <dev@hc.apache.org>
Date: Mon, 09 Jul 2012 15:13:18 +0200
In-Reply-To: 
 <CAOGo0VYaGDP-VDWHZ_=GJo3rb+OamGhenVHWjUgS6vo9-LtDcQ@mail.gmail.com>
References: 
	<CAOGo0VZC=3FnQaS-ipBbhQYCi7ZZLLJ771qKQBeOT0y=jrOwqg@mail.gmail.com>
	 <1340714618.5475.24.camel@ubuntu>
	 <CAOGo0VbXdT6L-MsqcN60i1sE2BsWYHt02qqNkHtUcBEcke_b_A@mail.gmail.com>
	 <CAOGo0VYwBcL=Cn21vZYBgpFRod70hxVUPQrhmxgzcUFa5VBOQw@mail.gmail.com>
	 <1341327898.5804.18.camel@ubuntu>
	 <CAOGo0VZ02pqXwxcosYfQDU_GwbduS6Cmh9XW5NQt06285kDFFw@mail.gmail.com>
	 <1341393528.5804.21.camel@ubuntu>
	 <CAOGo0VaP0TNUE0NccAYj1d2QdspW21PWURmWKjKter9BCX4exA@mail.gmail.com>
	 <1341779466.4959.66.camel@ubuntu>
	 <CAOGo0VYaGDP-VDWHZ_=GJo3rb+OamGhenVHWjUgS6vo9-LtDcQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0

On Mon, 2012-07-09 at 00:10 +0100, sebb wrote:
> 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.
> 

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