On Mon, 2012-07-09 at 00:10 +0100, sebb wrote: > On 8 July 2012 21:31, Oleg Kalnichevski <[email protected]> wrote: > > On Fri, 2012-07-06 at 18:25 +0100, sebb wrote: > >> On 4 July 2012 10:18, Oleg Kalnichevski <[email protected]> wrote: > >> > On Tue, 2012-07-03 at 16:39 +0100, sebb wrote: > >> >> On 3 July 2012 16:04, Oleg Kalnichevski <[email protected]> wrote: > >> >> > On Tue, 2012-06-26 at 17:55 +0100, sebb wrote: > >> >> >> On 26 June 2012 14:33, sebb <[email protected]> wrote: > >> >> >> > On 26 June 2012 13:43, Oleg Kalnichevski <[email protected]> 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: [email protected] For additional commands, e-mail: [email protected]
