-
Notifications
You must be signed in to change notification settings - Fork 21
Adding ServiceBus Trigger Chnages #353
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
base: v4.x-preview
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends fromRpcTypedData
to handle the collectionModelBindingData
case and adds comprehensive tests for this new path.
- Implements a new branch in
fromRpcTypedData
to create clients fromcollectionModelBindingData
- Adds unit tests covering valid, edge, and error scenarios for
collectionModelBindingData
- Ensures fallback behavior remains unchanged
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/converters/fromRpcTypedData.ts | Added else if branch for collectionModelBindingData handling and error wrapping |
test/converters/fromRpcTypedData.test.ts | Introduced tests for successful client creation, undefined source, thrown errors, and non-Error exceptions in the new path |
|
||
it('should handle non-Error exceptions by converting to string for collectionModelBindingData', () => { | ||
const mockResolver = { | ||
createClient: sinon.stub().throws('String exception for collection'), // Non-Error exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stubbed exception message ('String exception for collection') does not match the expected test assertion ('Sinon-provided String exception for collection'). Update either the thrown string to 'Sinon-provided String exception for collection' or adjust the expected error message in the test.
createClient: sinon.stub().throws('String exception for collection'), // Non-Error exception | |
createClient: sinon.stub().throws('Sinon-provided String exception for collection'), // Non-Error exception |
Copilot uses AI. Check for mistakes.
data?.collectionModelBindingData?.modelBindingData[0]?.source, | ||
data?.collectionModelBindingData?.modelBindingData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The optional chaining on data
and its properties is unnecessary here since the enclosing else if
ensures collectionModelBindingData
and modelBindingData
are defined. You can simplify this to data.collectionModelBindingData.modelBindingData[0].source
.
data?.collectionModelBindingData?.modelBindingData[0]?.source, | |
data?.collectionModelBindingData?.modelBindingData | |
data.collectionModelBindingData.modelBindingData[0].source, | |
data.collectionModelBindingData.modelBindingData |
Copilot uses AI. Check for mistakes.
} else if (data.collectionModelBindingData && isDefined(data.collectionModelBindingData.modelBindingData)) { | ||
try { | ||
const resourceFactoryResolver: ResourceFactoryResolver = ResourceFactoryResolver.getInstance(); | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call | ||
return resourceFactoryResolver.createClient( | ||
data?.collectionModelBindingData?.modelBindingData[0]?.source, | ||
data?.collectionModelBindingData?.modelBindingData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider validating that modelBindingData
is non-empty before accessing the first element. If the array is empty, calling createClient
with an undefined source may lead to unintended behavior.
} else if (data.collectionModelBindingData && isDefined(data.collectionModelBindingData.modelBindingData)) { | |
try { | |
const resourceFactoryResolver: ResourceFactoryResolver = ResourceFactoryResolver.getInstance(); | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call | |
return resourceFactoryResolver.createClient( | |
data?.collectionModelBindingData?.modelBindingData[0]?.source, | |
data?.collectionModelBindingData?.modelBindingData | |
} else if ( | |
data.collectionModelBindingData && | |
isDefined(data.collectionModelBindingData.modelBindingData) && | |
data.collectionModelBindingData.modelBindingData.length > 0 | |
) { | |
try { | |
const resourceFactoryResolver: ResourceFactoryResolver = ResourceFactoryResolver.getInstance(); | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call | |
return resourceFactoryResolver.createClient( | |
data.collectionModelBindingData.modelBindingData[0].source, | |
data.collectionModelBindingData.modelBindingData |
Copilot uses AI. Check for mistakes.
Summary
This PR adds support for the autoCompleteMessages configuration option to Service Bus triggers, allowing developers to control whether messages are automatically completed after successful processing.
Background
Currently, the Service Bus trigger in Azure Functions for Node.js lacks explicit control over message completion behavior. This enhancement brings parity with other Azure Functions runtimes by introducing the autoCompleteMessages option that was already defined in the TypeScript definitions but not fully implemented.
Changes Made
Type Definitions (already present)
ServiceBusQueueTriggerOptions.autoCompleteMessages?: boolean - Controls automatic message completion for queue triggers
Enhanced JSDoc documentation referencing official Azure documentation