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

Support Redis-rb v5 #314

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Support Redis-rb v5 #314

merged 1 commit into from
Nov 27, 2024

Conversation

hieuk09
Copy link
Contributor

@hieuk09 hieuk09 commented Nov 24, 2024

This is an attempt to fix #281. This PR was also tested using the test suite of our service.

There are a few decisions I made regarding this PR, please let me know if it's not appropriate:

  1. Error from Redis now contains the redis connection URL
  • To fix this, I created MockRedis::Error module which decorate Redis::CommandError and related errors with the connection URL
  • I replace most of the Redis::CommandError with the new module wrapper
  1. Some methods now return integer as output instead of boolean and exists_returns_integer is removed
  • I replace the output as is
  1. Interfaces of most data structures (Hash, Set, ...) now validate type to be 4 main primitive types
  • assert_type is added to validate the arguments. Currently it doesn't look very clean, so any improvements would be great.
  1. sadd, srem now support multiple arguments
  2. Transaction: multi now needs to call with a block and discard/exec cannot be called outside that block anymore
  • I tried to refactor the logic to make the tests pass, but I'm not entirely confident that it works correctly

@hieuk09 hieuk09 force-pushed the feature/upgrade-to-redis-5 branch 2 times, most recently from a5662c7 to 987bc93 Compare November 24, 2024 13:58
assert_valid_integer(seconds)
seconds = Integer(seconds)
Copy link
Contributor Author

@hieuk09 hieuk09 Nov 24, 2024

Choose a reason for hiding this comment

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

These changes mimic how redis-rb handles the number conversion

Comment on lines -160 to +161
raise Redis::CommandError, 'BUSYKEY Target key name already exists.'
raise Error.command_error('BUSYKEY Target key name already exists.', self)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is tested only with MockRedis and not with the multiplexer.

Comment on lines +38 to +39
rescue Redis::CommandError => e
raise Error.command_error(e.message, self)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't access connection URL inside Stream object so I need to rescue the error & wrap it here

@hieuk09 hieuk09 force-pushed the feature/upgrade-to-redis-5 branch from 987bc93 to 8a974a4 Compare November 24, 2024 14:02
@hieuk09 hieuk09 force-pushed the feature/upgrade-to-redis-5 branch from 8a974a4 to 9477e66 Compare November 24, 2024 14:24
@sds sds merged commit 615c184 into sds:main Nov 27, 2024
11 checks passed
@sds
Copy link
Owner

sds commented Nov 27, 2024

Phenomenal effort—thank you.

If after release bugs are found related to this change, I will redirect any issues opened to you for the time being (unless trivial to address myself). Thanks again.

@sds sds mentioned this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

From redis-rb 5.0.0, sadd and srem always return integers
2 participants