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

Incompatibility with static serialization formats #466

Closed
tamasfe opened this issue Apr 28, 2022 · 9 comments
Closed

Incompatibility with static serialization formats #466

tamasfe opened this issue Apr 28, 2022 · 9 comments
Assignees
Labels
A-third-party Area: implementations of traits from other crates C-bug Category: bug in current code

Comments

@tamasfe
Copy link
Contributor

tamasfe commented Apr 28, 2022

Currently it is not possible to deserialize most time items using formats that require the layout to be known, such as bincode or postcard due to the call to deserialize_any.

The fix should be rather straightforward by calling deserialize_tuple instead if the serde-human-readable feature is not enabled. I can follow up with a PR if this change is desirable.

@jhpratt
Copy link
Member

jhpratt commented Apr 29, 2022

Ah, I didn't realize this. I'll get a patch put together tonight, most likely.

@jhpratt jhpratt self-assigned this Apr 29, 2022
@jhpratt jhpratt added C-bug Category: bug in current code A-third-party Area: implementations of traits from other crates labels Apr 29, 2022
@jhpratt
Copy link
Member

jhpratt commented Apr 29, 2022

Hmm. There's an interesting problem here. I guarantee the following:

Deserializers for human readable formats will fall back to the binary format if the human readable format fails to deserialize.

(from the 0.3.6 release notes)

I don't see how this could be implemented for a generic deserializer while upholding this guarantee, as the deserialize_X methods all require ownership of the deserializer. In hindsight, I probably shouldn't have worded it so strongly. In practice, I suspect no one is actually relying on this fact. I might be willing to overlook the minor breakage. I'll have to think about it.

Edit: Trying to write a test that actually demonstrates the incorrect behavior doesn't appear possible with serde_test, as it's extremely lax in what it accepts. See serde-deprecated/test#2.

@tamasfe
Copy link
Contributor Author

tamasfe commented Apr 29, 2022

I don't see how this could be implemented for a generic deserializer while upholding this guarantee

I believe the current behaviour can be tied to the serde-human-readable feature, so binary serialization works out of the box without it with no fallback required. The issue is that once that feature is enabled, it cannot be opted out from, but this problem is not unique to this library so I believe we can live with it.

Additionally there could be a #[serde(with = "time::serde::binary")] option to ensure compatibility even if the feature is enabled.

@jhpratt
Copy link
Member

jhpratt commented Apr 30, 2022

with no fallback required

The current behavior has a fallback. If serde-human-readable is enabled, then the human-readable format will be tried first. If that fails, the binary format is tried. I don't believe that is possible in a non-self-describing format, as the deserializer requires ownership to be passed.

@tamasfe
Copy link
Contributor Author

tamasfe commented Apr 30, 2022

I'm talking about the case when serde-human-readable is not enabled.

@jhpratt
Copy link
Member

jhpratt commented Apr 30, 2022

Sure, but that's not the issue. When it is enabled, the current behavior should not break. That is my concern that I don't see a way around.

@tamasfe
Copy link
Contributor Author

tamasfe commented Apr 30, 2022

I see, well, making it work without the feature enabled would satisfy my use-case at least, I suspect most people who serialize to binary formats wouldn't bother with that feature anyway.

I don't see a way to fall back either for all cases, even untagged serde enums seem to use deserialize_any.

@jhpratt
Copy link
Member

jhpratt commented Apr 30, 2022

True. I could conditionally call the method directly, but it doesn't solve the issue entirely. I'll do that later.

@jhpratt
Copy link
Member

jhpratt commented May 1, 2022

Should be all set on main!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-third-party Area: implementations of traits from other crates C-bug Category: bug in current code
Projects
None yet
Development

No branches or pull requests

2 participants