+
Skip to content

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

Merged
merged 2 commits into from
Jul 6, 2025

Conversation

nodeav
Copy link
Contributor

@nodeav nodeav commented Jun 25, 2025

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 of From) would be a major change throughout the codebase as far as I can tell.

@ytanay
Copy link

ytanay commented Jun 25, 2025

Wow - I was just about to make this change myself; we've had a security near-miss due to the miss error here. Thanks!!

@mastershif
Copy link

This is a must

Copy link
Member

@Expurple Expurple left a 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

@Expurple Expurple requested review from tyt2y3 and billy1624 June 26, 2025 08:44
@nodeav nodeav force-pushed the panic-on-serialization-failure branch from 3b2edc5 to ae4345f Compare June 26, 2025 09:04
@nodeav
Copy link
Contributor Author

nodeav commented Jun 26, 2025

@Expurple Thanks for the review!
I see that there are some unrelated cargo fmt and clips failures; Should I fix them in this PR as well?

(re-pushed to fix a test)

Copy link
Member

@Expurple Expurple left a 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

@Expurple
Copy link
Member

Expurple commented Jun 26, 2025

Meanwhile, you can add this in your project's Cargo.toml:

[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)

@Expurple
Copy link
Member

@nodeav oops, something more serious broke this time 😁 As I think about it, let's roll back that Cargo.toml change. It can also cause conflicts when cherry-picking the commit into master later

@nodeav nodeav force-pushed the panic-on-serialization-failure branch from ae4345f to 12bd307 Compare June 26, 2025 15:23
@nodeav
Copy link
Contributor Author

nodeav commented Jun 26, 2025

@nodeav oops, something more serious broke this time 😁 As I think about it, let's roll back that Cargo.toml change. It can also cause conflicts when cherry-picking the commit into master later

Oops, should be resolved now hopefully

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 29, 2025

sorry, as much as I'd like to fix a bug, this contradicts the effort to reduce panics in SeaORM.
ideally, we should somehow propagate and return this error.
do you have a full example of the scenario (with entity, and db operations)?
there might be something we can do without breaking the API

@ytanay
Copy link

ytanay commented Jun 29, 2025

sorry, as much as I'd like to fix a bug, this contradicts the effort to reduce panics in SeaORM. ideally, we should somehow propagate and return this error. do you have a full example of the scenario (with entity, and db operations)? there might be something we can do without breaking the API

The situation we experienced is an object that fails serde serialization, e.g.

#[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
Copy link
Member

tyt2y3 commented Jun 29, 2025

@nodeav @ytanay if you can add a test case under tests/ that'd be great, and I can see if it's possible to return an Error with some plumbing work

@Expurple
Copy link
Member

Expurple commented Jun 29, 2025

@tyt2y3, I agree with them on this:

panics are a last resort but they're always better than silently failing

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 Error experiments to improve the situation further.

@nodeav nodeav force-pushed the panic-on-serialization-failure branch from 12bd307 to ce30021 Compare June 30, 2025 14:30
@nodeav nodeav force-pushed the panic-on-serialization-failure branch from ce30021 to faa4f5a Compare June 30, 2025 14:44
@nodeav
Copy link
Contributor Author

nodeav commented Jun 30, 2025

@tyt2y3

@nodeav @ytanay if you can add a test case under tests/ that'd be great, and I can see if it's possible to return an Error with some plumbing work

Moved the test to the tests/ directory.

I'm willing to try going through with the move to TryFrom - though that's arguably more of a breaking change than panicking since it can cause compilation failures in users that rely on the From impl (or we can panic in the From impl)

Anyway, up to you obviously

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 3, 2025

I see. thanks. can you revert the sea-orm-codegen diff? I will merge this afterwards

@nodeav nodeav force-pushed the panic-on-serialization-failure branch from faa4f5a to 65286ae Compare July 6, 2025 05:38
@nodeav
Copy link
Contributor Author

nodeav commented Jul 6, 2025

I see. thanks. can you revert the sea-orm-codegen diff? I will merge this afterwards

@tyt2y3 Done!

Copy link
Member

@Expurple Expurple left a 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

@tyt2y3 tyt2y3 merged commit 698c98f into SeaQL:1.1.x Jul 6, 2025
36 checks passed
tyt2y3 pushed a commit that referenced this pull request Jul 6, 2025
* FromJsonQueryResult: panic on serialization failures

* root: fix clippy warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载