Add ability to configure the behavior of writing to a non-writable field#4734
Add ability to configure the behavior of writing to a non-writable field#4734green-david wants to merge 2 commits into
Conversation
| message = "attempted to write to non-writable field #{inspect(name)} during #{action}" | ||
|
|
||
| case on_writable_violation do | ||
| :raise -> | ||
| raise ArgumentError, message | ||
| :warn -> | ||
| Logger.warning(message) | ||
| _ -> | ||
| :ok | ||
| end |
There was a problem hiding this comment.
The message here likely needs to be more robust and contain additional information, open to feedback. This is just a simple message for now to illustrate the functionality.
| * `:on_writable_violation` - Defines what action to take when performing an insert or update | ||
| attempts to modify a field that should not be modified according to it's `:writable` value. | ||
| Must be one of `:nothing`, `:warn`, or `:raise`. If set to `:nothing`, the modification is | ||
| silently ignored. If set to `:warn`, the modification is ignored and a warning is logged. If set | ||
| to `:raise`, an exception is raised and the operation is aborted. If `:writable` is set to `:always`, | ||
| `:on_writable_violation` must be set to `:nothing`. Defaults to `:nothing`. |
There was a problem hiding this comment.
I added this as "the path of least resistance" to illustrate the functionality, but I am not confident this is the desired way forward as to the public facing API of how to configure this behavior.
Some other options:
writable: [when: :never, on_violation: :raise]
writable: {:never, :raise}
writable: [:never, :raise]
Open to suggestions :)
|
Looking at the code, I like the on_writable_violation approach. However, I wonder if the implementation should be |
|
WDYT? |
Sounds good, I will make this change 👍 |
|
@josevalim I pushed a commit simplifying things a bit by using the Would also like your thoughts on the warning and exception. It feels like the messages could be more useful by including possibly the schema name? It also seems like the warning could benefit from a stacktrace, but im not sure of the best way to go about that or if that is overkill / a potential performance killer. I also considered a structured Thank you for the feedback as always! |
| case Map.pop(changes, field) do | ||
| {nil, changes} -> | ||
| changes | ||
| {_change, changes} -> | ||
| handle_writable_violation(field, schema, action) | ||
| changes | ||
| end |
There was a problem hiding this comment.
This can accidentally ignore a field being written to nil. The best is probably:
case changes do
%{^field => _} ->
handle_writable_violation(field, schema, action)
Map.delete(changes, field)
%{} ->
changes
| if writable == :always and on_writable_violation != :nothing do | ||
| raise ArgumentError, "on_writable_violation must be :nothing for always writable fields" | ||
| end | ||
|
|
There was a problem hiding this comment.
We probably don't need this check. And maybe users want to set on_writable_violation: :warn as a default somewhere and this would get in the way. :)
| end | ||
|
|
||
| Module.put_attribute(mod, :ecto_fields, {name, {type, writable}}) | ||
| Module.put_attribute(mod, :ecto_fields, {name, {type, {writable, on_writable_violation}}}) |
There was a problem hiding this comment.
We should not need to change this now.
2 participants
This PR adds the ability to change the default behavior of silently ignoring changes to non-writable fields as discussed on the mailing list.
Attempting to write to a non-writable field can lead to code which appears to update the field but actually doesn't, leading to confusing behavior or hidden bugs. By allowing the user to specify that a warning should be logged or an exception raised, these errant writes can be detected and remedied.