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

[green-david]

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]

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]

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]

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]

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]

@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]

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]

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

[green-david]

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

[josevalim]

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]

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

Yeah, I see where you are coming from.

To my grug brain the current approach is slightly less confusing as it seems like first pass -> figure out what the user is trying to do (surface the changes), second pass -> detect which of those things are bad (look for violations).

I also kindof like that in the current approach the logic is as similar as possible between update and insert, but that is mostly aesthetic.

Im not stuck on the current approach though if we think special handling it will be more obvious / straightforward to maintain in the long run.

[josevalim]

Let’s go with the one I proposed then. We already have special handling for insert in this patch too and I generally dislike when we have code that undoes what we did earlier (in this case, we are unsurfacing the changes). Thanks!

[green-david]

Let’s go with the one I proposed then. We already have special handling for insert in this patch too and I generally dislike when we have code that undoes what we did earlier (in this case, we are unsurfacing the changes). Thanks!

Done - I moved the special handling for insertion out to its own function that is only called in do_insert and not do_update and no longer surface the non-writable changes. Let me know if this is what you had in mind, hopefully it is close 😄

[josevalim]

Is this how the previous code would behave? If we didn't surface it, I thought we would just leave the field value as is, no?

This is important because if a field has a default value different from nil, we will start setting it back to nil now.

[green-david]

The behavior is intentionally different as it appeared to be a bug:

#4524 proposed that the most obvious return value for a field whose value was not updated due to :writable would be the value actually committed to the database, and #4525 made the changes to do so, but only for actual changes, not surfaced changes.

This led to an odd discrepancy in the previous behavior:

# Returned struct does not reflect database state when inserted via a surfaced change
%{never: 10} = 
%SchemaWritable{never: 10}
|> Ecto.Changeset.change(%{})
|> TestRepo.insert!()

# Returned struct does reflect database state when inserted via an actual change
%{never: nil} = 
%SchemaWritable{}
|> Ecto.Changeset.change(%{never: 10})
|> TestRepo.insert!()

This change aligns the behavior amongst the two so at least we are consistent, as the former certainly appears at first glance to be a bug.

If the field has a default value different from nil, and we go to insert the struct, and the field is writable :never, we wouldn't have inserted the default value, so should we not return nil to reflect that? (That's the idea anyways)

[josevalim]

If the field has a default value different from nil, and we go to insert the struct, and the field is writable :never, we wouldn't have inserted the default value, so should we not return nil to reflect that? (That's the idea anyways)

The problem with this assumption is that it assumes nil is a reasonable value for the field, which may not be the case. If you declare a field with a default of 0, now we are inserting nil where no nil was ever expected. :(

At the same time, if said field is marked as non-writable, then it means it will always warn, regardless of what the value is, forcing users to use nil when an actual value such as 0 may be better.

So I think the best solution overall may be to not warn nor change the value at all in those cases. :(

[green-david]

The problem with this assumption is that it assumes nil is a reasonable value for the field, which may not be the case. If you declare a field with a default of 0, now we are inserting nil where no nil was ever expected. :(

Was this not already the case due to the fact that we filtered out all of the changes before the write silently? I can double check. I think it would get set to the default value in the database, which wouldn't get returned previously unless :returning is specified so im not sure the actual write behavior actually changed? (I could be misunderstanding you).

At the same time, if said field is marked as non-writable, then it means it will always warn, regardless of what the value is, forcing users to use nil when an actual value such as 0 may be better.

Yeah, there is a definite not-intuitive conflict between default and writable. Greg and I discussed it a bit in the original PR here.

It wouldn't have to always warn, no? If they configured on_writable_violation: :nothing (which is the default still), it would continue to behave the same (I think?)

So I think the best solution overall may be to not warn nor change the value at all in those cases. :(

This would certainly be unfortunate. If we do end up having to go this route where surfaced changes can't be detected as writable violations, I guess the best we can do is document it.

[josevalim]

Was this not already the case due to the fact that we filtered out all of the changes before the right silently? I can double check.

We don't use database values unless you opt-in via returning. In that case, it makes sense whatever is returned from the database is always ignored.

It wouldn't have to always warn, no? If they configured on_writable_violation: :nothing (which is the default still), it would continue to behave the same (I think?)

You are right, given you can always opt-out at by saying nothing, then that concern is fine (which is the same conclusion you arrived, sorry for being a slow poke). I'd still say though writable changes the behaviour of default, which we should probably avoid. The good news is that it would be just removing code!

[green-david]

@josevalim Would you mind humoring me with a small summary of what you are suggesting as the path forward? I am happy to make the changes; I just want to be 100% sure I understand what you are proposing before doing so. 🙏

[josevalim]

Write this as:

Suggested change

	changeset = update_in(changeset.data, &nilify_unsurfaced_non_writable_data!(&1, drop_fields, schema))
	changeset = nilify_unsurfaced_non_writable_data!(changeset, drop_fields, schema)

And make it a no-op if drop_fields is an empty list, so we don't do work in the most common case (drop_fields is empty).

[green-david]

Shall I make the same changes to this?

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

to

changeset = drop_non_writable_changes!(changeset, drop_fields, schema, :insert)

for consistency?