Skip to content

Commit

Permalink
[ServiceBus] fix issue of accepting session with empty string name (#…
Browse files Browse the repository at this point in the history
…31736)

We have a check for when asking for any session, i.e., not providing a
session id), but not receiving a link having a session id. However, the
check is `if (!this._providedSessionId && !receivedSessionId)` which
also evaluates to `true` for session id of empty string, which is valid
session id.

This PR changes it to check for `undefined` so that session id of empty
string doesn't lead to error being thrown.
  • Loading branch information
jeremymeng authored Nov 20, 2024
1 parent cd338ec commit 0c82aca
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 4 deletions.
11 changes: 7 additions & 4 deletions sdk/servicebus/service-bus/src/session/messageSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export class MessageSession extends LinkEntity<Receiver> {
this._intermediateLink = link;

const receivedSessionId = link.source?.filter?.[Constants.sessionFilterName];
if (!this._providedSessionId && !receivedSessionId) {
if (this._providedSessionId === undefined && receivedSessionId === undefined) {
// When we ask for any sessions (passing option of session-filter: undefined),
// but don't receive one back, check whether service has sent any error.
if (
Expand All @@ -281,7 +281,10 @@ export class MessageSession extends LinkEntity<Receiver> {
// Ideally this code path should never be reached as `MessageSession.createReceiver()` should fail instead
// TODO: https://github.com/Azure/azure-sdk-for-js/issues/9775 to figure out why this code path indeed gets hit.
errorMessage = `Failed to create a receiver. No unlocked sessions available.`;
} else if (this._providedSessionId && receivedSessionId !== this._providedSessionId) {
} else if (
this._providedSessionId !== undefined &&
receivedSessionId !== this._providedSessionId
) {
// This code path is reached if the session is already locked by another receiver.
// TODO: Check why the service would not throw an error or just timeout instead of giving a misleading successful receiver
errorMessage = `Failed to create a receiver for the requested session '${this._providedSessionId}'. It may be locked by another receiver.`;
Expand Down Expand Up @@ -321,7 +324,7 @@ export class MessageSession extends LinkEntity<Receiver> {

const receivedSessionId = this.link.source?.filter?.[Constants.sessionFilterName];

if (!this._providedSessionId) this.sessionId = receivedSessionId;
if (this._providedSessionId === undefined) this.sessionId = receivedSessionId;
this.sessionLockedUntilUtc = convertTicksToDate(
this.link.properties["com.microsoft:locked-until-utc"],
);
Expand All @@ -347,7 +350,7 @@ export class MessageSession extends LinkEntity<Receiver> {

// Fix the unhelpful error messages for the OperationTimeoutError that comes from `rhea-promise`.
if ((errObj as MessagingError).code === "OperationTimeoutError") {
if (this._providedSessionId) {
if (this._providedSessionId !== undefined) {
errObj.message = `Failed to create a receiver for the requested session '${this._providedSessionId}' within allocated time and retry attempts.`;
} else {
errObj.message = "Failed to create a receiver within allocated time and retry attempts.";
Expand Down
19 changes: 19 additions & 0 deletions sdk/servicebus/service-bus/test/public/sessionsTests.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,25 @@ describe("session tests", () => {
err.name.should.equal("AbortError");
}
});

["acceptSession", "acceptNextSession"].forEach((method) => {
it(`${method} on session id of empty string`, async () => {
serviceBusClient = createServiceBusClientForTests();
const entityNames = await serviceBusClient.test.createTestEntities(testClientType);
const testMessage = TestMessage.getSessionSample();
testMessage.sessionId = "";
sender = serviceBusClient.test.addToCleanup(
serviceBusClient.createSender(entityNames.queue ?? entityNames.topic!),
);
await sender.sendMessages(testMessage);
receiver =
method === "acceptSession"
? await serviceBusClient.test.acceptSessionWithPeekLock(entityNames, "")
: await serviceBusClient.test.acceptNextSessionWithPeekLock(entityNames);
const msgs = await receiver.receiveMessages(10);
should.equal(msgs.length, 1, "Unexpected number of messages received");
});
});
});
});

Expand Down

0 comments on commit 0c82aca

Please sign in to comment.