serenity-rs / serenity

A Rust library for the Discord API.
https://discord.gg/serenity-rs
ISC License
4.65k stars 571 forks source link

Fix deserialization of burst_colours on MessageReaction #2893

Closed jamesbt365 closed 2 months ago

jamesbt365 commented 2 months ago

Fixes #2892.

jamesbt365 commented 2 months ago

I couldn't think of a nicer way at the time, It would probably involve the creation of another module (so that it doesn't reexport the function in serenity::all, or changing it carefully so it reexports everything but this function) alongside somehow handling the difference between the optional and non optional version.

Do you have any suggestions in mind?

GnomedDev commented 2 months ago

Can this not go in /src/model/utils.rs?

jamesbt365 commented 2 months ago

I didn't think about moving it there to be fair, I don't think much, that only leaves one issue with one being an option and the other not.

jamesbt365 commented 2 months ago

Would this pass?

fn discord_colours_opt<'de, D>(deserializer: D) -> Result<Option<Vec<Colour>>, D::Error>
where
    D: Deserializer<'de>,
{
    let vec_str: Option<Vec<CowStr<'_>>> = Deserialize::deserialize(deserializer)?;

    let Some(vec_str) = vec_str else { return Ok(None) };

    if vec_str.is_empty() {
        return Ok(None);
    }

    deserialize_colours::<D>(vec_str).map(Some)
}

fn discord_colours<'de, D>(deserializer: D) -> Result<Vec<Colour>, D::Error>
where
    D: Deserializer<'de>,
{
    let vec_str: Vec<CowStr<'_>> = Deserialize::deserialize(deserializer)?;

    deserialize_colours::<D>(vec_str)
}

fn deserialize_colours<'de, D>(vec_str: Vec<CowStr<'_>>) -> Result<Vec<Colour>, D::Error>
where
    D: Deserializer<'de>,
{
    vec_str
        .into_iter()
        .map(|s| {
            let s = s.0.strip_prefix('#').ok_or_else(|| DeError::custom("Invalid colour data"))?;

            if s.len() != 6 {
                return Err(DeError::custom("Invalid colour data length"));
            }

            u32::from_str_radix(s, 16)
                .map(Colour::new)
                .map_err(|_| DeError::custom("Invalid colour data"))
        })
        .collect()
}
GnomedDev commented 2 months ago

Sure

AlexCzar commented 2 months ago

Do I understand correctly, that 0.12.* won't work with reactions until this is merged? I'm trying to update my bot to latest version and see following error:

missing field `burst_colours` at line 1 column 816

at

    ctx.http
        .get_message(reaction.channel_id.into(), message_id.into())
        .await
jamesbt365 commented 2 months ago

This is a 2 part fix, the 1st part was #2887.

AlexCzar commented 2 months ago

Can't wait :) I tried applying it myself and specifying git dependency, but some magic happens, and my shuttle app refuses to recognize serenity::Client 🤷🏽