[service-bus] AdminClient: ForwardTo wasn't applying on update due to bad property ordering in XML (#14692)

The ordering of properties in the XML requests to the Service Bus ATOM API is significant and changing it can have side effects.

In this instance, the ordering issues caused us to appear to have setup forwarding properly for a queue but forwarding was NOT actually enabled. Our previous testing missed this because this data actually round-trips properly through the API but it doesn't trigger whatever actual setting needs to happen to cause forwarding to happen.

This PR reconciles our queue, topic and subscription entities ordering against the .net layer, which acts as the de-facto authority.

Fixes #14539

sdk/servicebus/service-bus/CHANGELOG.md

@@ -6,6 +6,10 @@
   These credential types support rotation via their `update` methods and are an alternative to using the `SharedAccessKeyName/SharedAccessKey` or `SharedAccessSignature` properties in a connection string.
   Resolves [#11891](https://github.com/Azure/azure-sdk-for-js/issues/11891).
 
+### Bug fixes
+
+- Some of the queue properties such as "forwardTo" and "autoDeleteOnIdle" were not being set as requested through the `ServiceBusAdministrationClient.createQueue` method because of a bug w.r.t the ordering of XML properties. The issue has been fixed in [#14692](https://github.com/Azure/azure-sdk-for-js/pull/14692).
+
 ## 7.0.4 (2021-03-31)
 
 ### Bug fixes

sdk/servicebus/service-bus/src/serializers/queueResourceSerializer.ts

@@ -31,6 +31,13 @@ import {
  */
 export function buildQueueOptions(queue: CreateQueueOptions): InternalQueueOptions {
   return {
+    // NOTE: this ordering is extremely important. As an example, misordering of the ForwardTo property
+    // resulted in a customer bug where the Forwarding attributes appeared to be set but the portal was
+    // not picking up on it.
+    //
+    // The authority on this ordering is here:
+    // https://github.com/Azure/azure-sdk-for-net/blob/8af2dfc32c96ef3e340f9d20013bf588d97ea756/sdk/servicebus/Azure.Messaging.ServiceBus/src/Administration/QueuePropertiesExtensions.cs#L20
+
     LockDuration: queue.lockDuration,
     MaxSizeInMegabytes: getStringOrUndefined(queue.maxSizeInMegabytes),
     RequiresDuplicateDetection: getStringOrUndefined(queue.requiresDuplicateDetection),
@@ -42,11 +49,11 @@ export function buildQueueOptions(queue: CreateQueueOptions): InternalQueueOptio
     EnableBatchedOperations: getStringOrUndefined(queue.enableBatchedOperations),
     AuthorizationRules: getRawAuthorizationRules(queue.authorizationRules),
     Status: getStringOrUndefined(queue.status),
+    ForwardTo: getStringOrUndefined(queue.forwardTo),
+    UserMetadata: getStringOrUndefined(queue.userMetadata),
     AutoDeleteOnIdle: getStringOrUndefined(queue.autoDeleteOnIdle),
     EnablePartitioning: getStringOrUndefined(queue.enablePartitioning),
     ForwardDeadLetteredMessagesTo: getStringOrUndefined(queue.forwardDeadLetteredMessagesTo),
-    ForwardTo: getStringOrUndefined(queue.forwardTo),
-    UserMetadata: getStringOrUndefined(queue.userMetadata),
     EntityAvailabilityStatus: getStringOrUndefined(queue.availabilityStatus),
     EnableExpress: getStringOrUndefined(queue.enableExpress)
   };

sdk/servicebus/service-bus/src/serializers/subscriptionResourceSerializer.ts

@@ -36,6 +36,12 @@ export function buildSubscriptionOptions(
   subscription: CreateSubscriptionOptions
 ): InternalSubscriptionOptions {
   return {
+    // NOTE: this ordering is extremely important. As an example, misordering of the ForwardTo property
+    // resulted in a customer bug where the Forwarding attributes appeared to be set but the portal was
+    // not picking up on it.
+    //
+    // The authority on this ordering is here:
+    // https://github.com/Azure/azure-sdk-for-net/blob/8af2dfc32c96ef3e340f9d20013bf588d97ea756/sdk/servicebus/Azure.Messaging.ServiceBus/src/Administration/SubscriptionPropertiesExtensions.cs#L191
     LockDuration: subscription.lockDuration,
     RequiresSession: getStringOrUndefined(subscription.requiresSession),
     DefaultMessageTimeToLive: getStringOrUndefined(subscription.defaultMessageTimeToLive),

sdk/servicebus/service-bus/src/serializers/topicResourceSerializer.ts

@@ -31,6 +31,13 @@ import {
  */
 export function buildTopicOptions(topic: CreateTopicOptions): InternalTopicOptions {
   return {
+    // NOTE: this ordering is extremely important. As an example, misordering of the ForwardTo property
+    // resulted in a customer bug where the Forwarding attributes appeared to be set but the portal was
+    // not picking up on it.
+    //
+    // The authority on this ordering is here:
+    // https://github.com/Azure/azure-sdk-for-net/blob/8af2dfc32c96ef3e340f9d20013bf588d97ea756/sdk/servicebus/Azure.Messaging.ServiceBus/src/Administration/TopicPropertiesExtensions.cs#L175
+
     DefaultMessageTimeToLive: topic.defaultMessageTimeToLive,
     MaxSizeInMegabytes: getStringOrUndefined(topic.maxSizeInMegabytes),
     RequiresDuplicateDetection: getStringOrUndefined(topic.requiresDuplicateDetection),

sdk/servicebus/service-bus/test/public/atomManagement.spec.ts

@@ -17,9 +17,10 @@ import { ServiceBusAdministrationClient, WithResponse } from "../../src";
 import { EntityStatus, EntityAvailabilityStatus } from "../../src";
 import { EnvVarNames, getEnvVars } from "./utils/envVarUtils";
 import { recreateQueue, recreateSubscription, recreateTopic } from "./utils/managementUtils";
-import { EntityNames } from "./utils/testUtils";
+import { EntityNames, TestClientType } from "./utils/testUtils";
 import { TestConstants } from "./testConstants";
 import { AzureNamedKeyCredential } from "@azure/core-auth";
+import { createServiceBusClientForTests, ServiceBusClientForTests } from "./utils/testutils2";
 
 chai.use(chaiAsPromised);
 chai.use(chaiExclude);
@@ -60,6 +61,83 @@ const newManagementEntity2 = EntityNames.MANAGEMENT_NEW_ENTITY_2;
 type AccessRights = ("Manage" | "Send" | "Listen")[];
 const randomDate = new Date();
 
+/**
+ * These tests are just a sanity check that our updates are actually
+ * _doing_ something. We've run into some bugs where we've done things like
+ * enabled forwarding only to find that forwarding isn't happening even though
+ * we can _read_ the attributes back.
+ */
+describe("Atom management - forwarding", () => {
+  let serviceBusClient: ServiceBusClientForTests;
+
+  before(() => {
+    serviceBusClient = createServiceBusClientForTests();
+  });
+
+  after(() => serviceBusClient.test.after());
+
+  afterEach(async () => {
+    serviceBusClient.test.afterEach();
+  });
+
+  it("queue: forwarding", async () => {
+    const willForward = await serviceBusClient.test.createTestEntities(
+      TestClientType.PartitionedQueue
+    );
+    const willBeForwardedTo = await serviceBusClient.test.createTestEntities(
+      TestClientType.UnpartitionedQueue
+    );
+
+    // make it so all messages from `willForward` are forwarded to `willBeForwardedTo`
+    const queueProperties = await serviceBusAtomManagementClient.getQueue(willForward.queue!);
+    queueProperties.forwardTo = willBeForwardedTo.queue!;
+    await serviceBusAtomManagementClient.updateQueue(queueProperties);
+
+    const receiver = await serviceBusClient.test.createReceiveAndDeleteReceiver(willBeForwardedTo);
+    const sender = await serviceBusClient.test.createSender(willForward);
+
+    await sender.sendMessages({
+      body: "forwarded message with queues!"
+    });
+
+    const messages = await receiver.receiveMessages(1);
+    assert.deepEqual(
+      [{ body: "forwarded message with queues!" }],
+      messages.map((m) => ({ body: m.body }))
+    );
+  });
+
+  it("subscription: forwarding", async () => {
+    const willForward = await serviceBusClient.test.createTestEntities(
+      TestClientType.PartitionedSubscription
+    );
+    const willBeForwardedTo = await serviceBusClient.test.createTestEntities(
+      TestClientType.UnpartitionedQueue
+    );
+
+    // make it so all messages from `willForward` are forwarded to `willBeForwardedTo`
+    const subscriptionProperties = await serviceBusAtomManagementClient.getSubscription(
+      willForward.topic!,
+      willForward.subscription!
+    );
+    subscriptionProperties.forwardTo = willBeForwardedTo.queue!;
+    await serviceBusAtomManagementClient.updateSubscription(subscriptionProperties);
+
+    const receiver = await serviceBusClient.test.createReceiveAndDeleteReceiver(willBeForwardedTo);
+    const sender = await serviceBusClient.test.createSender(willForward);
+
+    await sender.sendMessages({
+      body: "forwarded message with subscriptions!"
+    });
+
+    const messages = await receiver.receiveMessages(1);
+    assert.deepEqual(
+      [{ body: "forwarded message with subscriptions!" }],
+      messages.map((m) => ({ body: m.body }))
+    );
+  });
+});
+
 describe("Atom management - Namespace", function(): void {
   it("Get namespace properties", async () => {
     const namespaceProperties = await serviceBusAtomManagementClient.getNamespaceProperties();