Skip to content

Commit

Permalink
Cleanup CosmosJsonIdConvention
Browse files Browse the repository at this point in the history 
Fixes #35325
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Dec 18, 2024
1 parent b8ddc5b commit d9003e0
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 42 deletions.
41 changes: 14 additions & 27 deletions src/EFCore.Cosmos/Metadata/Conventions/CosmosJsonIdConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,10 @@ private void ProcessEntityType(IConventionEntityType entityType, IConventionCont
var computedIdProperty = entityType.FindDeclaredProperty(DefaultIdPropertyName);

var primaryKey = entityType.FindPrimaryKey();
if (entityType.BaseType != null // Requires: IEntityTypeBaseTypeChangedConvention
|| !entityType.IsDocumentRoot() // Requires: IEntityTypeAnnotationChangedConvention (ContainerName)
|| entityType.GetForeignKeys()
.Any(fk => fk.IsOwnership) // Requires: IForeignKeyOwnershipChangedConvention, IForeignKeyRemovedConvention
|| primaryKey == null) // Requires: IKeyAddedConvention, IKeyRemovedConvention
if (entityType.BaseType != null
|| !entityType.IsDocumentRoot()
|| entityType.GetForeignKeys().Any(fk => fk.IsOwnership)
|| primaryKey == null)
{
// If the entity type is not a keyed, root document in the container, then it doesn't have an `id` mapping, so
// undo anything that was done by previous execution of this convention.
Expand All @@ -93,7 +92,8 @@ private void ProcessEntityType(IConventionEntityType entityType, IConventionCont
if (computedIdProperty is not null
&& computedIdProperty != jsonIdProperty)
{
entityType.Builder.RemoveUnusedImplicitProperties([computedIdProperty]); }
entityType.Builder.RemoveUnusedImplicitProperties([computedIdProperty]);
}

return;
}
Expand All @@ -102,17 +102,9 @@ private void ProcessEntityType(IConventionEntityType entityType, IConventionCont
// key is represented by a single string property, and the discriminator is not being included in the JSON `id`.
// If these conditions are not met, or if the user has opted-in, then we will create a computed property that transforms
// the appropriate values into a single string for the JSON `id` property.

// The line below requires: IModelAnnotationChangedConvention, IPropertyAnnotationChangedConvention
var alwaysCreateId = entityType.GetHasShadowId();
if (alwaysCreateId != true)
{
// The line below requires:
// - IModelAnnotationChangedConvention, IPropertyAnnotationChangedConvention
// - IKeyAddedConvention, IKeyRemovedConvention
// - IPropertyAddedConvention, IPropertyRemovedConvention
// - IDiscriminatorPropertySetConvention
// - IEntityTypeBaseTypeChangedConvention
var idDefinition = DefinitionFactory.Create((IEntityType)entityType)!;
if (idDefinition is { IncludesDiscriminator: false, Properties.Count: 1 })
{
Expand All @@ -125,15 +117,16 @@ private void ProcessEntityType(IConventionEntityType entityType, IConventionCont
?? mapping?.ClrType
?? keyProperty!.ClrType;

if (clrType == typeof(string))
if (clrType == typeof(string)
&& keyProperty.Builder.CanSetJsonProperty(IdPropertyJsonName))
{
// We are at the point where we are going to map the `id` directly to the PK.
// However, if a previous run of this convention create the computed property, then we need to remove that
// mapping since it is now not needed.
if (computedIdProperty != null)
if (computedIdProperty != null
&& entityType.Builder.HasNoProperty(computedIdProperty) == null)
{
computedIdProperty.Builder.ToJsonProperty(null);
entityType.Builder.HasNoProperty(computedIdProperty);
}

// If there was previously a different property mapped to `id`, but not one of our computed properties,
Expand All @@ -146,10 +139,7 @@ private void ProcessEntityType(IConventionEntityType entityType, IConventionCont
}

// Finally, actually map the primary key directly to the JSON `id`.
if (keyProperty.GetJsonPropertyName() != IdPropertyJsonName)
{
keyProperty.Builder.ToJsonProperty(IdPropertyJsonName);
}
keyProperty.Builder.ToJsonProperty(IdPropertyJsonName);

return;
}
Expand Down Expand Up @@ -183,13 +173,10 @@ private void ProcessEntityType(IConventionEntityType entityType, IConventionCont
return;
}

if (computedIdPropertyBuilder.Metadata.GetJsonPropertyName() != IdPropertyJsonName)
{
computedIdPropertyBuilder = computedIdPropertyBuilder.ToJsonProperty(IdPropertyJsonName)
?? computedIdPropertyBuilder;
}

// Don't chain, because each of these could return null if the property has been explicitly configured with some other value.
computedIdPropertyBuilder = computedIdPropertyBuilder.ToJsonProperty(IdPropertyJsonName)
?? computedIdPropertyBuilder;

computedIdPropertyBuilder = computedIdPropertyBuilder.IsRequired(true)
?? computedIdPropertyBuilder;

Expand Down
35 changes: 29 additions & 6 deletions test/EFCore.Cosmos.FunctionalTests/ModelBuilding/CosmosModelBuilderGenericTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ public virtual void Hierarchical_partition_key_is_added_to_the_alternate_key_if_
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<Customer>().AlwaysHasShadowId();
modelBuilder.Entity<Customer>().HasShadowId();
modelBuilder.Entity<Customer>().HasKey(CosmosJsonIdConvention.DefaultIdPropertyName);

modelBuilder.Entity<Customer>()
Expand All @@ -257,6 +257,29 @@ public virtual void Hierarchical_partition_key_is_added_to_the_alternate_key_if_
entity.FindPrimaryKey()!.Properties.Select(p => p.Name));
}

[ConditionalFact]
public virtual void Id_property_created_if_key_not_mapped_to_id()
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<Customer>()
.Property(c => c.Name)
.ToJsonProperty("Name");
modelBuilder.Entity<Customer>()
.Ignore(b => b.Details)
.Ignore(b => b.Orders)
.HasKey(c => c.Name);

var model = modelBuilder.FinalizeModel();

var entity = model.FindEntityType(typeof(Customer))!;

Assert.Equal(CosmosJsonIdConvention.IdPropertyJsonName,
entity.FindProperty(CosmosJsonIdConvention.DefaultIdPropertyName)!.GetJsonPropertyName());

Assert.Equal(1, entity.GetKeys().Count());
}

[ConditionalFact]
public virtual void No_id_property_created_if_another_property_mapped_to_id()
{
Expand Down Expand Up @@ -311,7 +334,7 @@ public virtual void No_alternate_key_is_created_if_primary_key_contains_id()
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<Customer>().AlwaysHasShadowId();
modelBuilder.Entity<Customer>().HasShadowId();
modelBuilder.Entity<Customer>().HasKey(CosmosJsonIdConvention.DefaultIdPropertyName);

modelBuilder.Entity<Customer>()
Expand All @@ -335,7 +358,7 @@ public virtual void No_alternate_key_is_created_if_primary_key_contains_id_and_p
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<Customer>().AlwaysHasShadowId();
modelBuilder.Entity<Customer>().HasShadowId();
modelBuilder.Entity<Customer>().HasKey(nameof(Customer.AlternateKey), CosmosJsonIdConvention.DefaultIdPropertyName);

modelBuilder.Entity<Customer>()
Expand All @@ -359,7 +382,7 @@ public virtual void No_alternate_key_is_created_if_primary_key_contains_id_and_h
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<Customer>().AlwaysHasShadowId();
modelBuilder.Entity<Customer>().HasShadowId();

modelBuilder.Entity<Customer>().HasKey(
nameof(Customer.AlternateKey),
Expand Down Expand Up @@ -404,7 +427,7 @@ public virtual void No_alternate_key_is_created_if_primary_key_contains_id_and_h
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<Customer>().AlwaysHasShadowId();
modelBuilder.Entity<Customer>().HasShadowId();

modelBuilder.Entity<Customer>().HasKey(
nameof(Customer.Title),
Expand Down Expand Up @@ -449,7 +472,7 @@ public virtual void Hierarchical_partition_key_is_added_to_the_alternate_key_if_
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<Customer>().AlwaysHasShadowId();
modelBuilder.Entity<Customer>().HasShadowId();

modelBuilder.Entity<Customer>().HasKey(
nameof(Customer.Title),
Expand Down
64 changes: 55 additions & 9 deletions test/EFCore.Cosmos.FunctionalTests/ModelBuilding/CosmosTestModelBuilderExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,49 @@ namespace Microsoft.EntityFrameworkCore.ModelBuilding;

public static class CosmosTestModelBuilderExtensions
{
public static ModelBuilderTest.TestEntityTypeBuilder<TEntity> HasPartitionKey<TEntity, TProperty>(
public static ModelBuilderTest.TestModelBuilder HasShadowIds(
this ModelBuilderTest.TestModelBuilder builder,
bool? alwaysCreate = true)
{
if (builder is IInfrastructure<ModelBuilder> nonGenericBuilder)
{
nonGenericBuilder.Instance.HasShadowIds(alwaysCreate);
}

return builder;
}

public static ModelBuilderTest.TestEntityTypeBuilder<TEntity> HasShadowId<TEntity>(
this ModelBuilderTest.TestEntityTypeBuilder<TEntity> builder,
Expression<Func<TEntity, TProperty>> propertyExpression)
bool? alwaysCreate = true)
where TEntity : class
{
switch (builder)
{
case IInfrastructure<EntityTypeBuilder<TEntity>> genericBuilder:
genericBuilder.Instance.HasPartitionKey(propertyExpression);
genericBuilder.Instance.HasShadowId(alwaysCreate);
break;
case IInfrastructure<EntityTypeBuilder> nonGenericBuilder:
var names = propertyExpression.GetMemberAccessList().Select(e => e.GetSimpleMemberName()).ToList();
nonGenericBuilder.Instance.HasPartitionKey(names.FirstOrDefault(), names.Count > 1 ? names.Skip(1).ToArray() : []);
nonGenericBuilder.Instance.HasShadowId(alwaysCreate);
break;
}

return builder;
}

public static ModelBuilderTest.TestEntityTypeBuilder<TEntity> AlwaysHasShadowId<TEntity>(
public static ModelBuilderTest.TestEntityTypeBuilder<TEntity> HasPartitionKey<TEntity, TProperty>(
this ModelBuilderTest.TestEntityTypeBuilder<TEntity> builder,
bool? alwaysCreate = true)
Expression<Func<TEntity, TProperty>> propertyExpression)
where TEntity : class
{
switch (builder)
{
case IInfrastructure<EntityTypeBuilder<TEntity>> genericBuilder:
genericBuilder.Instance.HasShadowId(alwaysCreate);
genericBuilder.Instance.HasPartitionKey(propertyExpression);
break;
case IInfrastructure<EntityTypeBuilder> nonGenericBuilder:
nonGenericBuilder.Instance.HasShadowId(alwaysCreate);
var names = propertyExpression.GetMemberAccessList().Select(e => e.GetSimpleMemberName()).ToList();
nonGenericBuilder.Instance.HasPartitionKey(names.FirstOrDefault(), names.Count > 1 ? names.Skip(1).ToArray() : []);
break;
}

Expand All @@ -63,6 +75,40 @@ public static ModelBuilderTest.TestEntityTypeBuilder<TEntity> HasPartitionKey<TE
return builder;
}

public static ModelBuilderTest.TestEntityTypeBuilder<TEntity> ToContainer<TEntity>(
this ModelBuilderTest.TestEntityTypeBuilder<TEntity> builder,
string name)
where TEntity : class
{
switch (builder)
{
case IInfrastructure<EntityTypeBuilder<TEntity>> genericBuilder:
genericBuilder.Instance.ToContainer(name);
break;
case IInfrastructure<EntityTypeBuilder> nonGenericBuilder:
nonGenericBuilder.Instance.ToContainer(name);
break;
}

return builder;
}
public static ModelBuilderTest.TestEntityTypeBuilder<TEntity> UseETagConcurrency<TEntity>(
this ModelBuilderTest.TestEntityTypeBuilder<TEntity> builder)
where TEntity : class
{
switch (builder)
{
case IInfrastructure<EntityTypeBuilder<TEntity>> genericBuilder:
genericBuilder.Instance.UseETagConcurrency();
break;
case IInfrastructure<EntityTypeBuilder> nonGenericBuilder:
nonGenericBuilder.Instance.UseETagConcurrency();
break;
}

return builder;
}

public static ModelBuilderTest.TestPropertyBuilder<TProperty> ToJsonProperty<TProperty>(
this ModelBuilderTest.TestPropertyBuilder<TProperty> builder,
string name)
Expand Down

0 comments on commit d9003e0

Please sign in to comment.