Skip to content

Commit

Permalink
Add entity splitting tests for migrations and update pipeline
Browse files Browse the repository at this point in the history
Don't use identity by default for columns with FKs even when the property is mapped to other columns that should use identity
Update value generation on properties when an entity type is no longer mapped to a table or is mapped again

Part of #620
  • Loading branch information
AndriySvyryd committed Jun 29, 2022
1 parent 7debdaa commit f4aa232
Show file tree
Hide file tree
Showing 13 changed files with 314 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,23 @@ public virtual void ProcessEntityTypeAnnotationChanged(
IConventionAnnotation? oldAnnotation,
IConventionContext<IConventionAnnotation> context)
{
if (name == RelationalAnnotationNames.TableName)
if (name == RelationalAnnotationNames.ViewName
|| name == RelationalAnnotationNames.FunctionName
|| name == RelationalAnnotationNames.SqlQuery)
{
if (annotation?.Value != null
&& oldAnnotation?.Value == null
&& entityTypeBuilder.Metadata.GetTableName() == null)
{
ProcessTableChanged(
entityTypeBuilder,
entityTypeBuilder.Metadata.GetDefaultTableName(),
entityTypeBuilder.Metadata.GetDefaultSchema(),
null,
null);
}
}
else if (name == RelationalAnnotationNames.TableName)
{
var schema = entityTypeBuilder.Metadata.GetSchema();
ProcessTableChanged(
Expand Down Expand Up @@ -105,29 +121,43 @@ private static void ProcessTableChanged(
string? newTable,
string? newSchema)
{
if (newTable == null)
{
foreach (var property in entityTypeBuilder.Metadata.GetProperties())
{
property.Builder.ValueGenerated(null);
}

return;
}
else if (oldTable == null)
{
foreach (var property in entityTypeBuilder.Metadata.GetProperties())
{
property.Builder.ValueGenerated(GetValueGenerated(property, StoreObjectIdentifier.Table(newTable, newSchema)));
}

return;
}

var primaryKey = entityTypeBuilder.Metadata.FindPrimaryKey();
if (primaryKey == null)
{
return;
}

var oldLink = oldTable != null
? entityTypeBuilder.Metadata.FindRowInternalForeignKeys(StoreObjectIdentifier.Table(oldTable, oldSchema))
: null;
var newLink = newTable != null
? entityTypeBuilder.Metadata.FindRowInternalForeignKeys(StoreObjectIdentifier.Table(newTable, newSchema))
: null;
var oldLink = entityTypeBuilder.Metadata.FindRowInternalForeignKeys(StoreObjectIdentifier.Table(oldTable, oldSchema));
var newLink = entityTypeBuilder.Metadata.FindRowInternalForeignKeys(StoreObjectIdentifier.Table(newTable, newSchema));

if ((oldLink?.Any() != true
&& newLink?.Any() != true)
|| newLink == null)
if (!oldLink.Any()
&& !newLink.Any())
{
return;
}

foreach (var property in primaryKey.Properties)
{
property.Builder.ValueGenerated(GetValueGenerated(property, StoreObjectIdentifier.Table(newTable!, newSchema)));
property.Builder.ValueGenerated(GetValueGenerated(property, StoreObjectIdentifier.Table(newTable, newSchema)));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,19 @@ public int Compare(IColumnMappingBase? x, IColumnMappingBase? y)
return result;
}

result = StringComparer.Ordinal.Compare(x.Property.Name, y.Property.Name);
result = TableMappingBaseComparer.Instance.Compare(x.TableMapping, y.TableMapping);
if (result != 0)
{
return result;
}

result = StringComparer.Ordinal.Compare(x.Column.Name, y.Column.Name);
result = StringComparer.Ordinal.Compare(x.Property.Name, y.Property.Name);
if (result != 0)
{
return result;
}

return TableMappingBaseComparer.Instance.Compare(x.TableMapping, y.TableMapping);
return StringComparer.Ordinal.Compare(x.Column.Name, y.Column.Name);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System.Collections;
using System.Globalization;
using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Update.Internal;

Expand Down
37 changes: 29 additions & 8 deletions src/EFCore.SqlServer/Extensions/SqlServerPropertyExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -406,31 +406,53 @@ internal static SqlServerValueGenerationStrategy GetValueGenerationStrategy(
ITypeMappingSource? typeMappingSource)
{
var annotation = property.FindAnnotation(SqlServerAnnotationNames.ValueGenerationStrategy);
if (annotation != null)
if (annotation?.Value != null
&& StoreObjectIdentifier.Create(property.DeclaringEntityType, storeObject.StoreObjectType) == storeObject)
{
return (SqlServerValueGenerationStrategy?)annotation.Value ?? SqlServerValueGenerationStrategy.None;
return (SqlServerValueGenerationStrategy)annotation.Value;
}

var table = storeObject;
var sharedTableRootProperty = property.FindSharedStoreObjectRootProperty(storeObject);
if (sharedTableRootProperty != null)
{
return sharedTableRootProperty.GetValueGenerationStrategy(storeObject)
return sharedTableRootProperty.GetValueGenerationStrategy(storeObject, typeMappingSource)
== SqlServerValueGenerationStrategy.IdentityColumn
&& property.GetContainingForeignKeys().All(fk => fk.IsBaseLinking())
&& table.StoreObjectType == StoreObjectType.Table
&& !property.GetContainingForeignKeys().Any(fk =>
!fk.IsBaseLinking()
|| (StoreObjectIdentifier.Create(fk.PrincipalEntityType, StoreObjectType.Table)
is StoreObjectIdentifier principal
&& fk.GetConstraintName(table, principal) != null))
? SqlServerValueGenerationStrategy.IdentityColumn
: SqlServerValueGenerationStrategy.None;
}

if (property.ValueGenerated != ValueGenerated.OnAdd
|| property.GetContainingForeignKeys().Any(fk => !fk.IsBaseLinking())
|| table.StoreObjectType != StoreObjectType.Table
|| property.TryGetDefaultValue(storeObject, out _)
|| property.GetDefaultValueSql(storeObject) != null
|| property.GetComputedColumnSql(storeObject) != null)
|| property.GetComputedColumnSql(storeObject) != null
|| property.GetContainingForeignKeys()
.Any(fk =>
!fk.IsBaseLinking()
|| (StoreObjectIdentifier.Create(fk.PrincipalEntityType, StoreObjectType.Table)
is StoreObjectIdentifier principal
&& fk.GetConstraintName(table, principal) != null)))
{
return SqlServerValueGenerationStrategy.None;
}

return GetDefaultValueGenerationStrategy(property, storeObject, typeMappingSource);
var defaultStategy = GetDefaultValueGenerationStrategy(property, storeObject, typeMappingSource);
if (defaultStategy != SqlServerValueGenerationStrategy.None)
{
if (annotation != null)
{
return (SqlServerValueGenerationStrategy?)annotation.Value ?? SqlServerValueGenerationStrategy.None;
}
}

return defaultStategy;
}

private static SqlServerValueGenerationStrategy GetDefaultValueGenerationStrategy(IReadOnlyProperty property)
Expand All @@ -455,7 +477,6 @@ private static SqlServerValueGenerationStrategy GetDefaultValueGenerationStrateg
ITypeMappingSource? typeMappingSource)
{
var modelStrategy = property.DeclaringEntityType.Model.GetValueGenerationStrategy();

if (modelStrategy == SqlServerValueGenerationStrategy.SequenceHiLo
&& IsCompatibleWithValueGeneration(property, storeObject, typeMappingSource))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ public override void ProcessEntityTypeAnnotationChanged(
return null;
}

// If the first mapping can be value generated then we'll consider all mappings to be value generated
// as this is a client-side configuration and can't be specified per-table.
return GetValueGenerated(property, declaringTable, Dependencies.TypeMappingSource);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.



// ReSharper disable once CheckNamespace
namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,7 @@ public override IEnumerable<IAnnotation> For(IColumn column, bool designTime)

var table = StoreObjectIdentifier.Table(column.Table.Name, column.Table.Schema);
var identityProperty = column.PropertyMappings.Where(
m => (m.TableMapping.IsSharedTablePrincipal ?? true)
&& m.TableMapping.EntityType == m.Property.DeclaringEntityType)
m => m.TableMapping.EntityType == m.Property.DeclaringEntityType)
.Select(m => m.Property)
.FirstOrDefault(
p => p.GetValueGenerationStrategy(table)
Expand Down
10 changes: 0 additions & 10 deletions test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,6 @@ public void Views_are_stored_in_the_model_snapshot()
b.Property<int>(""Id"")
.HasColumnType(""int"");
SqlServerPropertyBuilderExtensions.UseIdentityColumn(b.Property<int>(""Id""), 1L, 1);
b.HasKey(""Id"");
b.ToView(""EntityWithOneProperty"", (string)null);
Expand All @@ -596,8 +594,6 @@ public void Views_with_schemas_are_stored_in_the_model_snapshot()
b.Property<int>(""Id"")
.HasColumnType(""int"");
SqlServerPropertyBuilderExtensions.UseIdentityColumn(b.Property<int>(""Id""), 1L, 1);
b.HasKey(""Id"");
b.ToView(""EntityWithOneProperty"", ""ViewSchema"");
Expand Down Expand Up @@ -947,8 +943,6 @@ public virtual void Entity_splitting_is_stored_in_snapshot_with_views()
b.Property<int>(""Id"")
.HasColumnType(""int"");
SqlServerPropertyBuilderExtensions.UseIdentityColumn(b.Property<int>(""Id""), 1L, 1);
b.Property<int>(""Shadow"")
.HasColumnType(""int"");
Expand Down Expand Up @@ -1040,7 +1034,6 @@ public void Unmapped_entity_types_are_stored_in_the_model_snapshot()
modelBuilder.Entity(""Microsoft.EntityFrameworkCore.Migrations.ModelSnapshotSqlServerTest+EntityWithOneProperty"", b =>
{
b.Property<int>(""Id"")
.ValueGeneratedOnAdd()
.HasColumnType(""int"");
b.HasKey(""Id"");
Expand Down Expand Up @@ -1122,7 +1115,6 @@ public void Entity_types_mapped_to_queries_are_stored_in_the_model_snapshot()
modelBuilder.Entity(""Microsoft.EntityFrameworkCore.Migrations.ModelSnapshotSqlServerTest+EntityWithOneProperty"", b =>
{
b.Property<int>(""Id"")
.ValueGeneratedOnAdd()
.HasColumnType(""int"");
b.HasKey(""Id"");
Expand Down Expand Up @@ -3329,8 +3321,6 @@ protected override void BuildModel(ModelBuilder modelBuilder)
b1.Property<int>(""Id"")
.HasColumnType(""int"");
SqlServerPropertyBuilderExtensions.UseIdentityColumn(b1.Property<int>(""Id""), 1L, 1);
b1.Property<int>(""TestEnum"")
.HasColumnType(""int"");
Expand Down
111 changes: 111 additions & 0 deletions test/EFCore.Relational.Specification.Tests/EntitySplittingTestBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// ReSharper disable InconsistentNaming
namespace Microsoft.EntityFrameworkCore;

public abstract class EntitySplittingTestBase : NonSharedModelTestBase
{
protected EntitySplittingTestBase(ITestOutputHelper testOutputHelper)
{
//TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
}

[ConditionalFact(Skip = "Entity splitting query Issue #620")]
public virtual async Task Can_roundtrip()
{
await InitializeAsync(OnModelCreating, sensitiveLogEnabled: false);

await using (var context = CreateContext())
{
var meterReading = new MeterReading { ReadingStatus = MeterReadingStatus.NotAccesible, CurrentRead = "100" };

context.Add(meterReading);

TestSqlLoggerFactory.Clear();

await context.SaveChangesAsync();

Assert.Empty(TestSqlLoggerFactory.Log.Where(l => l.Level == LogLevel.Warning));
}

await using (var context = CreateContext())
{
var reading = await context.MeterReadings.SingleAsync();

Assert.Equal(MeterReadingStatus.NotAccesible, reading.ReadingStatus);
Assert.Equal("100", reading.CurrentRead);
}
}

protected override string StoreName { get; } = "EntitySplittingTest";

protected TestSqlLoggerFactory TestSqlLoggerFactory
=> (TestSqlLoggerFactory)ListLoggerFactory;

protected ContextFactory<EntitySplittingContext> ContextFactory { get; private set; }

protected void AssertSql(params string[] expected)
=> TestSqlLoggerFactory.AssertBaseline(expected);

protected virtual void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<MeterReading>(
ob =>
{
ob.ToTable("MeterReadings");
ob.SplitToTable(
"MeterReadingDetails", t =>
{
t.Property(o => o.PreviousRead);
t.Property(o => o.CurrentRead);
});
});
}

protected async Task InitializeAsync(Action<ModelBuilder> onModelCreating, bool sensitiveLogEnabled = true)
=> ContextFactory = await InitializeAsync<EntitySplittingContext>(
onModelCreating,
shouldLogCategory: _ => true,
onConfiguring: options =>
{
options.ConfigureWarnings(w => w.Log(RelationalEventId.OptionalDependentWithAllNullPropertiesWarning))
.ConfigureWarnings(w => w.Log(RelationalEventId.OptionalDependentWithoutIdentifyingPropertyWarning))
.EnableSensitiveDataLogging(sensitiveLogEnabled);
}
);

protected virtual EntitySplittingContext CreateContext()
=> ContextFactory.CreateContext();

public override void Dispose()
{
base.Dispose();

ContextFactory = null;
}

protected class EntitySplittingContext : PoolableDbContext
{
public EntitySplittingContext(DbContextOptions options)
: base(options)
{
}

public DbSet<MeterReading> MeterReadings { get; set; }
}

protected class MeterReading
{
public int Id { get; set; }
public MeterReadingStatus? ReadingStatus { get; set; }
public string CurrentRead { get; set; }
public string PreviousRead { get; set; }
}

protected enum MeterReadingStatus
{
Running = 0,
NotAccesible = 2
}
}
Loading

0 comments on commit f4aa232

Please sign in to comment.