From 7cc7429377b20fb1b964d6761eb07fb354bd7513 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Thu, 28 Jul 2022 20:13:23 +0100 Subject: [PATCH] Warn for composite generated keys on SQLite (#28534) --- .../RelationalModelValidator.cs | 18 ++++----- .../Internal/SqlServerModelValidator.cs | 14 +++---- .../Internal/SqliteLoggingDefinitions.cs | 2 +- .../Diagnostics/SqliteEventId.cs | 15 +++++++ .../Internal/SqliteLoggerExtensions.cs | 39 +++++++++++++++++++ .../Internal/SqliteModelValidator.cs | 27 +++++++++++++ .../Properties/SqliteStrings.Designer.cs | 25 ++++++++++++ .../Properties/SqliteStrings.resx | 6 ++- .../OwnedEntityQueryRelationalTestBase.cs | 2 + .../RelationalModelValidatorTest.cs | 23 +++++++++++ .../CompositeKeyEndToEndSqliteTest.cs | 3 ++ .../GraphUpdatesSqliteTestBase.cs | 3 ++ .../Query/OwnedEntityQuerySqliteTest.cs | 3 ++ .../Query/OwnedQuerySqliteTest.cs | 3 ++ .../SqliteModelValidatorTest.cs | 16 ++++++++ test/EFCore.Sqlite.Tests/SqliteEventIdTest.cs | 2 + .../ModelValidatorTest.PropertyMapping.cs | 6 +++ 17 files changed, 187 insertions(+), 20 deletions(-) diff --git a/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs b/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs index ee04a9239d1..b1da78717b9 100644 --- a/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs +++ b/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs @@ -1594,11 +1594,11 @@ protected override void ValidateInheritanceMapping( throw new InvalidOperationException( RelationalStrings.AbstractTpc(entityType.DisplayName(), storeObject)); } - - foreach (var key in entityType.GetKeys()) - { - ValidateValueGenerationForMappingStrategy(entityType, key, mappingStrategy, logger); - } + } + + foreach (var key in entityType.GetKeys()) + { + ValidateValueGeneration(entityType, key, logger); } if (entityType.BaseType != null) @@ -1681,20 +1681,18 @@ protected override void ValidateInheritanceMapping( } /// - /// Validates the key value generation is valid for the given inheritance mapping strategy. + /// Validates the key value generation is valid. /// /// The entity type. /// The key. - /// The inheritance mapping strategy. /// The logger to use. - protected virtual void ValidateValueGenerationForMappingStrategy( + protected virtual void ValidateValueGeneration( IEntityType entityType, IKey key, - string mappingStrategy, IDiagnosticsLogger logger) { if (entityType.GetTableName() != null - && mappingStrategy == RelationalAnnotationNames.TpcMappingStrategy) + && (string?)entityType[RelationalAnnotationNames.MappingStrategy] == RelationalAnnotationNames.TpcMappingStrategy) { foreach (var storeGeneratedProperty in key.Properties.Where(p => (p.ValueGenerated & ValueGenerated.OnAdd) != 0)) { diff --git a/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs b/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs index 3a0bae92329..5df5e28c435 100644 --- a/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs +++ b/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs @@ -141,20 +141,18 @@ protected virtual void ValidateNonKeyValueGeneration( } /// - /// Validates the key value generation is valid for the given inheritance mapping strategy. + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - /// The entity type. - /// The key. - /// The inheritance mapping strategy. - /// The logger to use. - protected override void ValidateValueGenerationForMappingStrategy( + protected override void ValidateValueGeneration( IEntityType entityType, IKey key, - string mappingStrategy, IDiagnosticsLogger logger) { if (entityType.GetTableName() != null - && mappingStrategy == RelationalAnnotationNames.TpcMappingStrategy) + && (string?)entityType[RelationalAnnotationNames.MappingStrategy] == RelationalAnnotationNames.TpcMappingStrategy) { foreach (var storeGeneratedProperty in key.Properties.Where( p => (p.ValueGenerated & ValueGenerated.OnAdd) != 0 diff --git a/src/EFCore.Sqlite.Core/Diagnostics/Internal/SqliteLoggingDefinitions.cs b/src/EFCore.Sqlite.Core/Diagnostics/Internal/SqliteLoggingDefinitions.cs index 2a2f1c92d50..791ab4de185 100644 --- a/src/EFCore.Sqlite.Core/Diagnostics/Internal/SqliteLoggingDefinitions.cs +++ b/src/EFCore.Sqlite.Core/Diagnostics/Internal/SqliteLoggingDefinitions.cs @@ -129,5 +129,5 @@ public class SqliteLoggingDefinitions : RelationalLoggingDefinitions /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public EventDefinitionBase? LogTableRebuildsPendingWarning; + public EventDefinitionBase? LogCompositeKeyWithValueGeneration; } diff --git a/src/EFCore.Sqlite.Core/Diagnostics/SqliteEventId.cs b/src/EFCore.Sqlite.Core/Diagnostics/SqliteEventId.cs index 20200457423..e90221f6e4c 100644 --- a/src/EFCore.Sqlite.Core/Diagnostics/SqliteEventId.cs +++ b/src/EFCore.Sqlite.Core/Diagnostics/SqliteEventId.cs @@ -29,6 +29,7 @@ private enum Id // Model validation events SchemaConfiguredWarning = CoreEventId.ProviderBaseId, SequenceConfiguredWarning, + CompositeKeyWithValueGeneration, // Infrastructure events UnexpectedConnectionTypeWarning = CoreEventId.ProviderBaseId + 100, @@ -80,6 +81,20 @@ private static EventId MakeValidationId(Id id) /// public static readonly EventId SequenceConfiguredWarning = MakeValidationId(Id.SequenceConfiguredWarning); + /// + /// An entity type has composite key which is configured to use generated values. SQLite does not support generated values + /// on composite keys. + /// + /// + /// + /// This event is in the category. + /// + /// + /// This event uses the payload when used with a . + /// + /// + public static readonly EventId CompositeKeyWithValueGeneration = MakeValidationId(Id.CompositeKeyWithValueGeneration); + private static readonly string InfraPrefix = DbLoggerCategory.Infrastructure.Name + "."; private static EventId MakeInfraId(Id id) diff --git a/src/EFCore.Sqlite.Core/Extensions/Internal/SqliteLoggerExtensions.cs b/src/EFCore.Sqlite.Core/Extensions/Internal/SqliteLoggerExtensions.cs index 59789d20c9b..2447b2ef3cc 100644 --- a/src/EFCore.Sqlite.Core/Extensions/Internal/SqliteLoggerExtensions.cs +++ b/src/EFCore.Sqlite.Core/Extensions/Internal/SqliteLoggerExtensions.cs @@ -374,4 +374,43 @@ private static string TableRebuildPendingWarning(EventDefinitionBase definition, var p = (TableRebuildEventData)payload; return d.GenerateMessage(p.OperationType.ShortDisplayName(), p.TableName); } + + /// + /// Logs the event. + /// + /// The diagnostics logger to use. + /// The key. + public static void CompositeKeyWithValueGeneration( + this IDiagnosticsLogger diagnostics, + IKey key) + { + var definition = SqliteResources.LogCompositeKeyWithValueGeneration(diagnostics); + + if (diagnostics.ShouldLog(definition)) + { + definition.Log( + diagnostics, + key.DeclaringEntityType.DisplayName(), + key.Properties.Format()); + } + + if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled)) + { + var eventData = new KeyEventData( + definition, + CompositeKeyWithValueGeneration, + key); + + diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled); + } + } + + private static string CompositeKeyWithValueGeneration(EventDefinitionBase definition, EventData payload) + { + var d = (EventDefinition)definition; + var p = (KeyEventData)payload; + return d.GenerateMessage( + p.Key.DeclaringEntityType.DisplayName(), + p.Key.Properties.Format()); + } } diff --git a/src/EFCore.Sqlite.Core/Infrastructure/Internal/SqliteModelValidator.cs b/src/EFCore.Sqlite.Core/Infrastructure/Internal/SqliteModelValidator.cs index 25445d3fea6..44a785ddc3b 100644 --- a/src/EFCore.Sqlite.Core/Infrastructure/Internal/SqliteModelValidator.cs +++ b/src/EFCore.Sqlite.Core/Infrastructure/Internal/SqliteModelValidator.cs @@ -101,4 +101,31 @@ protected override void ValidateCompatible( storeObject.DisplayName())); } } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override void ValidateValueGeneration( + IEntityType entityType, + IKey key, + IDiagnosticsLogger logger) + { + base.ValidateValueGeneration(entityType, key, logger); + + var keyProperties = key.Properties; + if (key.IsPrimaryKey() + && keyProperties.Count(p => p.ClrType.UnwrapNullableType().IsInteger()) > 1 + && keyProperties.Any( + p => p.ValueGenerated == ValueGenerated.OnAdd + && p.ClrType.UnwrapNullableType().IsInteger() + && !p.TryGetDefaultValue(out _) + && p.GetDefaultValueSql() == null + && !p.IsForeignKey())) + { + logger.CompositeKeyWithValueGeneration(key); + } + } } diff --git a/src/EFCore.Sqlite.Core/Properties/SqliteStrings.Designer.cs b/src/EFCore.Sqlite.Core/Properties/SqliteStrings.Designer.cs index 34cfffdef88..8a1476847d1 100644 --- a/src/EFCore.Sqlite.Core/Properties/SqliteStrings.Designer.cs +++ b/src/EFCore.Sqlite.Core/Properties/SqliteStrings.Designer.cs @@ -99,6 +99,31 @@ public static class SqliteResources private static readonly ResourceManager _resourceManager = new ResourceManager("Microsoft.EntityFrameworkCore.Sqlite.Properties.SqliteStrings", typeof(SqliteResources).Assembly); + /// + /// The entity type '{entityType}' has composite key '{key}' which is configured to use generated values. SQLite does not support generated values on composite keys. + /// + public static EventDefinition LogCompositeKeyWithValueGeneration(IDiagnosticsLogger logger) + { + var definition = ((Diagnostics.Internal.SqliteLoggingDefinitions)logger.Definitions).LogCompositeKeyWithValueGeneration; + if (definition == null) + { + definition = NonCapturingLazyInitializer.EnsureInitialized( + ref ((Diagnostics.Internal.SqliteLoggingDefinitions)logger.Definitions).LogCompositeKeyWithValueGeneration, + logger, + static logger => new EventDefinition( + logger.Options, + SqliteEventId.CompositeKeyWithValueGeneration, + LogLevel.Warning, + "SqliteEventId.CompositeKeyWithValueGeneration", + level => LoggerMessage.Define( + level, + SqliteEventId.CompositeKeyWithValueGeneration, + _resourceManager.GetString("LogCompositeKeyWithValueGeneration")!))); + } + + return (EventDefinition)definition; + } + /// /// Skipping foreign key with identity '{id}' on table '{tableName}' since principal table '{principalTableName}' was not found in the model. This usually happens when the principal table was not included in the selection set. /// diff --git a/src/EFCore.Sqlite.Core/Properties/SqliteStrings.resx b/src/EFCore.Sqlite.Core/Properties/SqliteStrings.resx index 27ed6ce53bb..50fe50fa1c9 100644 --- a/src/EFCore.Sqlite.Core/Properties/SqliteStrings.resx +++ b/src/EFCore.Sqlite.Core/Properties/SqliteStrings.resx @@ -129,6 +129,10 @@ SQLite does not support this migration operation ('{operation}'). See http://go.microsoft.com/fwlink/?LinkId=723262 for more information and examples. + + The entity type '{entityType}' has composite key '{key}' which is configured to use generated values. SQLite does not support generated values on composite keys. + Warning SqliteEventId.CompositeKeyWithValueGeneration string? string? + Skipping foreign key with identity '{id}' on table '{tableName}' since principal table '{principalTableName}' was not found in the model. This usually happens when the principal table was not included in the selection set. Warning SqliteEventId.ForeignKeyReferencesMissingTableWarning string? string? string? @@ -194,4 +198,4 @@ SQLite does not support sequences. See http://go.microsoft.com/fwlink/?LinkId=723262 for more information and examples. - \ No newline at end of file + diff --git a/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs index ba4435abfc4..92a9a0369d9 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/OwnedEntityQueryRelationalTestBase.cs @@ -55,6 +55,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) { oob.ToTable(nameof(Leaf24777)); oob.HasKey(x => new { ProductCommissionRulesetId = x.ModdleAId, x.UnitThreshold }); + oob.Property(x => x.ModdleAId).ValueGeneratedNever(); + oob.Property(x => x.UnitThreshold).ValueGeneratedNever(); oob.WithOwner().HasForeignKey(e => e.ModdleAId); oob.HasData( new Leaf24777 { ModdleAId = 1, UnitThreshold = 1 }, diff --git a/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs b/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs index 62915675966..75e0aa1899f 100644 --- a/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs +++ b/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs @@ -1181,6 +1181,7 @@ public virtual void Detects_duplicate_foreignKey_names_within_hierarchy_with_dif modelBuilder.Entity().HasOne().WithMany().HasForeignKey("FriendId", "Shadow").HasPrincipalKey( p => new { p.Id, p.Name }).HasConstraintName("FK"); modelBuilder.Entity().HasOne().WithMany().HasForeignKey("FriendId").HasConstraintName("FK"); + modelBuilder.Entity().Property(e => e.Id).ValueGeneratedNever(); VerifyError( RelationalStrings.DuplicateForeignKeyColumnMismatch( @@ -3214,6 +3215,28 @@ public virtual void Throws_when_non_tph_entity_type_discriminator_set_to_non_str VerifyError(RelationalStrings.NonTphDiscriminatorValueNotString(1, "TpcDerived"), modelBuilder); } + [ConditionalFact] + public virtual void Store_generated_in_composite_key() + { + var modelBuilder = CreateConventionModelBuilder(); + modelBuilder.Entity( + b => + { + b.HasKey(e => new { e.Id1, e.Id2 }); + b.Property(e => e.Id2).ValueGeneratedOnAdd(); + }); + + Validate(modelBuilder); + + var entityType = modelBuilder.Model.FindEntityType(typeof(CarbonComposite))!; + var keyProperties = entityType.FindPrimaryKey()!.Properties; + Assert.Equal(2, keyProperties.Count); + Assert.Equal(nameof(CarbonComposite.Id1), keyProperties[0].Name); + Assert.Equal(nameof(CarbonComposite.Id2), keyProperties[1].Name); + Assert.Equal(ValueGenerated.Never, keyProperties[0].ValueGenerated); + Assert.Equal(ValueGenerated.OnAdd, keyProperties[1].ValueGenerated); + } + private class TpcBase { public int Id { get; set; } diff --git a/test/EFCore.Sqlite.FunctionalTests/CompositeKeyEndToEndSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/CompositeKeyEndToEndSqliteTest.cs index c3ad40fea8d..391262719b5 100644 --- a/test/EFCore.Sqlite.FunctionalTests/CompositeKeyEndToEndSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/CompositeKeyEndToEndSqliteTest.cs @@ -17,6 +17,9 @@ public override Task Can_use_generated_values_in_composite_key_end_to_end() public class CompositeKeyEndToEndSqliteFixture : CompositeKeyEndToEndFixtureBase { + public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder) + => base.AddOptions(builder.ConfigureWarnings(b => b.Ignore(SqliteEventId.CompositeKeyWithValueGeneration))); + protected override ITestStoreFactory TestStoreFactory => SqliteTestStoreFactory.Instance; } diff --git a/test/EFCore.Sqlite.FunctionalTests/GraphUpdates/GraphUpdatesSqliteTestBase.cs b/test/EFCore.Sqlite.FunctionalTests/GraphUpdates/GraphUpdatesSqliteTestBase.cs index cdef7a37d3b..6d17df8ae33 100644 --- a/test/EFCore.Sqlite.FunctionalTests/GraphUpdates/GraphUpdatesSqliteTestBase.cs +++ b/test/EFCore.Sqlite.FunctionalTests/GraphUpdates/GraphUpdatesSqliteTestBase.cs @@ -49,6 +49,9 @@ public TestSqlLoggerFactory TestSqlLoggerFactory protected override ITestStoreFactory TestStoreFactory => SqliteTestStoreFactory.Instance; + public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder) + => base.AddOptions(builder.ConfigureWarnings(b => b.Ignore(SqliteEventId.CompositeKeyWithValueGeneration))); + protected virtual bool AutoDetectChanges => false; diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/OwnedEntityQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/OwnedEntityQuerySqliteTest.cs index 7de22a0ea90..bd914ef8727 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/OwnedEntityQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/OwnedEntityQuerySqliteTest.cs @@ -7,4 +7,7 @@ public class OwnedEntityQuerySqliteTest : OwnedEntityQueryRelationalTestBase { protected override ITestStoreFactory TestStoreFactory => SqliteTestStoreFactory.Instance; + + protected override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder) + => base.AddOptions(builder.ConfigureWarnings(b => b.Ignore(SqliteEventId.CompositeKeyWithValueGeneration))); } diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/OwnedQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/OwnedQuerySqliteTest.cs index 476c9993095..544b1878404 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/OwnedQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/OwnedQuerySqliteTest.cs @@ -12,6 +12,9 @@ public OwnedQuerySqliteTest(OwnedQuerySqliteFixture fixture) public class OwnedQuerySqliteFixture : RelationalOwnedQueryFixture { + public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder) + => base.AddOptions(builder.ConfigureWarnings(b => b.Ignore(SqliteEventId.CompositeKeyWithValueGeneration))); + protected override ITestStoreFactory TestStoreFactory => SqliteTestStoreFactory.Instance; } diff --git a/test/EFCore.Sqlite.Tests/Infrastructure/SqliteModelValidatorTest.cs b/test/EFCore.Sqlite.Tests/Infrastructure/SqliteModelValidatorTest.cs index 0ca1505fe31..bb28e0f1603 100644 --- a/test/EFCore.Sqlite.Tests/Infrastructure/SqliteModelValidatorTest.cs +++ b/test/EFCore.Sqlite.Tests/Infrastructure/SqliteModelValidatorTest.cs @@ -72,6 +72,22 @@ public void Detects_sequences() modelBuilder); } + public override void Store_generated_in_composite_key() + { + var modelBuilder = CreateConventionModelBuilder(); + modelBuilder.Entity( + b => + { + b.HasKey(e => new { e.Id1, e.Id2 }); + b.Property(e => e.Id2).ValueGeneratedOnAdd(); + }); + + VerifyWarning( + SqliteResources.LogCompositeKeyWithValueGeneration( + new TestLogger()).GenerateMessage(nameof(CarbonComposite), "{'Id1', 'Id2'}"), + modelBuilder); + } + protected override TestHelpers TestHelpers => SqliteTestHelpers.Instance; } diff --git a/test/EFCore.Sqlite.Tests/SqliteEventIdTest.cs b/test/EFCore.Sqlite.Tests/SqliteEventIdTest.cs index 4b0b06ae85d..88bc0c85bda 100644 --- a/test/EFCore.Sqlite.Tests/SqliteEventIdTest.cs +++ b/test/EFCore.Sqlite.Tests/SqliteEventIdTest.cs @@ -14,12 +14,14 @@ public class SqliteEventIdTest : EventIdTestBase public void Every_eventId_has_a_logger_method_and_logs_when_level_enabled() { var entityType = new EntityType(typeof(object), new Model(new ConventionSet()), owned: false, ConfigurationSource.Convention); + var property = entityType.AddProperty("A", typeof(int), ConfigurationSource.Convention, ConfigurationSource.Convention); entityType.Model.FinalizeModel(); var fakeFactories = new Dictionary> { { typeof(string), () => "Fake" }, { typeof(IEntityType), () => entityType }, + { typeof(IKey), () => new Key(new[] { property }, ConfigurationSource.Convention) }, { typeof(IReadOnlySequence), () => new FakeSequence() }, { typeof(Type), () => typeof(object) } }; diff --git a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.PropertyMapping.cs b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.PropertyMapping.cs index f11ab5785c2..abbdd0f5987 100644 --- a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.PropertyMapping.cs +++ b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.PropertyMapping.cs @@ -361,4 +361,10 @@ protected class OwnedEntity { public string Value { get; set; } } + + protected class CarbonComposite + { + public int Id1 { get; set; } + public int Id2 { get; set; } + } }