-
Notifications
You must be signed in to change notification settings - Fork 5
Generate trigger when using transactions #240
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
Generate trigger when using transactions #240
Conversation
| /// <summary> | ||
| /// Enable cross-entity transactions. | ||
| /// </summary> | ||
| public bool EnableCrossEntityTransactions { get; set; } |
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.
is CrossEntityTransactions a bit too specific in this context? What would be the difference to just calling it EnableTransactions for example? Or maybe something more connected to our transport transactions like SendsAtomicWithReceive?
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.
Perhaps the name is very specific but given the context of Azure Functions with Service Bus, being explicit is closer to what customers will do. I wouldn't even mind calling it AutoComplete because it is a trigger function. However, I would not want to call it anything like the transport transaction mode as that term is really specific to NServiceBus. Not to mention that EnableTransactions would mislead users to believe they are fully transactional.
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.
Not to mention that EnableTransactions would mislead users to believe they are fully transactional.
agreed
However, I would not want to call it anything like the transport transaction mode as that term is really specific to NServiceBus
that might be good though, assuming they are familiar with NSB, it would be fairly clear to them what this means.
I wouldn't even mind calling it AutoComplete
I think I wouldn't do that given AutoComplete isn't directly tied to transactions, just something that is a "side effect" of using transactions.
...ests/ApprovalFiles/SourceGeneratorApprovals2.Can_override_trigger_function_name.approved.txt
Outdated
Show resolved
Hide resolved
src/NServiceBus.AzureFunctions.SourceGenerator/TriggerFunctionGenerator2.cs
Outdated
Show resolved
Hide resolved
src/NServiceBus.AzureFunctions.SourceGenerator/TriggerFunctionGenerator2.cs
Show resolved
Hide resolved
|
the parameter and the property itself allowed the same config setting to be configured in two ways, e.g. |
| ILogger logger, | ||
| ExecutionContext executionContext) | ||
| {{ | ||
| {(syntaxReceiver.enableCrossEntityTransactions |
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.
This is why I'm in favour of using a single method all the time and pass messageReceiver no matter what.
- Less logic here
- Less cognitive load for customers that choose to override the trigger - no need to distinguish between modes and always a single method/same parameters to pass in.
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.
happy to discuss that further, I don't have strong opinions on this.
Less cognitive load for customers that choose to override the trigger
that might be dangerous for customers that write their own trigger as long as we don't have better support to detect whether AutoComplete has been properly configured.
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.
Using a canonical signature to pass in MessageReceiver, how is that dangerous?
src/NServiceBus.AzureFunctions.SourceGenerator.Tests/SourceGeneratorApprovals.cs
Show resolved
Hide resolved
|
I'll go ahead and merge but suggest looking into the comment as I feel we're inflicting unnecessary work on the segment of users that want to override the trigger. |
Add support for cross-entity transactions configuration via a new
NServiceBusTriggerFunctionAttributeattribute.TODO:
NServiceBusEndpointNameAttributewith warning.