xadd type signature is confusingly permissive

[wesrer]

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

  redis = Redis::Client.new

  test = {} of String => String
  test["name"] = "Homie"
  test["curr"] = "USD"
  redis.xadd "my-stream", "*", test.flat_map {|k, v| [k, v]}

It's throwing this error:

Showing last frame. Use --error-trace for full trace.

In /opt/homebrew/Cellar/crystal/1.7.2/share/crystal/src/pointer.cr:132:29

 132 | (self + offset).value = value
                               ^----
Error: type must be (Array(Redis::Value) | Int64 | String | Nil), not Array(String)

but Redis::Value is defined as:

 alias Value = String | Int64 | Nil | Array(Value)

Should this imply that Array(String) is a valid Redis::Value?

[jgaskins]

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.

[wesrer]

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

[jgaskins]

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.

[jgaskins]

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.