Amqp Message Update - API Change

[hemanttanwar]

Design decisions:

• Use one type to support Data, SEQUENCE, VALUE
• We will use AmqpMessageBody for this.

This PR has changes in Amqp Message API based on various discussion and changes in dotnet API.
Since we have released Amqp Message API core-amqp in 1.6.0 GA. Following are breaking API update

1. New AmqpMessageId ( Based on discussion from Clemens, dotnet had)

2. New AmqpAddress ( Based on discussion from Clemens, dotnet had)

3. Return type change in AmqpMessageProperties for above two new types.

Following changes are to keep consistency with dotnet and common prefix -> AmqpMessage

4. Renamed AmqpDataBody to AmqpMessageBody (to be same name as in dotnet)

5. Renamed AmqpBodyType to AmqpMessageBodyType (Same name in dotnet)

6. Added AmqpMessageBody .getFirstData () Based on team review

7. Returning IterableStream in AmqpMessageBody .getData () Based on team review

8. Removed Copy constructor AmqpAnnotatedMessage (matching with dotnet) and because it is servicebus specific . So moving this logic into servicebus

Note: Having prefix AmqpMessage in all the Amqp type will help to bind all the types with one prefix.

dotnet : https://apiview.dev/Assemblies/Review/150b1bb087774addafaac6f04f00cdc5#Azure.Core.Amqp.AmqpMessageBody

Java API VIew : https://apiview.dev/Assemblies/Review/30680d3c215c4312a14e787044475bc9

fixes #17080

[srnagar]

I guess we should make this javadoc crisp on why we do this. For the customer, this decision seems arbitrary and we need to document the justification for doing this. This, as we discussed earlier, has the potential for dropping data that is sent from another client that does support more than one byte[].

[srnagar]

Why do we create a new singletonList each time getData() is called? We can create the singletonList just once when fromData() is called.

[JoshLove-msft]

Just noting that we don't make a copy in .NET.

[JoshLove-msft]

Why do we not copy here, but do in the AmqpMessage ctor?

[JonathanGiles]

nit: upper-case 'C'
nit: 'supports', not 'support'

[JonathanGiles]

Why are you copying the byte[] here?

[hemanttanwar]

I actually removed copy constructor from AmqpAnnotatedMessage , since this was created for service bus needs. dotnet also do not have copy constructor.
And moved this logic into ServiceBusMessage, I still do Arrays.copyOf because when use is copying a ServiceBusReceivedMessage into ServiceBusMessage , changing one bytes should not effect bytes in other object. Thus I need to use Arrays.copyOf

[JonathanGiles]

You should clarify the behavior with @JoshLove-msft to ensure we are not doing needless copies. Specifically, where, if anywhere, does .NET copy in their amqp or service bus library. I would guess it is less than we do.

[JoshLove-msft]

We do not copy the bytes - we just create a new instance of AmqpMessageBody wrapping the same bytes - https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusMessage.cs#L66-L72

[hemanttanwar]

Okay I will remove this copy from here.

[JoshLove-msft]

The API View is out of date.

[JonathanGiles]

I'm curious, what are the repercussions of a user doing this:

messageBody = amqpAnnotatedMessage.getBody();

[JonathanGiles]

Put another way...this code is demonstrating how to switch off the body type, but to me it raises questions about what it is actually demonstrating, and what the real use case is.

[JonathanGiles]

It would be good to clarify to the user in this sample code and / or Java why the code I put above is wrong, because I would guess that most people would do that by default.

[hemanttanwar]

updated sample to clarify this why user should not call this directly.

[JonathanGiles]

So the Service Bus APIs will continue to be String-only forever? @hemanttanwar Can you provide sample code on how this would be done (either today or in the future) for Java using the approach Josh shares above?

[hemanttanwar]

Response to this comment :
#17464 (comment)
Question :
"So the Service Bus APIs will continue to be String-only forever? @hemanttanwar Can you provide sample code on how this would be done (either today or in the future) for Java using the approach Josh shares above?"

I have discussed with @JonathanGiles and here are how we will provide support for UUID and other types in future

// User  want to set UUID via ServiceBusMessage, user will have to get to amqp message
// Sending a message with UUID as message ID
ServiceBusMessage messageToSend = new ServiceBusMessage("data");

// A Separate constructor for java native UUID class , 
// I have tested with current proton-j library and it works.

AmqpMessageId uuidId =  new AmqpMessageId(UUID.randomUUID()); 
messageToSend.getAmqpAnnotatedMessage().getProperties().setMessageId(uuidId);

We can have something like this in AmqpMessageId.java
AmqpMessageId {

    private UUID messageIdUUID;
    private Long messageIdLong;

    private MessageIdType type;

    public AmqpMessageId (String);
    public AmqpMessageId (UUID);
    public AmqpMessageId (Long);
   
  public Object getMessageAsUUID () ;

    @Override
    public String toString() {
        switch (type) {
            case STRING:
                return this.messageId;
            case UUID:
                return this.messageIdUUID.toString();
            case LONG:
                return this.messageIdLong.toString();
            default:
                // throw Error
        }
       
    }

    private enum MessageIdType {
        STRING,
        UUID,
        LONG;
    }

[JoshLove-msft]

Response to this comment :
#17464 (comment)
Question :
"So the Service Bus APIs will continue to be String-only forever? @hemanttanwar Can you provide sample code on how this would be done (either today or in the future) for Java using the approach Josh shares above?"

I have discussed with @JonathanGiles and here are how we will provide support for UUID and other types in future

// User  want to set UUID via ServiceBusMessage, user will have to get to amqp message
// Sending a message with UUID as message ID
ServiceBusMessage messageToSend = new ServiceBusMessage("data");

// A Separate constructor for java native UUID class , 
// I have tested with current proton-j library and it works.

AmqpMessageId uuidId =  new AmqpMessageId(UUID.randomUUID()); 
messageToSend.getAmqpAnnotatedMessage().getProperties().setMessageId(uuidId);

We can have something like this in AmqpMessageId.java
AmqpMessageId {

    private UUID messageIdUUID;
    private Long messageIdLong;

    private MessageIdType type;

    public AmqpMessageId (String);
    public AmqpMessageId (UUID);
    public AmqpMessageId (Long);
   
  public Object getMessageAsUUID () ;

    @Override
    public String toString() {
        switch (type) {
            case STRING:
                return this.messageId;
            case UUID:
                return this.messageIdUUID.toString();
            case LONG:
                return this.messageIdLong.toString();
            default:
                // throw Error
        }
       
    }

    private enum MessageIdType {
        STRING,
        UUID,
        LONG;
    }

• We will probably want to expose a way to get the strongly typed data (in addition to getting the string representation).
• The spec also calls for Binary MessageId - http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-message-id-binary

[hemanttanwar]

Response to this comment :
#17464 (comment)
Question :
"So the Service Bus APIs will continue to be String-only forever? @hemanttanwar Can you provide sample code on how this would be done (either today or in the future) for Java using the approach Josh shares above?"
I have discussed with @JonathanGiles and here are how we will provide support for UUID and other types in future

// User  want to set UUID via ServiceBusMessage, user will have to get to amqp message
// Sending a message with UUID as message ID
ServiceBusMessage messageToSend = new ServiceBusMessage("data");

// A Separate constructor for java native UUID class , 
// I have tested with current proton-j library and it works.

AmqpMessageId uuidId =  new AmqpMessageId(UUID.randomUUID()); 
messageToSend.getAmqpAnnotatedMessage().getProperties().setMessageId(uuidId);

We can have something like this in AmqpMessageId.java
AmqpMessageId {

    private UUID messageIdUUID;
    private Long messageIdLong;

    private MessageIdType type;

    public AmqpMessageId (String);
    public AmqpMessageId (UUID);
    public AmqpMessageId (Long);
   
  public Object getMessageAsUUID () ;

    @Override
    public String toString() {
        switch (type) {
            case STRING:
                return this.messageId;
            case UUID:
                return this.messageIdUUID.toString();
            case LONG:
                return this.messageIdLong.toString();
            default:
                // throw Error
        }
       
    }

    private enum MessageIdType {
        STRING,
        UUID,
        LONG;
    }

• We will probably want to expose a way to get the strongly typed data (in addition to getting the string representation).
• The spec also calls for Binary MessageId - http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-message-id-binary

Yes we can take care of Binary message id just like other types as well.
So we are on same page with this design.
When we implement this feature, we can take care of all the types for message id.

[JonathanGiles]

Assuming that this meets the goals of the service bus v-team, then I can no longer see any reason why this PR can't be merged. Let's make this the last time we break any AMQP API.