Skip to content

Commit

Permalink
Add a warning for unmapped foreign keys
Browse files Browse the repository at this point in the history
Throw by default for unmapped foreign keys and indexes
Only map identifying FKs to the table for the declaring type in TPT
Separate ModelBuilderOtherTest into the appropriate classes
  • Loading branch information
AndriySvyryd committed Aug 22, 2020
1 parent 2c4fd98 commit 23f0a34
Show file tree
Hide file tree
Showing 27 changed files with 638 additions and 487 deletions.
14 changes: 14 additions & 0 deletions src/EFCore.Relational/Diagnostics/RelationalEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ private enum Id
AllIndexPropertiesNotToMappedToAnyTable,
IndexPropertiesBothMappedAndNotMappedToTable,
IndexPropertiesMappedToNonOverlappingTables,
ForeignKeyPropertiesMappedToUnrelatedTables,

// Update events
BatchReadyForExecution = CoreEventId.RelationalBaseId + 700,
Expand Down Expand Up @@ -694,6 +695,19 @@ private enum Id
/// </summary>
public static readonly EventId IndexPropertiesMappedToNonOverlappingTables = MakeValidationId(Id.IndexPropertiesMappedToNonOverlappingTables);

/// <summary>
/// <para>
/// A foreign key specifies properties which don't map to the related tables.
/// </para>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Model.Validation" /> category.
/// </para>
/// <para>
/// This event uses the <see cref="ForeignKeyEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
public static readonly EventId ForeignKeyPropertiesMappedToUnrelatedTables = MakeValidationId(Id.ForeignKeyPropertiesMappedToUnrelatedTables);

private static readonly string _updatePrefix = DbLoggerCategory.Update.Name + ".";
private static EventId MakeUpdateId(Id id) => new EventId((int)id, _updatePrefix + id);

Expand Down
64 changes: 60 additions & 4 deletions src/EFCore.Relational/Diagnostics/RelationalLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4555,10 +4555,10 @@ public static void IndexPropertiesMappedToNonOverlappingTables(
definition.Log(diagnostics,
entityType.DisplayName(),
index.Properties.Format(),
property1Name,
tablesMappedToProperty1.FormatTables(),
property2Name,
tablesMappedToProperty2.FormatTables());
property1Name,
tablesMappedToProperty1.FormatTables(),
property2Name,
tablesMappedToProperty2.FormatTables());
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
Expand Down Expand Up @@ -4646,6 +4646,62 @@ private static string NamedIndexPropertiesMappedToNonOverlappingTables(EventDefi
p.TablesMappedToProperty2.FormatTables()));
}

/// <summary>
/// Logs the <see cref="RelationalEventId.IndexPropertiesMappedToNonOverlappingTables" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="foreignKey"> The foreign key. </param>
public static void ForeignKeyPropertiesMappedToUnrelatedTables(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
[NotNull] IForeignKey foreignKey)
{
var definition = RelationalResources.LogForeignKeyPropertiesMappedToUnrelatedTables(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics,
l => l.Log(
definition.Level,
definition.EventId,
definition.MessageFormat,
foreignKey.Properties.Format(),
foreignKey.DeclaringEntityType.DisplayName(),
foreignKey.PrincipalEntityType.DisplayName(),
foreignKey.Properties.Format(),
foreignKey.DeclaringEntityType.GetSchemaQualifiedTableName(),
foreignKey.PrincipalKey.Properties.Format(),
foreignKey.PrincipalEntityType.GetSchemaQualifiedTableName()));
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new ForeignKeyEventData(
definition,
ForeignKeyPropertiesMappedToUnrelatedTables,
foreignKey);

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

private static string ForeignKeyPropertiesMappedToUnrelatedTables(EventDefinitionBase definition, EventData payload)
{
var d = (FallbackEventDefinition)definition;
var p = (ForeignKeyEventData)payload;
return d.GenerateMessage(
l => l.Log(
d.Level,
d.EventId,
d.MessageFormat,
p.ForeignKey.Properties.Format(),
p.ForeignKey.DeclaringEntityType.DisplayName(),
p.ForeignKey.PrincipalEntityType.DisplayName(),
p.ForeignKey.Properties.Format(),
p.ForeignKey.DeclaringEntityType.GetSchemaQualifiedTableName(),
p.ForeignKey.PrincipalKey.Properties.Format(),
p.ForeignKey.PrincipalEntityType.GetSchemaQualifiedTableName()));
}

/// <summary>
/// Logs for the <see cref="RelationalEventId.BatchExecutorFailedToRollbackToSavepoint" /> event.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,15 @@ public abstract class RelationalLoggingDefinitions : LoggingDefinitions
[EntityFrameworkInternal]
public EventDefinitionBase LogUnnamedIndexPropertiesMappedToNonOverlappingTables;

/// <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 LogForeignKeyPropertiesMappedToUnrelatedTables;

/// <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
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ public static class RelationalForeignKeyExtensions
/// <param name="foreignKey"> The foreign key. </param>
/// <returns> The foreign key constraint name. </returns>
public static string GetConstraintName([NotNull] this IForeignKey foreignKey)
=> foreignKey.GetConstraintName(
StoreObjectIdentifier.Create(foreignKey.DeclaringEntityType, StoreObjectType.Table).Value,
StoreObjectIdentifier.Create(foreignKey.PrincipalKey.IsPrimaryKey()
? foreignKey.PrincipalEntityType
: foreignKey.PrincipalKey.DeclaringEntityType,
StoreObjectType.Table).Value);
{
var annotation = foreignKey.FindAnnotation(RelationalAnnotationNames.Name);
return annotation != null
? (string)annotation.Value
: foreignKey.GetDefaultName();
}

/// <summary>
/// Returns the foreign key constraint name.
Expand Down
28 changes: 23 additions & 5 deletions src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,30 @@ public static string GetColumnName([NotNull] this IProperty property, in StoreOb
return overrides.ColumnName;
}

if (!property.IsPrimaryKey()
&& storeObject.StoreObjectType != StoreObjectType.Function
&& storeObject.StoreObjectType != StoreObjectType.SqlQuery
&& StoreObjectIdentifier.Create(property.DeclaringEntityType, storeObject.StoreObjectType) != storeObject)
if (storeObject.StoreObjectType != StoreObjectType.Function
&& storeObject.StoreObjectType != StoreObjectType.SqlQuery)
{
return null;
if (property.IsPrimaryKey())
{
var tableFound = false;
foreach (var containingType in property.DeclaringEntityType.GetDerivedTypesInclusive())
{
if (StoreObjectIdentifier.Create(containingType, storeObject.StoreObjectType) == storeObject)
{
tableFound = true;
break;
}
}

if (!tableFound)
{
return null;
}
}
else if (StoreObjectIdentifier.Create(property.DeclaringEntityType, storeObject.StoreObjectType) != storeObject)
{
return null;
}
}

var columnAnnotation = property.FindAnnotation(RelationalAnnotationNames.ColumnName);
Expand Down
19 changes: 13 additions & 6 deletions src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,17 @@ protected virtual void ValidateSharedForeignKeysCompatibility(
var foreignKeyName = foreignKey.GetConstraintName(storeObject, principalTable.Value);
if (foreignKeyName == null)
{
var derivedTables = foreignKey.DeclaringEntityType.GetDerivedTypes()
.Select(t => StoreObjectIdentifier.Create(t, StoreObjectType.Table))
.Where(t => t != null);
if (foreignKey.GetConstraintName() != null
&& derivedTables.All(t => foreignKey.GetConstraintName(
t.Value,
principalTable.Value) == null))
{
logger.ForeignKeyPropertiesMappedToUnrelatedTables(foreignKey);
}

continue;
}

Expand Down Expand Up @@ -1227,8 +1238,7 @@ protected virtual void ValidateIndexProperties(

if (firstPropertyTables != null)
{
// Property is not mapped but we already found
// a property that is mapped.
// Property is not mapped but we already found a property that is mapped.
break;
}

Expand All @@ -1237,21 +1247,18 @@ protected virtual void ValidateIndexProperties(

if (firstPropertyTables == null)
{
// store off which tables the first member maps to
firstPropertyTables =
new Tuple<string, List<(string Table, string Schema)>>(property.Name, tablesMappedToProperty);
}
else
{
// store off which tables the last member we encountered maps to
lastPropertyTables =
new Tuple<string, List<(string Table, string Schema)>>(property.Name, tablesMappedToProperty);
}

if (propertyNotMappedToAnyTable != null)
{
// Property is mapped but we already found
// a property that is not mapped.
// Property is mapped but we already found a property that is not mapped.
overlappingTables = null;
break;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,18 @@ public virtual void Validate(IDbContextOptions options)
{
}

/// <summary>
/// Adds default <see cref="WarningBehavior"/> for relational events.
/// </summary>
/// <param name="coreOptionsExtension"> The core options extension. </param>
/// <returns> The new core options extension. </returns>
public static CoreOptionsExtension WithDefaultWarningConfiguration([NotNull] CoreOptionsExtension coreOptionsExtension)
=> coreOptionsExtension.WithWarningsConfiguration(coreOptionsExtension.WarningsConfiguration
.TryWithExplicit(RelationalEventId.AmbientTransactionWarning, WarningBehavior.Throw)
.TryWithExplicit(RelationalEventId.IndexPropertiesBothMappedAndNotMappedToTable, WarningBehavior.Throw)
.TryWithExplicit(RelationalEventId.IndexPropertiesMappedToNonOverlappingTables, WarningBehavior.Throw)
.TryWithExplicit(RelationalEventId.ForeignKeyPropertiesMappedToUnrelatedTables, WarningBehavior.Throw));

/// <summary>
/// Information/metadata for a <see cref="RelationalOptionsExtension" />.
/// </summary>
Expand All @@ -385,7 +397,7 @@ protected abstract class RelationalExtensionInfo : DbContextOptionsExtensionInfo
private string _logFragment;

/// <summary>
/// Creates a new <see cref="RelationalOptionsExtension.RelationalExtensionInfo" /> instance containing
/// Creates a new <see cref="RelationalExtensionInfo" /> instance containing
/// info/metadata for the given extension.
/// </summary>
/// <param name="extension"> The extension. </param>
Expand Down
9 changes: 9 additions & 0 deletions src/EFCore.Relational/Metadata/Internal/RelationalModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,15 @@ private static void PopulateConstraints(Table table)
break;
}

if (entityTypeMapping.IncludesDerivedTypes
&& foreignKey.DeclaringEntityType != entityType
&& foreignKey.Properties.SequenceEqual(entityType.FindPrimaryKey().Properties))
{
// The identifying FK constraint is needed to be created only on the table that corresponds
// to the declaring entity type
break;
}

constraint = new ForeignKeyConstraint(name, table, principalTable, columns, principalColumns,
ToReferentialAction(foreignKey.DeleteBehavior));
constraint.MappedForeignKeys.Add(foreignKey);
Expand Down
21 changes: 21 additions & 0 deletions src/EFCore.Relational/Properties/RelationalStrings.Designer.cs

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

Loading

0 comments on commit 23f0a34

Please sign in to comment.