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.

Sign up for GitHub

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

[Service Bus] Bug Fix: Correlation Rule Filter with the "label" doesn't filter the messages #13069

Merged
15 commits merged into from
Jan 7, 2021
Merged

[Service Bus] Bug Fix: Correlation Rule Filter with the "label" doesn't filter the messages #13069

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a44a36a
getConnectionString helper method
HarshaNalluru Jan 5, 2021
84494ea
new helper method to sanitize the serializable object
HarshaNalluru Jan 5, 2021
08b872e
New test suite - Potentially, can be expanded to test all kinds of rules
HarshaNalluru Jan 5, 2021
526a83c
todo comment
HarshaNalluru Jan 5, 2021
a4635b7
export sanitizeSerializableObject to use it in the tests
HarshaNalluru Jan 5, 2021
9f46679
sanitizeSerializableObject tests
HarshaNalluru Jan 5, 2021
43e9adf
fix exisiting tests for createRule
HarshaNalluru Jan 5, 2021
3204422
updateRule test fix
HarshaNalluru Jan 5, 2021
1ba7660
changelog
HarshaNalluru Jan 5, 2021
aae2759
doc comments
HarshaNalluru Jan 6, 2021
3f88c75
Helper method
HarshaNalluru Jan 6, 2021
1a58699
remove try catch
HarshaNalluru Jan 6, 2021
40ae9ad
Add a negative case too
HarshaNalluru Jan 6, 2021
98c3d07
isJSONLikeObject
HarshaNalluru Jan 7, 2021
4212559
remove .only
HarshaNalluru Jan 7, 2021
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
2 changes: 2 additions & 0 deletions sdk/servicebus/service-bus/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
33 changes: 24 additions & 9 deletions sdk/servicebus/service-bus/src/util/atomXmlHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,28 @@ export async function executeAtomXmlOperation(
return serializer.deserialize(response);
}

/**
* @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
*/
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
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen a check like this used before, what's the reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to differentiate among the "object"s. This would filter the JSON objects and won't match any array or other kinds of objects.

function func(obj:any){
    console.log(typeof obj ==="object", obj.constructor==={}.constructor);
}

func({abc:1});            // true,  true 
func(["a","b"]);          // true,  false 
func([{"a":1},{"b":2}]);  // true,  false 
func(new Date());         // true,  false 
func(123);                // false,  false

Copy link
Member Author

Choose a reason for hiding this comment

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

SOF References:
https://stackoverflow.com/a/11183002/4137356
https://stackoverflow.com/a/36675341/4137356

Copy link
Member

Choose a reason for hiding this comment

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

That's probably worth putting in as a comment (or even just factoring this out into a small function with a comment).

Copy link
Member

Choose a reason for hiding this comment

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

btw, does {}.constructor construct a new empty object every time it's called? Or is JS smart enough to treat that empty literal as a constant of some sort?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it constructs a new empty object. But nobody references this object, so should be fine? (because the garbage collection takes care of it)

Or do you think there is a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking you only need to create one, statically, unless there's some benefit to recreating it each time.

Copy link
Member Author

@HarshaNalluru HarshaNalluru Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Added a const EMPTY_JSON_OBJECT_CONSTRUCTOR, let me know if that's good/

) {
sanitizeSerializableObject(resource[property]);
}
});
}

/**
* @internal
* @hidden
Expand All @@ -103,15 +125,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",
Expand Down
Loading