Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten array arguments in legacyMode multi commands #2064

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

JPricey
Copy link
Contributor

@JPricey JPricey commented Mar 31, 2022

Description

#2071

In legacy mode multi commands, array arguments are not flattened. This is a bug.
For example, this command that adds two elements to a set:
sadd('a', ['b', 'c'])
when called in a multi will instead add the single element "['b', 'c']".

This bug is a blocker that surfaced while trying to upgrade from node-redis v3 to v4.

The root problem seems to be an inconsistency with the schema of queued multi vs single command arguments.
In a non-multi, the command that gets queued up in our sadd example would be:
["sAdd", "a", ["b", "c"]]
This is later flattened before getting serialized into a redis command.

However, multi commands queue up commands as the tuple: [commandName, args]. In this example:
["sAdd", ["a", ["b", "c"]]]
This is also flattened before getting serialized into a redis command, but only to one level, which is why this issue only surfaces for array arguments.

We fix this inconsistency in this diff by enqueing multi commands in the same format as their non-multi counterparts.

Thank you!


Checklist

  • Does npm test pass with this change (including linting)?
    • I am not able to run the entire test suite on my machine (even master), however I have confirmed that the entire client/lib/client/index.spec.ts file passes on this branch, and the newly added unit test fails on master
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
    • No document changes needed

@leibale
Copy link
Collaborator

leibale commented Apr 6, 2022

@JPricey Nice catch! Thanks for contributing! :)

@leibale leibale merged commit 329885b into redis:master Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants