From a44a36a50aa5e97d146a32bf28747eec5b4497a7 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 12:03:30 -0800 Subject: [PATCH 01/15] getConnectionString helper method --- sdk/servicebus/service-bus/test/utils/testutils2.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/servicebus/service-bus/test/utils/testutils2.ts b/sdk/servicebus/service-bus/test/utils/testutils2.ts index 69b1021a820a..eb7c4910fb9a 100644 --- a/sdk/servicebus/service-bus/test/utils/testutils2.ts +++ b/sdk/servicebus/service-bus/test/utils/testutils2.ts @@ -494,7 +494,7 @@ export function createServiceBusClientForTests( options?: ServiceBusClientOptions ): ServiceBusClientForTests { const serviceBusClient = new ServiceBusClient( - connectionString(), + getConnectionString(), options ) as ServiceBusClientForTests; @@ -516,7 +516,7 @@ export async function drainReceiveAndDeleteReceiver(receiver: ServiceBusReceiver } } -function connectionString() { +export function getConnectionString() { if (env[EnvVarNames.SERVICEBUS_CONNECTION_STRING] == null) { throw new Error( `No service bus connection string defined in ${EnvVarNames.SERVICEBUS_CONNECTION_STRING}. If you're in a unit test you should not be depending on the deployed environment!` From 84494ea4139dfdd968c513be7214c037d8c6e398 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 12:04:40 -0800 Subject: [PATCH 02/15] new helper method to sanitize the serializable object --- .../service-bus/src/util/atomXmlHelper.ts | 31 +++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts index 3b84f716d3d2..34c8c3e41746 100644 --- a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts +++ b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts @@ -91,6 +91,26 @@ export async function executeAtomXmlOperation( return serializer.deserialize(response); } +/** + * The key value pairs having undefined/null as the values are removed recursively from the object in order to address the following issues + * - ATOM based management operations throw a "Bad Request" error if empty tags are included in the xml request body at top level. + * - At the inner levels, Service assigns the empty strings as values to the empty tags instead of throwing. + * + * @param {{ [key: string]: any }} resource + */ +function sanitizeSerializableObject(resource: { [key: string]: any }) { + Object.keys(resource).forEach(function(property) { + if (resource[property] == undefined) { + delete resource[property]; + } else if ( + typeof resource[property] === "object" && + resource[property].constructor === {}.constructor + ) { + sanitizeSerializableObject(resource[property]); + } + }); +} + /** * @internal * @hidden @@ -103,15 +123,8 @@ export async function executeAtomXmlOperation( export function serializeToAtomXmlRequest(resourceName: string, resource: any): object { const content: any = {}; - // The top level key value pairs having undefined/null as the value are removed in order to address issue where the Service Bus' - // ATOM based management operations throw a "Bad Request" error if empty tags are included in the xml request body at top level. - const processedResource = Object.assign({}, resource); - Object.keys(processedResource).forEach(function(property) { - if (processedResource[property] == undefined) { - delete processedResource[property]; - } - }); - content[resourceName] = processedResource; + content[resourceName] = Object.assign({}, resource); + sanitizeSerializableObject(content[resourceName]); content[resourceName][Constants.XML_METADATA_MARKER] = { xmlns: "http://schemas.microsoft.com/netservices/2010/10/servicebus/connect", From 08b872e053675f24228bf34c489d787bda5f162b Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 12:05:19 -0800 Subject: [PATCH 03/15] New test suite - Potentially, can be expanded to test all kinds of rules --- .../service-bus/test/atomRuleFilters.spec.ts | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts diff --git a/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts new file mode 100644 index 000000000000..e3f64051868e --- /dev/null +++ b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts @@ -0,0 +1,75 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import chai from "chai"; +import chaiAsPromised from "chai-as-promised"; +import chaiExclude from "chai-exclude"; +import * as dotenv from "dotenv"; +import { + CorrelationRuleFilter, + ServiceBusReceivedMessage, + ServiceBusClient, + ServiceBusMessage, + SqlRuleFilter +} from "../src"; +import { ServiceBusAdministrationClient } from "../src/serviceBusAtomManagementClient"; +import { DEFAULT_RULE_NAME } from "../src/util/constants"; +import { recreateSubscription, recreateTopic } from "./utils/managementUtils"; +import { getConnectionString } from "./utils/testutils2"; + +chai.use(chaiAsPromised); +chai.use(chaiExclude); +const should = chai.should(); + +dotenv.config(); + +const serviceBusAtomManagementClient: ServiceBusAdministrationClient = new ServiceBusAdministrationClient( + getConnectionString() +); + +describe("Filter messages with the rules set by the ATOM API", () => { + const topicName = "new-topic"; + const subscriptionName = "new-subscription"; + const serviceBusClient = new ServiceBusClient(getConnectionString()); + + beforeEach(async () => { + await recreateTopic(topicName); + await recreateSubscription(topicName, subscriptionName); + await serviceBusAtomManagementClient.deleteRule(topicName, subscriptionName, DEFAULT_RULE_NAME); + }); + + after(async () => { + await serviceBusClient.close(); + }); + + async function verifyRuleFilter( + messageToSend: ServiceBusMessage, + filter: SqlRuleFilter | CorrelationRuleFilter, + toCheck: (msg: ServiceBusReceivedMessage) => void + ) { + await serviceBusAtomManagementClient.createRule( + topicName, + subscriptionName, + "rule-name", + filter + ); + try { + await serviceBusClient.createSender(topicName).sendMessages(messageToSend); + } catch (error) { + console.log(error); + } + const receivedMessages = await serviceBusClient + .createReceiver(topicName, subscriptionName) + .receiveMessages(1); + should.equal(receivedMessages.length, 1, "Unexpected number of messages received"); + + toCheck(receivedMessages[0]); + } + + it("subject", async () => { + const subject = "new-subject"; + await verifyRuleFilter({ body: "random-body", subject }, { subject }, (msg) => { + chai.assert.deepEqual(msg.subject, subject, "Unexpected subject on the message"); + }); + }); +}); From 526a83c0d1800db130bc7ef85193f2190fc31212 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 12:06:49 -0800 Subject: [PATCH 04/15] todo comment --- sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts index e3f64051868e..fc3d754ad21d 100644 --- a/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts +++ b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts @@ -72,4 +72,6 @@ describe("Filter messages with the rules set by the ATOM API", () => { chai.assert.deepEqual(msg.subject, subject, "Unexpected subject on the message"); }); }); + + // TODO: New tests for rule filters to match the sent messages can be added }); From a4635b7de3d263f44dd78c56332fc165af0ce05c Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 12:50:56 -0800 Subject: [PATCH 05/15] export sanitizeSerializableObject to use it in the tests --- sdk/servicebus/service-bus/src/util/atomXmlHelper.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts index 34c8c3e41746..2b6ce7522878 100644 --- a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts +++ b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts @@ -92,13 +92,15 @@ export async function executeAtomXmlOperation( } /** + * @internal + * @hidden * The key value pairs having undefined/null as the values are removed recursively from the object in order to address the following issues * - ATOM based management operations throw a "Bad Request" error if empty tags are included in the xml request body at top level. * - At the inner levels, Service assigns the empty strings as values to the empty tags instead of throwing. * * @param {{ [key: string]: any }} resource */ -function sanitizeSerializableObject(resource: { [key: string]: any }) { +export function sanitizeSerializableObject(resource: { [key: string]: any }) { Object.keys(resource).forEach(function(property) { if (resource[property] == undefined) { delete resource[property]; From 9f466795e0766215b8fd3ec1b1db0efa320f15a0 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 12:57:15 -0800 Subject: [PATCH 06/15] sanitizeSerializableObject tests --- .../service-bus/test/internal/atomXml.spec.ts | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts index 66fb885de082..222955cabfd9 100644 --- a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts +++ b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts @@ -9,7 +9,8 @@ const assert = chai.assert; import { AtomXmlSerializer, deserializeAtomXmlResponse, - executeAtomXmlOperation + executeAtomXmlOperation, + sanitizeSerializableObject } from "../../src/util/atomXmlHelper"; import * as Constants from "../../src/util/constants"; import { ServiceBusAdministrationClient } from "../../src/serviceBusAtomManagementClient"; @@ -1121,4 +1122,76 @@ describe("ATOM Serializers", () => { assertEmptyArray(result); }); }); + + describe("key-value pairs having undefined/null as the values to be sanitized with sanitizeSerializableObject", function() { + [ + { + title: "queue options with undefined fields", + input: { + DefaultMessageTimeToLive: undefined, + MaxSizeInMegabytes: undefined, + RequiresDuplicateDetection: undefined, + DuplicateDetectionHistoryTimeWindow: undefined, + EnableBatchedOperations: undefined, + AuthorizationRules: undefined, + Status: undefined, + UserMetadata: undefined, + SupportOrdering: undefined, + AutoDeleteOnIdle: undefined, + EnablePartitioning: undefined, + EntityAvailabilityStatus: undefined, + EnableExpress: undefined + }, + output: {} + }, + { + title: "correlation filter with some fields undefined ", + input: { + Filter: { + CorrelationId: undefined, + Label: "new-subject", + To: undefined, + ReplyTo: undefined, + ReplyToSessionId: undefined, + ContentType: undefined, + SessionId: undefined, + MessageId: undefined, + Properties: undefined, + $: { + "p4:type": "CorrelationFilter", + "xmlns:p4": "http://www.w3.org/2001/XMLSchema-instance" + } + }, + Action: { + $: { + "p4:type": "EmptyRuleAction", + "xmlns:p4": "http://www.w3.org/2001/XMLSchema-instance" + } + }, + Name: "rule-name" + }, + output: { + Filter: { + Label: "new-subject", + $: { + "p4:type": "CorrelationFilter", + "xmlns:p4": "http://www.w3.org/2001/XMLSchema-instance" + } + }, + Action: { + $: { + "p4:type": "EmptyRuleAction", + "xmlns:p4": "http://www.w3.org/2001/XMLSchema-instance" + } + }, + Name: "rule-name" + } + } + ].forEach((testCase) => { + it(testCase.title, () => { + sanitizeSerializableObject(testCase.input); + chai.assert.deepEqual(testCase.input, testCase.output as any); + }); + }); + }); }); From 43e9adf93663ac29a796f305f94f77796e59459b Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 13:12:14 -0800 Subject: [PATCH 07/15] fix exisiting tests for createRule --- .../service-bus/test/atomManagement.spec.ts | 244 +++++++++--------- 1 file changed, 122 insertions(+), 122 deletions(-) diff --git a/sdk/servicebus/service-bus/test/atomManagement.spec.ts b/sdk/servicebus/service-bus/test/atomManagement.spec.ts index a16b577dd17f..52cf5acd85f1 100644 --- a/sdk/servicebus/service-bus/test/atomManagement.spec.ts +++ b/sdk/servicebus/service-bus/test/atomManagement.spec.ts @@ -1712,135 +1712,135 @@ describe(`createSubscription() using different variations to the input parameter }); }); -// Rule tests -const createRuleTests: { - testCaseTitle: string; - input: Omit | undefined; - output: RuleProperties; -}[] = [ - { - testCaseTitle: "Undefined rule options", - input: undefined, - output: { - filter: { - sqlExpression: "1=1", - sqlParameters: undefined - }, - action: { - sqlExpression: undefined, - sqlParameters: undefined - }, - name: managementRule1 - } - }, - { - testCaseTitle: "Sql Filter rule options", - input: { - filter: { - sqlExpression: "stringValue = @stringParam AND intValue = @intParam", - sqlParameters: { "@intParam": 1, "@stringParam": "b" } - }, - action: { sqlExpression: "SET a='b'" } +describe(`createRule() using different variations to the input parameter "ruleOptions"`, function(): void { + beforeEach(async () => { + await recreateTopic(managementTopic1); + await recreateSubscription(managementTopic1, managementSubscription1); + }); + afterEach(async () => { + await deleteEntity(EntityType.TOPIC, managementTopic1); + }); + + // Rule tests + const createRuleTests: { + testCaseTitle: string; + input: Omit | undefined; + output: RuleProperties; + }[] = [ + { + testCaseTitle: "Undefined rule options", + input: undefined, + output: { + filter: { + sqlExpression: "1=1", + sqlParameters: undefined + }, + action: { + sqlExpression: undefined, + sqlParameters: undefined + }, + name: managementRule1 + } }, - output: { - filter: { - sqlExpression: "stringValue = @stringParam AND intValue = @intParam", - sqlParameters: { "@intParam": 1, "@stringParam": "b" } - }, - action: { - sqlExpression: "SET a='b'", - sqlParameters: undefined - }, - name: managementRule1 - } - }, - { - testCaseTitle: "Correlation Filter rule options with a single property", - input: { - filter: { - correlationId: "abcd", - applicationProperties: { - randomState: "WA" - } + { + testCaseTitle: "Sql Filter rule options", + input: { + filter: { + sqlExpression: "stringValue = @stringParam AND intValue = @intParam", + sqlParameters: { "@intParam": 1, "@stringParam": "b" } + }, + action: { sqlExpression: "SET a='b'" } }, - action: { sqlExpression: "SET sys.label='GREEN'" } + output: { + filter: { + sqlExpression: "stringValue = @stringParam AND intValue = @intParam", + sqlParameters: { "@intParam": 1, "@stringParam": "b" } + }, + action: { + sqlExpression: "SET a='b'", + sqlParameters: undefined + }, + name: managementRule1 + } }, - output: { - filter: { - correlationId: "abcd", - contentType: "", - subject: "", - messageId: "", - replyTo: "", - replyToSessionId: "", - sessionId: "", - to: "", - applicationProperties: { - randomState: "WA" - } - }, - action: { - sqlExpression: "SET sys.label='GREEN'", - sqlParameters: undefined - }, - name: managementRule1 - } - }, - { - testCaseTitle: "Correlation Filter rule options with multiple properties", - input: { - filter: { - correlationId: "abcd", - applicationProperties: { - randomState: "WA", - randomCountry: "US", - randomInt: 25, - randomIntDisguisedAsDouble: 3.0, - randomDouble: 12.4, - randomBool: true, - randomDate: randomDate - } + { + testCaseTitle: "Correlation Filter rule options with a single property", + input: { + filter: { + correlationId: "abcd", + applicationProperties: { + randomState: "WA" + } + }, + action: { sqlExpression: "SET sys.label='GREEN'" } }, - action: { sqlExpression: "SET sys.label='GREEN'" } + output: { + filter: { + correlationId: "abcd", + contentType: undefined, + subject: undefined, + messageId: undefined, + replyTo: undefined, + replyToSessionId: undefined, + sessionId: undefined, + to: undefined, + applicationProperties: { + randomState: "WA" + } + }, + action: { + sqlExpression: "SET sys.label='GREEN'", + sqlParameters: undefined + }, + name: managementRule1 + } }, - output: { - filter: { - correlationId: "abcd", - contentType: "", - subject: "", - messageId: "", - replyTo: "", - replyToSessionId: "", - sessionId: "", - to: "", - applicationProperties: { - randomState: "WA", - randomCountry: "US", - randomInt: 25, - randomIntDisguisedAsDouble: 3.0, - randomDouble: 12.4, - randomBool: true, - randomDate: randomDate - } - }, - action: { - sqlExpression: "SET sys.label='GREEN'", - sqlParameters: undefined + { + testCaseTitle: "Correlation Filter rule options with multiple properties", + input: { + filter: { + correlationId: "abcd", + applicationProperties: { + randomState: "WA", + randomCountry: "US", + randomInt: 25, + randomIntDisguisedAsDouble: 3.0, + randomDouble: 12.4, + randomBool: true, + randomDate: randomDate + } + }, + action: { sqlExpression: "SET sys.label='GREEN'" } }, - name: managementRule1 + output: { + filter: { + correlationId: "abcd", + contentType: undefined, + subject: undefined, + messageId: undefined, + replyTo: undefined, + replyToSessionId: undefined, + sessionId: undefined, + to: undefined, + applicationProperties: { + randomState: "WA", + randomCountry: "US", + randomInt: 25, + randomIntDisguisedAsDouble: 3.0, + randomDouble: 12.4, + randomBool: true, + randomDate: randomDate + } + }, + action: { + sqlExpression: "SET sys.label='GREEN'", + sqlParameters: undefined + }, + name: managementRule1 + } } - } -]; -createRuleTests.forEach((testCase) => { - describe(`createRule() using different variations to the input parameter "ruleOptions"`, function(): void { - beforeEach(async () => { - await recreateTopic(managementTopic1); - await recreateSubscription(managementTopic1, managementSubscription1); - }); - afterEach(async () => { - await deleteEntity(EntityType.TOPIC, managementTopic1); - }); - + ]; + createRuleTests.forEach((testCase) => { it(`${testCase.testCaseTitle}`, async () => { const response = await createEntity( EntityType.RULE, From 3204422ab238e5b2028a4d895b121f50068b941d Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 13:16:00 -0800 Subject: [PATCH 08/15] updateRule test fix --- .../service-bus/test/atomManagement.spec.ts | 118 +++++++++--------- 1 file changed, 56 insertions(+), 62 deletions(-) diff --git a/sdk/servicebus/service-bus/test/atomManagement.spec.ts b/sdk/servicebus/service-bus/test/atomManagement.spec.ts index 52cf5acd85f1..56af3185452d 100644 --- a/sdk/servicebus/service-bus/test/atomManagement.spec.ts +++ b/sdk/servicebus/service-bus/test/atomManagement.spec.ts @@ -2300,74 +2300,68 @@ describe(`createRule() using different variations to the input parameter "ruleOp }); // Rule tests -[ - { - testCaseTitle: "Sql Filter rule options", - input: { - filter: { - sqlExpression: "stringValue = @stringParam", - sqlParameters: { "@stringParam": "b" } - }, - action: { sqlExpression: "SET a='c'" } - }, - output: { - filter: { - sqlExpression: "stringValue = @stringParam", - sqlParameters: { "@stringParam": "b" } - }, - action: { - sqlExpression: "SET a='c'", - sqlParameters: undefined - }, +describe(`updateRule() using different variations to the input parameter "ruleOptions"`, function(): void { + beforeEach(async () => { + await recreateTopic(managementTopic1); + await recreateSubscription(managementTopic1, managementSubscription1); + await createEntity(EntityType.RULE, managementRule1, managementTopic1, managementSubscription1); + }); - name: managementRule1 - } - }, - { - testCaseTitle: "Correlation Filter rule options", - input: { - filter: { - correlationId: "defg" + afterEach(async () => { + await deleteEntity(EntityType.TOPIC, managementTopic1); + }); + [ + { + testCaseTitle: "Sql Filter rule options", + input: { + filter: { + sqlExpression: "stringValue = @stringParam", + sqlParameters: { "@stringParam": "b" } + }, + action: { sqlExpression: "SET a='c'" } }, - action: { sqlExpression: "SET sys.label='RED'" } + output: { + filter: { + sqlExpression: "stringValue = @stringParam", + sqlParameters: { "@stringParam": "b" } + }, + action: { + sqlExpression: "SET a='c'", + sqlParameters: undefined + }, + + name: managementRule1 + } }, - output: { - filter: { - correlationId: "defg", - contentType: "", - subject: "", - messageId: "", - replyTo: "", - replyToSessionId: "", - sessionId: "", - to: "", - applicationProperties: undefined - }, - action: { - sqlExpression: "SET sys.label='RED'", - sqlParameters: undefined + { + testCaseTitle: "Correlation Filter rule options", + input: { + filter: { + correlationId: "defg" + }, + action: { sqlExpression: "SET sys.label='RED'" } }, + output: { + filter: { + correlationId: "defg", + contentType: undefined, + subject: undefined, + messageId: undefined, + replyTo: undefined, + replyToSessionId: undefined, + sessionId: undefined, + to: undefined, + applicationProperties: undefined + }, + action: { + sqlExpression: "SET sys.label='RED'", + sqlParameters: undefined + }, - name: managementRule1 + name: managementRule1 + } } - } -].forEach((testCase) => { - describe(`updateRule() using different variations to the input parameter "ruleOptions"`, function(): void { - beforeEach(async () => { - await recreateTopic(managementTopic1); - await recreateSubscription(managementTopic1, managementSubscription1); - await createEntity( - EntityType.RULE, - managementRule1, - managementTopic1, - managementSubscription1 - ); - }); - - afterEach(async () => { - await deleteEntity(EntityType.TOPIC, managementTopic1); - }); - + ].forEach((testCase) => { it(`${testCase.testCaseTitle}`, async () => { try { const response = await updateEntity( From 1ba76605bfdf2dd5f1d2d456a0a486e8d9ce8d1f Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 13:24:17 -0800 Subject: [PATCH 09/15] changelog --- sdk/servicebus/service-bus/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/servicebus/service-bus/CHANGELOG.md b/sdk/servicebus/service-bus/CHANGELOG.md index 588bed883618..389779397c5a 100644 --- a/sdk/servicebus/service-bus/CHANGELOG.md +++ b/sdk/servicebus/service-bus/CHANGELOG.md @@ -8,6 +8,8 @@ - Updates documentation for `ServiceBusMessage` to call out that the `body` field must be converted to a byte array or `Buffer` when cross-language compatibility while receiving events is required. +- [Bug Fix] Correlation Rule Filter with the "label" set using the `createRule()` method doesn't filter the messages to the subscription. + The bug has been fixed in [PR 13069](https://github.com/Azure/azure-sdk-for-js/pull/13069), also fixes the related issues where the messages are not filtered when a subset of properties are set in the correlation filter. ## 7.0.0 (2020-11-23) From aae275993c406ceeb19c626529152f6cca6366f7 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 5 Jan 2021 17:26:40 -0800 Subject: [PATCH 10/15] doc comments --- sdk/servicebus/service-bus/src/util/atomXmlHelper.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts index 2b6ce7522878..cd037e90035f 100644 --- a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts +++ b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts @@ -94,9 +94,12 @@ export async function executeAtomXmlOperation( /** * @internal * @hidden - * The key value pairs having undefined/null as the values are removed recursively from the object in order to address the following issues - * - ATOM based management operations throw a "Bad Request" error if empty tags are included in the xml request body at top level. - * - At the inner levels, Service assigns the empty strings as values to the empty tags instead of throwing. + * The key-value pairs having undefined/null as the values would lead to the empty tags in the serialized XML request. + * Empty tags in the request body is problematic because of the following reasons. + * - ATOM based management operations throw a "Bad Request" error if empty tags are included in the XML request body at top level. + * - At the inner levels, Service assigns the empty strings as values to the empty tags instead of throwing an error. + * + * This method recursively removes the key-value pairs with undefined/null as the values from the request object that is to be serialized. * * @param {{ [key: string]: any }} resource */ From 3f88c75dbc1826c2704591eb69012dfcedeb74c8 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 6 Jan 2021 15:14:40 -0800 Subject: [PATCH 11/15] Helper method --- .../service-bus/src/util/atomXmlHelper.ts | 24 +++++++++++++++---- .../service-bus/test/internal/atomXml.spec.ts | 16 +++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts index cd037e90035f..246394a0685b 100644 --- a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts +++ b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts @@ -91,6 +91,25 @@ export async function executeAtomXmlOperation( return serializer.deserialize(response); } +/** + * This helper method returns true if an object is JSON, returns false otherwise. + */ +export function isJSONObject(value: any): boolean { + // `value.constructor === {}.constructor` differentiates among the "object"s, + // would filter the JSON objects and won't match any array or other kinds of objects + + // ------------------------------------------------------------------------------- + // Few examples | typeof obj ==="object" | obj.constructor==={}.constructor + // ------------------------------------------------------------------------------- + // {abc:1} | true | true + // ["a","b"] | true | false + // [{"a":1},{"b":2}] | true | false + // new Date() | true | false + // 123 | false | false + // ------------------------------------------------------------------------------- + return typeof value === "object" && value.constructor === {}.constructor; +} + /** * @internal * @hidden @@ -107,10 +126,7 @@ export function sanitizeSerializableObject(resource: { [key: string]: any }) { Object.keys(resource).forEach(function(property) { if (resource[property] == undefined) { delete resource[property]; - } else if ( - typeof resource[property] === "object" && - resource[property].constructor === {}.constructor - ) { + } else if (isJSONObject(resource[property])) { sanitizeSerializableObject(resource[property]); } }); diff --git a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts index 222955cabfd9..b87e6b0304d1 100644 --- a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts +++ b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts @@ -10,6 +10,7 @@ import { AtomXmlSerializer, deserializeAtomXmlResponse, executeAtomXmlOperation, + isJSONObject, sanitizeSerializableObject } from "../../src/util/atomXmlHelper"; import * as Constants from "../../src/util/constants"; @@ -1194,4 +1195,19 @@ describe("ATOM Serializers", () => { }); }); }); + + describe("isJSONObject helper method", () => { + [ + { input: { abc: 1 }, output: true }, + { input: ["a", "b"], output: false }, + { input: [{ a: 1 }, { b: { c: 3, d: "x" } }], output: false }, + { input: new Date(), output: false }, + { input: 123, output: false }, + { input: "abc", output: false } + ].forEach((testCase) => { + it(`${JSON.stringify(testCase.input)}`, () => { + chai.assert.equal(isJSONObject(testCase.input), testCase.output); + }); + }); + }); }); From 1a58699122cc23daf1b8cbbf95a584f1efb261df Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 6 Jan 2021 15:31:34 -0800 Subject: [PATCH 12/15] remove try catch --- sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts index fc3d754ad21d..00d0224a09fd 100644 --- a/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts +++ b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts @@ -53,11 +53,9 @@ describe("Filter messages with the rules set by the ATOM API", () => { "rule-name", filter ); - try { - await serviceBusClient.createSender(topicName).sendMessages(messageToSend); - } catch (error) { - console.log(error); - } + + await serviceBusClient.createSender(topicName).sendMessages(messageToSend); + const receivedMessages = await serviceBusClient .createReceiver(topicName, subscriptionName) .receiveMessages(1); From 40ae9ad5b48ce285d3e10eadbe5d5793c646dcd9 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 6 Jan 2021 15:58:10 -0800 Subject: [PATCH 13/15] Add a negative case too --- .../service-bus/test/atomRuleFilters.spec.ts | 40 +++++++++++++++---- 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts index 00d0224a09fd..bdf10d32987c 100644 --- a/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts +++ b/sdk/servicebus/service-bus/test/atomRuleFilters.spec.ts @@ -43,8 +43,9 @@ describe("Filter messages with the rules set by the ATOM API", () => { }); async function verifyRuleFilter( - messageToSend: ServiceBusMessage, + messagesToSend: ServiceBusMessage[], filter: SqlRuleFilter | CorrelationRuleFilter, + numberOfMessagesToBeFiltered: number, toCheck: (msg: ServiceBusReceivedMessage) => void ) { await serviceBusAtomManagementClient.createRule( @@ -54,21 +55,46 @@ describe("Filter messages with the rules set by the ATOM API", () => { filter ); - await serviceBusClient.createSender(topicName).sendMessages(messageToSend); + await serviceBusClient.createSender(topicName).sendMessages(messagesToSend); + + // Making sure the subscription has the expected number of messages + should.equal( + ( + await serviceBusAtomManagementClient.getSubscriptionRuntimeProperties( + topicName, + subscriptionName + ) + ).totalMessageCount, + numberOfMessagesToBeFiltered, + "Unexpected number of messages filtered" + ); const receivedMessages = await serviceBusClient .createReceiver(topicName, subscriptionName) - .receiveMessages(1); - should.equal(receivedMessages.length, 1, "Unexpected number of messages received"); + .receiveMessages(5); + should.equal( + receivedMessages.length, + numberOfMessagesToBeFiltered, + "Unexpected number of messages received" + ); + // Making sure the filtered message is same as the expected one. toCheck(receivedMessages[0]); } it("subject", async () => { const subject = "new-subject"; - await verifyRuleFilter({ body: "random-body", subject }, { subject }, (msg) => { - chai.assert.deepEqual(msg.subject, subject, "Unexpected subject on the message"); - }); + await verifyRuleFilter( + [ + { body: "msg-1", subject }, // to be filtered + { body: "msg-2" } // not to be filtered + ], + { subject }, + 1, + (msg) => { + chai.assert.deepEqual(msg.subject, subject, "Unexpected subject on the message"); + } + ); }); // TODO: New tests for rule filters to match the sent messages can be added From 98c3d075396aecc9688de154bc41141965b29436 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 6 Jan 2021 16:05:44 -0800 Subject: [PATCH 14/15] isJSONLikeObject --- .../service-bus/src/util/atomXmlHelper.ts | 22 ++----------------- sdk/servicebus/service-bus/src/util/utils.ts | 21 +++++++++++++++++- .../service-bus/test/internal/atomXml.spec.ts | 10 +++++---- 3 files changed, 28 insertions(+), 25 deletions(-) diff --git a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts index 246394a0685b..8aa45fd9e2dc 100644 --- a/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts +++ b/sdk/servicebus/service-bus/src/util/atomXmlHelper.ts @@ -19,6 +19,7 @@ import { administrationLogger as logger } from "../log"; import { Buffer } from "buffer"; import { parseURL } from "./parseUrl"; +import { isJSONLikeObject } from "./utils"; /** * @internal @@ -91,25 +92,6 @@ export async function executeAtomXmlOperation( return serializer.deserialize(response); } -/** - * This helper method returns true if an object is JSON, returns false otherwise. - */ -export function isJSONObject(value: any): boolean { - // `value.constructor === {}.constructor` differentiates among the "object"s, - // would filter the JSON objects and won't match any array or other kinds of objects - - // ------------------------------------------------------------------------------- - // Few examples | typeof obj ==="object" | obj.constructor==={}.constructor - // ------------------------------------------------------------------------------- - // {abc:1} | true | true - // ["a","b"] | true | false - // [{"a":1},{"b":2}] | true | false - // new Date() | true | false - // 123 | false | false - // ------------------------------------------------------------------------------- - return typeof value === "object" && value.constructor === {}.constructor; -} - /** * @internal * @hidden @@ -126,7 +108,7 @@ export function sanitizeSerializableObject(resource: { [key: string]: any }) { Object.keys(resource).forEach(function(property) { if (resource[property] == undefined) { delete resource[property]; - } else if (isJSONObject(resource[property])) { + } else if (isJSONLikeObject(resource[property])) { sanitizeSerializableObject(resource[property]); } }); diff --git a/sdk/servicebus/service-bus/src/util/utils.ts b/sdk/servicebus/service-bus/src/util/utils.ts index c04e8d95143a..7b8b289ff388 100644 --- a/sdk/servicebus/service-bus/src/util/utils.ts +++ b/sdk/servicebus/service-bus/src/util/utils.ts @@ -282,6 +282,13 @@ export function getBooleanOrUndefined(value: any): boolean | undefined { ); } +/** + * @internal + * @hidden + * Helps in differentiating JSON like objects from other kinds of objects. + */ +const EMPTY_JSON_OBJECT_CONSTRUCTOR = {}.constructor; + /** * @internal * @hidden @@ -289,7 +296,19 @@ export function getBooleanOrUndefined(value: any): boolean | undefined { * @param value */ export function isJSONLikeObject(value: any): boolean { - return typeof value === "object" && !(value instanceof Number) && !(value instanceof String); + // `value.constructor === {}.constructor` differentiates among the "object"s, + // would filter the JSON objects and won't match any array or other kinds of objects + + // ------------------------------------------------------------------------------- + // Few examples | typeof obj ==="object" | obj.constructor==={}.constructor + // ------------------------------------------------------------------------------- + // {abc:1} | true | true + // ["a","b"] | true | false + // [{"a":1},{"b":2}] | true | false + // new Date() | true | false + // 123 | false | false + // ------------------------------------------------------------------------------- + return typeof value === "object" && value.constructor === EMPTY_JSON_OBJECT_CONSTRUCTOR; } /** diff --git a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts index b87e6b0304d1..003e959cd074 100644 --- a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts +++ b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts @@ -10,7 +10,6 @@ import { AtomXmlSerializer, deserializeAtomXmlResponse, executeAtomXmlOperation, - isJSONObject, sanitizeSerializableObject } from "../../src/util/atomXmlHelper"; import * as Constants from "../../src/util/constants"; @@ -20,6 +19,7 @@ import { HttpHeaders, HttpOperationResponse, WebResource } from "@azure/core-htt import { TopicResourceSerializer } from "../../src/serializers/topicResourceSerializer"; import { SubscriptionResourceSerializer } from "../../src/serializers/subscriptionResourceSerializer"; import { RuleResourceSerializer } from "../../src/serializers/ruleResourceSerializer"; +import { isJSONLikeObject } from "../../src/util/utils"; const queueProperties = [ Constants.LOCK_DURATION, @@ -1196,9 +1196,11 @@ describe("ATOM Serializers", () => { }); }); - describe("isJSONObject helper method", () => { + describe.only("isJSONLikeObject helper method", () => { [ - { input: { abc: 1 }, output: true }, + { input: {}, output: true }, + { input: { abc: 1, d: undefined }, output: true }, + { input: { a: "2", b: { c: 3, d: "x" } }, output: true }, { input: ["a", "b"], output: false }, { input: [{ a: 1 }, { b: { c: 3, d: "x" } }], output: false }, { input: new Date(), output: false }, @@ -1206,7 +1208,7 @@ describe("ATOM Serializers", () => { { input: "abc", output: false } ].forEach((testCase) => { it(`${JSON.stringify(testCase.input)}`, () => { - chai.assert.equal(isJSONObject(testCase.input), testCase.output); + chai.assert.equal(isJSONLikeObject(testCase.input), testCase.output); }); }); }); From 42125596ee47f486270523e1225c2941fb2b01eb Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 6 Jan 2021 16:12:24 -0800 Subject: [PATCH 15/15] remove .only --- sdk/servicebus/service-bus/test/internal/atomXml.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts index 003e959cd074..c8f2d25363fe 100644 --- a/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts +++ b/sdk/servicebus/service-bus/test/internal/atomXml.spec.ts @@ -1196,7 +1196,7 @@ describe("ATOM Serializers", () => { }); }); - describe.only("isJSONLikeObject helper method", () => { + describe("isJSONLikeObject helper method", () => { [ { input: {}, output: true }, { input: { abc: 1, d: undefined }, output: true },