[Bug] RedisValue::Bytes came back as RedisValue::String after roundtrip to Redis

[feelingsonice]

Fred version - 9.1
Redis version - 7.2.4
Platform - mac
Deployment type - centralized

Describe the bug

This might be 2 different bugs combined here:

1. When I send a custom type implementing the FromRedis trait, and send data to Redis matching the implementation (serializing and deserializing itself as bytes::Bytes/RedisValue::Bytes), I'm expecting it to come back as bytes. In the following snippet annotated with ***** SENDING A BYTE TYPE *****, you see me send to the pubsub a TileId type, which convert itself to a RedisValue::Bytes type. However, on the receiver end, I'm getting it returned to me as RedisValue::String type.
2. A minor but related issue here annotated at !!!!! SHOULD ERRROR HERE !!!!!, because the conversion is expecting a RedisValue::Bytes but got RedisValue::String, the error should be propagated and up from on_message at the line that says BIG ERROR, but I didn't see this print any errors.

To Reproduce
Steps to reproduce the behavior:

First run a default redis with redis-server, then run this code:

use bytes::{BufMut, Bytes, BytesMut};
use fred::prelude::*;

#[derive(Hash, Eq, PartialEq, Debug, Clone, Copy)]
pub struct TileId {
    pub z: u32,
    pub x: u32,
    pub y: u32,
}

fn bytes_to_u32(bytes: &[u8]) -> Result<u32, std::io::Error> {
    bytes
        .try_into()
        .map(u32::from_be_bytes)
        .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))
}

impl TryFrom<Bytes> for TileId {
    type Error = std::io::Error;

    fn try_from(value: Bytes) -> Result<Self, Self::Error> {
        if value.len() != 12 {
            return Err(std::io::Error::new(
                std::io::ErrorKind::InvalidData,
                "Invalid byte length to convert to TileId",
            ));
        }
        Ok(TileId {
            z: bytes_to_u32(&value[0..4])?,
            x: bytes_to_u32(&value[4..8])?,
            y: bytes_to_u32(&value[8..12])?,
        })
    }
}

impl From<TileId> for Bytes {
    fn from(value: TileId) -> Self {
        let mut bytes = BytesMut::with_capacity(12);
        bytes.put_u32(value.z);
        bytes.put_u32(value.x);
        bytes.put_u32(value.y);
        bytes.freeze()
    }
}

impl FromRedis for TileId {
    fn from_value(value: RedisValue) -> Result<Self, RedisError> {
        println!("Calling value, got: {:?}", value);
        match value {
            RedisValue::Bytes(b) => b.try_into().map_err(|e| {
                RedisError::new(
                    RedisErrorKind::Parse,
                    format!("Failed parsing TileId from bytes: {}", e),
                )
            }),
            _ => Err(RedisError::new(
                RedisErrorKind::Parse,
                "Expected RedisValue::Bytes type",
            )),
        }
    }
}

// CONVERTION TO REDIS VALUE
impl From<TileId> for RedisValue {
    fn from(value: TileId) -> Self {
        let bytes: Bytes = value.into();
        bytes.into()
    }
}

impl From<TileId> for RedisKey {
    fn from(value: TileId) -> Self {
        let bytes: Bytes = value.into();
        bytes.into()
    }
}

#[tokio::main]
async fn main() -> Result<(), anyhow::Error> {
    let publisher_client = RedisClient::default();
    let subscriber_client = publisher_client.clone_new();
    publisher_client.init().await?;
    subscriber_client.init().await?;

    subscriber_client.subscribe("foo").await?;
    let _message_task = subscriber_client.on_message(|message| {
        println!(
            "{}: {:?}",
            message.channel,
            message.value.convert::<TileId>()? // !!!!! SHOULD ERRROR HERE !!!!!
        );
        Ok::<_, RedisError>(())
    });

    for idx in 0..50 {
        let id = TileId { z: idx, x: 2, y: 3 }; // ***** SENDING A BYTE TYPE *****
        publisher_client.publish("foo", id).await?;
    }

    // SHOULD PRINT ERROR BUT DID NOT
    let _r = _message_task.await.expect("BIG ERROR");

    publisher_client.quit().await?;
    subscriber_client.quit().await?;
    Ok(())
}

Logs

The only line printed:

Calling value, got: String("\0\0\0\0\0\0\0\u{2}\0\0\0\u{3}")

[feelingsonice]

@aembke entire thing can be found here https://github.com/feelingsonice/broadcast-test

[aembke]

Hi @feelingsonice, first off my apologies on the delay getting back to you here.

A bit of background quick - this is due to the client doing a str::from_utf8 call early in the parsing logic to check for specific response types (redirections, certain errors, etc). Instead of throwing out the "work" done by that parsing function I instead changed the response type to keep the parsed representation since strings seemed to be almost always more useful in scenarios where the data is actually valid UTF8 (such as logging, at least at the time). So ultimately the client optimistically tries to parse bulk strings as UTF8 (since it has to anyways for other common use cases), and falls back to the Bytes representation when that fails. The protocol layer uses Str types to do this in a zero-copy way so even on UTF8 parsing failures you're not paying for another allocation.

Regarding a fix or workaround here - I think I'd prefer to keep the current logic since this would be a really invasive change for folks. Ultimately every existing use cases that logically depends on strings would have to add in their own from_utf8 call somewhere, and this would be effectively equivalent to removing the RedisValue::String variant from response types. Given how common Redis is used with UTF8 strings this would likely be a big change for many other use cases. Unfortunately there's no mechanism in RESP responses to easily handle this - in practice every response value is a BulkString/BlobString or number.

Instead, in this case I would recommend matching on the result of into_bytes() or as_bytes() if you know that you want to operate on bytes. More generally - all of the into_* and as_* functions handle lots of strange duck-typing scenarios or type-casting sharp edges, such as different encoding patterns used for maps and numbers, so I usually try to steer folks towards those interfaces. I'll update the docs to better reflect this though - I just realized there are no practical docs or examples for this kind of thing.

Regarding the task failure not returning the error - that one will take a bit more digging but I'll get back to you. Thanks for providing a repro too.