Skip to content

Pass associatedLinkName to updateDispositionStatus() API #2787

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

Merged
ramya-rao-a merged 8 commits into from
May 15, 2019
Merged

Pass associatedLinkName to updateDispositionStatus() API #2787

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f7899cd
Pass associatedLinkName to updateDispositionStatus() API
May 9, 2019
24d2e52
Combine if-else
May 9, 2019
dd71f9b
Use private helper
May 10, 2019
442bc79
Move helper to util and update 7 other usages
May 10, 2019
054ddc6
Address comments
May 13, 2019
44ee9a3
Merge branch 'master' into issue-2611-v2
ramya0820 May 13, 2019
355fb6d
Merge branch 'master' into issue-2611-v2
ramya0820 May 14, 2019
558fbcc
Merge branch 'master' into issue-2611-v2
ramya0820 May 14, 2019
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
4 changes: 4 additions & 0 deletions sdk/servicebus/service-bus/src/core/managementClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,7 @@ export class ManagementClient extends LinkEntity {
async updateDispositionStatus(
lockToken: string,
dispositionStatus: DispositionStatus,
associatedLinkName?: string,
options?: DispositionStatusOptions
): Promise<void> {
throwErrorIfConnectionClosed(this._context.namespace);
Expand Down Expand Up @@ -757,6 +758,9 @@ export class ManagementClient extends LinkEntity {
operation: Constants.operations.updateDisposition
}
};
if (associatedLinkName) {
request.application_properties![Constants.associatedLinkName] = associatedLinkName;
}
request.application_properties![Constants.trackingId] = generate_uuid();
log.mgmt(
"[%s] Update disposition status request body: %O.",
Expand Down
40 changes: 40 additions & 0 deletions sdk/servicebus/service-bus/src/serviceBusMessage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,9 +817,19 @@ export class ServiceBusMessage implements ReceivedMessage {
this.messageId
);
if (this._context.requestResponseLockedMessages.has(this.lockToken!)) {
let receiverName;
if (this.sessionId !== undefined) {
receiverName = this._context.messageSessions[this.sessionId].name;

@ramya-rao-a ramya-rao-a May 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is not guaranteed that this._context.messageSessions[this.sessionId] would exist. The messageSession for the session could have been closed in which case this_context.messageSessions map will not have an entry for this.sessionId.

Also, we are repeating this logic of finding the associated link name in 4 places. Can we create a private helper method, say getAssociatedLinkName() instead?

@ramya-rao-a ramya-rao-a May 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we make such a helper method, then we can use it in all other places, where we do similar if/else checks to get the associated link name. Here is my proposal, let me know what you think

  • Export 2 helpers from the clientEntityContext.ts (or other file as you see fit)
    • getAssociatedReceiverLinkName(clientContext: ClientEntityContext, sessionId?: string)
    • getAssociatedSenderLinkName(clientContext: ClientEntityContext)
  • All the functions in ManagementClient class that need the associatedLinkName can call one of the above instead of expecting the caller to pass in the value.

Thoughts?

@ramya0820 ramya0820 May 10, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

About the sessionId not being present when it's expired, in such cases there is no associated link name to pass. Hence, the implementations holds good.

About refactoring to use these 2 additions on the interface, I think it would need a design discussion and further efforts. For this PR, we can unblock ourselves with current changes and take it up later?

@ramya-rao-a ramya-rao-a May 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

About the sessionId not being present when it's expired, in such cases there is no associated link name to pass. Hence, the implementations holds good.

I was referring to the error we would get when trying to get the name i.e
for the session expired scenario, in the below code, we will get "name doesnt exist on undefined" error since this._context.messageSessions[this.sessionId] will return undefined

if (this.sessionId !== undefined) {
      receiverName = this._context.messageSessions[this.sessionId].name;
    }

About refactoring to use these 2 additions on the interface, I think it would need a design discussion

The helper functions need not be on the interface of ClientEntityContext. The way I have suggested above, they can be stand-alone helper functions and not inside any interface or class.

For this PR, we can unblock ourselves with current changes and take it up later?

At present we have the _getAssociatedReceiverName() as part of this PR to reduce code duplication in 4 places. With slight modification (take context and sessionId as input parameters instead of using from this), the same helper can be used from 7 other places (3 in Receiver, 2 in QueueClient and 2 in SubscriptionClient). We should atleast pick this up for this PR.

Updating the functions in the ManagementClient class to use this helper (and another for sender) instead of taking associatedLinkName can be taken up later.

@ramya0820 ramya0820 May 10, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh okay, updating to include the undefined check on messageSessions.

Adding the helper function to utils.ts and updating the receiver related references
(Adding it to ClientEntityContext seemed confusing/difficult at first since this was bookkeeping a lot of things.)

} else if (this._context.batchingReceiver) {
receiverName = this._context.batchingReceiver.name;
} else if (this._context.streamingReceiver) {
receiverName = this._context.streamingReceiver.name;
}

await this._context.managementClient!.updateDispositionStatus(
this.lockToken!,
DispositionStatus.completed,
receiverName,
{
sessionId: this.sessionId
}
Expand Down Expand Up @@ -849,9 +859,19 @@ export class ServiceBusMessage implements ReceivedMessage {
this.messageId
);
if (this._context.requestResponseLockedMessages.has(this.lockToken!)) {
let receiverName;
if (this.sessionId !== undefined) {
receiverName = this._context.messageSessions[this.sessionId].name;
} else if (this._context.batchingReceiver) {
receiverName = this._context.batchingReceiver.name;
} else if (this._context.streamingReceiver) {
receiverName = this._context.streamingReceiver.name;
}

await this._context.managementClient!.updateDispositionStatus(
this.lockToken!,
DispositionStatus.abandoned,
receiverName,
{ propertiesToModify: propertiesToModify, sessionId: this.sessionId }
);

Expand Down Expand Up @@ -881,9 +901,19 @@ export class ServiceBusMessage implements ReceivedMessage {
this.messageId
);
if (this._context.requestResponseLockedMessages.has(this.lockToken!)) {
let receiverName;
if (this.sessionId !== undefined) {
receiverName = this._context.messageSessions[this.sessionId].name;
} else if (this._context.batchingReceiver) {
receiverName = this._context.batchingReceiver.name;
} else if (this._context.streamingReceiver) {
receiverName = this._context.streamingReceiver.name;
}

await this._context.managementClient!.updateDispositionStatus(
this.lockToken!,
DispositionStatus.defered,
receiverName,
{ propertiesToModify: propertiesToModify, sessionId: this.sessionId }
);

Expand Down Expand Up @@ -923,9 +953,19 @@ export class ServiceBusMessage implements ReceivedMessage {
this.messageId
);
if (this._context.requestResponseLockedMessages.has(this.lockToken!)) {
let receiverName;
if (this.sessionId !== undefined) {
receiverName = this._context.messageSessions[this.sessionId].name;
} else if (this._context.batchingReceiver) {
receiverName = this._context.batchingReceiver.name;
} else if (this._context.streamingReceiver) {
receiverName = this._context.streamingReceiver.name;
}

await this._context.managementClient!.updateDispositionStatus(
this.lockToken!,
DispositionStatus.suspended,
receiverName,
{
deadLetterReason: error.condition,
deadLetterDescription: error.description,
Expand Down