AR::RecordNotSaved & RecordNotDestroyed from save!/destroy! should include an error message #17824
Conversation
| @@ -139,7 +139,8 @@ def save(*) | |||
| # Attributes marked as readonly are silently ignored if the record is | |||
| # being updated. | |||
| def save!(*) | |||
| create_or_update || raise(RecordNotSaved.new(nil, self)) | |||
| create_or_update || raise(RecordNotSaved.new("Failed to save the record " \ | |||
| "because one of the before callbacks returned false", self)) | |||
There was a problem hiding this comment.
Talking about callbacks is confusing here since it can fail to save due to validations. Maybe just Failed to save the record?
There was a problem hiding this comment.
If saving fails due to validations, ActiveRecord::RecordInvalid will be raised. See line 121 in lib/active_record/persistence.rb.
There was a problem hiding this comment.
Any word on this? I'm more than happy to change the massage to Failed to save the record. Perhaps one of the before callbacks returned false?, but I strongly disagree with not mentioning why at all.
There was a problem hiding this comment.
Talking about callbacks on the Persistence module sounds weird, as it should not have any callback concern.
There was a problem hiding this comment.
Error messages should always mention why to help understand what went wrong.
- You get
NameErrorbecause its anundefined local variable or method. - You get
ArgumentErrorbecause ofwrong number of arguments (Y for X). - You get
ActiveRecord::RecordInvalidbecauseField can't be blank. - You get
ActiveRecord::RecordNotSavedbecausebefore callback cancelled execution.
What could be weird in the above examples? Don't you think the messages below are not weird? What's the point of having a error message?
- You get
ActiveRecord::RecordNotSavedand saysfailed to save the record. - You get
ActiveRecord::RecordInvalidand saysrecord is invalid. - You get
ArgumentErrorand saysargument(s) is incorrect. - You get
NameErrorand sayswrong name.
There was a problem hiding this comment.
At master callbacks halt the execution using throw(:abort) we need to update the message now.
There was a problem hiding this comment.
ok, I'll change it to just Failed to save the record.
yuki24
force-pushed
the
change-record-not-saved-and-not-destroyed-to-include-error-msg
branch
2 times, most recently
from
9689daf
to
d33a80d
Compare
|
updated the message and rebased. Now ready for merging. |
|
LGTM, @rafaelfranca final thoughts? |
|
@rafaelfranca any words on this? |
|
Is there any reason this pull request can not be merged? |
yuki24
force-pushed
the
change-record-not-saved-and-not-destroyed-to-include-error-msg
branch
from
d33a80d
to
ca94307
Compare
|
I ran into this issue today where the message from error = begin
User.find(id).destroy!
rescue ActiveRecord::RecordNotDestroyed => e
e
end
error.message
# => #<User:0x00000004e82f20>This is still happening on Rails 4.2 and 5.0. I don't think nothing stops from merging this request and it should be merged and backported. cc/ @rafaelfranca |
When `AR::Base.save!` or `AR::Base.destroy!` is called and an exception is raised, the exception doesn't have any error message or has a weird message like `#<FailedBulb:0x0000000907b4b8>`. Give a better message so we can easily understand why it's failing to save/destroy.
yuki24
force-pushed
the
change-record-not-saved-and-not-destroyed-to-include-error-msg
branch
from
ca94307
to
ad5824b
Compare
…-destroyed-to-include-error-msg AR::RecordNotSaved & RecordNotDestroyed from save!/destroy! should include an error message
|
Backported at c61b17d |
…-destroyed-to-include-error-msg AR::RecordNotSaved & RecordNotDestroyed from save!/destroy! should include an error message
|
@rafaelfranca Thanks! ❤️ 💚 💛 |
yuki24
deleted the
change-record-not-saved-and-not-destroyed-to-include-error-msg
branch
When calling
AR::Base.save!orAR::Base.destroy!and raising one of the exceptions, give a meaningful error message so that people can easily understand why it's failing to save/destroy.cc @arthurnn @rafaelfranca