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. > Cheers > > Oleg > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
