diff --git a/src/EFCore.Cosmos/Metadata/Conventions/CosmosDiscriminatorConvention.cs b/src/EFCore.Cosmos/Metadata/Conventions/CosmosDiscriminatorConvention.cs index 2990718e5c0..1e007b7aaf5 100644 --- a/src/EFCore.Cosmos/Metadata/Conventions/CosmosDiscriminatorConvention.cs +++ b/src/EFCore.Cosmos/Metadata/Conventions/CosmosDiscriminatorConvention.cs @@ -83,11 +83,10 @@ private static void ProcessEntityType(IConventionEntityTypeBuilder entityTypeBui { return; } - if (entityType.IsDocumentRoot()) { - entityTypeBuilder.HasDiscriminator(entityType.Model.GetEmbeddedDiscriminatorName(), typeof(string)) - ?.HasValue(entityType, entityType.ShortName()); + var discriminator = HasDiscriminator(entityTypeBuilder); + discriminator?.HasValue(entityType, entityType.ShortName()); } else { @@ -125,7 +124,7 @@ public override void ProcessEntityTypeBaseTypeChanged( { if (entityType.IsDocumentRoot()) { - entityTypeBuilder.HasDiscriminator(entityType.Model.GetEmbeddedDiscriminatorName(), typeof(string)); + HasDiscriminator(entityTypeBuilder); } } else @@ -137,7 +136,7 @@ public override void ProcessEntityTypeBaseTypeChanged( return; } - var discriminator = rootType.Builder.HasDiscriminator(entityType.Model.GetEmbeddedDiscriminatorName(), typeof(string)); + var discriminator = HasDiscriminator(rootType.Builder); if (discriminator != null) { SetDefaultDiscriminatorValues(entityTypeBuilder.Metadata.GetDerivedTypesInclusive(), discriminator); @@ -145,6 +144,20 @@ public override void ProcessEntityTypeBaseTypeChanged( } } + private static IConventionDiscriminatorBuilder? HasDiscriminator(IConventionEntityTypeBuilder entityTypeBuilder) + { + var discriminator = entityTypeBuilder.HasDiscriminator(typeof(string)); + var discriminatorProperty = discriminator?.EntityType.FindDiscriminatorProperty(); + if (discriminatorProperty != null) + { + CosmosPropertyBuilderExtensions.ToJsonProperty( + discriminatorProperty.Builder, + entityTypeBuilder.Metadata.Model.GetEmbeddedDiscriminatorName()); + } + + return discriminator; + } + /// protected override void SetDefaultDiscriminatorValues( IEnumerable entityTypes, diff --git a/src/EFCore.Design/Design/Internal/CSharpHelper.cs b/src/EFCore.Design/Design/Internal/CSharpHelper.cs index c62d8194854..953469b0133 100644 --- a/src/EFCore.Design/Design/Internal/CSharpHelper.cs +++ b/src/EFCore.Design/Design/Internal/CSharpHelper.cs @@ -1633,72 +1633,8 @@ public virtual string Expression( } private static bool IsIdentifierStartCharacter(char ch) - { - if (ch < 'a') - { - return ch is >= 'A' and (<= 'Z' or '_'); - } - - if (ch <= 'z') - { - return true; - } - - return ch > '\u007F' && IsLetterChar(CharUnicodeInfo.GetUnicodeCategory(ch)); - } + => char.IsLetter(ch) || ch == '_'; private static bool IsIdentifierPartCharacter(char ch) - { - if (ch < 'a') - { - return (ch < 'A' - ? ch is >= '0' and <= '9' - : ch <= 'Z') - || ch == '_'; - } - - if (ch <= 'z') - { - return true; - } - - if (ch <= '\u007F') - { - return false; - } - - var cat = CharUnicodeInfo.GetUnicodeCategory(ch); - if (IsLetterChar(cat)) - { - return true; - } - - switch (cat) - { - case UnicodeCategory.DecimalDigitNumber: - case UnicodeCategory.ConnectorPunctuation: - case UnicodeCategory.NonSpacingMark: - case UnicodeCategory.SpacingCombiningMark: - case UnicodeCategory.Format: - return true; - } - - return false; - } - - private static bool IsLetterChar(UnicodeCategory cat) - { - switch (cat) - { - case UnicodeCategory.UppercaseLetter: - case UnicodeCategory.LowercaseLetter: - case UnicodeCategory.TitlecaseLetter: - case UnicodeCategory.ModifierLetter: - case UnicodeCategory.OtherLetter: - case UnicodeCategory.LetterNumber: - return true; - } - - return false; - } + => char.IsLetter(ch) || char.IsAsciiDigit(ch) || ch == '_'; } diff --git a/src/EFCore/Diagnostics/CoreEventId.cs b/src/EFCore/Diagnostics/CoreEventId.cs index e68acbf39c9..8ec0fc0f7fb 100644 --- a/src/EFCore/Diagnostics/CoreEventId.cs +++ b/src/EFCore/Diagnostics/CoreEventId.cs @@ -129,6 +129,7 @@ private enum Id NoEntityTypeConfigurationsWarning = CoreBaseId + 632, AccidentalEntityType = CoreBaseId + 633, AccidentalComplexPropertyCollection = CoreBaseId + 634, + ShadowPropertyNameNotValidIdentifierWarning = CoreBaseId + 635, // ChangeTracking events DetectChangesStarting = CoreBaseId + 800, @@ -750,6 +751,20 @@ private static EventId MakeModelValidationId(Id id) /// public static readonly EventId AccidentalComplexPropertyCollection = MakeModelValidationId(Id.AccidentalComplexPropertyCollection); + /// + /// A shadow property has a name that is not a valid identifier, which can cause issues in generated code. + /// + /// + /// + /// This event is in the category. + /// + /// + /// This event uses the payload when used with a . + /// + /// + public static readonly EventId ShadowPropertyNameNotValidIdentifierWarning = + MakeModelValidationId(Id.ShadowPropertyNameNotValidIdentifierWarning); + /// /// The on the collection navigation property was ignored. /// diff --git a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs index 3b76077e276..79e201f5fe1 100644 --- a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs +++ b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs @@ -1432,6 +1432,40 @@ private static string ShadowPropertyCreated(EventDefinitionBase definition, Even return d.GenerateMessage(p.Property.DeclaringType.DisplayName(), p.Property.Name); } + /// + /// Logs for the event. + /// + /// The diagnostics logger to use. + /// The property. + public static void ShadowPropertyNameNotValidIdentifierWarning( + this IDiagnosticsLogger diagnostics, + IProperty property) + { + var definition = CoreResources.LogShadowPropertyNameNotValidIdentifier(diagnostics); + + if (diagnostics.ShouldLog(definition)) + { + definition.Log(diagnostics, property.DeclaringType.DisplayName(), property.Name); + } + + if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled)) + { + var eventData = new PropertyEventData( + definition, + ShadowPropertyNameNotValidIdentifier, + property); + + diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled); + } + } + + private static string ShadowPropertyNameNotValidIdentifier(EventDefinitionBase definition, EventData payload) + { + var d = (EventDefinition)definition; + var p = (PropertyEventData)payload; + return d.GenerateMessage(p.Property.DeclaringType.DisplayName(), p.Property.Name); + } + /// /// Logs for the event. /// diff --git a/src/EFCore/Diagnostics/LoggingDefinitions.cs b/src/EFCore/Diagnostics/LoggingDefinitions.cs index 55133e004e3..d0498c902df 100644 --- a/src/EFCore/Diagnostics/LoggingDefinitions.cs +++ b/src/EFCore/Diagnostics/LoggingDefinitions.cs @@ -825,4 +825,13 @@ public abstract class LoggingDefinitions /// [EntityFrameworkInternal] public EventDefinitionBase? LogQueryCompilationStarting; + + /// + /// 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. + /// + [EntityFrameworkInternal] + public EventDefinitionBase? LogShadowPropertyNameNotValidIdentifier; } diff --git a/src/EFCore/EFCore.baseline.json b/src/EFCore/EFCore.baseline.json index 7c9812b71e2..23cc7464300 100644 --- a/src/EFCore/EFCore.baseline.json +++ b/src/EFCore/EFCore.baseline.json @@ -3092,6 +3092,9 @@ { "Member": "static readonly Microsoft.Extensions.Logging.EventId ShadowPropertyCreated" }, + { + "Member": "static readonly Microsoft.Extensions.Logging.EventId ShadowPropertyNameNotValidIdentifierWarning" + }, { "Member": "static readonly Microsoft.Extensions.Logging.EventId SkipCollectionChangeDetected" }, @@ -3355,6 +3358,9 @@ { "Member": "static void ShadowPropertyCreated(this Microsoft.EntityFrameworkCore.Diagnostics.IDiagnosticsLogger diagnostics, Microsoft.EntityFrameworkCore.Metadata.IProperty property);" }, + { + "Member": "static void ShadowPropertyNameNotValidIdentifierWarning(this Microsoft.EntityFrameworkCore.Diagnostics.IDiagnosticsLogger diagnostics, Microsoft.EntityFrameworkCore.Metadata.IProperty property);" + }, { "Member": "static void SkipCollectionChangeDetected(this Microsoft.EntityFrameworkCore.Diagnostics.IDiagnosticsLogger diagnostics, Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry internalEntityEntry, Microsoft.EntityFrameworkCore.Metadata.ISkipNavigation navigation, System.Collections.Generic.ISet added, System.Collections.Generic.ISet removed);" }, diff --git a/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs index e0a7e8f743c..c3974a1cfc3 100644 --- a/src/EFCore/Infrastructure/ModelValidator.cs +++ b/src/EFCore/Infrastructure/ModelValidator.cs @@ -174,6 +174,36 @@ protected virtual void ValidateProperty( ValidateTypeMapping(property, logger); ValidatePrimitiveCollection(property, logger); ValidateAutoLoaded(property, structuralType, logger); + + if (property.IsShadowProperty() + && !IsValidIdentifier(property.Name)) + { + logger.ShadowPropertyNameNotValidIdentifierWarning(property); + } + } + + /// + /// Returns if the given name only uses letters, ASCII digits and underscores and does not start with a digit; + /// that is, if it can be used as-is as an identifier in generated code. + /// + internal static bool IsValidIdentifier(string? name) + { + if (string.IsNullOrEmpty(name) + || (!char.IsLetter(name[0]) && name[0] != '_')) + { + return false; + } + + for (var i = 1; i < name.Length; i++) + { + var ch = name[i]; + if (!char.IsLetter(ch) && !char.IsAsciiDigit(ch) && ch != '_') + { + return false; + } + } + + return true; } /// diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index 186af2df532..b7c29e725f2 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -5557,6 +5557,31 @@ public static EventDefinition LogShadowPropertyCreated(IDiagnost return (EventDefinition)definition; } + /// + /// The shadow property '{entityType}.{property}' has a name that is not a valid identifier. This can cause issues in generated code. Consider renaming the property to use only letters, digits (except for the first character), and underscore. + /// + public static EventDefinition LogShadowPropertyNameNotValidIdentifier(IDiagnosticsLogger logger) + { + var definition = ((LoggingDefinitions)logger.Definitions).LogShadowPropertyNameNotValidIdentifier; + if (definition == null) + { + definition = NonCapturingLazyInitializer.EnsureInitialized( + ref ((LoggingDefinitions)logger.Definitions).LogShadowPropertyNameNotValidIdentifier, + logger, + static logger => new EventDefinition( + logger.Options, + CoreEventId.ShadowPropertyNameNotValidIdentifierWarning, + LogLevel.Warning, + "CoreEventId.ShadowPropertyNameNotValidIdentifierWarning", + level => LoggerMessage.Define( + level, + CoreEventId.ShadowPropertyNameNotValidIdentifierWarning, + _resourceManager.GetString("LogShadowPropertyNameNotValidIdentifier")!))); + } + + return (EventDefinition)definition; + } + /// /// {addedCount} entities were added and {removedCount} entities were removed from skip navigation '{entityType}.{property}'. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see key values. /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 09ee563d2d1..0166459de44 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -1230,6 +1230,10 @@ The property '{entityType}.{property}' was created in shadow state because there are no eligible CLR members with a matching name. Debug CoreEventId.ShadowPropertyCreated string string + + The shadow property '{entityType}.{property}' has a name that is not a valid identifier. This can cause issues in generated code. Consider renaming the property to use only letters, digits (except for the first character), and underscore. + Warning CoreEventId.ShadowPropertyNameNotValidIdentifierWarning string string + {addedCount} entities were added and {removedCount} entities were removed from skip navigation '{entityType}.{property}'. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see key values. Debug CoreEventId.SkipCollectionChangeDetected int int string string diff --git a/src/EFCore/Query/LiftableConstantProcessor.cs b/src/EFCore/Query/LiftableConstantProcessor.cs index 525d460bb1c..80dd5a61aa8 100644 --- a/src/EFCore/Query/LiftableConstantProcessor.cs +++ b/src/EFCore/Query/LiftableConstantProcessor.cs @@ -126,7 +126,11 @@ public virtual Expression LiftConstants(Expression expression, ParameterExpressi continue; } - var name = liftedConstant.Parameter.Name ?? "unknown"; + // Lifted constant variable names are partly derived from model metadata (e.g. property names), which may not be valid C# + // identifiers (shadow properties can be given arbitrary names). Fall back to a generic name in that case. + var name = ModelValidator.IsValidIdentifier(liftedConstant.Parameter.Name) + ? liftedConstant.Parameter.Name! + : "unknown"; var baseName = name; for (var j = 0; variableNames.Contains(name); j++) { diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindQueryCosmosFixture.cs b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindQueryCosmosFixture.cs index fd75389851b..dae02c001a8 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindQueryCosmosFixture.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindQueryCosmosFixture.cs @@ -52,32 +52,35 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con .HasRootDiscriminatorInJsonId() .ToContainer("ProductsAndOrders"); - modelBuilder.Entity() - .ToContainer("ProductsAndOrders") - .HasRootDiscriminatorInJsonId() - .HasDiscriminator("$type").HasValue("Order"); + var orderQuery = modelBuilder.Entity().ToContainer("ProductsAndOrders").HasRootDiscriminatorInJsonId(); + orderQuery.HasDiscriminator().HasValue("Order"); + orderQuery.Property("Discriminator").ToJsonProperty("$type"); - modelBuilder + var productQuery = modelBuilder .Entity() .ToContainer("ProductsAndOrders") - .HasRootDiscriminatorInJsonId() - .HasDiscriminator("$type").HasValue("Product"); + .HasRootDiscriminatorInJsonId(); + productQuery.HasDiscriminator().HasValue("Product"); + productQuery.Property("Discriminator").ToJsonProperty("$type"); - modelBuilder + var productView = modelBuilder .Entity() .ToContainer("ProductsAndOrders") - .HasRootDiscriminatorInJsonId() - .HasDiscriminator("$type").HasValue("ProductView"); + .HasRootDiscriminatorInJsonId(); + productView.HasDiscriminator().HasValue("ProductView"); + productView.Property("Discriminator").ToJsonProperty("$type"); - modelBuilder + var customerQueryWithQueryFilter = modelBuilder .Entity() - .ToContainer("Customers") - .HasDiscriminator("$type").HasValue("Customer"); + .ToContainer("Customers"); + customerQueryWithQueryFilter.HasDiscriminator().HasValue("Customer"); + customerQueryWithQueryFilter.Property("Discriminator").ToJsonProperty("$type"); - modelBuilder + var customerQuery = modelBuilder .Entity() - .ToContainer("Customers") - .HasDiscriminator("$type").HasValue("Customer"); + .ToContainer("Customers"); + customerQuery.HasDiscriminator().HasValue("Customer"); + customerQuery.Property("Discriminator").ToJsonProperty("$type"); modelBuilder.Entity().Metadata.RemoveIndex( modelBuilder.Entity().Property(e => e.City).Metadata.GetContainingIndexes().Single()); diff --git a/test/EFCore.Cosmos.Tests/Extensions/CosmosBuilderExtensionsTest.cs b/test/EFCore.Cosmos.Tests/Extensions/CosmosBuilderExtensionsTest.cs index 193fe2084af..91a680b2538 100644 --- a/test/EFCore.Cosmos.Tests/Extensions/CosmosBuilderExtensionsTest.cs +++ b/test/EFCore.Cosmos.Tests/Extensions/CosmosBuilderExtensionsTest.cs @@ -135,7 +135,8 @@ public void Default_discriminator_can_be_removed() var entityType = modelBuilder.Model.FindEntityType(typeof(Customer))!; - Assert.Equal("$type", entityType.FindDiscriminatorProperty()!.Name); + Assert.Equal("Discriminator", entityType.FindDiscriminatorProperty()!.Name); + Assert.Equal("$type", entityType.FindDiscriminatorProperty()!.GetJsonPropertyName()); Assert.Equal(nameof(Customer), entityType.GetDiscriminatorValue()); modelBuilder.Entity().HasNoDiscriminator(); @@ -145,7 +146,8 @@ public void Default_discriminator_can_be_removed() modelBuilder.Entity().HasBaseType(); - Assert.Equal("$type", entityType.FindDiscriminatorProperty()!.Name); + Assert.Equal("Discriminator", entityType.FindDiscriminatorProperty()!.Name); + Assert.Equal("$type", entityType.FindDiscriminatorProperty()!.GetJsonPropertyName()); Assert.Equal(nameof(Customer), entityType.GetDiscriminatorValue()); modelBuilder.Entity().HasBaseType((string)null); @@ -166,6 +168,20 @@ public void Can_set_etag_concurrency_entity() Assert.True(etagProperty.IsConcurrencyToken); } + [Fact] + public void Default_discriminator_property_uses_embedded_discriminator_json_name() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder.HasEmbeddedDiscriminatorName("Terminator"); + modelBuilder.Entity(); + + var discriminatorProperty = modelBuilder.Model.FindEntityType(typeof(Customer))!.FindDiscriminatorProperty()!; + + Assert.Equal("Discriminator", discriminatorProperty.Name); + Assert.Equal("Terminator", discriminatorProperty.GetJsonPropertyName()); + } + [Fact] public void Can_set_etag_concurrency_property() { diff --git a/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs index 865ab6f8b86..a36e2612871 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs @@ -387,6 +387,36 @@ public class InvalidNameNestedEntity public string Name2 { get; set; } = ""; } + [Fact] + public virtual async Task Invalid_identifier_shadow_property_name() + { + var contextFactory = await InitializeNonSharedTest( + onConfiguring: o => o.ConfigureWarnings(w => w.Ignore(CoreEventId.ShadowPropertyNameNotValidIdentifierWarning))); + var options = contextFactory.GetOptions(); + + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.InvalidShadowNameContext(dbContextOptions); +var entities = await context.Entities.ToListAsync(); +""", + typeof(InvalidShadowNameContext), + options); + } + + public class InvalidShadowNameContext(DbContextOptions options) : DbContext(options) + { + public DbSet Entities { get; set; } = null!; + + protected override void OnModelCreating(ModelBuilder modelBuilder) + => modelBuilder.Entity() + .Property("NOT VALID !!!1").HasConversion(x => 0, x => ""); + } + + public class InvalidShadowNameEntity + { + public Guid Id { get; set; } + } + #endregion protected TestSqlLoggerFactory TestSqlLoggerFactory diff --git a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs index 275a48afaea..a0c3b9bb75f 100644 --- a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs +++ b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs @@ -462,6 +462,40 @@ public virtual void Passes_on_shadow_primary_key_created_by_convention_in_depend .GenerateMessage("A", "Key"), modelBuilder, LogLevel.Debug); } + [Fact] + public virtual void Warns_on_shadow_property_name_that_is_not_a_valid_identifier() + { + var modelBuilder = CreateConventionlessModelBuilder(); + var model = modelBuilder.Model; + + var entityType = model.AddEntityType(typeof(A)); + SetPrimaryKey(entityType); + AddProperties(entityType); + + entityType.AddProperty("NOT VALID !!!1", typeof(string)); + + VerifyWarning( + CoreResources.LogShadowPropertyNameNotValidIdentifier(new TestLogger()) + .GenerateMessage("A", "NOT VALID !!!1"), modelBuilder, LogLevel.Warning); + } + + [Fact] + public virtual void Does_not_warn_on_shadow_property_with_valid_identifier_name() + { + var modelBuilder = CreateConventionlessModelBuilder(); + var model = modelBuilder.Model; + + var entityType = model.AddEntityType(typeof(A)); + SetPrimaryKey(entityType); + AddProperties(entityType); + + entityType.AddProperty("ValidName", typeof(string)); + + VerifyLogDoesNotContain( + CoreResources.LogShadowPropertyNameNotValidIdentifier(new TestLogger()) + .GenerateMessage("A", "ValidName"), modelBuilder); + } + [Fact] // Issue #33484 public virtual void Does_not_log_for_shadow_property_when_creating_indexer_property() {