Skip to content

Commit

Permalink
[ServiceBus] Graceful noops for methods taking empty lists. (#15286)
Browse files Browse the repository at this point in the history
Make send_messages, schedule_messages, cancel_scheduled_messages, and receive_deferred_messages gracefully no-op when provided an empty list or empty batch (where appropriate).  Adds tests and changelog notes for these scenarios.  Closes #15211
  • Loading branch information
KieranBrantnerMagee authored Nov 19, 2020
1 parent 9f4e92b commit 9926619
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 10 deletions.
6 changes: 2 additions & 4 deletions sdk/servicebus/azure-servicebus/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@

**Breaking Changes**

* `ServiceBusSender` and `ServiceBusReceiver` are no longer reusable and will raise `ValueError` when trying to operate on a closed handler.
* `ServiceBusSender` and `ServiceBusReceiver` are no more reusable and will raise `ValueError` when trying to operate on a closed handler.
* `send_messages`, `schedule_messages`, `cancel_scheduled_messages` and `receive_deferred_messages` now performs a no-op rather than raising a `ValueError` if provided an empty list of messages or an empty batch.

**BugFixes**

* FQDNs and Connection strings are now supported even with strippable whitespace or protocol headers (e.g. 'sb://').

**Bug Fixes**

* Using parameter `auto_lock_renewer` on a sessionful receiver alongside `ReceiveMode.ReceiveAndDelete` will no longer fail during receipt due to failure to register the message with the renewer.

## 7.0.0b8 (2020-11-05)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -555,8 +555,8 @@ def receive_deferred_messages(self, sequence_numbers, **kwargs):
raise ValueError("The timeout must be greater than 0.")
if isinstance(sequence_numbers, six.integer_types):
sequence_numbers = [sequence_numbers]
if not sequence_numbers:
raise ValueError("At least one sequence number must be specified.")
if len(sequence_numbers) == 0:
return [] # no-op on empty list.
self._open()
try:
receive_mode = self._receive_mode.value.value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,8 @@ def schedule_messages(self, messages, schedule_time_utc, **kwargs):
if isinstance(messages, ServiceBusMessage):
request_body = self._build_schedule_request(schedule_time_utc, messages)
else:
if len(messages) == 0:
return [] # No-op on empty list.
request_body = self._build_schedule_request(schedule_time_utc, *messages)
return self._mgmt_request_response_with_retry(
REQUEST_RESPONSE_SCHEDULE_MESSAGE_OPERATION,
Expand Down Expand Up @@ -296,6 +298,8 @@ def cancel_scheduled_messages(self, sequence_numbers, **kwargs):
numbers = [types.AMQPLong(sequence_numbers)]
else:
numbers = [types.AMQPLong(s) for s in sequence_numbers]
if len(numbers) == 0:
return None # no-op on empty list.
request_body = {MGMT_REQUEST_SEQUENCE_NUMBERS: types.AMQPArray(numbers)}
return self._mgmt_request_response_with_retry(
REQUEST_RESPONSE_CANCEL_SCHEDULED_MESSAGE_OPERATION,
Expand Down Expand Up @@ -347,7 +351,7 @@ def send_messages(self, message, **kwargs):
except TypeError: # Message was not a list or generator.
pass
if isinstance(message, ServiceBusMessageBatch) and len(message) == 0: # pylint: disable=len-as-condition
raise ValueError("A ServiceBusMessageBatch or list of Message must have at least one Message")
return # Short circuit noop if an empty list or batch is provided.
if not isinstance(message, ServiceBusMessageBatch) and not isinstance(message, ServiceBusMessage):
raise TypeError(
"Can only send azure.servicebus.<ServiceBusMessageBatch,ServiceBusMessage> or "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -557,8 +557,8 @@ async def receive_deferred_messages(
raise ValueError("The timeout must be greater than 0.")
if isinstance(sequence_numbers, six.integer_types):
sequence_numbers = [sequence_numbers]
if not sequence_numbers:
raise ValueError("At least one sequence number must be specified.")
if len(sequence_numbers) == 0:
return [] # no-op on empty list.
await self._open()
try:
receive_mode = self._receive_mode.value.value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ async def schedule_messages(
if isinstance(messages, ServiceBusMessage):
request_body = self._build_schedule_request(schedule_time_utc, messages)
else:
if len(messages) == 0:
return [] # No-op on empty list.
request_body = self._build_schedule_request(schedule_time_utc, *messages)
return await self._mgmt_request_response_with_retry(
REQUEST_RESPONSE_SCHEDULE_MESSAGE_OPERATION,
Expand Down Expand Up @@ -240,6 +242,8 @@ async def cancel_scheduled_messages(self, sequence_numbers: Union[int, List[int]
numbers = [types.AMQPLong(sequence_numbers)]
else:
numbers = [types.AMQPLong(s) for s in sequence_numbers]
if len(numbers) == 0:
return None # no-op on empty list.
request_body = {MGMT_REQUEST_SEQUENCE_NUMBERS: types.AMQPArray(numbers)}
return await self._mgmt_request_response_with_retry(
REQUEST_RESPONSE_CANCEL_SCHEDULED_MESSAGE_OPERATION,
Expand Down Expand Up @@ -294,7 +298,7 @@ async def send_messages(
except TypeError: # Message was not a list or generator.
pass
if isinstance(message, ServiceBusMessageBatch) and len(message) == 0: # pylint: disable=len-as-condition
raise ValueError("A ServiceBusMessageBatch or list of Message must have at least one Message")
return # Short circuit noop if an empty list or batch is provided.
if not isinstance(message, ServiceBusMessageBatch) and not isinstance(message, ServiceBusMessage):
raise TypeError(
"Can only send azure.servicebus.<ServiceBusMessageBatch,ServiceBusMessage>"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@ async def test_async_queue_by_queue_client_conn_str_receive_handler_peeklock(sel
message = ServiceBusMessage("Handler message no. {}".format(i))
await sender.send_messages(message, timeout=5)

# Test that noop empty send works properly.
await sender.send_messages([])
await sender.send_messages(ServiceBusMessageBatch())
assert len(await sender.schedule_messages([], utc_now())) == 0
await sender.cancel_scheduled_messages([])

# Then test expected failure modes.
with pytest.raises(ValueError):
async with sender:
raise AssertionError("Should raise ValueError")
Expand All @@ -85,6 +92,7 @@ async def test_async_queue_by_queue_client_conn_str_receive_handler_peeklock(sel

receiver = sb_client.get_queue_receiver(servicebus_queue.name, max_wait_time=5)
async with receiver:
assert len(await receiver.receive_deferred_messages([])) == 0
with pytest.raises(ValueError):
await receiver.receive_messages(max_wait_time=0)

Expand Down
8 changes: 8 additions & 0 deletions sdk/servicebus/azure-servicebus/tests/test_queues.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,15 @@ def test_queue_by_queue_client_conn_str_receive_handler_peeklock(self, servicebu
message.to = 'to'
message.reply_to = 'reply_to'
sender.send_messages(message)

# Test that noop empty send works properly.
sender.send_messages([])
sender.send_messages(ServiceBusMessageBatch())
assert len(sender.schedule_messages([], utc_now())) == 0
sender.cancel_scheduled_messages([])
sender.close()

# Then test expected failure modes.
with pytest.raises(ValueError):
with sender:
raise AssertionError("Should raise ValueError")
Expand All @@ -145,6 +152,7 @@ def test_queue_by_queue_client_conn_str_receive_handler_peeklock(self, servicebu

receiver = sb_client.get_queue_receiver(servicebus_queue.name, max_wait_time=5)

assert len(receiver.receive_deferred_messages([])) == 0
with pytest.raises(ValueError):
receiver.receive_messages(max_wait_time=0)

Expand Down

0 comments on commit 9926619

Please sign in to comment.