aembke / fred.rs

An async Redis client for Rust.
Apache License 2.0
390 stars 63 forks source link

[Bug] Vec<u8> is recognised as Array rather than Bytes #296

Closed dracarys18 closed 1 week ago

dracarys18 commented 1 month ago

Fred version - 9.2.1 Redis version - 7.0.1 Platform - mac Deployment type - cluster|sentinel|centralized

Describe the bug For the following example below, Fred throws and error value: Redis Error - kind: InvalidArgument, details: Invalid argument type: Array while it's a valid type.

    let v = client.hsetnx::<E, &str, &str, V>("bruh1", "what1", serde_json::to_vec(&data).unwrap()).await;

To Reproduce Steps to reproduce the behavior:

  1. Call hsetnx function with Vec<u8> as value type.

Logs (If possible set RUST_LOG=fred=trace and run with --features debug-ids)

Redis Error - kind: InvalidArgument, details: Invalid argument type: Array

dracarys18 commented 1 month ago

I can take this up we need a different TryInto implementation for Vec<u8> and Vec<T> where T is not u8

aembke commented 1 month ago

Hi @dracarys18,

Unfortunately I'm not sure how to do this on stable rust at the moment without specialization, but feel free to submit a PR if you can make it work.

After looking at this a bit more - If I'm reading this correctly it looks like TryFrom<T> creates a From<T> fallback automatically, and since From<u8> already exists we can't implement both TryFrom<Vec<u8>> for RedisValue and TryFrom<Vec<T>> for RedisValue.

I think there's a few ways to work around this, but they have some downsides:

For the next major version I'll probably remove the From<u8> for RedisValue since Vec<u8> is likely much more common, but for now I want to avoid any breaking changes.

dracarys18 commented 1 month ago

@aembke I like the idea of last point can we make a feature flag for this so it wont break for others and slowly release it in the next release

aembke commented 2 weeks ago

Sounds good. In 9.4.0 there a new FF specialize-into-bytes that will disable From<u8> for RedisValue and enable TryFrom<Vec<u8>> for RedisValue such that it uses RedisValue::Bytes.

dracarys18 commented 2 weeks ago

Oh that's awesome! Thanks @aembke

aembke commented 1 week ago

Added in 9.4.0