16 character string converted to UUID

[sh41]

Hi, Thanks again for this project!

There seems to be something not quite right around converting parameters to UUIDs. All 16 character strings seem to get converted into UUIDs.

Running the SQL in the log doesn't result in the same output as the query in Ecto.

I've had a look at the code and I'm not sure what can be done about this. By the time dev_logger sees the parameter there is no longer any information to help it to understand the schema, so it's making the assumption that all 128 bit bitstrings are UUIDs, but that also matches 16 character strings.

[sh41]

And just to add another data point, it appears the same in Postgres:

iex(2)> Ecto.Adapters.SQL.query!(MyRepo, "SELECT $1::text" , ["abcdefghijklmnop"])

07:52:14.988 [debug] QUERY OK db=0.9ms decode=1.6ms
SELECT '61626364-6566-6768-696a-6b6c6d6e6f70'::text
%Postgrex.Result{
  columns: ["text"],
  command: :select,
  connection_id: 4598,
  messages: [],
  num_rows: 1,
  rows: [["abcdefghijklmnop"]]
}
iex(3)>

[fuelen]

It just happens that such binaries are valid UUID as well. We simply try to guess the Ecto type by Elixir type, as we don't have other metadata in telemetry. Such approach doesn't work well for all scenarios.

I assume, the issue will be resolved when https://github.com/elixir-ecto/ecto/pull/3869 is done, so we wouldn't have to guess uuids in this library.

[sh41]

Thanks @fuelen. That makes sense. 👍

[fuelen]

ecto_sql now has cast_params parameter in telemetry (starting from v3.9.2) which contains original values. In general, it is possible to combine params and cast_params somehow to understand that parameter in params is UUID, but I'm not sure if it is worth it, as this would require new API for Ecto.DevLogger.PrintableParameter protocol and thus major version bump.

[maxsalven]

Just an FYI, I ran into this today when using the string "America/New_York", a common time zone identifier.

[halostatue]

ecto_sql now has cast_params parameter in telemetry (starting from v3.9.2) which contains original values. In general, it is possible to combine params and cast_params somehow to understand that parameter in params is UUID, but I'm not sure if it is worth it, as this would require new API for Ecto.DevLogger.PrintableParameter protocol and thus major version bump.

I just started playing with Ecto.DevLogger again and went through the issues and this paragraph has been mildly bugging me. This project is still in the 0.x.y phase of semantic versioning, in which case the x versions may signify breaking API changes. A stable API is not promised for 0.x.y versions, only for >= 1.0.0.

As I also store time zone strings in the database, I think it would be worthwhile seeing this improvement to prevent the America/New_York issue mentioned above.

[fuelen]

I wanted to make a major change and to initiate changes in ecto, so we can do precise logging https://groups.google.com/g/elixir-ecto/c/5AwUoaEuq4E/m/J9Kbr9FOBAAJ?pli=1 but seems like this won't be possible, only guessing. About the particular issue, I think we can relatively easy fix UUIDs with the hack without making breaking changes - move converting uuid to readable string outside of Ecto.DevLogger.PrintableParameter, where we can compare values from params and cast_params. It is not a nice solution, but in general I see that Ecto.DevLogger.PrintableParameter abstraction is leaked.

[dkuku]

with the function introduced in #33 it's a matter of extending the preprocess_params with new pattern matches, then the maybe_convert_binary_uuid_to_string(binary) can be removed:

[[hex | _], [uuid | _] = uuids] when byte_size(hex) == 16 and byte_size(uuid) == 36 -> uuids
[hex, uuid] when byte_size(hex) == 16 and byte_size(uuid) == 36 -> uuid