Do not reflect changes ignored due to :writable in returned struct by green-david · Pull Request #4736 · elixir-ecto/ecto

green-david · 2026-06-16T15:59:35Z

Oops @greg-rychlewski, I realized just slightly too late that I accidentally broke #4524 / #4525 in #4734. This PR fixes the regression and adds accompanying unit tests.

greg-rychlewski · 2026-06-16T16:42:23Z

Wait do I understand correctly that before we were not removing the fields that should be dropped? I'm really confused why our old tests passed then

greg-rychlewski · 2026-06-16T16:45:39Z

Oh I guess the old tests were only checking that we sent the correct fields to the write but not that we were dropping them correctly from the returned struct?

green-david · 2026-06-16T16:46:59Z

Oh I guess the old tests were only checking that we sent the correct fields to the write but not that we were dropping them correctly from the returned struct?

Exactly! I didnt apply Jose's suggestion correctly in my last PR. We filtered them later before the write.

greg-rychlewski · 2026-06-16T16:52:59Z

Suggested change

test "update with on_writable_violation: :nothing saves changes for writable: :always and ignores changes for writable: :insert/:never" do

%{never: nil} =

%MySchemaWritable{id: 1}

|> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})

|> TestRepo.update!()

assert_received {:update, %{changes: [always: 10]}}

end

I think this is enough and then you can also make this minimal change to the existing insert test.

green-david · 2026-06-16T17:53:05Z

@greg-rychlewski I realized as well that even with this fix (and even prior to any of my changes I think) that non-writable changes are not reflected in the returned struct as mentioned in #4524, but that if a non-writable field has a value in the underlying data that is surfaced as a change during insert, the returned struct does reflect an updated value which I dont think should be happening?

For example, this seems to fail:

%{never: nil} = 
%SchemaWritable{never: 10}
|> Ecto.Changeset.change(%{})
|> TestRepo.insert!()

And this seems to succeed:

%{never: nil} = 
%SchemaWritable{}
|> Ecto.Changeset.change(%{never: 10})
|> TestRepo.insert!()

I can try to come up with a fix later tonight. Perhaps we just force the underlying field in the data to nil, not sure. Would you prefer a separate PR for that specifically or put it in here?

greg-rychlewski · 2026-06-16T18:22:28Z

I think this PR is ok but I also have to think about it. This is kind of a weird case and I'm a bit wary of over-complicating the already complicated code.

greg-rychlewski · 2026-06-16T18:28:59Z

I guess niling them out at the end in the same place we clean up the changes is probably best. But this is weird enough I'd like @josevalim to also take a look after you put the change in

greg-rychlewski · 2026-06-16T19:27:22Z

Suggested change

changeset = update_in(changeset.changes, &drop_non_writable_changes!(&1, drop_fields, schema, :insert))

changeset = update_in(changeset.data, &best_way_to_nil_out_keys_in_elixir(&1, drop_fields))

@green-david something like this...i'm not sure off the top of my head if there is a built-in function to do this rather than rolling our own reducer but this is the idea

…accurately reflects skipped non-writable fields

…for non-writable fields

green-david · 2026-06-17T00:48:49Z

@greg-rychlewski @josevalim This appears to work, I think its ready for another look. 🙇‍♂️

greg-rychlewski · 2026-06-17T03:23:11Z

+    changes =
+      Enum.reduce(non_writable_fields, changeset.changes, fn field, changes ->
+        case changes do
+          %{^field => _change} ->
+            handle_writable_violation(field, schema, action)
+            Map.delete(changes, field)
+          %{} ->
+            changes
+        end
+      end)
+
+    # On insert, if the underlying struct has a value for a non-writable
+    # field, we need to nilify it so that the struct returned from the
+    # insert operation reflects that the write was not actually performed.
+    # Discarding the change above only prevents the actual write from happening.
+    data = case action do
+      :insert ->
+        updates = Enum.map(non_writable_fields, fn field -> {field, nil} end)
+        struct!(changeset.data, updates)
+      _ ->
+        changeset.data
+    end


Suggested change

changes =

Enum.reduce(non_writable_fields, changeset.changes, fn field, changes ->

case changes do

%{^field => _change} ->

handle_writable_violation(field, schema, action)

Map.delete(changes, field)

%{} ->

changes

end

end)

# On insert, if the underlying struct has a value for a non-writable

# field, we need to nilify it so that the struct returned from the

# insert operation reflects that the write was not actually performed.

# Discarding the change above only prevents the actual write from happening.

data = case action do

:insert ->

updates = Enum.map(non_writable_fields, fn field -> {field, nil} end)

struct!(changeset.data, updates)

_ ->

changeset.data

end

{changes, data_updates} =

Enum.reduce(non_writable_fields, {changeset.changes, []}, fn field, {changes, data_updates} ->

case changes do

%{^field => _change} ->

handle_writable_violation(field, schema, action)

data_updates = if action == :insert, do: [{field, nil} | data_updates], else: data_updates

{Map.delete(changes, field), data-updates}

%{} ->

{changes, data_updates}

end

end)

data = if data_updates != [], do: struct!(changeset.data, data_updates), else: changeset.data

maybe something like this...then you only iterate once and also you only update fields that were not nil to begin with

greg-rychlewski · 2026-06-17T03:25:15Z

+      %{always: 12, never: nil, insert: nil} =
+        %MySchemaWritableRaise{id: 3}
+        |> Ecto.Changeset.change(%{always: 12})
+        |> TestRepo.update!()


I don't believe we need to change this since it is not trying to update things that shouldn't be updated and we have other tests for the happy path

removed 👍

greg-rychlewski · 2026-06-17T03:28:31Z

-      %MySchemaWritableRaise{id: 2}
-      |> Ecto.Changeset.change(%{insert: 11, always: 12})
-      |> TestRepo.insert()
+      %{always: 12, never: nil, insert: 11} =


I believe we don't need this change either since we are not attempting to change something that shouldn't be changed and we have other tests already asserting the fields that should be changed are present

removed 👍

josevalim · 2026-06-17T06:55:25Z

I am a bit unsure what to do in this case. It feels surfacing the field as changed, only to discard it, is a bit confusing. Perhaps what we whould do is to not surface it and then to a separate pass on insert on the struct for drop fields. So in a nutshell, we remove the drop_fields from surface_changes and then we do:

if insert
  for field <- drop_fields do
    case struct do
      %{^field => value} when value != nil -> 
        # warn

green-david added 2 commits June 16, 2026 11:58

Do not reflect changes ignored due to :writable in returned struct

401efa8

Fix incorrect docs

bfe9ab7

greg-rychlewski reviewed Jun 16, 2026

View reviewed changes

green-david added 2 commits June 16, 2026 20:46

Ensure that the returned struct from an insert with surfaced changes …

58413a6

…accurately reflects skipped non-writable fields

Simplify tests and test that the returned struct values are accurate …

73fac16

…for non-writable fields

greg-rychlewski reviewed Jun 17, 2026

View reviewed changes

green-david added 2 commits June 17, 2026 02:20

Optimize non-writable field processing

51183d8

Remove extraneous test assertions

f4bb632

+test "update with on_writable_violation: :nothing saves changes for writable: :always and ignores changes for writable: :insert/:never" do
+%{never: nil} =
+%MySchemaWritable{id: 1}
+|> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
+|> TestRepo.update!()
+assert_received {:update, %{changes: [always: 10]}}
+end


	changeset = update_in(changeset.changes, &drop_non_writable_changes!(&1, drop_fields, schema, :insert))
	changeset = update_in(changeset.data, &best_way_to_nil_out_keys_in_elixir(&1, drop_fields))

Conversation

green-david commented Jun 16, 2026

Uh oh!

greg-rychlewski commented Jun 16, 2026

Uh oh!

greg-rychlewski commented Jun 16, 2026

Uh oh!

green-david commented Jun 16, 2026

Uh oh!

greg-rychlewski Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

green-david commented Jun 16, 2026

Uh oh!

greg-rychlewski commented Jun 16, 2026

Uh oh!

greg-rychlewski commented Jun 16, 2026

Uh oh!

greg-rychlewski Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

green-david commented Jun 17, 2026

Uh oh!

greg-rychlewski Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

green-david Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

greg-rychlewski Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

green-david Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

greg-rychlewski Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

green-david Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

josevalim commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greg-rychlewski Jun 16, 2026 •

edited

Loading

greg-rychlewski Jun 17, 2026 •

edited

Loading

greg-rychlewski Jun 17, 2026 •

edited

Loading