-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Cosmos: Add support for executing pre- and post-triggers #36505
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
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 adds support for executing pre- and post-triggers in Cosmos DB by implementing trigger configuration and execution in Entity Framework Core. The changes enable developers to define database triggers on entity types and have them automatically executed during SaveChanges operations.
Key changes:
- Added trigger type configuration through fluent API with
IsPreTrigger()andIsPostTrigger()methods - Implemented trigger execution in the Cosmos client wrapper during create and replace operations
- Added comprehensive validation to ensure triggers are only defined on root entity types and have proper type configuration
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
CosmosTriggersTest.cs |
New functional test that verifies trigger execution during SaveChanges operations |
CosmosModelValidatorTest.cs |
Unit tests for trigger validation including derived type and missing type detection |
CosmosBuilderExtensionsTest.cs |
Tests for trigger builder extensions and configuration source handling |
CosmosTriggerType.cs |
New enum defining Pre and Post trigger types |
CosmosTriggerExtensions.cs |
Extension methods for getting/setting trigger types on trigger metadata |
CosmosTriggerBuilderExtensions.cs |
Fluent API extensions for configuring trigger types |
CosmosEntityTypeBuilderExtensions.cs |
Entity type builder extensions for defining triggers |
CosmosClientWrapper.cs |
Core implementation that attaches triggers to Cosmos DB operations |
CosmosModelValidator.cs |
Validation logic for trigger configuration rules |
CosmosStrings.resx |
Error message resources for trigger validation |
CosmosAnnotationNames.cs |
New annotation name constant for trigger type metadata |
| Entity type interfaces | Added GetTriggers() method across all entity type interfaces |
Files not reviewed (1)
- src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Language not supported
c5b9812 to
3382dfc
Compare
| /// <param name="triggerBuilder">The builder for the trigger being configured.</param> | ||
| /// <param name="triggerType">The trigger type to configure.</param> | ||
| /// <returns>The same builder instance so that multiple configuration calls can be chained.</returns> | ||
| public static TriggerBuilder HasTriggerType(this TriggerBuilder triggerBuilder, TriggerType triggerType) |
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 introduces public methods, so API review needs to be done as part of the PR review.
Note that TriggerType and TriggerOperation are from Cosmos SDK
3382dfc to
32e7845
Compare
|
cc fyi @Pilchie |
|
Also tagging @kirankumarkolli . |
| entity.HasTrigger("PreInsertTrigger") | ||
| .HasTriggerType(TriggerType.Pre).HasTriggerOperation(TriggerOperation.Create); | ||
| entity.HasTrigger("PostDeleteTrigger") | ||
| .HasTriggerType(TriggerType.Post).HasTriggerOperation(TriggerOperation.Delete); | ||
| entity.HasTrigger("UpdateTrigger") | ||
| .HasTriggerType(TriggerType.Pre).HasTriggerOperation(TriggerOperation.Replace); |
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.
Here's an example of the new API usage
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.
Aren't the trigger type and operation mandatory? If so, I'd expect HasTrigger to have them as required parameters, no?
I get that there will be edge cases where someone wants to change the type/operation for a trigger that has already been defined, but in the 99% case users just want to define a new trigger (it might be acceptable for the edge case to just use the metadata APIs directly).
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.
Makes sense. I'll combine them into one method here, but leave separate on the Metadata API
| entity.HasTrigger("PreInsertTrigger") | ||
| .HasTriggerType(TriggerType.Pre).HasTriggerOperation(TriggerOperation.Create); | ||
| entity.HasTrigger("PostDeleteTrigger") | ||
| .HasTriggerType(TriggerType.Post).HasTriggerOperation(TriggerOperation.Delete); | ||
| entity.HasTrigger("UpdateTrigger") | ||
| .HasTriggerType(TriggerType.Pre).HasTriggerOperation(TriggerOperation.Replace); |
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.
Aren't the trigger type and operation mandatory? If so, I'd expect HasTrigger to have them as required parameters, no?
I get that there will be edge cases where someone wants to change the type/operation for a trigger that has already been defined, but in the 99% case users just want to define a new trigger (it might be acceptable for the edge case to just use the metadata APIs directly).
| var container = wrapper.Client.GetDatabase(wrapper._databaseId).GetContainer(parameters.ContainerId); | ||
| var itemRequestOptions = CreateItemRequestOptions(entry, wrapper._enableContentResponseOnWrite); | ||
| var partitionKeyValue = ExtractPartitionKeyValue(entry); | ||
| var preTriggers = GetTriggers(entry, TriggerType.Pre, TriggerOperation.Create); |
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.
Am noting that when performing an update using the Cosmos SDK, users need to manually specify which triggers they want executed; this is very different from relational database triggers, where the trigger is defined on the table and then automatically (and always) runs when the table is updated, without any client request.
This EF implementation seems to provide relational-like behavior, i.e. the user defines the trigger on the model and then the trigger gets implicitly used in SaveChanges (a more Cosmos-faithful implementation would allow the user to specify triggers to be executed on each SaveChanges). FWIW the relational behavior generally makes more sense to me as a developer experience, but maybe we should confirm this proposed behavior with the Cosmos people? i.e. maybe there's some reason we're missing for wanting users to manually specify triggers?
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.
I'd be interested to hear thoughts on this from the Cosmos team. But from what I've seen online this is the behavior that most users are looking for.
Also, from a technical perspective, we don't have a way of flowing provider-specific parameters through SaveChanges. If we did, we could add a way of opting out for specific triggers.
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.
Filed #36525
| } | ||
| } | ||
|
|
||
| private async Task CreateTriggersInCosmosAsync(TriggersContext context) |
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.
Don't we want EF to create the triggers with the Cosmos SDK? i.e. allow the user to configure the Javascript in the model and then do the CreateTriggerAsync call?
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.
That's out of scope for this PR. Also, without schema versioning, this would not be useful/recommended in production
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.
Filed #36524
|
And tagging @TheovanKraay too. |
32e7845 to
fff0a1a
Compare
|
Oops this had auto-merge :( |
Fixes #19944