Skip to content

Most elegant way I could figure to fix from_json - #3184

Open
PacificBird wants to merge 7 commits into
SeaQL:masterfrom
PacificBird:fix-active-model-from-json
Open

Most elegant way I could figure to fix from_json#3184
PacificBird wants to merge 7 commits into
SeaQL:masterfrom
PacificBird:fix-active-model-from-json

Conversation

@PacificBird

@PacificBird PacificBird commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

PR Info

Closes #3175

It's honestly not the prettiest fix in the world, but as I mention in the comments of the original issue this is a very difficult problem to solve cleanly. Modified-deserialization-by-proxy is very much not an intended or supported use case of serde. I tried a fully macro-based implementation of from_json, but ran into big problems of the duplication of work between the derives for Models and ActiveModels. In any case I do believe that this is the least-bad way to solve this general design problem short of a very clever workaround and a big refactor, so I'm happy with

I took the idea of making the dummy map by doing a round trip from the dummy active model, to model, into a JSON map using serde_json::to_values, and then merging with serde_json::Map::append with the input JSON values from the AI slop pull, although I hand-coded all of the active model and macro logic without the use of any AI (I straight up don't have a subscription to any and my laptop is not powerful enough to meaningful run a local model...). I did initially copy and then heavily modify the regression test from that PR to test both time and chrono. I left the original comment explaining the test though, I hope that's okay.

The addition of ColumnTrait::serialize_json_key was a necessary evil, as I needed a way to map the serialized keys from roundtripping the dummy values into their deserialize keys so that the final conversion of the merged map into a model (and ultimately an active model) would actually see the dummy values. The way I did it is pretty simple, I just made a HashMap by iterating over Column::iter keyed by the serialize keys from json_key_serialize to the deserialize keys from json_key. That's then used to remap the keys from the dummy values.

This pull may not be my cleanest Rust work, sorry about that. Running rust-analyzer on this codebase eats all of my RAM and soft-locks Linux for me...

Checklist

  • I have read the AI Policy and confirm that this contribution complies with it.

Release Notes

  • Fix incorrect deserialization of missing DateTime (both time and chrono fields) for ActiveModel::from_json
  • Add method: ColumnTrait::serialize_json_key as well as macro implementations for it in DeriveEntityTrait.

Because the new trait method has a default implementation, the addition does not constitute a breaking version change!

Comment thread src/entity/column.rs Outdated
Comment thread src/entity/column.rs Outdated
Comment thread src/entity/active_model.rs Outdated
Comment thread src/entity/active_model.rs Outdated
Comment thread src/entity/active_model.rs Outdated
@PacificBird

Copy link
Copy Markdown
Contributor Author

Added changes for these review comments and also ensured that for #[serde(rename/-all = "...")] (i.e., symmetric ser/de names), it will apply to both serialize and deserialize json keys.

@PacificBird

Copy link
Copy Markdown
Contributor Author

not sure if maintainers can rerun tests, the failing test is some sort of CI error, not a failing test

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.

ActiveModel::from_json fails to deserialize TimeDateTimeWithTimezone fields to NotSet when field is missing from JSON payload

2 participants