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

[ServiceBus] Add optional boolean skipParsingBodyAsJson option #18692

Merged
merged 10 commits into from
Dec 10, 2021

Conversation

jeremymeng
Copy link
Member

@jeremymeng jeremymeng commented Nov 16, 2021

to ServiceBusReceiverOptions and
ServiceBusSessionReceiverOptions. This allows users to control
whether the SDK should skip parsing message body as Json object. By
default, the SDK will attempt parsing message body as Json object.

While updating code, I also fixed sample unit test which is using
out-dated paths thus not finding any sample code files, and removed
two await that are redundant.

Resolves #18630

to `ReceiveMessagesOptions`, `SubscribeOptions`, and
`ServiceBusSessionReceiverOptions`. This allows users to control whether the SDK
should skip parsing message body as Json object. By default the SDK will attempt
parsing message body as Json object.

While updating code, I also fixed sample unit test which is using out-dated
paths thus not finding any sample code files, and removed two `await` that are
redundant.

Resolves Azure#18630
@jeremymeng
Copy link
Member Author

I wonder whether we should make this a property of ServiceBusClientOptions instead

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/service-bus. You can review API changes here

API changes

+     skipParsingBodyAsJson?: boolean;
+     skipParsingBodyAsJson?: boolean;
+     skipParsingBodyAsJson?: boolean;

@jeremymeng
Copy link
Member Author

/azp run js - service-bus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ramya-rao-a
Copy link
Contributor

I wonder whether we should make this a property of ServiceBusClientOptions instead

Since it only applies to receiving side, I wouldnt put it in the ServiceBusclientOptions.
How about adding it only to ServiceBusReceiverOptions and ServiceBusSessionReceiverOptions instead of ReceiveMessagesOptions and SubscribeOptions?

to `ServiceBusReceiverOptions` and remove those on `ReceiveMessagesOptions` and
`SubscribeOptions`
@jeremymeng
Copy link
Member Author

How about adding it only to ServiceBusReceiverOptions and ServiceBusSessionReceiverOptions instead of ReceiveMessagesOptions and SubscribeOptions?

yeah that makes more sense! Updated in last commit

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/service-bus. You can review API changes here

API changes

+     skipParsingBodyAsJson?: boolean;
+     skipParsingBodyAsJson?: boolean;

// this.body = undefined;
// }
// }
// why above when `fromRheaMessage()` already called `defaultDataTransformer.decodeWithType()` earlier on message body
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you sync with @richardpark-msft about 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.

Discussed with @richardpark-msft and we agreed that this code was doing double-decoding. So deleting.

but the message body is already decoded, and _rawAmqpMessage is assigned in
`fromRheaMessage()`
@jeremymeng
Copy link
Member Author

/azp run js - service-bus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Can we have an entry to the changelog?


message.body = defaultDataTransformer.decode(message.body);
message.body = defaultDataTransformer.decode(
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is existing code, but can you check if we even need this step? Shouldnt fromRheaMessage() already call defaultDataTransformer.decode() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, it feels like we should not have this option for management client, and should always either try or skip?

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/service-bus. You can review API changes here

API changes

+     skipParsingBodyAsJson?: boolean;
+     skipParsingBodyAsJson?: boolean;

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

Looks good!

@jeremymeng jeremymeng enabled auto-merge (squash) December 10, 2021 19:00
@jeremymeng jeremymeng merged commit 3bdb490 into Azure:main Dec 10, 2021
@jeremymeng jeremymeng deleted the sb/option-skip-json-parsing branch December 10, 2021 19:28
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Service Bus] Add skipParsingBodyAsJson client option to disable attempting to parse message body as Json
4 participants