-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Cosmos: allow for concurrency token on '_etag' property #19581
Conversation
- Adds Cosmos concurrency token wrapper struct - Passes Entry's ConcurrencyToken as ItemRequestOptions.IfMatchEtag if present
- Add 'UseEtagConcurrency' extension method for Cosmos provider. - Validate only '_etag' json property is concurrency token for cosmos.
- Catches cosmos conflicg exception and rethrows as efcore exception - Add arg checks to UseEtagConcurrency - Add and correct new cosmos strings
- Add model validation tests for etag concurrency - Test etag concurrency extension directly
test/EFCore.Cosmos.Tests/Extensions/CosmosEntityTypeBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
- Add [NotNull] to UseEtagConcurrency - Return null for when there is no concurrency
- Add [NotNull] to CosmosConcurrencyToken - Remove [NotNull] from CosmosClientWrapper
Yes. #10443 tracks making this configurable
Yes, it would also set the JSON name to "_etag"
Yes. The provider type should be
Ideally they would derive from |
- Extract CosmosConcurrencyMode to own file - Validate etag provider type is string - Add etag entity annotation - Add PropertyBuilder SetEtagConcurrency extension method.
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.
Should this be added to DeleteItemAsync as well?
Yes. #10443 tracks making this configurable
Reading that I am unsure what is the correct behavior here. Should I add the IfMatchEtag
option to all delete requests when etag concurrency is present? This would mean if an item is modified externally before a delete, the delete will fail. Is that the desired behavior? It sounds like that issue OP doesn't want that.
src/EFCore.Cosmos/Metadata/Conventions/Internal/CosmosConventionSetBuilder.cs
Outdated
Show resolved
Hide resolved
Yes, the IfMatchEtag should be added. This will make the Cosmos provider behave in the same way as other providers do, even though it might not be ideal. |
- Ensure ETag convention runs before validation - Add IfMatchEtag option on delete for cosmos
Update on this: I am working on implementing |
@jviau You don't need to implement OptimisticConcurrencyTestBase, just follow its general structure. Don't use ExecutionStrategy or transactions, just clean the database before each test or if you just make one test work that should be enough for now. |
@AndriySvyryd. Okay I have made a separate test class and have the it working for the most part. But I have hit an interesting issue, wondering if this is a problem with my code, a known issue, or something new. Properties with public class MyEntity
{
public string Id { get; set; }
public string Generated { get; set; ] // this is configured with .ValueGeneratedOnAddOrUpdate()
}
using var ctx = new MyContext();
ctx.MyEntities.Add(new MyEntity { Id = "1" });
ctx.SaveChanges()
var myEntity = ctx.MyEntities.Single(e => e.Id == "1");
myEntity.Generated != null; // false! this is still null. but...
using var ctx2 = new MyContext();
var myEntity2 = ctx2.MyEntities.Single(e => e.Id == "1");
myEntity2.Generated != null; // true! When fetched from a different context, this is finally retrieved. |
- Add CosmosConcurrencyTests - Capture and convert some CosmosException to DbUpdate(Concurrency)Exception
That's expected, since the etags are the only thing that can be store generated by Cosmos, so there's currently no general support to get the updated values. You would need to modify |
Ah I see. That seems pretty important for If there are any further changes you want in this PR, or more cases tested, let me know. |
@jviau If you could add the store generated support in this PR, it would be better, should be like 3-4 lines in each method. |
Do you mean the work to read back the new |
I was thinking of adding an var etagProperty = entry.EntityType.GetETagProperty();
if (etagProperty != null)
{
entry.SetStoreGeneratedValue(etagProperty, response.Headers.ETag);
} With this you could actually remove the |
Ah okay. I was not aware of |
- Update etag value for cosmos updates & inserts - Fix concurrency test to use single context for seed and update
@AndriySvyryd the store update is done. Some things we can address if you want:
public class CosmosUpsertRequest // name TBD
{
public JToken Document { get; }
public string PartitionKey { get; }
public CosmosConcurrencyToken ConcurrencyToken { get; }
public void ProcessResponse(ResponseMessage response) { } // can update the store values here.
} |
As I said:
|
- Reduce parameters in CosmosClientWrapper methods by using IUpdateEntry
Good point, missed that in the first message. Took care of it. |
Thanks for your contribution! This is a very useful feature. |
@AndriySvyryd @jviau I guess this will be available in 3.1.2? |
@richardrobberse No, this will ship in 5.0.0, we don't add features in patch releases. You can also use the daily builds |
@AndriySvyryd Ouch, I see.
|
@richardrobberse November
You mean without this feature? You can add a property mapped to "_etag", however the value won't be used for concurrency checks. |
#17299
Adds the following:
UseEtagConcurrency
extension method forEntityTypeBuilder
andEntityTypeBuilder<TEntity>
, which adds a shadow property for the_etag
field and sets it as the concurrency token._etag
is the concurrency token for a cosmos entity._etag
value in theIfMatchEtag
portion of the request options duringReplaceItemStreamAsync
.Things to consider:
DeleteItemAsync
as well?PropertyBuilder
to configure it as an etag?_etag
property is ofstring
type?TODO:
If there was another design in mind and this one isn't acceptable, no worries! Feel free to reject or ask for other changes. I am just sharing back the changes I made locally when playing around with cosmos efcore.