#555 Uri.plusQuery bug?

cheeser Tue 28 Apr 2009

OK. This one I think is a bug but perhaps someone might correct me. With plusQuery(), I could do something like

uri = uri.plusQuery(["param": value == null ?  "" : value])
uri = uri.plusQuery(["param2": value2 == null ?  "" : value2])

and that will generate something like

http://www.blah.com?param=&param2=foo

if value should be null. Shouldn't it skip the param parameter if its value is null? At least in twitter's case, it actually produces different results. Maybe it's just that twitter is really that dumb about interpreting RESTful urls (I blame ruby!) but that URL just seems "wrong" to me. So for now I have to wrap the call to plusQuery() in an if rather than using the ternary operator. So which is way is "correct?" I'm sure a case could be made either way...

JohnDG Tue 28 Apr 2009

Roll your own:

URI plusQuery(URI uri, Map map) {
   newMap := [String:String]

   map.each |v,k| { if (v != null && v != "") newMap.put(k, v) }

   return uri.plusQuery(newMap)
}

tactics Tue 28 Apr 2009

I'm not sure what the issue is here.

The tertiary operator expression is evaluated first. If value is null, then value ? "" : value evaluates to the empty string and plusQuery never sees a null at all.

What problem is the Twitter API causing you? Something like param=&param2=foo is a legitimate query string. If the Twitter API is expecting param to be completely omitted, then do as JohnDG suggests. Alternatively, I believe you could also just do:

uri = uri.plusQuery(["param": value])
uri = uri.plusQuery(["param2": value2])

Without the tertiary operator. In Fan, null is returned from a Map whenever a lookup fails, so to the API, ["param":null] is the same as [:] (the empty map).

Lastly, I want to mention Fan has support for what we affectionately refer to as the Elvis Operator ?: (Look at it like an emoticon and it looks a little like Elvis). These expressions are equivalent:

x == null ? y : x
x ?: y

The second is a little easier to read.

cheeser Tue 28 Apr 2009

Yes. I've rolled my own method as I've already mentioned. It works but it seems it should be unnecessary...

@tactics Just passing a map with null values results in an NPE when the java code tries to call toString() on the value Obj. Using the ternary/elvis operator still results in those empty parameters being sent which twitter trips over. I guess ultimately the problem probably rests with the twitter back end. Sending empty parameters might be valid but, imo, is semantically meaningless. I'd think not sending those params at all would be "better" but that'd probably break someone else's code.

Oh, well. I have a workaround. I'll just curse the twitter devs and move on I suppose.

andy Tue 28 Apr 2009

Technically speaking the query string is opaque, and how Twitter chooses to implement it is up to itself. In our case though, HTML form encodings were the intention of the plusQuery method. So I believe our implementation is correct, as a zero-length value is a valid form value.

tactics Tue 28 Apr 2009

If this is a common case (and it just might be), it might make sense to extend plusQuery with an optional parameter:

Uri plusQuery([Str:Str]? query, Bool hideEmpty := false)

Where when hideEmpty is true, keys mapping to empty strings are omitted.

Looking at the API, for whatever reason, query is a nullable map. If query is null, plusQuery is just the identity function. So you could do this as well:

uri = uri.plusQuery(value  ? ["param": value]   : null)
uri = uri.plusQuery(value2 ? ["param2": value2] : null])

cheeser Tue 28 Apr 2009

Fair enough. I can deal with that. Just had to make sure I wasn't off. The coffee this morning was doing strange things to my head so I thought I'd double check. :)

brian Tue 28 Apr 2009

Sounds like this has been hashed out, but I'll inject a couple of comments:

The plusQuery method takes a Str:Str, not a Str:Str?, so the type signature is an indication that nullable types are not allowed (hence the NullErr). Technically we might check all the values on the auto-cast from Str:Str? to Str:Str, but for obvious performance reasons I don't actually do that.

The reason I allow the whole map to be null, is because that case comes up all the time when using view queries in flux and webapp. So I decided to just let you pass null for the whole map.

The most efficient way to build up a query is to build up the whole map, then call plusQuery. I try to "nudge" you in the direction by taking a map versus just a single name/value:

q := Str:Str?[:]
// build up map
q = q.exclude |v| { v == null } // strip out null values
uri := `http://base/`.plusQuery(q)

Login or Signup to reply.