From 4ce52b6f0e654f2f5c7ca1fdce8b4badc6907522 Mon Sep 17 00:00:00 2001 From: AndriySvyryd Date: Wed, 24 Jun 2020 12:31:35 -0700 Subject: [PATCH] Cosmos: Allow PK with just the partition key and using id as the partition key Fixes #21396 --- .../Internal/IdValueGenerator.cs | 3 +- .../EndToEndCosmosTest.cs | 166 ++++++++++++++---- .../CosmosModelValidatorTest.cs | 21 ++- 3 files changed, 152 insertions(+), 38 deletions(-) diff --git a/src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs b/src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs index 124b57cdc0d..949c25512da 100644 --- a/src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs +++ b/src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs @@ -48,7 +48,8 @@ protected override object NextValue(EntityEntry entry) var partitionKey = entityType.GetPartitionKeyPropertyName(); foreach (var property in primaryKey.Properties) { - if (property.Name == partitionKey) + if (property.Name == partitionKey + && primaryKey.Properties.Count > 1) { continue; } diff --git a/test/EFCore.Cosmos.FunctionalTests/EndToEndCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/EndToEndCosmosTest.cs index 356880fb741..ba672c7e5fa 100644 --- a/test/EFCore.Cosmos.FunctionalTests/EndToEndCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/EndToEndCosmosTest.cs @@ -413,7 +413,7 @@ private class Customer public int PartitionKey { get; set; } } - private class Customer_WithResourceId + private class CustomerWithResourceId { public string id { get; set; } public string Name { get; set; } @@ -427,7 +427,7 @@ private class CustomerGuid public int PartitionKey { get; set; } } - private class Customer_NoPartitionKey + private class CustomerNoPartitionKey { public int Id { get; set; } public string Name { get; set; } @@ -471,20 +471,23 @@ public async Task Can_read_with_find_with_resource_id_async() const int pk1 = 1; const int pk2 = 2; - var customer = new Customer_WithResourceId + var customer = new CustomerWithResourceId { id = "42", Name = "Theon", PartitionKey = pk1 }; - await using (var context = new PartitionKeyContext_WithResourceId(options)) + await using (var context = new PartitionKeyContextWithResourceId(options)) { await context.Database.EnsureCreatedAsync(); + Assert.Null(context.Model.FindEntityType(typeof(CustomerWithResourceId)) + .FindProperty(StoreKeyConvention.DefaultIdPropertyName)); + context.Add(customer); context.Add( - new Customer_WithResourceId + new CustomerWithResourceId { id = "42", Name = "Theon Twin", @@ -494,9 +497,9 @@ public async Task Can_read_with_find_with_resource_id_async() await context.SaveChangesAsync(); } - await using (var context = new PartitionKeyContext_WithResourceId(options)) + await using (var context = new PartitionKeyContextWithResourceId(options)) { - var customerFromStore = await context.Set() + var customerFromStore = await context.Set() .FindAsync(pk1, "42"); Assert.Equal("42", customerFromStore.id); @@ -509,9 +512,9 @@ public async Task Can_read_with_find_with_resource_id_async() await context.SaveChangesAsync(); } - await using (var context = new PartitionKeyContext_WithResourceId(options)) + await using (var context = new PartitionKeyContextWithResourceId(options)) { - var customerFromStore = await context.Set() + var customerFromStore = await context.Set() .WithPartitionKey(partitionKey: pk1.ToString()) .FirstAsync(); @@ -528,20 +531,20 @@ public void Can_read_with_find_with_resource_id() const int pk1 = 1; const int pk2 = 2; - var customer = new Customer_WithResourceId + var customer = new CustomerWithResourceId { id = "42", Name = "Theon", PartitionKey = pk1 }; - using (var context = new PartitionKeyContext_WithResourceId(options)) + using (var context = new PartitionKeyContextWithResourceId(options)) { context.Database.EnsureCreated(); context.Add(customer); context.Add( - new Customer_WithResourceId + new CustomerWithResourceId { id = "42", Name = "Theon Twin", @@ -551,9 +554,9 @@ public void Can_read_with_find_with_resource_id() context.SaveChanges(); } - using (var context = new PartitionKeyContext_WithResourceId(options)) + using (var context = new PartitionKeyContextWithResourceId(options)) { - var customerFromStore = context.Set() + var customerFromStore = context.Set() .Find(pk1, "42"); Assert.Equal("42", customerFromStore.id); @@ -566,9 +569,9 @@ public void Can_read_with_find_with_resource_id() context.SaveChanges(); } - using (var context = new PartitionKeyContext_WithResourceId(options)) + using (var context = new PartitionKeyContextWithResourceId(options)) { - var customerFromStore = context.Set() + var customerFromStore = context.Set() .WithPartitionKey(partitionKey: pk1.ToString()) .First(); @@ -582,12 +585,12 @@ public void Can_read_with_find_with_resource_id() public void Find_with_empty_resource_id_throws() { var options = Fixture.CreateOptions(); - using (var context = new PartitionKeyContext_WithResourceId(options)) + using (var context = new PartitionKeyContextWithResourceId(options)) { context.Database.EnsureCreated(); Assert.Equal(CosmosStrings.InvalidResourceId, - Assert.Throws(() => context.Set().Find(1, "")).Message); + Assert.Throws(() => context.Set().Find(1, "")).Message); } } @@ -796,13 +799,13 @@ public async Task Can_read_with_find_without_partition_key() { var options = Fixture.CreateOptions(); - var customer = new Customer_NoPartitionKey + var customer = new CustomerNoPartitionKey { Id = 42, Name = "Theon" }; - await using (var context = new PartitionKeyContext_EntityWithNoPartitionKey(options)) + await using (var context = new PartitionKeyContextEntityWithNoPartitionKey(options)) { await context.Database.EnsureCreatedAsync(); @@ -811,13 +814,78 @@ public async Task Can_read_with_find_without_partition_key() await context.SaveChangesAsync(); } - await using (var context = new PartitionKeyContext_EntityWithNoPartitionKey(options)) + await using (var context = new PartitionKeyContextEntityWithNoPartitionKey(options)) { - var customerFromStore = context.Set().Find(42); + var customerFromStore = context.Set().Find(42); Assert.Equal(42, customerFromStore.Id); Assert.Equal("Theon", customerFromStore.Name); - AssertSql(context, @"ReadItem(, Customer_NoPartitionKey|42)"); + AssertSql(context, @"ReadItem(, CustomerNoPartitionKey|42)"); + } + } + + [ConditionalFact] + public async Task Can_read_with_find_with_PK_partition_key() + { + var options = Fixture.CreateOptions(); + + var customer = new Customer + { + Id = 42, + Name = "Theon" + }; + + await using (var context = new PartitionKeyContextPrimaryKey(options)) + { + await context.Database.EnsureCreatedAsync(); + + context.Add(customer); + + await context.SaveChangesAsync(); + } + + await using (var context = new PartitionKeyContextPrimaryKey(options)) + { + var customerFromStore = context.Set().Find(42); + + Assert.Equal(42, customerFromStore.Id); + Assert.Equal("Theon", customerFromStore.Name); + AssertSql(context, @"ReadItem(42, 42)"); + } + } + + [ConditionalFact] + public async Task Can_read_with_find_with_PK_resource_id() + { + var options = Fixture.CreateOptions(); + + var customer = new CustomerWithResourceId + { + id = "42", + Name = "Theon" + }; + + await using (var context = new PartitionKeyContextWithPrimaryKeyResourceId(options)) + { + await context.Database.EnsureCreatedAsync(); + + context.Add(customer); + + await context.SaveChangesAsync(); + } + + await using (var context = new PartitionKeyContextWithPrimaryKeyResourceId(options)) + { + var customerFromStore = context.Set().Find("42"); + + Assert.Equal("42", customerFromStore.id); + Assert.Equal("Theon", customerFromStore.Name); + AssertSql(context, @"@__p_0='42' + +SELECT c +FROM root c +WHERE ((c[""Discriminator""] = ""CustomerWithResourceId"") AND (c[""id""] = @__p_0)) +OFFSET 0 LIMIT 1"); } } @@ -840,16 +908,16 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) } } - private class PartitionKeyContext_EntityWithNoPartitionKey : DbContext + private class PartitionKeyContextEntityWithNoPartitionKey : DbContext { - public PartitionKeyContext_EntityWithNoPartitionKey(DbContextOptions dbContextOptions) + public PartitionKeyContextEntityWithNoPartitionKey(DbContextOptions dbContextOptions) : base(dbContextOptions) { } protected override void OnModelCreating(ModelBuilder modelBuilder) { - modelBuilder.Entity(); + modelBuilder.Entity(); } } @@ -870,7 +938,6 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) cb.Property(StoreKeyConvention.DefaultIdPropertyName) .HasValueGenerator((p, e) => valueGeneratorFactory.Create(p)); - cb.Property(c => c.Id); cb.Property(c => c.PartitionKey).HasConversion(); cb.HasPartitionKey(c => c.PartitionKey); @@ -895,7 +962,6 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) cb.Property(StoreKeyConvention.DefaultIdPropertyName).HasValueGenerator((Type)null); - cb.Property(c => c.Id); cb.Property(c => c.PartitionKey).HasConversion(); cb.HasPartitionKey(c => c.PartitionKey); @@ -911,6 +977,19 @@ public PartitionKeyContextNonPrimaryKey(DbContextOptions dbContextOptions) { } + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity(); + } + } + + private class PartitionKeyContextPrimaryKey : DbContext + { + public PartitionKeyContextPrimaryKey(DbContextOptions dbContextOptions) + : base(dbContextOptions) + { + } + protected override void OnModelCreating(ModelBuilder modelBuilder) { modelBuilder.Entity( @@ -918,22 +997,43 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) { var valueGeneratorFactory = new CustomPartitionKeyIdValueGeneratorFactory(); - cb.Property(c => c.Id); - cb.HasKey(c => new { c.Id }); + cb.HasNoDiscriminator(); + cb.Property(c => c.Id).HasConversion(); + cb.HasPartitionKey(c => c.Id); + }); + } + } + + private class PartitionKeyContextWithPrimaryKeyResourceId : DbContext + { + public PartitionKeyContextWithPrimaryKeyResourceId(DbContextOptions dbContextOptions) + : base(dbContextOptions) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity( + cb => + { + cb.HasPartitionKey(c => c.PartitionKey); + cb.Property(c => c.PartitionKey).HasConversion(); + cb.Property(c => c.id).HasConversion(); + cb.HasKey(c => new { c.id }); }); } } - private class PartitionKeyContext_WithResourceId : DbContext + private class PartitionKeyContextWithResourceId : DbContext { - public PartitionKeyContext_WithResourceId(DbContextOptions dbContextOptions) + public PartitionKeyContextWithResourceId(DbContextOptions dbContextOptions) : base(dbContextOptions) { } protected override void OnModelCreating(ModelBuilder modelBuilder) { - modelBuilder.Entity( + modelBuilder.Entity( cb => { cb.HasPartitionKey(c => c.PartitionKey); diff --git a/test/EFCore.Cosmos.Tests/Infrastructure/CosmosModelValidatorTest.cs b/test/EFCore.Cosmos.Tests/Infrastructure/CosmosModelValidatorTest.cs index ba701497adf..87290f1077c 100644 --- a/test/EFCore.Cosmos.Tests/Infrastructure/CosmosModelValidatorTest.cs +++ b/test/EFCore.Cosmos.Tests/Infrastructure/CosmosModelValidatorTest.cs @@ -95,8 +95,22 @@ public virtual void Passes_on_valid_partition_keys() modelBuilder.Entity().ToContainer("Orders").HasPartitionKey(o => o.PartitionId) .Property(o => o.PartitionId).HasConversion(); - var model = modelBuilder.Model; - Validate(model); + Validate(modelBuilder.Model); + } + + [ConditionalFact] + public virtual void Passes_PK_partition_key() + { + var modelBuilder = CreateConventionalModelBuilder(); + modelBuilder.Entity(b => + { + b.HasKey(o => o.PartitionId); + b.Ignore(o => o.Customer); + b.Ignore(o => o.OrderDetails); + b.Ignore(o => o.Products); + }); + + Validate(modelBuilder.Model); } [ConditionalFact] @@ -115,8 +129,7 @@ public virtual void Detects_non_key_partition_key_property() b.Ignore(o => o.Products); }); - var model = modelBuilder.Model; - VerifyError(CosmosStrings.NoPartitionKeyKey(typeof(Order).Name, nameof(Order.PartitionId), "id"), model); + VerifyError(CosmosStrings.NoPartitionKeyKey(typeof(Order).Name, nameof(Order.PartitionId), "id"), modelBuilder.Model); } [ConditionalFact]