elixir-ecto / myxql

MySQL 5.5+ driver for Elixir
Apache License 2.0
271 stars 66 forks source link

BUG: Can't decode FLOAT/DOUBLE in TextQuery when the fractional portion of the number is 0 #180

Closed benkimpel closed 4 months ago

benkimpel commented 4 months ago

Can't decode FLOAT/DOUBLE in TextQuery when the fractional portion of the number is 0.

SQL Setup

CREATE TABLE example (id INT, value DOUBLE);

INSERT INTO example
VALUES (1, 100.123),
       (2, 100.0)
;

Query Code

{:ok, conn} = MyXQL.start_link(...)

# All good
MyXQL.query!(conn, "SELECT * FROM example WHERE id = 1;", [], query_type: :text)

# Fails
MyXQL.query!(conn, "SELECT * FROM example WHERE id = 2;", [], query_type: :text)

# ** (ArgumentError) errors were found at the given arguments:
#   * 1st argument: not a textual representation of a float
#
# ...snip...
#
# :erlang.binary_to_float("100")
# (myxql 0.6.4) lib/myxql/protocol/values.ex:132: MyXQL.Protocol.Values.decode_text_value/2

Test Case

Here's a test case that reproduces the issue:

# test/myxql/protocol/values_test.exs

# ...

      test "MYSQL_TYPE_FLOAT", c do
        assert Float.round(insert_and_get(c, "my_float", 10.0), 2) == 10.0 # <== HERE
        assert Float.round(insert_and_get(c, "my_float", -13.37), 2) == -13.37
        assert Float.round(insert_and_get(c, "my_float", 13.37), 2) == 13.37

        assert Float.round(insert_and_get(c, "my_unsigned_float", 13.37), 2) == 13.37
      end

# ...

Potential Fix

# lib/myxql/protocol/values.ex

  def decode_text_value(value, type) when type in [:float, :double] do
    value = if String.contains(value, "."), do: value, else: "#{value}.0"
    String.to_float(value)
  end

I can open a pull request if you want.

benkimpel commented 4 months ago

I imagine this isn't a bigger problem because it doesn't seem to effect prepared statements and it seems like that is mostly (exclusively?) what Ecto does.

greg-rychlewski commented 4 months ago

I imagine this isn't a bigger problem because it doesn't seem to effect prepared statements and it seems like that is mostly (exclusively?) what Ecto does.

That's correct.

def decode_text_value(value, type) when type in [:float, :double] do
  value = if String.contains(value, "."), do: value, else: "#{value}.0"
  String.to_float(value)
end

Maybe Float.parse instead...it seems to take care of it automatically https://hexdocs.pm/elixir/1.11.0/Float.html#parse/1

benkimpel commented 4 months ago

Maybe Float.parse instead...it seems to take care of it automatically https://hexdocs.pm/elixir/1.11.0/Float.html#parse/1

Yeah, Float.parse could work. I'm not sure how you'd prefer to handle errors in these functions so that would definitely be a factor in the fix.

case Float.parse(value) do
  {value, _rest} -> value
  :error -> ?
end
greg-rychlewski commented 4 months ago

I think raising so it matches the old behaviour. But we can put a better message. Something that says we are trying to decode this value into a float and it's not a float.

We should also probably raise when rest is not empty string. That would be more in line with the old behaviour. i.e. String.to_float("123.xtz") raises

benkimpel commented 4 months ago

I think raising so it matches the old behaviour. But we can put a better message. Something that says we are trying to decode this value into a float and it's not a float.

We should also probably raise when rest is not empty string. That would be more in line with the old behaviour. i.e. String.to_float("123.xtz") raises

I like that. We can raise an ArgumentError (what it previously raised) with a better message.

case Float.parse(value) do
  {value, ""} -> value
  _ -> raise ArgumentError, "cannot parse FLOAT/DOUBLE"
end
greg-rychlewski commented 4 months ago

perfect !

benkimpel commented 4 months ago

Here's a PR if you're interested #181.

Thanks for the quick response.