build queries containing boolean values #286

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@lanej
lanej commented Dec 15, 2011

fixes generated queries with empty values when value is a boolean

Rack::Utils.build_nested_query(:foo => true)

currently produces foo=
now produces foo=true

@lanej
lanej commented Dec 15, 2011

While I'm not thrilled with the solution, I believe its better behavior than just having empty values

@manveru manveru commented on the diff Dec 15, 2011
lib/rack/utils.rb
@@ -92,7 +92,11 @@ def normalize_params(params, name, v = nil)
return if k.empty?
if after == ""
- params[k] = v
+ if v == "false" || v == "true"
+ params[k] = eval(v)
@manveru
manveru Dec 15, 2011 Member

I'm not happy with having them automatically transformed, but even more is that eval hurting my eyes :)

case v
when /false/i
  params[k] = false
when /true/i
  params[k] = true
else
  params[k] = v
end

A bit more verbose, but way faster.

@lanej
lanej Dec 15, 2011

I can see the benefits of removing the eval statement but is case insensitive matching for the whole string appropriate? I don't thnk trUe and FALse were ever intended to get boiled down to booleans.

How about:

case v
when /[F|f]alse/
  params[k] = false
 when /[T|t]rue/
  params[k] = true
@raggi
raggi Dec 17, 2011 Member

Now we're getting into real opinion territory. If we want to restrict the format, then full uppercase, lowercase and camel case would seem to make sense. I'm not sure why we're invoking the regex engine here. Why don't we just use the case statement the whole way?

case v
when "true", "True", "TRUE"
  params[k] = true
when "false", "False", "FALSE"
  params[k] = false
else
  params[k] = v
end

Sadly, this probably optimizes better like so:

BOOLEAN_COERCIONS = {
  "true" => true,
  "True" => true,
  "TRUE" => true,
  "false" => false,
  "False" => false,
  "FALSE" => false
}

def coerce_param(v)
  BOOLEAN_COERCIONS[v] || v
end

Something like this could also be considered configurable, at least you don't have to alter the code to alter it's input and output sets.

@raggi
Member
raggi commented Dec 17, 2011

This could have knock on impacts to rails and other frameworks. As our most major consumers, I'd love to get some opinions first.

@manveru
Member
manveru commented Dec 17, 2011

After thinking it a bit more through, I'm opposing the patch, this is not something rack should be deciding.

@josh
Contributor
josh commented Dec 17, 2011

I'm -1.

This is more a general issue of passing any non-string to build nested query. Think the important thing is that the build and parse methods are inverse functions of each other.

@raggi raggi closed this Dec 17, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment