Pass associatedLinkName to updateDispositionStatus() API#2787
Conversation
| if (this._context.requestResponseLockedMessages.has(this.lockToken!)) { | ||
| let receiverName; | ||
| if (this.sessionId !== undefined) { | ||
| receiverName = this._context.messageSessions[this.sessionId].name; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
ManagementClientclass that need theassociatedLinkNamecan call one of the above instead of expecting the caller to pass in the value.
Thoughts?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
|
|
||
| /** | ||
| * Helper function to retrieve active receiver name, if it exists. | ||
| * |
There was a problem hiding this comment.
Please add the tag @internal here otherwise, the function gets picked up by our doc generation process. For example: https://docs.microsoft.com/en-us/javascript/api/%40azure/service-bus/index?view=azure-node-preview#functions
There was a problem hiding this comment.
Wouldn't all of the methods in utils.ts need to be marked as internal?
| } else if (clientEntityContext.streamingReceiver) { | ||
| receiverName = clientEntityContext.streamingReceiver.name; | ||
| } | ||
| return receiverName!; |
There was a problem hiding this comment.
Since the return value can be undefined as well, please update the return type to be string | undefined and remove the ! here
We use ! on a variable whose type allows undefined but we are sure that the value is not undefined. It is a way to tell typescript compiler to stop complaining as we know the variable cannot be undefined, but couldn't change the type, maybe because it was declared in a code section we don't own.
In this case, we define the variable receiverName and we know it can have undefined, so ! is misused here
2 participants
For more context refer to - #2611 and #2611 (comment)