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

[service-bus] Add support for sending AmqpAnnotatedMessage (and control destination section of body) #14703

Merged
10 commits merged into from
Apr 7, 2021

Conversation

richardpark-msft
Copy link
Member

Adding in support to send AmqpAnnotatedMessage and also to control where the body is encoded in the sent message (via the bodyType) field.

the body is encoded in the sent message (via the `bodyType`) field.
@richardpark-msft
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

@chradek chradek left a comment

Choose a reason for hiding this comment

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

Overall looks good, especially for a preview.

export function isRheaAmqpSection(
possibleSection: any | RheaAmqpSection
): possibleSection is RheaAmqpSection {
return possibleSection != null && typeof possibleSection.typecode === "number";
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt it also have a "content"?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean this?

/** @internal */
export function isRheaAmqpSection(
  possibleSection: any | RheaAmqpSection
): possibleSection is RheaAmqpSection {
  return (
    possibleSection != null &&
    typeof possibleSection.typecode === "number" &&
    isDefined(possibleSection.content)
  );
}

Copy link
Member

Choose a reason for hiding this comment

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

Updated to that.. let me know what you think.

return (
possibleSection != null &&
typeof possibleSection.typecode === "number" &&
isDefined(possibleSection.content)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for the content to be null/undefined? What if I had sent a message with no body, but all other properties set?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd leave out the content check . We could possibly improve this by making sure typecode is one of the 3 values we accept, otherwise I think it's a "good enough" check for something used internally.

Copy link
Member

@HarshaNalluru HarshaNalluru Apr 6, 2021

Choose a reason for hiding this comment

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

Just checked.. it can be undefined/null.

One observation.. If set undefined and sent, it comes back as null.

Copy link
Member

Choose a reason for hiding this comment

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

Added the check to make sure the received typecode is of the expected ones.

Copy link
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

Let's get this in the preview!

@ghost
Copy link

ghost commented Apr 6, 2021

Hello @HarshaNalluru!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit b8672bb into Azure:master Apr 7, 2021
ghost pushed a commit that referenced this pull request Apr 15, 2021
#14703 missed a few things while translating Annotated message into RheaMessage.
This PR attempts to fill the gaps.
This pull request was closed.
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.

4 participants