[event-hubs] set known amqp props on systemProperties #7973
Conversation
chradek
added
Client
| /** | ||
| * Converts the AMQP message to an EventData. | ||
| * @param msg The AMQP message that needs to be converted to EventData. | ||
| * @ignore | ||
| */ | ||
| export function fromAmqpMessage(msg: Message): EventDataInternal { | ||
| const data: EventDataInternal = { | ||
| body: msg.body | ||
| body: msg.body, | ||
| systemProperties: {} |
There was a problem hiding this comment.
Any concern that in the absence of any system properties, this would be undefined before this PR and moving forward, it would be empty object?
There was a problem hiding this comment.
I don’t think so. Really, I think it will nearly, if not always be defined now given all the new properties that can be added (for example, the to field). This reduces the number of times we had to check if the object already existed and doesn't hurt the API (if anything I think of we made it a required property that makes things easier by removing an existence check.)
That said, it could lead to more objects being allocated which isn’t free if none of the properties exist. I was testing with iothub events but can check with plain event hub events to see if they are ever all undefined.
There was a problem hiding this comment.
I was testing with iothub events but can check with plain event hub events to see if they are ever all undefined
Lets do that
There was a problem hiding this comment.
All the new properties can be undefined if sending a message with just a body using to event hubs.
I still think it's ok to return an empty object for systemProperties from an API perspective but I can revert that if you have concerns.
There was a problem hiding this comment.
Reverted. Favoring memory optimization over (potentially) compute.
sdk/eventhub/event-hubs/CHANGELOG.md
Outdated
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| ## 5.0.3 (Unreleased) | |||
|
|
|||
| - Fixes an issue where known properties were not being set on a received event's `systemProperties`. | |||
There was a problem hiding this comment.
| - Fixes an issue where known properties were not being set on a received event's `systemProperties`. | |
| - Fixes [issue #7801](https://github.com/Azure/azure-sdk-for-js/pull/7973) where known properties were not being set on a received event's `systemProperties`. |
I wonder if we should call this out as a "feature request". It didn't exist in previous versions of the library either
There was a problem hiding this comment.
Sounds fair enough. I can change the wording and update the version accordingly.
|
/azp run js - eventhubs-client - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #7801
Since
systemPropertieswas already an object with an index signature of string to any, no API changes were needed to support this.It also turned out every one of the properties added is a typed field on the amqp message.