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

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();