Skip to content
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

Don't ignore additional entries in UntypedReflectDeserializerVisitor #7112

Merged

Conversation

SkiFire13
Copy link
Contributor

@SkiFire13 SkiFire13 commented Jan 6, 2023

Objective

Currently when UntypedReflectDeserializerVisitor deserializes a Box<dyn Reflect> it only considers the first entry of the map, silently ignoring any additional entries. For example the following RON data:

{
    "f32": 1.23,
    "u32": 1,
}

is successfully deserialized as a f32, completly ignoring the "u32": 1 part.

Solution

UntypedReflectDeserializerVisitor was changed to check if any other key could be deserialized, and in that case returns an error.


Changelog

UntypedReflectDeserializer now errors on malformed inputs instead of silently disgarding additional data.

Migration Guide

If you were deserializing Box<dyn Reflect> values with multiple entries (i.e. entries other than "type": { /* fields */ }) you should remove them or deserialization will fail.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 6, 2023
crates/bevy_reflect/src/serde/de.rs Show resolved Hide resolved
@MrGVSV
Copy link
Member

MrGVSV commented Jan 7, 2023

@SkiFire13 Alice is right that this could be a potentially breaking change. It was not an intended behavior but there may be users who happen to have a map with more than one entry (perhaps generated via a tool and left by accident).

Could you add a Migration Guide section to the PR just indicating that affected users simply need to remove the second entry? This will help with the Migration Guide automation when putting together the release notes.

@SkiFire13 SkiFire13 force-pushed the reflect-deserialize-expect-one-key branch from 1252d38 to 55c1382 Compare January 7, 2023 08:42
@SkiFire13
Copy link
Contributor Author

I've added a Migration Guide section. I feel like the wording could be clearer though, let me know if you have any idea on how to improve it.

@MrGVSV
Copy link
Member

MrGVSV commented Jan 7, 2023

I've added a Migration Guide section. I feel like the wording could be clearer though, let me know if you have any idea on how to improve it.

I think the wording is okay (or at least good enough to build off when putting together the full guide haha). Thanks for adding it!

@SkiFire13 SkiFire13 force-pushed the reflect-deserialize-expect-one-key branch from 55c1382 to 4e355d6 Compare January 11, 2023 10:11
@SkiFire13
Copy link
Contributor Author

Rebased to solve the merge conflicts.

@MrGVSV MrGVSV added this to the 0.11 milestone Apr 23, 2023
@MrGVSV MrGVSV added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 30, 2023
@alice-i-cecile
Copy link
Member

@SkiFire13 can you rebase this? CI isn't detectingsome of the runs, and I think that a trivial rebase should fix it up.

@SkiFire13 SkiFire13 force-pushed the reflect-deserialize-expect-one-key branch from 4e355d6 to 95c025c Compare June 12, 2023 20:43
@SkiFire13
Copy link
Contributor Author

I rebased but I'm not sure why CI decided to cancel the build-wasm job.

@alice-i-cecile alice-i-cecile removed this from the 0.11 milestone Jun 19, 2023
@SkiFire13 SkiFire13 force-pushed the reflect-deserialize-expect-one-key branch from 95c025c to 66cfada Compare June 26, 2023 18:51
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 26, 2023
@alice-i-cecile
Copy link
Member

Fingers crossed let's go...

Merged via the queue into bevyengine:main with commit 0f4d16a Jun 26, 2023
20 checks passed
@SkiFire13 SkiFire13 deleted the reflect-deserialize-expect-one-key branch June 26, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants