Skip to content

Fail gracefully with a default return value when 0 keys are are provided to a command expecting at least 1 key #1052

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

Merged
merged 3 commits into from
Nov 5, 2018
Merged
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
31 changes: 22 additions & 9 deletions redis/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
)

SYM_EMPTY = b('')
EMPTY_RESPONSE = 'EMPTY_RESPONSE'


def list_or_args(keys, args):
Expand Down Expand Up @@ -757,7 +758,12 @@ def execute_command(self, *args, **options):

def parse_response(self, connection, command_name, **options):
"Parses a response from the Redis server"
response = connection.read_response()
try:
response = connection.read_response()
except ResponseError:
if EMPTY_RESPONSE in options:
return options[EMPTY_RESPONSE]
raise
if command_name in self.response_callbacks:
return self.response_callbacks[command_name](response, **options)
return response
Expand Down Expand Up @@ -1120,7 +1126,10 @@ def mget(self, keys, *args):
Returns a list of values ordered identically to ``keys``
"""
args = list_or_args(keys, args)
return self.execute_command('MGET', *args)
options = {}
if not args:
options[EMPTY_RESPONSE] = []
return self.execute_command('MGET', *args, **options)

def mset(self, *args, **kwargs):
"""
Expand Down Expand Up @@ -3214,7 +3223,8 @@ def pipeline_execute_command(self, *args, **options):

def _execute_transaction(self, connection, commands, raise_on_error):
cmds = chain([(('MULTI', ), {})], commands, [(('EXEC', ), {})])
all_cmds = connection.pack_commands([args for args, _ in cmds])
all_cmds = connection.pack_commands([args for args, options in cmds
if EMPTY_RESPONSE not in options])
connection.send_packed_command(all_cmds)
errors = []

Expand All @@ -3229,12 +3239,15 @@ def _execute_transaction(self, connection, commands, raise_on_error):

# and all the other commands
for i, command in enumerate(commands):
try:
self.parse_response(connection, '_')
except ResponseError:
ex = sys.exc_info()[1]
self.annotate_exception(ex, i + 1, command[0])
errors.append((i, ex))
if EMPTY_RESPONSE in command[1]:
errors.append((i, command[1][EMPTY_RESPONSE]))
else:
try:
self.parse_response(connection, '_')
except ResponseError:
ex = sys.exc_info()[1]
self.annotate_exception(ex, i + 1, command[0])
errors.append((i, ex))

# parse the EXEC.
try:
Expand Down
1 change: 1 addition & 0 deletions tests/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ def test_keys(self, r):
assert set(r.keys(pattern='test*')) == keys

def test_mget(self, r):
assert r.mget([]) == []
assert r.mget(['a', 'b']) == [None, None]
r['a'] = '1'
r['b'] = '2'
Expand Down
28 changes: 28 additions & 0 deletions tests/test_pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,34 @@ def test_exec_error_raised(self, r):
assert pipe.set('z', 'zzz').execute() == [True]
assert r['z'] == b('zzz')

def test_transaction_with_empty_error_command(self, r):
"""
Commands with custom EMPTY_ERROR functionality return their default
values in the pipeline no matter the raise_on_error preference
"""
for error_switch in (True, False):
with r.pipeline() as pipe:
pipe.set('a', 1).mget([]).set('c', 3)
result = pipe.execute(raise_on_error=error_switch)

assert result[0]
assert result[1] == []
assert result[2]

def test_pipeline_with_empty_error_command(self, r):
"""
Commands with custom EMPTY_ERROR functionality return their default
values in the pipeline no matter the raise_on_error preference
"""
for error_switch in (True, False):
with r.pipeline(transaction=False) as pipe:
pipe.set('a', 1).mget([]).set('c', 3)
result = pipe.execute(raise_on_error=error_switch)

assert result[0]
assert result[1] == []
assert result[2]

def test_parse_error_raised(self, r):
with r.pipeline() as pipe:
# the zrem is invalid because we don't pass any keys to it
Expand Down