Add support for :writable option on belongs_to

[sheharyarn]

PR for my proposal to add :writable option to belongs_to.

This PR simply whitelists the :writable option in @valid_belongs_to_options, which has already been implemented in #4335 & #4472.

cast_assoc & put_assoc currently do not respect this option, so if it makes sense, I can send a follow-up PR for that.

[josevalim]

Looks good to me, but I wonder if @greg-rychlewski has any additional thoughts?

cast_assoc & put_assoc currently do not respect this option, so if it makes sense, I can send a follow-up PR for that.

How do you think it would be handled? I can still see someone using cast_assoc for something else, even though they never write the field, so perhaps we should not change anything.

[greg-rychlewski]

My feeling is to leave it out of cast_assoc/put_assoc as well just because it's not as obvious if there are use cases that will screw up. I think as long as its being handled before the persistence happens it's ok.

[josevalim]

💚 💙 💜 💛 ❤️

[greg-rychlewski]

Reminding myself I need to look to see if this is affected at all #4299

[greg-rychlewski]

@josevalim There is a bit of a weird interaction. This test passes:

test "insert unwritable assoc" do
    assoc = %MyAssoc{x: "xyz"}

    changeset =
      %RestrictedSchema{}
      |> Ecto.Changeset.change
      |> Ecto.Changeset.put_assoc(:unwritable, assoc)
    schema = TestRepo.insert!(changeset)
    assoc = schema.unwritable
    assert assoc.id
    assert assoc.id == schema.unwritable_id
  end

So if you insert the foreign key directly the value is ignored. If you insert the assoc then the foreign key is saved.

I'm thinking it's weird if the assoc record is inserted but the foreign key silently isn't updated. And it's also weird if it just saves the foreign key and ignores the option.

Maybe the best option is to also not insert the assoc? We might have to add a new writable key to the assoc struct or something like that.

[josevalim]

Great catch. I agree we should either make it so it is not written or revert this.

[greg-rychlewski]

I’ll give it a shot seeing what it would take to make it not written

[sheharyarn]

I'm also happy to contribute if you can point me in the right direction.

[greg-rychlewski]

@sheharyarn

In lib/ecto/repo/schema.ex there is a process_parents helper that will handle the modification of the related records. This would need to be able to detect if the belongs to association is writable or not. So that means writable probably has to go into Ecto.Association.BelongsTo struct when the schema is defined.

Where it gets complicated is process_parents calls Ecto.Assocation.on_repo_change which handles what to do with each associated record. And it's not as simple as just check the writable value against the repo action. Because even if the repo action is insert or update, the associated record's changeset might have a different action on it. That is the one that matters.

And then if the new value of the associated record is nil you have to check what the on_replace option will do against the writable option. But this is where it gets the most complicated. Because one of the options is to delete the associated record. So we have to decide what that means against writable. The only thing I can think is to consider it an update.

[greg-rychlewski]

It's also possible you can combine all those checks I mentioned above in the pop_assocs helper function in lib/ecto/repo/schema.ex. But I'm not too sure yet if that's possible. But that would be what I would try first. It could reduce the number of places we have to change code pretty dramatically

[josevalim]

I reverted this for now, as we had to do a new release. Please include the old commit once you push the changes above. :) Thank you!

[greg-rychlewski]

@josevalim The revert doesn't seem to be in the master branch

[josevalim]

done!