-
-
Notifications
You must be signed in to change notification settings - Fork 593
FromJsonQueryResult: panic on serialization failures #2635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Wow - I was just about to make this change myself; we've had a security near-miss due to the miss error here. Thanks!! |
This is a must |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with this area, so I'm not sure about the impact of TryFrom
. I agree that panicking is already better than the current behavior. I'd definitely merge this into master
. Right now, it's a dev branch for the upcoming 2.0.0 release, so we can land breaking changes without hesitation.
@tyt2y3, what do you think about merging into 1.1.x
? I'd do that too. I doubt that anyone intentionally relies on the current behavior.
Afterwards, we can explore the TryFrom
direction on master
3b2edc5
to
ae4345f
Compare
@Expurple Thanks for the review! (re-pushed to fix a test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not. We'd merge anyway, cause we see when CI failures are caused by something unrelated in the base branch
Meanwhile, you can add this in your project's [patch.crates-io]
sea-orm = { git = "https://github.com/nodeav/sea-orm", branch = "panic-on-serialization-failure" } (in case someone's affected and doesn't know about this Cargo feature) |
@nodeav oops, something more serious broke this time 😁 As I think about it, let's roll back that |
ae4345f
to
12bd307
Compare
Oops, should be resolved now hopefully |
sorry, as much as I'd like to fix a bug, this contradicts the effort to reduce panics in SeaORM. |
The situation we experienced is an object that fails #[derive(Serialize])
#[serde(tag = "type")]
enum Foo {
Opt1(String)
// ...
} When we try to update a column containing this type, the update operation is ignored without errors or warnings. Not only did this eat up considerable engineerings efforts to isolate (it's pretty unusual that ORMs silently ignore operations), but when these objects are sensitive (e.g. authentication/authorization states), they can lead to very dangerous situations. I completely agree that panics are a last resort but they're always better than silently failing, especially when the error state threatens the integrity of the database. |
@tyt2y3, I agree with them on this:
A panic is not ideal long-term, but it's already better than what we have now. We can quickly merge the panic, let everyone sleep peacefully, and then proceed with the |
12bd307
to
ce30021
Compare
ce30021
to
faa4f5a
Compare
Moved the test to the I'm willing to try going through with the move to Anyway, up to you obviously |
I see. thanks. can you revert the sea-orm-codegen diff? I will merge this afterwards |
faa4f5a
to
65286ae
Compare
@tyt2y3 Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@tyt2y3, don't forget to cherry-pick into master
too
* FromJsonQueryResult: panic on serialization failures * root: fix clippy warnings
PR Info
Breaking Changes
As of 1.1.12, attempting to save/insert/update a model with a json-backed struct fails silently when the struct fails to serialize (which is quite a rare occurrence in my experience).
It makes sense to panic here since this is almost certainly a programmer error, and the alternative (using
TryFrom
instead ofFrom
) would be a major change throughout the codebase as far as I can tell.