Ecto: Changeset.filters (Ecto.Changeset.optimistic_lock/3) doesn't work with nil fields

[umamaistempo]

Environment

• Elixir version (elixir -v): Elixir 1.4.2
• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): PostgreSQL 9.6
• Ecto version (mix deps): 2.2.0
• Database adapter and version (mix deps): postgrex 0.13
• Operating system: Arch Linux

Current behavior

Right now, if using Changeset.optimistic_lock on a nil field, it will always return an Ecto.StaleEntryError

Example code:

user = %User{updated_at: nil} |> Repo.insert!()

  User
  |> Repo.get!(user.id)
  |> Ecto.Changeset.change(%{})
  |> Ecto.Changeset.optimistic_lock(:updated_at, fn _ -> NaiveDateTime.utc_now() end)
  |> Repo.update!()

This causes an Ecto.StaleEntryError.
Inspecting the changeset filters key it returns %{updated_at: nil}
The update query is UPDATE "users" SET "updated_at" = $1 WHERE "updated_at" = $2 AND "id" = $3 [{{2017, 10, 13}, {19, 15, 57, 888374}}, nil, 10]

Based on the query, i'd assume the problem is with translating the nil value. In this case, for this to work, i'd assume that instead of binding and using the nil value, the IS NULL comparator would have to be used.

Expected behavior

When using optimistic_lock with a nil field, the update query would pass as long as the row in database is still null

[josevalim]

Good catch!

[qcam]

I can work on this if it has not been taken :).

[josevalim]

Yes please! Now that we support dynamic in Ecto.Query, I am thinking we can

allow any dynamic to be set as a filter.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

[qcam]

@josevalim can you please elaborate on that? What do you think would be the desired fix for this?

[josevalim]

@qcam apologies for the quick answer.

We can separate this issue in two. One is to fix this particular scenario and write a test for it. Another one is to convert the changeset.filters entry to keep a list of fragments returned by Ecto.Query.dynamic. This way, we can handle the nil logic directly in Ecto.Changeset and not elsewhere. But let's go with baby steps. So a simple fix for this bug is a great starting point!

[josevalim]

We have merged a fix but we should look into making the filters mechanism more generic now that we support dynamic in Ecto, for this reason, we will keep this open.

[qcam]

I can take dynamic filters as well :).

[josevalim]

@qcam beautiful! The idea here is to store multiple dynamic fragments in the changeset.filters and then we pass it to the query layer instead of passing keyword lists.

This complicates the update a bit, because now you need to build a query from the dynamic parts during update and then prepare it using the Ecto.Query.Planner API.

[michalmuskala]

Do we still plan on tackling this one?

[josevalim]

Yes!!!!

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

[qcam]

@josevalim Sorry I had some personal affairs last 2 months so didn't have much progress on this issue. Would it still be ok for me to continue?

[josevalim]

Yes, please.

[qcam]

@josevalim @michalmuskala I think I'm able to build up dynamic parts for changeset.filters and pass it through the SQL adapter. I have a WIP commit at https://github.com/qcam/ecto/commit/bf4b7eb9f452fed3a55ed3b3feea92a857c5b59c.

Anyway I'm stuck on the second part of José's earlier comment. Could you please point me what/where would be the ideal way/place to transform these dynamics to raw SQL? Thank you.

[josevalim]

@qcam the next step is to pass this information to the planner, that will prepare and normalize the query before we send it down to adapters.

[josevalim]

I have removed the "Bug" tag, as the bug has been fixed, and we are now working on improving the feature.

[qcam]

I'm sorry for dragging this issue on. I think I don't have time to continue with this so would like to release it for future takers. Many thanks to @josevalim for all the helps so far ❤️.

[soteras]

Something can i do to help with this issue?

[josevalim]

So I gave this issue a try and I got the basics working but I decided to not follow through with it because it means we are mixing the schema API with the query API, and it is probably best to keep them decoupled. For this reason, we won't be pursuing the improvements (the bug is fixed though). Thanks for PRs @qcam and sorry for the false positive.