Skip to content

Add the getdel command available from redis 6.2.0 #1460

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docker/base/Dockerfile
Original file line number Diff line number Diff line change
@@ -1 +1 @@
FROM redis:6.0.9-buster
FROM redis:6.2-buster
7 changes: 7 additions & 0 deletions redis/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1666,6 +1666,13 @@ def get(self, name):
"""
return self.execute_command('GET', name)

def getdel(self, name):
"""
Return the value at key ``name``, or None if the key doesn't exist.
Delete the key once read.
"""
return self.execute_command('GETDEL', name)

def __getitem__(self, name):
"""
Return the value at key ``name``, raises a KeyError if the key
Expand Down
2 changes: 1 addition & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# redis 6 release candidates report a version number of 5.9.x. Use this
# constant for skip_if decorators as a placeholder until 6.0.0 is officially
# released
REDIS_6_VERSION = '5.9.0'
REDIS_6_VERSION = '6.2.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. REDIS_6_VERSION actually means "Since which version Redis6 feature introduced". Please check the following usage.
@skip_if_server_version_lt(REDIS_6_VERSION)

So you cannot change it to 6.2.0

Copy link
Author

Choose a reason for hiding this comment

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

You are right, this is a poor work around. The issue is that I do not understand why the tests are failing with Redis 6.2. All I want here is to add the getdel verb that is now available in Redis. I would appreciate a lot some help to resolve the Unit Test issues of that PR. If not I will cancel it and wait for somebody else to add getdel in another PR.



REDIS_INFO = {}
Expand Down
19 changes: 14 additions & 5 deletions tests/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,10 @@ def teardown():
'categories': ['-@all'],
'commands': [],
'enabled': False,
'flags': ['off'],
'flags': ['off', 'sanitize-payload'],
'keys': [],
'passwords': [],
'channels': []
}

# test nopass=True
Expand All @@ -123,9 +124,10 @@ def teardown():
'categories': ['-@all'],
'commands': [],
'enabled': True,
'flags': ['on', 'nopass'],
'flags': ['on', 'nopass', 'sanitize-payload'],
'keys': [],
'passwords': [],
'channels': [],
}

# test all args
Expand All @@ -138,7 +140,7 @@ def teardown():
assert set(acl['categories']) == set(['-@all', '+@set', '+@hash'])
assert set(acl['commands']) == set(['+get', '+mget', '-hset'])
assert acl['enabled'] is True
assert acl['flags'] == ['on']
assert acl['flags'] == ['on', 'sanitize-payload']
assert set(acl['keys']) == set([b'cache:*', b'objects:*'])
assert len(acl['passwords']) == 2

Expand All @@ -157,7 +159,7 @@ def teardown():
assert set(acl['categories']) == set(['-@all', '+@set', '+@hash'])
assert set(acl['commands']) == set(['+get', '+mget'])
assert acl['enabled'] is True
assert acl['flags'] == ['on']
assert acl['flags'] == ['on', 'sanitize-payload']
assert set(acl['keys']) == set([b'cache:*', b'objects:*'])
assert len(acl['passwords']) == 2

Expand Down Expand Up @@ -196,7 +198,7 @@ def teardown():

assert r.acl_setuser(username, enabled=False, reset=True)
users = r.acl_list()
assert 'user %s off -@all' % username in users
assert 'user %s off sanitize-payload -@all' % username in users

@skip_if_server_version_lt(REDIS_6_VERSION)
def test_acl_log(self, r, request):
Expand Down Expand Up @@ -705,6 +707,13 @@ def test_get_and_set(self, r):
assert r.get('integer') == str(integer).encode()
assert r.get('unicode_string').decode('utf-8') == unicode_string

def test_getdel(self, r):
value = b'value'
r.set('a', value)
assert r.getdel('a') == value
assert r.get('a') is None
assert r.getdel('a') is None

def test_getitem_and_setitem(self, r):
r['a'] = 'bar'
assert r['a'] == b'bar'
Expand Down
10 changes: 5 additions & 5 deletions tests/test_pubsub.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ class TestPubSubSubcommands:
def test_pubsub_channels(self, r):
p = r.pubsub()
p.subscribe('foo', 'bar', 'baz', 'quux')
for i in range(4):
for _ in range(4):
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! But personally speaking, I don't prefer irrelevant changes being included in this PR. What do you think @andymccurdy

Copy link
Author

Choose a reason for hiding this comment

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

I am very new contributing in open source project. So please forgive my silly question. Are you saying here that resolving this Pylint warning should not be done in this PR because it is not its main focus? I would have thought we should use every opportunity to reduce the technical debt, especially if it is clear that it will not break anything.

assert wait_for_message(p)['type'] == 'subscribe'
expected = [b'bar', b'baz', b'foo', b'quux']
assert all([channel in r.pubsub_channels() for channel in expected])
Expand All @@ -483,11 +483,11 @@ def test_pubsub_channels(self, r):
def test_pubsub_numsub(self, r):
p1 = r.pubsub()
p1.subscribe('foo', 'bar', 'baz')
for i in range(3):
for _ in range(3):
assert wait_for_message(p1)['type'] == 'subscribe'
p2 = r.pubsub()
p2.subscribe('bar', 'baz')
for i in range(2):
for _ in range(2):
assert wait_for_message(p2)['type'] == 'subscribe'
p3 = r.pubsub()
p3.subscribe('baz')
Expand All @@ -500,7 +500,7 @@ def test_pubsub_numsub(self, r):
def test_pubsub_numpat(self, r):
p = r.pubsub()
p.psubscribe('*oo', '*ar', 'b*z')
for i in range(3):
for _ in range(3):
assert wait_for_message(p)['type'] == 'psubscribe'
assert r.pubsub_numpat() == 3

Expand Down Expand Up @@ -564,6 +564,6 @@ def exception_handler(ex, pubsub, thread):
exception_handler=exception_handler
)

assert event.wait(timeout=1.0)
assert event.wait(timeout=10.0)
pubsub_thread.join(timeout=1.0)
assert not pubsub_thread.is_alive()