Set writable: :insert for inserted_at field in timestamps() macro

[green-david]

This is a follow-up to #4734.

When actually configuring on_writable_violation in my app, I noticed that it wasn't possible to set on inserted_at due to it being behind the timestamps() macro. This seems like a textbook case for writable: :insert, which this PR facilitates.

[green-david]

As it is currently, the only way to actually change the on_writable_violation behavior of inserted_at is via the module-wide @on_writable_violation attribute. There is prior-art with inserted_at_source if we want to add inserted_at_on_writable_violation in a similar manner to let this be configured on the field-level.

[green-david]

I guess this isn't 100% backwards compatible as the behavior will change subtly if you have @on_writable_violation set to warn or raise attempts to updated inserted_at will warn or raise whereas they previously would be silently ignored, but 🤷‍♂️

To be fully backwards compatible, I guess we would need to not default this and instead allow the user to specify it via an option like inserted_at_writable

[josevalim]

Can you please expand on which scenarios this would trigger? Which code?

[green-david]

Can you please expand on which scenarios this would trigger? Which code?

Sure thing, thanks for taking a look!

I have to admit that it is a bit difficult to come up with "realistic" examples as almost any example I can think of is an obvious bug, primarily casting inserted_at explicitly.

The one scenario I have personally seen this in production code is in a scenario similar to the following, when "overlaying" fields of a record onto another (and im certainly not saying this is good code! 🤣 )

    defmodule TimestampsWritableSample do
      use Ecto.Schema

      schema "timestamps" do
        field :foo, :integer
        field :bar, :string
        field :sensitive, :string

        timestamps(type: :utc_datetime_usec)
      end

      def overlay_changeset(%__MODULE__{} = self, %__MODULE__{} = other) do
        changes =
          other
          |> Map.from_struct()
          # oops, forgot to include inserted_at and updated_at because they are behind
          # a macro
          |> Map.take(__MODULE__.__schema__(:fields) -- [:id, :sensitive])

        Ecto.Changeset.change(self, changes)
      end
    end

    test "overlaying a record onto another" do
      a = TestRepo.insert!(%TimestampsWritableSample{id: 1, foo: 10, bar: "test1", sensitive: "sensitive1"})
      b = TestRepo.insert!(%TimestampsWritableSample{id: 2, foo: 20, bar: "test2", sensitive: "sensitive2"})

      updated_a =
        a
        |> TimestampsWritableSample.overlay_changeset(b)
        |> TestRepo.update!()

      refute updated_a.id == b.id
      assert updated_a.foo == b.foo
      assert updated_a.bar == b.bar
      refute updated_a.sensitive == b.sensitive
      # accidentally overwrite inserted_at :(
      refute DateTime.compare(updated_a.inserted_at, b.inserted_at) == :eq
    end

In my experience inserted_at typically is more than just another data point but also has intrinsic value as a debugging tool and, more importantly, an auditability tool to be able to answer "when did that happen?", so allowing for a safer default to further prevent you from shooting yourself in the foot seems like it could be helpful. I can't come up with any scenarios off the top of my head as to when you would want to deliberately update inserted_at in application code (except test code perhaps?) 🤔

[josevalim]

So I think in this case the warning is doing its job and its warning you that you are passing it when you should not. You should fix the code instead!

[green-david]

So I think in this case the warning is doing its job and its warning you that you are passing it when you should not. You should fix the code instead!

@josevalim I am a bit confused, can you clarify what warning you are referring to? Today inserted_at just gets silently written to, no? By setting it to writable: :insert in this PR it allows you to configure @on_writable_violation :warn or @on_writable_violation :raise precisely so that you can start getting a warning or exception to indicate that you need to fix the code, whereas previously you would have never known!

[josevalim]

Oh, I see! It is not detected and you want to detect it. I think it should be opt-in, unfortunately, as to not break existing code. You can use @timestamp_opts or something to change the default behaviour, yeah.

[green-david]

Oh, I see! It is not detected and you want to detect it. I think it should be opt-in, unfortunately, as to not break existing code. You can use @timestamp_opts or something to change the default behaviour, yeah.

Perfect, I will update this PR to make it opt-in this evening! Thanks as always Jose!