Raise exceptions when updates fail #2159
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In 000fbee we fixed a data issue which was causing two artefacts in publisher's database to fail to update. There was effectively no visibility of this situation from the developers' perspective - only the user saw the error. We didn't get an error in sentry, a log message in logit, or even a 500 in the metrics. The [update](https://www.mongodb.com/docs/mongoid/master/api/Mongoid/Persistable/Updatable.html#update-instance_method) method in mongoid returns false if the record can't be updated because it's not valid. We weren't checking the result and logging anything, so the failure was effectively silent. Switching to [update!](https://www.mongodb.com/docs/mongoid/master/api/Mongoid/Persistable/Updatable.html#update!-instance_method) will raise Errors::Validations if the record being updated is not valid. This will result in the user seeing a 500 / Something went wrong error page, instead of an validation-style message they can't do anything about. It will also mean we get an error in Sentry, and in logit, and there will be a 500 in our metrics. Alternatively we could check the return value of `update` and log a warning, but I think this situation is genuinely an error, and it's better to raise.
ChrisBAshton
approved these changes
May 3, 2024
|
I think that makes sense. Is it worth capturing that behaviour in a test? |
|
Thanks folks! I don't think it's worth a test, as there are lots of places where we call e.g. I think I'll wait until Monday to merge this, just in case I'm wrong about this being a rare situation 😅 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 000fbee we fixed a data issue which was causing two artefacts in publisher's database to fail to update.
There was effectively no visibility of this situation from the developers' perspective - only the user saw the error. We didn't get an error in sentry, a log message in logit, or even a 500 in the metrics.
The update method in mongoid returns false if the record can't be updated because it's not valid. We weren't checking the result and logging anything, so the failure was effectively silent.
Switching to update! will raise Errors::Validations if the record being updated is not valid. This will result in the user seeing a 500 / Something went wrong error page, instead of an validation-style message they can't do anything about. It will also mean we get an error in Sentry, and in logit, and there will be a 500 in our metrics.
Alternatively we could check the return value of
updateand log a warning, but I think this situation is genuinely an error, and it's better to raise.