Add ability to configure the behavior of writing to a non-writable field#4734
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
There was a problem hiding this comment.
🤦 Thank you for catching this! Will fix 👍
| 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.
There was a problem hiding this comment.
I might be missing something - doesn't this need to be included here so that I can reference use the value to populate the data for the __schema__(:on_writable_violation) call below?
Or are you suggesting to separately populate something like :ecto_on_writable_violations and use that value in __schema__ instead perhaps?
I put it here so it was next to writable which was pre-existing 🤔
There was a problem hiding this comment.
Yeah, to use a separate attribute so we don't "pollute" all other fields to store this information.
|
@josevalim Hopefully this is a bit closer! Thank you for the feedback 🙇 |
josevalim
left a comment
josevalim
left a comment
There was a problem hiding this comment.
@greg-rychlewski, please merge if you see no issues :)
|
@green-david sorry do you mind rebasing from main? one of our docker images in the ci had an issue so the unit tests didn't even start. if you rebase it will let us make sure all the tests pass on that job |
green-david
force-pushed
the
dg-writable-violation-handling
branch
from
bdb8c69 to
fc4ae9d
Compare
|
@greg-rychlewski No problem! I noticed while I was rebasing that there was an issue where we were still silently ignoring surfaced changes even if configured otherwise which I pushed a fix and tests for in a new commit if you wouldn't mind taking a look. I think there might still be a conflict between the I also wonder if it would be desirable to have a Schema attribute like |
That's a good idea. I think it would be good for a follow up PR though because you basically got this one working and don't want to rock the boat.
I see what you are saying. I'm a bit conflicted because having |
|
Or even just |
Yeah, really the only use case I can think of for |
|
Maybe the best first step is to just leave it for now and see how users react to it and what reports we get. Because if people will start doing complicated things like you shared above then it's better to make less assumptions unless we see it's causing a lot of people problems. |
Sounds good, thank you for thinking it through with me!! |
|
nice work and great idea :). if you wanted to send another PR for the schema attribute that would be appreciated |
3 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.