Open wesrer opened 1 year ago
I was really hoping it wouldn’t affect this shard, but it looks like it has. The problem is that one of the recent releases of Crystal (I think it was 1.6, actually) has caused a lot of problems with recursive type aliases. They’ve been problematic in some ways for a while and they want to remove them from the language — the issues that link to that one illustrate a lot of the problems they can cause.
I had to replace recursive aliases in one of my other shards with the JSON::Any
-style wrapper because of this problem, but everything I was doing in this shard was still working okay. It looks like this change may require this shard to go in the same direction.
I used the recursive alias in this shard specifically because it simplified things a bit not to have the extra layer in between to unwrap.
Not sure if I'm misunderstanding the internal code of xadd
but when I pass in a flat array of keys and values like ["name", "Homie", "curr", "USD"]
, I would expect it to just generate a command like so:
["xadd", "my-stream", "*", "name", "Homie", "curr", "USD"]
But instead, it was treating the array like a value to the key data
, which I think was causing the type error. The error itself might be deeper, like you mentioned — I'm unsure.
But I changed the code to work like how I expected:
data.each do |key, val|
command << key.to_s << value
end
to
data.each do |key, value|
if value.is_a? Array
command.concat value
else
command << key.to_s << value
end
end
it works as intended.
If this is desired behavior, I can submit a PR
Oh, okay, looks like I misread the problem this morning and assumed it was the same thing I'd seen elsewhere with recursive type aliases. This is a completely different thing.
I had a look back at the code (I haven't used Redis streams in a while due to this bug) and noticed that there isn't an overload at all for passing arrays. See these method signatures for xadd
:
Since the data
argument for xadd
is intended to be key/value pairs, only Hash
and NamedTuple
(via keyword-argument splats) are supported. What's happening with your example is that the array you're passing in is not being passed as data
, but as maxlen
because there isn't a type restriction for it. maxlen
was intended to be a keyword argument — it isn't enforced, likely because when I wrote those methods I didn't know you could enforce keyword arguments in Crystal.
Because of this, in order to get your change to work as intended, I had to explicitly specify a keyword argument for the array so it wouldn't assign it to maxlen
:
redis.xadd "my-stream", "*", data: test.flat_map {|k, v| [k, v]}
This actually works no matter what the key is, though, because it's ignored when the value is an array. So it ends up working by accident based on the calling convention you used, which is pretty much the reason it works now — it was based on the calling convention I used when I wrote the method.
There are also other bugs with the hash overloads. Namely, they won't compile at all currently due to a namespacing issue. Specifically, the Hash
type name collides with the Redis::Commands::Hash
mixin inside this namespace.
Unfortunately, the code as it sits is too sloppily written and needs to be tightened up significantly. I think we could probably add more overloads here (the keyword-argument overload falls down if you want a maxlen
field in your stream entry, so one that allows data
to be a NamedTuple
would work there), fix the Hash
ones that are already there, enforce type restrictions better, etc. I'm uncertain that an Array
overload is the right move, but I could be convinced. I have a feeling you went that direction with it because the namespacing issue I mentioned above prevented you from passing the hash directly.
To be clear, your example works when using the keyword-argument overload for xadd
:
redis = Redis::Client.new
redis.xadd "my-stream", "*", name: "Homie", curr: "USD"
Unfortunately, this requires you to know the keys you'll be assigning to your stream entries at compile time, which isn't always feasible, so I still think the method signatures for this command should be tightened up.
Hi,
Sorry if this is a noob question, just been playing around with this shard, and I'm not sure why I'm encountering this error
It's throwing this error:
but
Redis::Value
is defined as:Should this imply that
Array(String)
is a validRedis::Value
?