elixir-ecto / ecto

A toolkit for data mapping and language integrated query.
https://hexdocs.pm/ecto
Apache License 2.0
6.16k stars 1.43k forks source link

`field/3`'s `:writable` seems to ignore the `:insert` option #4524

Open lucavenir opened 4 hours ago

lucavenir commented 4 hours ago

Elixir version

1.16.2

Database and Version

PostgreSQL 16.1

Ecto Versions

3.12.4

Database Adapter and Versions (postgrex, myxql, etc)

postgrex 0.19.1

Current behavior

I wanted to apply this :writable option mentioned in field/3' documentation. Unluckily writable: insert seems to be ignored.

Given this schema:

defmodule MyApp.Accounts.Account do
  use Ecto.Schema
  import Ecto.Changeset

  schema "accounts" do
    field :firebase_uid, :string, writable: :insert
    field :deactivated_at, :utc_datetime

    timestamps(type: :utc_datetime)
  end

  @doc false
  def changeset(account, attrs) do
    account
    |> cast(attrs, [:firebase_uid, :deactivated_at])
    |> validate_required([:firebase_uid])
  end
end

This tests fails:

test "update_account/2 doesn't allow firebase_uid to change" do
  account = account_fixture()
  invalid_attrs = %{ firebase_uid: "another different uid" }

  # this wont work
  # assert {:error, %Ecto.Changeset{}} = Accounts.update_account(account, invalid_attrs)
  # instead, these pass (???)
  {:ok, account} = Accounts.update_account(account, invalid_attrs)
  assert account.firebase_uid == "another different uid"  # !!
end

Expected behavior

I'd expect the previous test to have the commented parts to pass, and the uncommented parts to fail, i.e. I'd expect this test to pass:

test "update_account/2 doesn't allow firebase_uid to change" do
  account = account_fixture()
  invalid_attrs = %{ firebase_uid: "another different uid" }

  assert {:error, %Ecto.Changeset{}} = Accounts.update_account(account, invalid_attrs)
  # or
  # {:ok, account} = Accounts.update_account(account, invalid_attrs)
  # assert account.firebase_uid != "another different uid" 
end

(meaning: I'm not sure if the :insert option ignores the write or returns an error, but surely it shouldn't allow an edit!)

greg-rychlewski commented 4 hours ago

Hi @lucavenir ,

Thank you for the report. I believe the update is silently not happening. We just filter the non-updatable fields out before performing the operation. Are you able to select from the database afterwards to see what the real value is? In the test I just did, this is the case.

Then I guess the question is should do something more than silently ignore.

lucavenir commented 4 hours ago

Hello @gentoid,

Silently ignoring is coherent with much of Ecto's behaviors AFAIK. So it's fine for me.

But in the commented test above, I check for the updated struct, and I find it to be updated even though it should silently ignore it. I tried that as well and the test fails.

So you're not able to reproduce this? If that's the case I'll try and reproduce this in a sandbox.

greg-rychlewski commented 3 hours ago

You might have to pull the data again with Repo.one/Repo.all, etc... instead of looking at just the return from the update.

If you are not using the returning option or you are using that option and nothing is updated, you will just see the changeset you proposed for update. Not necessarily what's in the database.