Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when uniquifying an FK column name #25478

Merged
merged 3 commits into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public virtual void Generate(IProperty property, CSharpRuntimeAnnotationCodeGene
annotations.Remove(CoreAnnotationNames.ValueConverterType);
annotations.Remove(CoreAnnotationNames.ValueComparer);
annotations.Remove(CoreAnnotationNames.ValueComparerType);
annotations.Remove(CoreAnnotationNames.PreUniquificationName);
}

GenerateSimpleAnnotations(parameters);
Expand Down
17 changes: 17 additions & 0 deletions src/EFCore/Diagnostics/CoreEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ private enum Id
PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning,
RequiredAttributeOnSkipNavigation,
AmbiguousEndRequiredWarning,
ShadowForeignKeyPropertyCreated,

// ChangeTracking events
DetectChangesStarting = CoreBaseId + 800,
Expand Down Expand Up @@ -473,6 +474,22 @@ private static EventId MakeModelValidationId(Id id)
/// </summary>
public static readonly EventId ShadowPropertyCreated = MakeModelValidationId(Id.ShadowPropertyCreated);

/// <summary>
/// <para>
/// A foreign key property was created in shadow state because a conflicting property with the simple name for
/// this foreign key exists in the entity type, but is either not mapped, is already used for another relationship,
/// or is incompatible with the associated primary key type. See https://aka.ms/efcore-relationships for information
/// on mapping relationships in EF Core.
/// </para>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Model" /> category.
/// </para>
/// <para>
/// This event uses the <see cref="UniquifiedPropertyEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
public static readonly EventId ShadowForeignKeyPropertyCreated = MakeModelValidationId(Id.ShadowForeignKeyPropertyCreated);

/// <summary>
/// <para>
/// An index was not created as the properties are already covered.
Expand Down
37 changes: 37 additions & 0 deletions src/EFCore/Diagnostics/CoreLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,43 @@ public static void RedundantAddServicesCallWarning(
}
}

/// <summary>
/// Logs for the <see cref="CoreEventId.ShadowForeignKeyPropertyCreated" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="property"> The property. </param>
/// <param name="basePropertyName"> The property name that was uniquified. </param>
public static void ShadowForeignKeyPropertyCreated(
this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
IProperty property,
string basePropertyName)
{
var definition = CoreResources.LogShadowForeignKeyPropertyCreated(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics, property.DeclaringEntityType.DisplayName(), property.Name, basePropertyName);
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new UniquifiedPropertyEventData(
definition,
ShadowForeignKeyPropertyCreated,
property,
basePropertyName);

diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
}
}

private static string ShadowForeignKeyPropertyCreated(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string, string>)definition;
var p = (UniquifiedPropertyEventData)payload;
return d.GenerateMessage(p.Property.DeclaringEntityType.DisplayName(), p.Property.Name, p.BasePropertyName);
}

/// <summary>
/// Logs for the <see cref="CoreEventId.ShadowPropertyCreated" /> event.
/// </summary>
Expand Down
9 changes: 9 additions & 0 deletions src/EFCore/Diagnostics/LoggingDefinitions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,15 @@ public abstract class LoggingDefinitions
[EntityFrameworkInternal]
public EventDefinitionBase? LogShadowPropertyCreated;

/// <summary>
/// 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.
/// </summary>
[EntityFrameworkInternal]
public EventDefinitionBase? LogShadowForeignKeyPropertyCreated;

/// <summary>
/// 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
Expand Down
38 changes: 38 additions & 0 deletions src/EFCore/Diagnostics/UniquifiedPropertyEventData.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using Microsoft.EntityFrameworkCore.Metadata;

namespace Microsoft.EntityFrameworkCore.Diagnostics
{
/// <summary>
/// A <see cref="DiagnosticSource" /> event payload class for events that have
/// a property that has been uniquified.
/// </summary>
public class UniquifiedPropertyEventData : PropertyEventData
{
/// <summary>
/// Constructs the event payload.
/// </summary>
/// <param name="eventDefinition"> The event definition. </param>
/// <param name="messageGenerator"> A delegate that generates a log message for this event. </param>
/// <param name="property"> The property. </param>
/// <param name="basePropertyName"> The property name that was uniquified. </param>
public UniquifiedPropertyEventData(
EventDefinitionBase eventDefinition,
Func<EventDefinitionBase, EventData, string> messageGenerator,
IReadOnlyProperty property,
string basePropertyName)
: base(eventDefinition, messageGenerator, property)
{
BasePropertyName = basePropertyName;
}

/// <summary>
/// The property name that was uniquified.
/// </summary>
public virtual string BasePropertyName { get; }
}
}
11 changes: 10 additions & 1 deletion src/EFCore/Infrastructure/ModelValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,16 @@ protected virtual void LogShadowProperties(
{
if (property.IsImplicitlyCreated())
{
logger.ShadowPropertyCreated((IProperty)property);
var uniquifiedAnnotation = property.FindAnnotation(CoreAnnotationNames.PreUniquificationName);
if (uniquifiedAnnotation != null
&& property.IsForeignKey())
{
logger.ShadowForeignKeyPropertyCreated((IProperty)property, (string)uniquifiedAnnotation.Value!);
}
else
{
logger.ShadowPropertyCreated((IProperty)property);
}
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions src/EFCore/Metadata/Internal/CoreAnnotationNames.cs
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,14 @@ public static class CoreAnnotationNames
/// </summary>
public const string ReadOnlyModel = "ReadOnlyModel";

/// <summary>
/// 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.
/// </summary>
public const string PreUniquificationName = "PreUniquificationName";

/// <summary>
/// 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
Expand Down Expand Up @@ -319,6 +327,7 @@ public static class CoreAnnotationNames
ProviderClrType,
ModelDependencies,
ReadOnlyModel,
PreUniquificationName,
InverseNavigations,
DerivedTypes,
NavigationCandidates,
Expand Down
8 changes: 8 additions & 0 deletions src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4466,6 +4466,14 @@ public virtual bool ShouldReuniquifyTemporaryProperties(ForeignKey foreignKey)
return (false, null);
}

if (index > 0)
ajcvickers marked this conversation as resolved.
Show resolved Hide resolved
{
propertyBuilder.HasAnnotation(
CoreAnnotationNames.PreUniquificationName,
keyModifiedBaseName,
ConfigurationSource.Convention);
}

if (clrType.IsNullableType())
{
propertyBuilder.IsRequired(isRequired, ConfigurationSource.Convention);
Expand Down
25 changes: 25 additions & 0 deletions src/EFCore/Properties/CoreStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/EFCore/Properties/CoreStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,10 @@
<value>An additional 'IServiceProvider' was created for internal use by Entity Framework. An existing service provider was not used due to the following configuration changes: {debugInfo}.</value>
<comment>Debug CoreEventId.ServiceProviderDebugInfo string</comment>
</data>
<data name="LogShadowForeignKeyPropertyCreated" xml:space="preserve">
<value>The foreign key property '{entityType}.{property}' was created in shadow state because a conflicting property with the simple name '{baseName}' exists in the entity type, but is either not mapped, is already used for another relationship, or is incompatible with the associated primary key type. See https://aka.ms/efcore-relationships for information on mapping relationships in EF Core.</value>
<comment>Warning CoreEventId.ShadowForeignKeyPropertyCreated string string</comment>
</data>
<data name="LogShadowPropertyCreated" xml:space="preserve">
<value>The property '{entityType}.{property}' was created in shadow state because there are no eligible CLR members with a matching name.</value>
<comment>Debug CoreEventId.ShadowPropertyCreated string string</comment>
Expand Down
3 changes: 3 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/EmbeddedDocumentsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,9 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
.WithOne(e => e.FuelTank)
.HasForeignKey<FuelTank>(e => e.VehicleName)
.OnDelete(DeleteBehavior.Restrict);
eb.HasOne(e => e.Vehicle)
.WithOne()
.HasForeignKey<FuelTank>("VehicleName1");
});

modelBuilder.Entity<ContinuousCombustionEngine>();
Expand Down
1 change: 1 addition & 0 deletions test/EFCore.Specification.Tests/DataAnnotationTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2500,6 +2500,7 @@ public virtual void OwnedEntityTypeAttribute_configures_all_references_as_owned(
var modelBuilder = CreateModelBuilder();
var model = modelBuilder.Model;

modelBuilder.Entity<Book>().Ignore(e => e.AlternateLabel);
modelBuilder.Entity<Book>()
.HasOne(b => b.Label).WithOne(l => l.Book)
.HasForeignKey<BookLabel>(l => l.BookId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -462,16 +462,19 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
modelBuilder.Entity<InheritanceLeaf2>().Property(e => e.Id).ValueGeneratedNever();

// FK name needs to be explicitly provided because issue #9310
modelBuilder.Entity<InheritanceBase2>().HasOne(e => e.Reference).WithOne().HasForeignKey<InheritanceBase1>("InheritanceBase2Id")
modelBuilder.Entity<InheritanceBase2>().HasOne(e => e.Reference).WithOne()
.HasForeignKey<InheritanceBase1>("InheritanceBase2Id")
.IsRequired(false);
modelBuilder.Entity<InheritanceBase2>().HasMany(e => e.Collection).WithOne();
modelBuilder.Entity<InheritanceBase2>().HasMany(e => e.Collection).WithOne()
.HasForeignKey("InheritanceBase2Id1");

modelBuilder.Entity<InheritanceDerived1>().HasBaseType<InheritanceBase1>();
modelBuilder.Entity<InheritanceDerived1>().HasOne(e => e.ReferenceSameType).WithOne()
.HasForeignKey<InheritanceLeaf1>("SameTypeReference_InheritanceDerived1Id").IsRequired(false);
modelBuilder.Entity<InheritanceDerived1>().HasOne(e => e.ReferenceDifferentType).WithOne()
.HasForeignKey<InheritanceLeaf1>("DifferentTypeReference_InheritanceDerived1Id").IsRequired(false);
modelBuilder.Entity<InheritanceDerived1>().HasMany(e => e.CollectionSameType).WithOne().HasForeignKey("InheritanceDerived1Id")
modelBuilder.Entity<InheritanceDerived1>().HasMany(e => e.CollectionSameType).WithOne()
.HasForeignKey("InheritanceDerived1Id1")
.IsRequired(false);
modelBuilder.Entity<InheritanceDerived1>().HasMany(e => e.CollectionDifferentType).WithOne()
.HasForeignKey("InheritanceDerived1Id").IsRequired(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,11 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
modelBuilder.Entity<InheritanceLeaf2>().Property(e => e.Id).ValueGeneratedNever();

// FK name needs to be explicitly provided because issue #9310
modelBuilder.Entity<InheritanceBase2>().HasOne(e => e.Reference).WithOne().HasForeignKey<InheritanceBase1>("InheritanceBase2Id")
modelBuilder.Entity<InheritanceBase2>().HasOne(e => e.Reference).WithOne()
.HasForeignKey<InheritanceBase1>("InheritanceBase2Id1")
.IsRequired(false);
modelBuilder.Entity<InheritanceBase2>().HasMany(e => e.Collection).WithOne();
modelBuilder.Entity<InheritanceBase2>().HasMany(e => e.Collection).WithOne()
.HasForeignKey("InheritanceBase2Id");

modelBuilder.Entity<InheritanceDerived1>().HasBaseType<InheritanceBase1>();
modelBuilder.Entity<InheritanceDerived1>().HasOne(e => e.ReferenceSameType).WithOne()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
r => r.HasOne<EntityTwo>().WithMany().HasForeignKey(e => e.TwoId),
l => l.HasOne<EntityOne>().WithMany().HasForeignKey(e => e.OneId));

modelBuilder.Entity<JoinOneToThreePayloadFull>(
b =>
{
b.Property(e => e.OneId);
b.Property(e => e.ThreeId);
AndriySvyryd marked this conversation as resolved.
Show resolved Hide resolved
});

// Nav:6 Payload:Yes Join:Concrete Extra:None
modelBuilder.Entity<EntityOne>()
.HasMany(e => e.ThreeSkipPayloadFull)
Expand Down Expand Up @@ -373,7 +380,12 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
e.CompositeId2,
e.CompositeId3
}).IsRequired(),
r => r.HasOne(x => x.Three).WithMany(x => x.JoinCompositeKeyFull).IsRequired());
r => r.HasOne(x => x.Three).WithMany(x => x.JoinCompositeKeyFull).IsRequired(),
b =>
{
b.Property(e => e.Id);
b.Property(e => e.ThreeId);
});

// Nav:2 Payload:No Join:Shared Extra:Inheritance
// TODO: Remove UsingEntity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5259,7 +5259,7 @@ protected internal override void OnModelCreating(ModelBuilder modelBuilder)
modelBuilder.Entity<Thing>().OwnsMany(
p => p.OwnedByThings, a =>
{
a.WithOwner().HasForeignKey(e => e.ThingId);
a.WithOwner(e => e.Thing).HasForeignKey(e => e.ThingId);
a.HasKey(e => e.OwnedByThingId);
});
}
Expand Down
Loading