fuelen / ecto_dev_logger

An alternative logger for Ecto queries
Apache License 2.0
151 stars 16 forks source link

Invalid SQL generated for JSON fragment #21

Closed maxsalven closed 1 year ago

maxsalven commented 1 year ago
iex(2)> query = from m in MyApp.Model, where: fragment(~s(?#>'{field}' @> ?), m.details, ^[%{id: 1}])
#Ecto.Query<from m0 in MyApp.Model,
 where: fragment("?#>'{field}' @> ?", m0.details, ^[%{id: 1}])>

iex(3)> Repo.all(query)
[debug] QUERY OK source="" db=293.1ms decode=1.4ms
SELECT ... FROM "model" AS m0 WHERE (m0."details"#>'{field}' @> '{"{\"id\":1}"}')
ERROR:  invalid input syntax for type json
LINE 1: ...outs" AS w0 WHERE (w0."details"#>'{movements}' @> '{"{\"id\"...
                                                             ^
DETAIL:  Expected ":", but found "}".
CONTEXT:  JSON data, line 1: {"{\"id\":1}"}

Specifically, the array brackets have been turned into braces and the quotes were incorrectly escaped, the generated SQL should be: SELECT ... FROM "model" AS m0 WHERE (m0."details"#>'{field}' @> '[{"id":1}]')

fuelen commented 1 year ago

Thanks for the report.

as I pointed here https://github.com/fuelen/ecto_dev_logger/issues/20#issuecomment-1549156222, Ecto doesn't provide information about types in its telemetry events, so ecto_dev_logger guesses types and there is a chance to get invalid representation when type is ambiguous. In this case, [map()] is converted to an array of JSON elements rather than to a JSON directly.

Even if ecto sends info about type, it is not really possible to detect proper type in case of fragments.

This is a workaround

from m in "test", select: 1, where: fragment(~s(?#>'{field}' @> ? :: json), m.details, ^Jason.encode!([%{id: 1}]))
# SELECT TRUE FROM "test" AS t0 WHERE (t0."details"::jsonb#>'{field}' @> '[{"id":1}]' :: json)

but it's up to you whether to use it or not, as this is only for logging purpose

vanderhoop commented 8 months ago

@fuelen I'm running into a similar issue as the above, but specifically with an embeds_many. (I was alarmed to see my array of objects stored as an object of floating objects, and had to remember that I was using a custom SQL logger 😄)

I've only poked around a little bit, but it appears that source is now passed via ecto_sql telemetry as of v3.11.0. If you have the source for a query, you could, in theory, find the associated Ecto.Schema, and then introspect the schema to check if the embedded schema's cardinality is :one or :many, and represent the former as a JSON object and the latter as a JSON array.

In practice, I don't know if this is a good idea, but I wanted to put it out there to see if it inspired anything.

fuelen commented 8 months ago

@vanderhoop I'm afraid this won't work as well, as the schema can have multiple embedded schemas :)