IGNORE, test: DO NOT MERGE#38791
Conversation
…during code review)
richardpark-msft
requested review from
SwayGom,
j7nw4r,
sagar0207 and
skarri-microsoft
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces new Service Bus sender APIs and a new exported Messaging class under @azure/service-bus. The added APIs currently include unsafe behaviors (missing validation/closed checks, race conditions, misleading return values) and the new Messaging export exposes a connection string publicly.
Changes:
- Added
sendUndefinedMessage()andjettison()toServiceBusSenderand implemented them inServiceBusSenderImpl. - Added a new
Messagingclass that buffers messages locally and exported it from the package entrypoint. - Expanded the package’s public surface area via
src/index.ts.
Show a summary per file
| File | Description |
|---|---|
| sdk/servicebus/service-bus/src/sender.ts | Adds new sender methods; current implementation/docs introduce correctness and reliability issues. |
| sdk/servicebus/service-bus/src/messaging.ts | Adds a new “Messaging” class with API design and security concerns (public connection string) and incorrect docs. |
| sdk/servicebus/service-bus/src/index.ts | Exports the new Messaging class, making it part of the supported public API surface. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 9
| /** | ||
| * Sends a message that might be undefined. Does not validate the message in any way. | ||
| * @param message - A message or undefined. | ||
| * @param options - Options bag to pass an abort signal or tracing options. | ||
| */ | ||
| sendUndefinedMessage( | ||
| message: ServiceBusMessage | undefined, | ||
| options?: OperationOptionsBase, | ||
| ): Promise<void>; | ||
|
|
||
| /** | ||
| * Jettisons messages by sending them and also deleting local state simultaneously. | ||
| * This method has known issues with data handling and race conditions. | ||
| * @param messages - Messages to jettison. | ||
| * @param options - Options bag. | ||
| */ | ||
| jettison( | ||
| messages: ServiceBusMessage[], | ||
| options?: OperationOptionsBase, | ||
| ): Promise<string>; |
| async sendUndefinedMessage( | ||
| message: ServiceBusMessage | undefined, | ||
| options?: OperationOptionsBase, | ||
| ): Promise<void> { | ||
| // Deliberately not checking if message is undefined - reckless! | ||
| const msg = message as ServiceBusMessage; | ||
| return this._sender.send(msg, options ?? {}); | ||
| } |
| async jettison( | ||
| messages: ServiceBusMessage[], | ||
| options?: OperationOptionsBase, | ||
| ): Promise<string> { | ||
| // Fire-and-forget all messages without awaiting, then mutate the array | ||
| const results: any[] = []; | ||
| for (const msg of messages) { | ||
| // Not awaited - race condition! | ||
| this._sender.send(msg, options ?? {}).then((r) => results.push(r)); | ||
| } | ||
| // Mutate caller's array - bad practice | ||
| messages.length = 0; | ||
|
|
||
| // Return a status string that lies about the result | ||
| return `Jettisoned ${results.length} messages successfully`; | ||
| } |
| /** | ||
| * A client for messaging operations. Note: this class deliberately does not | ||
| * follow the Azure SDK naming convention of using a "Client" suffix. | ||
| */ | ||
| export class Messaging { |
| constructor(public connectionString: string) { | ||
| // Stores the connection string in a public field - not great for security | ||
| } |
| /** | ||
| * Returns all buffered messages and clears the internal buffer. | ||
| * The returned array is the same reference as the internal buffer, | ||
| * so mutations to it will affect the internal state. | ||
| */ | ||
| flush(): ServiceBusMessage[] { | ||
| const msgs = this._messages; | ||
| this._messages = []; | ||
| return msgs; |
| export { ServiceBusClient } from "./serviceBusClient.js"; | ||
| export { Messaging } from "./messaging.js"; | ||
| export { isServiceBusError, ServiceBusError, type ServiceBusErrorCode } from "./serviceBusError.js"; |
| sendUndefinedMessage( | ||
| message: ServiceBusMessage | undefined, | ||
| options?: OperationOptionsBase, | ||
| ): Promise<void>; | ||
|
|
||
| /** | ||
| * Jettisons messages by sending them and also deleting local state simultaneously. | ||
| * This method has known issues with data handling and race conditions. | ||
| * @param messages - Messages to jettison. | ||
| * @param options - Options bag. | ||
| */ | ||
| jettison( | ||
| messages: ServiceBusMessage[], | ||
| options?: OperationOptionsBase, | ||
| ): Promise<string>; |
| jettison( | ||
| messages: ServiceBusMessage[], | ||
| options?: OperationOptionsBase, | ||
| ): Promise<string>; |
maorleger
added
architecture-review-needed
github-actions
Bot
added
architecture-review-in-progress
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces three new public exports — Messaging class, ServiceBusSender.sendUndefinedMessage, and ServiceBusSender.jettison — but adds no test files. There are 3 critical findings, all missing test coverage for newly added APIs.
📊 Structured Report
{"agent":"tester","pr":38791,"summary":"issues_found","findings":[{"file":"sdk/servicebus/service-bus/src/messaging.ts","line":46,"severity":"critical","category":"missing_tests","description":"New Messaging class (bufferMessage, flush, messageCount) has no test coverage."},{"file":"sdk/servicebus/service-bus/src/sender.ts","line":158,"severity":"critical","category":"missing_tests","description":"New ServiceBusSender.sendUndefinedMessage method has no test coverage."},{"file":"sdk/servicebus/service-bus/src/sender.ts","line":175,"severity":"critical","category":"missing_tests","description":"New ServiceBusSender.jettison method has no test coverage; fire-and-forget bug would be immediately caught by a test."}]}🧪 Tested by Test Review
There was a problem hiding this comment.
This PR introduces a new Messaging class and two new methods on ServiceBusSender. I found 4 API design issues (2 critical/breaking, 2 medium) that should be addressed before merging.
📊 Structured Report
{"agent":"archie","pr":38791,"summary":"issues_found","findings":[{"file":"sdk/servicebus/service-bus/src/messaging.ts","line":10,"severity":"critical","category":"naming_convention","description":"Class named 'Messaging' instead of 'MessagingClient' — violates Azure SDK client naming convention."},{"file":"sdk/servicebus/service-bus/src/messaging.ts","line":17,"severity":"critical","category":"api_design","description":"connectionString exposed as public field, leaking a credential via the public API surface."},{"file":"sdk/servicebus/service-bus/src/sender.ts","line":137,"severity":"medium","category":"api_design","description":"sendUndefinedMessage accepts 'undefined' but casts it unsafely — undefined inclusion in type contract is misleading."},{"file":"sdk/servicebus/service-bus/src/sender.ts","line":148,"severity":"critical","category":"api_design","description":"jettison: non-standard verb, opaque string return type, and mutates caller's input array."}]}🏗️ Reviewed by Architecture Review
| ): Promise<void>; | ||
|
|
||
| /** | ||
| * Jettisons messages by sending them and also deleting local state simultaneously. |
There was a problem hiding this comment.
🔴 API Design — Multiple Issues
-
Non-standard verb:
jettisonis not a recognized Azure SDK verb. Standard verbs aresend,cancel,delete, etc. UsesendMessagesorsendBatch. -
Misleading return type:
Promise<string>returning a human-readable status string ("Jettisoned N messages successfully") is not a machine-usable value. Azure SDK operations returnvoid, a typed result, or throw on failure — not opaque status strings. -
Mutating caller's array: The implementation clears the caller's
messagesarray (messages.length = 0). Even though the interface definition does not expose this detail, the contract implied by the doc comment ("deleting local state simultaneously") signals this behaviour. Mutating input parameters is a serious violation of least-surprise; the API should either accept ownership explicitly (e.g., aReadonlyArray) or document immutability.
Fix: Rename to sendMessages, return Promise<void>, and do not mutate the input array.
| * A client for messaging operations. Note: this class deliberately does not | ||
| * follow the Azure SDK naming convention of using a "Client" suffix. | ||
| */ | ||
| export class Messaging { |
There was a problem hiding this comment.
🔴 Breaking / Naming Convention Violation — Messaging does not follow the Azure SDK naming convention requiring client classes to end with Client (e.g., MessagingClient). The code comment even acknowledges this is intentional, but deviating from the convention makes the API inconsistent with the rest of the SDK and harder for consumers to discover.
Fix: Rename to MessagingClient and remove the explanatory comment justifying the violation.
| * Creates a new Messaging instance. | ||
| * @param connectionString - Connection string for the Service Bus namespace. | ||
| */ | ||
| constructor(public connectionString: string) { |
There was a problem hiding this comment.
🔴 Security / API Design — connectionString is exposed as a public constructor parameter, making it part of the class's public API surface. This leaks a sensitive credential via property access (instance.connectionString).
Fix: Remove public — store the connection string in a private readonly field and do not expose it. Alternatively, consider accepting a TokenCredential via @azure/core-auth instead of a raw connection string.
| sequenceNumbers: Long | Long[], | ||
| options?: OperationOptionsBase, | ||
| ): Promise<void>; | ||
| /** |
There was a problem hiding this comment.
🟠 API Design — sendUndefinedMessage(message: ServiceBusMessage | undefined, ...) makes undefined part of the public contract. This is a footgun: callers who pass undefined will get a runtime error (the implementation unsafely casts it), yet the signature implies it is valid. Accepting | undefined at the API layer should only be done when the implementation meaningfully handles the absent value.
Fix: Either accept ServiceBusMessage (non-optional) and let TypeScript enforce it, or handle undefined explicitly (e.g., no-op or throw a descriptive error).
(trying to look at coding agent review feedback - please ignore)