Skip to content
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

[Messaging] Allow binary application properties #43181

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

jsquire
Copy link
Member

@jsquire jsquire commented Apr 3, 2024

Summary

The focus of these changes is to update AMQP conversion to remove the validation that disallows byte[] and ArraySegment<byte> values in application properties. These are confirmed to be encoded as binary by Microsoft.Azure.Amqp, making them allowable values according to section 3.2.5 of the AMQP specification.

The Event Hubs documentation for EventData has been updated to reflect this change. Also riding along is a non-trivial clean-up of the Event Hubs-specific AMQP message converter, which had a number of members that were replaced by the Azure.Core.Amqp converter implementation.

Due to a known bug in the Service Bus service, messages with byte[] application properties result in an error when being sent from the client. Until this is confirmed fixed, the Service Bus documentation still reflects byte[] as an invalid type for the application properties. While the client will no longer validate byte[], any use will trigger a service error. Regrettably, this does change the error behavior when this specific type is used, but this is both short-term (confirmed that the service bug will be fixed this semester) and it is enough of an edge case that I do not believe taking a performance hit on each serialization to maintain the current behavior is justified. Likewise, I don't believe that this is impactful enough to take on the technical debt required to add a short-term flag to customize serialization behavior.

The focus of these changes is to update AMQP conversion
to remove the validation that disallows `byte[]` and
`ArraySegment<byte>` values in application properties.
These are confirmed to be encoded as `binary` by
`Microsoft.Azure.Amqp`, making them allowable values
according to section 3.2.5 of the AMQP specification.

The Event Hubs documentation for `EventData` has been
updated to reflect this change.  Due to a known bug
in the Service Bus service, the service will reject
messages with `byte[]` application properties.  Until
this is confirmed fixed, the Service Bus documentation
still reflects `byte[]` as an invalid type for the
application properties.
@jsquire jsquire added Event Hubs Client This issue points to a problem in the data-plane of the library. labels Apr 3, 2024
@jsquire jsquire added this to the 2024-04 milestone Apr 3, 2024
@jsquire jsquire self-assigned this Apr 3, 2024
@jsquire
Copy link
Member Author

jsquire commented Apr 3, 2024

/azp run net - eventhub - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…operties by sequence rather than simple equality.
@jsquire
Copy link
Member Author

jsquire commented Apr 3, 2024

/azp run net - eventhub - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

and adding docs for the service error related to
binary properties with suggested workaround.
@jsquire jsquire marked this pull request as ready for review April 4, 2024 14:22
@jsquire jsquire requested a review from a team as a code owner April 4, 2024 14:22
Copy link
Member

@m-redding m-redding left a comment

Choose a reason for hiding this comment

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

this looks good to me - the tests are very comprehensive 👍

@jsquire jsquire merged commit 5f28227 into Azure:main Apr 4, 2024
62 checks passed
@jsquire jsquire deleted the messaging/binary-props branch April 4, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Event Hubs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants