Most elegant way I could figure to fix from_json - #3184
Open
PacificBird wants to merge 7 commits into
Open
Conversation
Huliiiiii
reviewed
Aug 25, 2026
Huliiiiii
reviewed
Aug 25, 2026
Huliiiiii
reviewed
Aug 25, 2026
Huliiiiii
reviewed
Aug 25, 2026
Huliiiiii
reviewed
Aug 25, 2026
Contributor
Author
|
Added changes for these review comments and also ensured that for |
Contributor
Author
|
not sure if maintainers can rerun tests, the failing test is some sort of CI error, not a failing test |
This file contains hidden or 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
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.
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 withI 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 withserde_json::Map::appendwith 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 bothtimeandchrono. I left the original comment explaining the test though, I hope that's okay.The addition of
ColumnTrait::serialize_json_keywas 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 aHashMapby iterating overColumn::iterkeyed by the serialize keys fromjson_key_serializeto the deserialize keys fromjson_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
Release Notes
timeandchronofields) forActiveModel::from_jsonColumnTrait::serialize_json_keyas well as macro implementations for it inDeriveEntityTrait.Because the new trait method has a default implementation, the addition does not constitute a breaking version change!