[ServiceBus] AMQPAnnotatedMessage API Review Feedback #18432

yunhaoling · 2021-04-29T15:19:55Z

The change is based upon the gist and the discussion:
https://gist.github.com/yunhaoling/4bf19d56fcb9c21c3944eba41d272433

Decision:

let's push advanced users to use ServiceBusReceivedMessage.raw_amqp_message

pull out body_type property on ServiceBusReceivedMessage
- (TBD) body return None vs raise Error

Introduce new classes for header and property, and inherit from DictMixin
- (TBD) whether to inherit from uamqp classes as well for backward-compatibility
Introduce new namespace azure.servicebus.amqp

API View is available here:
https://apiview.dev/Assemblies/Review/c4c7244a8c1d4968827e0163db9187c5

rakshith91 · 2021-04-29T20:54:01Z

sdk/servicebus/azure-servicebus/azure/servicebus/__init__.py

@@ -37,8 +35,6 @@
    "ServiceBusMessage",
    "ServiceBusMessageBatch",
    "ServiceBusReceivedMessage",
-    "AMQPAnnotatedMessage",


this would be breaking - i thought we never exposed this to customer?

this would be a breaking change for b1, and we never exposed this to customer in GA

yunhaoling · 2021-04-30T17:57:47Z

sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py

+        if self.message._body.type != uamqp.MessageBodyType.Data:
+            return None  # TODO: TBD, raise TypeError vs return None


thoughts:

if message.body == None: message.raw_amqp_message.body # vs try: message.body except TypeError: message.raw_amqp_message.body

although raising error on property seems not common in python, but it seems be a stronger indicator to me in this dedicated scenario..

there're some discussions:
https://stackoverflow.com/questions/48778945/by-design-should-a-property-getter-ever-throw-an-exception-in-python
https://softwareengineering.stackexchange.com/questions/16646/is-throwing-an-exception-from-a-property-bad-form

I wasn't there during the original discussion, but looking through these questions, it seems that raising an error is not preferred. Is there a specific scenario you have in mind where the try/except would work and the == wouldn't?

Having a case in mind which is pushing me to the return None approach:

corner case for sending None:

sb_msg = ServiceBusMessage(body=None, application_properties={"key": "value"}) amqp_msg = AMQPAnnotatedMessage(value_body=None, application_properties={"key": "value"})

if we want to adopt the raise error approach

our current sbmessage supports passing None (although it's not documented)

there is a reason, normal users might just want to send some app prop without a body

if users sending amqpannotatedmessage with None value body, and on the receiving side we didn't raise TypeError (because we can't differentiate)

then this feels like we're breaking the consistency with sequence, and non-None value body

(well, actually I have a way to convince myself that ServiceBusMessage.body is always trying to extract the data body, so in the sequence/value body case, data body is None which returns None is fair enough)

find one raising error property sample from requests content:

https://github.com/psf/requests/blob/f6d43b03fbb9a1e75ed63a9aa15738a8fce99b50/requests/models.py#L823-L841

similar in httpx lib:

https://github.com/encode/httpx/blob/9b17671f155aa857fb52911d94357147ba2a3ea4/httpx/_models.py#L1136-L1140

yunhaoling · 2021-04-30T18:23:58Z

sdk/servicebus/azure-servicebus/azure/servicebus/amqp/_amqp_message.py

+        """
+        Properties to add to the message.
+
+        :rtype: Optional[~azure.servicebus.amqp.AMQPMessageProperties]


in GAed it's returning uamqp.message.MessageProperties. This would be a breaking change to stuff like type hint / isinstance check. but we don't know how many existing users reply on the type check (as far as I understand, raw_amqp_message should be a rare use case).

way to hack isinstance:

@property def __class__(self): return uamqp.message.MessagePropreties # bad idea?

or

class AMQPMessageProperties(DicMixin, uamqp.message.MessageProperties)

is the first way (with setting class property to return uamqp.message.MessageProperties) the one that would be a breaking change?

@yunhaoling @annatisch I'd like to get rid of everything that shows uamqp, in case we replace that transport by something else. As long as the duck typing is consistent, I think I'm happy enough.

swathipil · 2021-05-03T16:19:28Z

sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py

+        if self.message._body.type != uamqp.MessageBodyType.Data:
+            return None  # TODO: TBD, raise TypeError vs return None


I wasn't there during the original discussion, but looking through these questions, it seems that raising an error is not preferred. Is there a specific scenario you have in mind where the try/except would work and the == wouldn't?

swathipil · 2021-05-03T16:31:30Z

sdk/servicebus/azure-servicebus/azure/servicebus/amqp/_amqp_message.py

+        message_header = None
+        message_properties = None
+        if header:
+            message_header = uamqp.message.MessageHeader()


will this need to be replaced/wrapped by AMQPMessageHeader()?

swathipil · 2021-05-03T16:31:44Z

sdk/servicebus/azure-servicebus/azure/servicebus/amqp/_amqp_message.py

+            message_header.priority = header.get("priority")
+
+        if properties:
+            message_properties = uamqp.message.MessageProperties(**properties)


same comment as above: replaced/wrapped by AMQPMessageProperties()?

swathipil · 2021-05-03T16:35:54Z

sdk/servicebus/azure-servicebus/azure/servicebus/amqp/_amqp_message.py

+        """
+        Properties to add to the message.
+
+        :rtype: Optional[~azure.servicebus.amqp.AMQPMessageProperties]


is the first way (with setting class property to return uamqp.message.MessageProperties) the one that would be a breaking change?

Update resource association swagger (Azure#18432) * update access mode values * update examples * Add 202 in delete spec * Add provisioning issues changes * update schema * review changs * update * typo in model fix * make provisioning issues readonly * Add one example in list association, provisioning state enum update * Add logggin categories in profile * examples update * update * update examples * reformat code * Add x-msidentifier for logger * update * provisioning state fix * Fix provisioning state * Add location header in delete nsp association * update location header * update location header * location header update Co-authored-by: Kaushal Kumar <kumarkaushal@microsoft.com>

sample code change

6b74c5c

ghost added the Service Bus label Apr 29, 2021

yunhaoling requested review from rakshith91, swathipil and annatisch April 29, 2021 15:20

yunhaoling changed the title ~~[ServiceBus] API Review Feedback~~ [ServiceBus] AMQPAnnotatedMessage API Review Feedback Apr 29, 2021

rakshith91 reviewed Apr 29, 2021

View reviewed changes

yunhaoling commented Apr 30, 2021

View reviewed changes

swathipil reviewed May 3, 2021

View reviewed changes

This was referenced May 5, 2021

[ServiceBus] Discussion on the expanding body return type to support amqp body types #17786

Closed

[ServiceBus] Discussion for redesigning the namespace and property for AMQPAnnotatedMessage #17977

Closed

[ServiceBus] GA AMQPAnnotatedMessage #18665

Closed

impl update

52298e7

yunhaoling closed this Jun 2, 2021

yunhaoling deleted the yuling/sb/annotated-msg-design-v2 branch September 28, 2021 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ServiceBus] AMQPAnnotatedMessage API Review Feedback #18432

[ServiceBus] AMQPAnnotatedMessage API Review Feedback #18432

yunhaoling commented Apr 29, 2021 •

edited

Loading

rakshith91 Apr 29, 2021

yunhaoling Apr 30, 2021

yunhaoling Apr 30, 2021 •

edited

Loading

swathipil May 3, 2021

yunhaoling May 12, 2021

yunhaoling May 17, 2021 •

edited

Loading

yunhaoling Apr 30, 2021 •

edited

Loading

swathipil May 3, 2021

lmazuel May 3, 2021

swathipil May 3, 2021

swathipil May 3, 2021 •

edited

Loading

swathipil May 3, 2021

swathipil May 3, 2021

		if self.message._body.type != uamqp.MessageBodyType.Data:
		return None # TODO: TBD, raise TypeError vs return None

[ServiceBus] AMQPAnnotatedMessage API Review Feedback #18432

[ServiceBus] AMQPAnnotatedMessage API Review Feedback #18432

Conversation

yunhaoling commented Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunhaoling Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunhaoling May 17, 2021 • edited Loading

Choose a reason for hiding this comment

yunhaoling Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swathipil May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunhaoling commented Apr 29, 2021 •

edited

Loading

yunhaoling Apr 30, 2021 •

edited

Loading

yunhaoling May 17, 2021 •

edited

Loading

yunhaoling Apr 30, 2021 •

edited

Loading

swathipil May 3, 2021 •

edited

Loading