Add deserializer for amf0 value and fix amf0 visitor fns#441
Open
philipch07 wants to merge 24 commits intomainfrom
Open
Add deserializer for amf0 value and fix amf0 visitor fns#441philipch07 wants to merge 24 commits intomainfrom
philipch07 wants to merge 24 commits intomainfrom
Conversation
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
from
e0d9c72 to
a70a46d
Compare
Deploying scuffle-docusaurus-docs with
|
| Latest commit: |
51e201e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://42898d9c.scuffle-docusaurus-docs.pages.dev |
| Branch Preview URL: | https://pr-441.scuffle-docusaurus-docs.pages.dev |
TroyKomodo
approved these changes
Apr 28, 2025
Deploying scuffle-docrs with
|
| Latest commit: |
51e201e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4c3f1547.scuffle-docrs.pages.dev |
| Branch Preview URL: | https://pr-441.scuffle-docrs.pages.dev |
TroyKomodo
reviewed
Apr 28, 2025
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
from
3e3dccf to
5f2294a
Compare
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
2 times, most recently
from
19cf429 to
da75542
Compare
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
from
c9acbd3 to
8e56bdd
Compare
Contributor
✅ Release Checks Passed⭐ Package Changes
cargo xtask release check |
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
from
9db97e6 to
2debfe2
Compare
TroyKomodo
reviewed
May 30, 2025
TroyKomodo
reviewed
May 30, 2025
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
2 times, most recently
from
a04e9b0 to
872c169
Compare
TroyKomodo
reviewed
May 31, 2025
TroyKomodo
reviewed
May 31, 2025
| serde::forward_to_deserialize_any! { | ||
| bool f64 char str string unit | ||
| seq map newtype_struct tuple | ||
| struct enum ignored_any identifier |
Member
There was a problem hiding this comment.
Perhaps in a future ticket we should take into account the struct field ordering, see samscott89/serde_qs#128 as an example of how to do that.
Member
Author
There was a problem hiding this comment.
I made a ticket for it for now, I might tackle it in the future: CLOUD-123
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
3 times, most recently
from
45d8dcb to
52ccb99
Compare
TroyKomodo
requested changes
Jun 3, 2025
crates/amf0/src/de/mod.rs
Outdated
Comment on lines
590
to
599
| // same as before about the string stuff | ||
| // let bytes = [Amf0Marker::String as u8]; | ||
| // let err = from_buf::<()>(Bytes::from_owner(bytes)).unwrap_err(); | ||
| // assert!(matches!( | ||
| // err, | ||
| // Amf0Error::UnexpectedType { | ||
| // expected: [Amf0Marker::Null, Amf0Marker::Undefined], | ||
| // got: Amf0Marker::String | ||
| // } | ||
| // )); |
Member
There was a problem hiding this comment.
Why is this test commented out?
Member
Author
There was a problem hiding this comment.
It was a leftover from when I was confused about how to handle the errors from attempting to deserialize the String. I think we can delete it though since it's just checking the error output.
TroyKomodo
requested changes
Jun 5, 2025
TroyKomodo
reviewed
Jun 6, 2025
TroyKomodo
reviewed
Jun 6, 2025
crates/amf0/src/de/mod.rs
Outdated
| @@ -202,8 +200,12 @@ where | |||
| where | |||
Member
Author
There was a problem hiding this comment.
Removing this causes the optional() test to fail. My current understanding is that forwarding to any doesn't handle the null properly so it keeps looping over and over causing a stack overflow.
TroyKomodo
reviewed
Jun 6, 2025
Member
|
@philipch07 i pushed a commit showing sort of what i was looking for. What is currently outstanding on this PR is testing forward / backward ser-de impls. Meaning that everytime we serialize should be able to be deserialized back. So the goal is to now: test serde-back-and-forth for all types Sort of like this flow let rust_type = something;
let amf0_bytes = rust_type.encode();
let rust_type2 = amf0_bytes.decode();
assert_eq(rust_type, rust_type2);
let amf0_value= amf0_bytes.decode();
let rust_type3 = amf0_value.decode();
assert_eq(rust_type, rust_type3); |
TroyKomodo
force-pushed
the
pch07/add-amf0-value-deserializer
branch
from
feb05eb to
3c284b8
Compare
…to include bytes-util change from troy
philipch07
force-pushed
the
pch07/add-amf0-value-deserializer
branch
from
3c284b8 to
8c429d9
Compare
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
2 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.
The deserializer is a qol feature for people using amf0.
The current amf0 visitor functions do not properly call the corresponding visit functions, for example, an
i8currently callsself.deserialize_i64(visitor)(incorrect) instead ofself.deserialize_i8(visitor)(correct).For more on the above topic, see the Notion doc. Just a heads up that it's also missing a rule which is mentioned in Troy's other #441 (comment)
Additionally this PR aims to address the following from Troy's comment (see below):