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

Amqp Message Update - API Change #17464

Merged
merged 25 commits into from
Nov 19, 2020
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions eng/code-quality-reports/src/main/resources/revapi/revapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,87 @@
"code": "java.annotation.added",
"new": "class com.azure.storage.blob.models.PageList",
"justification": "Annotation required to resolve deserialization bug."
},{
"code": "java.method.returnTypeChanged",
"old": "method com.azure.core.util.IterableStream<byte[]> com.azure.core.amqp.models.AmqpDataBody::getData()",
"new": "method java.util.List<byte[]> com.azure.core.amqp.models.AmqpDataBody::getData()",
"justification": "Updated "
},
{
"code": "java.class.removed",
"old": "class com.azure.core.amqp.models.AmqpDataBody",
"justification": "Renamed as AmqpMessageBody."
},
{
"code": "java.class.kindChanged",
"old": "interface com.azure.core.amqp.models.AmqpMessageBody",
"new": "class com.azure.core.amqp.models.AmqpMessageBody",
"justification": "AmqpMessageBody is class representing all the new AMQP data types."
},
{
"code": "java.class.nowFinal",
"old": "interface com.azure.core.amqp.models.AmqpMessageBody",
"new": "class com.azure.core.amqp.models.AmqpMessageBody",
"justification": "Made it final."
},
{
"code": "java.class.removed",
"old": "enum com.azure.core.amqp.models.AmqpBodyType",
"justification": "Because It is renamed to AmqpMessageBodyType"
},
{
"code": "java.method.returnTypeChanged",
"old": "method com.azure.core.amqp.models.AmqpBodyType com.azure.core.amqp.models.AmqpMessageBody::getBodyType()",
"new": "method com.azure.core.amqp.models.AmqpMessageBodyType com.azure.core.amqp.models.AmqpMessageBody::getBodyType()",
"justification": "Renamed to match AmqpMessage prefix."
},
{
"code": "java.method.returnTypeChanged",
"old": "method java.lang.String com.azure.core.amqp.models.AmqpMessageProperties::getCorrelationId()",
"new": "method com.azure.core.amqp.models.AmqpMessageId com.azure.core.amqp.models.AmqpMessageProperties::getCorrelationId()",
"justification": "New return type AmqpMessageId."
},
{
"code": "java.method.returnTypeChanged",
"old": "method java.lang.String com.azure.core.amqp.models.AmqpMessageProperties::getMessageId()",
"new": "method com.azure.core.amqp.models.AmqpMessageId com.azure.core.amqp.models.AmqpMessageProperties::getMessageId()",
"justification": "New return type."
},
{
"code": "java.method.returnTypeChanged",
"old": "method java.lang.String com.azure.core.amqp.models.AmqpMessageProperties::getReplyTo()",
"new": "method com.azure.core.amqp.models.AmqpAddress com.azure.core.amqp.models.AmqpMessageProperties::getReplyTo()",
"justification": "New return type."
},
{
"code": "java.method.returnTypeChanged",
"old": "method java.lang.String com.azure.core.amqp.models.AmqpMessageProperties::getTo()",
"new": "method com.azure.core.amqp.models.AmqpAddress com.azure.core.amqp.models.AmqpMessageProperties::getTo()",
"justification": "New return type."
},
{
"code": "java.method.parameterTypeChanged",
"old": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setCorrelationId(===java.lang.String===)",
"new": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setCorrelationId(===com.azure.core.amqp.models.AmqpMessageId===)",
"justification": "Introduced new type AmqpMessageId."
},
{
"code": "java.method.parameterTypeChanged",
"old": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setMessageId(===java.lang.String===)",
"new": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setMessageId(===com.azure.core.amqp.models.AmqpMessageId===)",
"justification":"Introduced new type AmqpMessageId."
},
{
"code": "java.method.parameterTypeChanged",
"old": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setReplyTo(===java.lang.String===)",
"new": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setReplyTo(===com.azure.core.amqp.models.AmqpAddress===)",
"justification": "Introduced new type AmqpAddress."
},
{
"code": "java.method.parameterTypeChanged",
"old": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setTo(===java.lang.String===)",
"new": "parameter com.azure.core.amqp.models.AmqpMessageProperties com.azure.core.amqp.models.AmqpMessageProperties::setTo(===com.azure.core.amqp.models.AmqpAddress===)",
"justification": "Introduced new type AmqpAddress."
}
]
}
Expand Down
1 change: 1 addition & 0 deletions eng/versioning/version_client.txt
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ com.microsoft:microsoft-opentelemetry-exporter-azuremonitor;1.0.0-beta.1;1.0.0-b
# unreleased_<groupId>:<artifactId>;dependency-version
# note: The unreleased dependencies will not be manipulated with the automatic PR creation code.
unreleased_com.azure:azure-core-experimental;1.0.0-beta.9
unreleased_com.azure:azure-core-amqp;1.7.0-beta.3
unreleased_com.azure:azure-messaging-servicebus;7.0.0-beta.7

# Released Beta dependencies: Copy the entry from above, prepend "beta_", remove the current
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.core.amqp.models;

import java.util.Objects;

/**
* This represents amqp address information. This will be used in populating information like 'To', 'ReplyTo' etc.
*
* <p><strong>Create and retrieve address</strong></p>
* {@codesnippet com.azure.core.amqp.models.AmqpAddress.createAndGet}
*
* @see <a href="http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-address-string">
* Address type Format.</a>
JonathanGiles marked this conversation as resolved.
Show resolved Hide resolved
*/
public final class AmqpAddress {

private final String address;

/**
* Creates the {@link AmqpAddress} with given {@code address}.
*
* @param address The address to set for this instance.
* @throws NullPointerException if {@code address} is null.
*/
public AmqpAddress(String address) {
this.address = Objects.requireNonNull(address, "'address' cannot be null.");
}

/**
* {@inheritDoc}
*/
@Override
public int hashCode() {
return address.hashCode();
}

/**
* {@inheritDoc}
*/
@Override
public boolean equals(Object other) {
if (other == null) {
return false;
}

if (this.getClass() != other.getClass()) {
return false;
}

if (this == other) {
return true;
}

return Objects.equals(address, other.toString());
}

/**
* {@inheritDoc}
*/
@Override
public String toString() {
return this.address;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,23 @@

package com.azure.core.amqp.models;

import com.azure.core.util.logging.ClientLogger;

import java.util.Arrays;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;

/**
* The representation of message as defined by AMQP protocol.
*
* @see <a href="http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#section-message-format">
* Amqp Message Format.</a>
* @see <a href="http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#section-message-format" target="_blank">
* Amqp Message Format</a>
* @see AmqpMessageBody
*/
public final class AmqpAnnotatedMessage {
private final ClientLogger logger = new ClientLogger(AmqpAnnotatedMessage.class);
private final AmqpMessageBody amqpMessageBody;
private final Map<String, Object> applicationProperties;
private final Map<String, Object> deliveryAnnotations;
Expand Down Expand Up @@ -42,14 +48,41 @@ public AmqpAnnotatedMessage(AmqpMessageBody body) {

/**
* Creates instance of {@link AmqpAnnotatedMessage} with given {@link AmqpAnnotatedMessage} instance.
* The Current implementation only support {@link AmqpMessageBodyType#DATA DATA} AMQP data type. Track this
Copy link
Member

Choose a reason for hiding this comment

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

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

* <a href="https://github.com/Azure/azure-sdk-for-java/issues/17614" target="_blank">issue</a> to find out support
* for other AMQP types.
*
* <p><strong>How to check for {@link AmqpMessageBodyType} before calling this constructor</strong></p>
* {@codesnippet com.azure.core.amqp.models.AmqpAnnotatedMessage.copyAmqpAnnotatedMessage}

* @param message used to create another instance of {@link AmqpAnnotatedMessage}.
*
* @throws NullPointerException if {@code message} or {@link AmqpAnnotatedMessage#getBody() body} is null.
* @throws UnsupportedOperationException if {@link AmqpMessageBodyType} is {@link AmqpMessageBodyType#SEQUENCE} or
* {@link AmqpMessageBodyType#VALUE}. See code sample above explaining how to check for {@link AmqpMessageBodyType}
* before calling this constructor.
* @throws IllegalStateException for invalid {@link AmqpMessageBodyType}.
*/
public AmqpAnnotatedMessage(AmqpAnnotatedMessage message) {
Objects.requireNonNull(message, "'message' cannot be null.");
amqpMessageBody = Objects.requireNonNull(message.getBody(), "'message.body' cannot be null.");

AmqpMessageBodyType bodyType = message.getBody().getBodyType();
switch (bodyType) {
case DATA:
final byte[] data = message.getBody().getFirstData();
amqpMessageBody = AmqpMessageBody.fromData(Arrays.copyOf(data, data.length));
Copy link
Member

Choose a reason for hiding this comment

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

Why are you copying the byte[] here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I will remove this copy from here.

Copy link
Member

Choose a reason for hiding this comment

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

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

break;
case SEQUENCE:
case VALUE:
throw logger.logExceptionAsError(new UnsupportedOperationException(
String.format(Locale.US, "This constructor only support body type [%s] at present. Track "
+ "this issue, https://github.com/Azure/azure-sdk-for-java/issues/17614 for other body type "
+ "support in future.", AmqpMessageBodyType.DATA.toString())));
hemanttanwar marked this conversation as resolved.
Show resolved Hide resolved
default:
throw logger.logExceptionAsError(new IllegalStateException("Body type not valid "
+ bodyType.toString()));
}

applicationProperties = new HashMap<>(message.getApplicationProperties());
deliveryAnnotations = new HashMap<>(message.getDeliveryAnnotations());
messageAnnotations = new HashMap<>(message.getMessageAnnotations());
Expand All @@ -58,6 +91,7 @@ public AmqpAnnotatedMessage(AmqpAnnotatedMessage message) {
properties = new AmqpMessageProperties(message.getProperties());
}


/**
* Gets the {@link Map} of application properties.
*
Expand All @@ -69,6 +103,10 @@ public Map<String, Object> getApplicationProperties() {

/**
* Gets the {@link AmqpMessageBody} of an amqp message.
* <b>Client should test for {@link AmqpMessageBodyType} before calling corresponding get method on
* {@link AmqpMessageBody}</b>
* <p><strong>How to check for {@link AmqpMessageBodyType}</strong></p>
* {@codesnippet com.azure.core.amqp.models.AmqpBodyType.checkBodyType}
*
* @return the {@link AmqpMessageBody} object.
*/
Expand Down

This file was deleted.

This file was deleted.

Loading