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

Fix zpopmax zpopmin in pipelines #1055

Closed

Conversation

illusori
Copy link
Contributor

@illusori illusori commented Jan 6, 2022

ZPOPMAX/ZPOPMIN cause an exception when used within a pipeline:

NoMethodError: undefined method `first' for <Redis::Future [:zpopmax, "sortedset"]>:Redis::Future

This is caused by attempting to modify the return value directly rather than within a transform block.

PR includes two commits:

  • first is an addition to the test suite to demonstrate the bug.
  • second is a fix for the bug, that passes the test suite.

Copy link
Collaborator

@byroot byroot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just one minor issue and it should be good to go. It's best to squash your commit together so that when we git blame we see both the change and the associated test.

Comment on lines +203 to +210
r.zadd("sortedset", 1.0, "value")
future = nil
result = r.pipelined do
future = r.zpopmax("sortedset")
end

assert_equal [["value", 1.0]], result
assert_equal ["value", 1.0], future.value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test needs to be wrapped in target_version('5.0.0') do, so it's skipped on older redis versions.

@byroot
Copy link
Collaborator

byroot commented Jan 20, 2022

Merged as bc7b0a2

@byroot byroot closed this Jan 20, 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.

None yet

2 participants