[Merged by Bors] - bevy_reflect: Fix deserialization with readers #6894
Conversation
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
MrGVSV
added
C-Bug
james7132
reviewed
Dec 9, 2022
james7132
requested changes
Dec 10, 2022
MrGVSV
force-pushed
the
reflect-deserialize-reader
branch
2 times, most recently
from
2200596 to
4ff041c
Compare
james7132
approved these changes
Dec 10, 2022
SkiFire13
reviewed
Dec 10, 2022
crates/bevy_reflect/src/serde/de.rs
Outdated
| { | ||
| let type_name = map | ||
| .next_key::<String>()? | ||
| .next_key::<Cow<'de, str>>()? |
There was a problem hiding this comment.
AFAIK Deserialize for Cow is implemented to always deserialize the owned variant (thus String in our case), see serde-rs/serde#1852
The issue also contains an example for how to make this work, although I think this is simplier:
#[derive(Deserialize)]
#[serde(transparent)]
struct BorrowableCowStr<'a>(
#[serde(borrow)]
Cow<'a, str>,
);There was a problem hiding this comment.
Oh interesting. Maybe I'll give this a shot then? @james7132 any thoughts?
There was a problem hiding this comment.
That's unfortunate we have to do that, but if that's what it takes to avoid the allocation, that's OK.
There was a problem hiding this comment.
To be clear, it only avoids the allocation when deserializing a &'de str is possible.
From a quick look I think it would be possible to remove the allocation even when a shorter-lived &str is given, but that probably requires using next_key_seed and a custom DeserializeSeed struct to directly deserialize a &TypeRegistration (that borrows from the registry ofc) instead of a &str.
But I could try this in a follow up PR, so no need to block this one.
There was a problem hiding this comment.
Yeah I think a follow up PR would be good if there's still room to optimize 🙂
MrGVSV
commented
Dec 11, 2022
| /// Based on [this comment](https://github.com/bevyengine/bevy/pull/6894#discussion_r1045069010). | ||
| #[derive(Deserialize)] | ||
| #[serde(transparent)] | ||
| struct BorrowableCowStr<'a>(#[serde(borrow)] Cow<'a, str>); |
There was a problem hiding this comment.
Since this is used in two different crates, should this construct be moved to something like bevy_utils? Or is it fine to keep this duplication since it's only a few lines?
There was a problem hiding this comment.
i think the implementation is short and niche enough to leave as is in this pr, but we should probably keep an eye on it. three uses is where i'd probably stick it into a shared crate.
edit: although it could be exported from bevy_reflect::de since bevy_scene depends on it.
There was a problem hiding this comment.
Hm, should this be something that is owned by bevy_reflect::de and depended on by bevy_scene? I feel like bevy_reflect shouldn't really have that responsibility, right?
I can export it if that's preferred.
MrGVSV
added
the
S-Ready-For-Final-Review
cart
approved these changes
Jan 4, 2023
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 4, 2023
# Objective Fixes #6891 ## Solution Replaces deserializing map keys as `&str` with deserializing them as `String`. This bug seems to occur when using something like `File` or `BufReader` rather than bytes or a string directly (I only tested `File` and `BufReader` for `rmp-serde` and `serde_json`). This might be an issue with other `Read` impls as well (except `&[u8]` it seems). We already had passing tests for Message Pack but none that use a `File` or `BufReader`. This PR also adds or modifies tests to check for this in the future. This change was also based on [feedback](#4561 (comment)) I received in a previous PR. --- ## Changelog - Fix bug where scene deserialization using certain readers could fail (e.g. `BufReader`, `File`, etc.)
|
Build failed: |
MrGVSV
force-pushed
the
reflect-deserialize-reader
branch
from
ff7b3e9 to
c2a8f25
Compare
|
Just added the |
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 4, 2023
# Objective Fixes #6891 ## Solution Replaces deserializing map keys as `&str` with deserializing them as `String`. This bug seems to occur when using something like `File` or `BufReader` rather than bytes or a string directly (I only tested `File` and `BufReader` for `rmp-serde` and `serde_json`). This might be an issue with other `Read` impls as well (except `&[u8]` it seems). We already had passing tests for Message Pack but none that use a `File` or `BufReader`. This PR also adds or modifies tests to check for this in the future. This change was also based on [feedback](#4561 (comment)) I received in a previous PR. --- ## Changelog - Fix bug where scene deserialization using certain readers could fail (e.g. `BufReader`, `File`, etc.)
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Jan 9, 2023
# Objective This a follow-up to #6894, see #6894 (comment) The goal is to avoid cloning any string when getting a `&TypeRegistration` corresponding to a string which is being deserialized. As a bonus code duplication is also reduced. ## Solution The manual deserialization of a string and lookup into the type registry has been moved into a separate `TypeRegistrationDeserializer` type, which implements `DeserializeSeed` with a `Visitor` that accepts any string with `visit_str`, even ones that may not live longer than that function call. `BorrowedStr` has been removed since it's no longer used. --- ## Changelog - The type `TypeRegistrationDeserializer` has been added, which simplifies getting a `&TypeRegistration` while deserializing a string.
bors bot
pushed a commit
that referenced
this pull request
Jan 9, 2023
# Objective This a follow-up to #6894, see #6894 (comment) The goal is to avoid cloning any string when getting a `&TypeRegistration` corresponding to a string which is being deserialized. As a bonus code duplication is also reduced. ## Solution The manual deserialization of a string and lookup into the type registry has been moved into a separate `TypeRegistrationDeserializer` type, which implements `DeserializeSeed` with a `Visitor` that accepts any string with `visit_str`, even ones that may not live longer than that function call. `BorrowedStr` has been removed since it's no longer used. --- ## Changelog - The type `TypeRegistrationDeserializer` has been added, which simplifies getting a `&TypeRegistration` while deserializing a string.
bors bot
pushed a commit
that referenced
this pull request
Jan 9, 2023
# Objective This a follow-up to #6894, see #6894 (comment) The goal is to avoid cloning any string when getting a `&TypeRegistration` corresponding to a string which is being deserialized. As a bonus code duplication is also reduced. ## Solution The manual deserialization of a string and lookup into the type registry has been moved into a separate `TypeRegistrationDeserializer` type, which implements `DeserializeSeed` with a `Visitor` that accepts any string with `visit_str`, even ones that may not live longer than that function call. `BorrowedStr` has been removed since it's no longer used. --- ## Changelog - The type `TypeRegistrationDeserializer` has been added, which simplifies getting a `&TypeRegistration` while deserializing a string.
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Jan 21, 2023
…ne#7094) # Objective This a follow-up to bevyengine#6894, see bevyengine#6894 (comment) The goal is to avoid cloning any string when getting a `&TypeRegistration` corresponding to a string which is being deserialized. As a bonus code duplication is also reduced. ## Solution The manual deserialization of a string and lookup into the type registry has been moved into a separate `TypeRegistrationDeserializer` type, which implements `DeserializeSeed` with a `Visitor` that accepts any string with `visit_str`, even ones that may not live longer than that function call. `BorrowedStr` has been removed since it's no longer used. --- ## Changelog - The type `TypeRegistrationDeserializer` has been added, which simplifies getting a `&TypeRegistration` while deserializing a string.
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
# Objective Fixes bevyengine#6891 ## Solution Replaces deserializing map keys as `&str` with deserializing them as `String`. This bug seems to occur when using something like `File` or `BufReader` rather than bytes or a string directly (I only tested `File` and `BufReader` for `rmp-serde` and `serde_json`). This might be an issue with other `Read` impls as well (except `&[u8]` it seems). We already had passing tests for Message Pack but none that use a `File` or `BufReader`. This PR also adds or modifies tests to check for this in the future. This change was also based on [feedback](bevyengine#4561 (comment)) I received in a previous PR. --- ## Changelog - Fix bug where scene deserialization using certain readers could fail (e.g. `BufReader`, `File`, etc.)
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
…ne#7094) # Objective This a follow-up to bevyengine#6894, see bevyengine#6894 (comment) The goal is to avoid cloning any string when getting a `&TypeRegistration` corresponding to a string which is being deserialized. As a bonus code duplication is also reduced. ## Solution The manual deserialization of a string and lookup into the type registry has been moved into a separate `TypeRegistrationDeserializer` type, which implements `DeserializeSeed` with a `Visitor` that accepts any string with `visit_str`, even ones that may not live longer than that function call. `BorrowedStr` has been removed since it's no longer used. --- ## Changelog - The type `TypeRegistrationDeserializer` has been added, which simplifies getting a `&TypeRegistration` while deserializing a string.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Fixes bevyengine#6891 ## Solution Replaces deserializing map keys as `&str` with deserializing them as `String`. This bug seems to occur when using something like `File` or `BufReader` rather than bytes or a string directly (I only tested `File` and `BufReader` for `rmp-serde` and `serde_json`). This might be an issue with other `Read` impls as well (except `&[u8]` it seems). We already had passing tests for Message Pack but none that use a `File` or `BufReader`. This PR also adds or modifies tests to check for this in the future. This change was also based on [feedback](bevyengine#4561 (comment)) I received in a previous PR. --- ## Changelog - Fix bug where scene deserialization using certain readers could fail (e.g. `BufReader`, `File`, etc.)
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…ne#7094) # Objective This a follow-up to bevyengine#6894, see bevyengine#6894 (comment) The goal is to avoid cloning any string when getting a `&TypeRegistration` corresponding to a string which is being deserialized. As a bonus code duplication is also reduced. ## Solution The manual deserialization of a string and lookup into the type registry has been moved into a separate `TypeRegistrationDeserializer` type, which implements `DeserializeSeed` with a `Visitor` that accepts any string with `visit_str`, even ones that may not live longer than that function call. `BorrowedStr` has been removed since it's no longer used. --- ## Changelog - The type `TypeRegistrationDeserializer` has been added, which simplifies getting a `&TypeRegistration` while deserializing a string.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Objective
Fixes #6891
Solution
Replaces deserializing map keys as
&strwith deserializing them asString.This bug seems to occur when using something like
FileorBufReaderrather than bytes or a string directly (I only testedFileandBufReaderforrmp-serdeandserde_json). This might be an issue with otherReadimpls as well (except&[u8]it seems).We already had passing tests for Message Pack but none that use a
FileorBufReader. This PR also adds or modifies tests to check for this in the future.This change was also based on feedback I received in a previous PR.
Changelog
BufReader,File, etc.)