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

Fix deserializing snowflakes from non-self-describing formats #2958

Closed

Conversation

MurrayGroves
Copy link

Previously snowflakes were deserialized using deserialize_any which is recommended against because it prevents usage with non-self-describing formats such as Bincode (which was my use-case which caused me to run up against this issue).

I simply replaced deserialize_any with deserialize_str. Although this causes the deserializer to expect to read a str, the bytes representation of 1234 and "1234" are the same so it works perfectly. All tests pass.

@github-actions github-actions bot added the model Related to the `model` module. label Aug 26, 2024
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a test be added to make sure that this behaviour is preserved into the future?

@MurrayGroves
Copy link
Author

Can a test be added to make sure that this behaviour is preserved into the future?

After adding a test I realised this doesn't actually work. After a few hours of investigation it's become clear to me it isn't actually possible to deserialize the IDs without deserialize_any because of the fact it can come from both u64 and string.

@MurrayGroves
Copy link
Author

@GnomedDev

What is interesting though is I'm not sure there are any actual cases where you will be deserializing from a u64 and not a string. The Discord API returns Snowflakes as strings in the first place. Additionally the Serialize implementation also serializes as a string, so I wonder if there actually is a need for being able to deserialize from a u64?

@GnomedDev
Copy link
Member

We may be deserialising from u64 in config files or other non-Discord sources, which isn't documented but people are going to be relying on. I'd be very reluctant to remove that behavior.

@MurrayGroves
Copy link
Author

We may be deserialising from u64 in config files or other non-Discord sources, which isn't documented but people are going to be relying on. I'd be very reluctant to remove that behavior.

That makes sense, will just have to handle deserialisation manually for my usecase I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants