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

Skip and warn of duplicated SQL Server foreign keys #25378

Merged
merged 13 commits into from
Aug 10, 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 @@ -149,6 +149,14 @@ public class SqlServerLoggingDefinitions : RelationalLoggingDefinitions
/// </summary>
public EventDefinitionBase? LogReflexiveConstraintIgnored;

/// <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 EventDefinitionBase? LogDuplicateForeignKeyConstraintIgnored;

/// <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
9 changes: 8 additions & 1 deletion src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ private enum Id
IndexFound,
ForeignKeyFound,
ForeignKeyPrincipalColumnMissingWarning,
ReflexiveConstraintIgnored
ReflexiveConstraintIgnored,
DuplicateForeignKeyConstraintIgnored,
}

private static readonly string _validationPrefix = DbLoggerCategory.Model.Validation.Name + ".";
Expand Down Expand Up @@ -230,5 +231,11 @@ private static EventId MakeScaffoldingId(Id id)
/// This event is in the <see cref="DbLoggerCategory.Scaffolding" /> category.
/// </summary>
public static readonly EventId ReflexiveConstraintIgnored = MakeScaffoldingId(Id.ReflexiveConstraintIgnored);

/// <summary>
/// A duplicate foreign key constraint was skipped.
/// This event is in the <see cref="DbLoggerCategory.Scaffolding" /> category.
/// </summary>
public static readonly EventId DuplicateForeignKeyConstraintIgnored = MakeScaffoldingId(Id.DuplicateForeignKeyConstraintIgnored);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,28 @@ public static void ReflexiveConstraintIgnored(
// No DiagnosticsSource events because these are purely design-time messages
}

/// <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 static void DuplicateForeignKeyConstraintIgnored(
this IDiagnosticsLogger<DbLoggerCategory.Scaffolding> diagnostics,
string foreignKeyName,
string tableName,
string duplicateForeignKeyName)
{
var definition = SqlServerResources.DuplicateForeignKeyConstraintIgnored(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics, foreignKeyName, tableName, duplicateForeignKeyName);
}

// No DiagnosticsSource events because these are purely design-time messages
}

/// <summary>
/// Logs for the <see cref="RelationalEventId.MultipleCollectionIncludeWarning" /> event.
/// </summary>
Expand Down
25 changes: 25 additions & 0 deletions src/EFCore.SqlServer/Properties/SqlServerStrings.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.SqlServer/Properties/SqlServerStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@
<value>Skipping foreign key '{foreignKeyName}' on table '{tableName}' since all of its columns reference themselves.</value>
<comment>Debug SqlServerEventId.ReflexiveConstraintIgnored string string</comment>
</data>
<data name="DuplicateForeignKeyConstraintIgnored" xml:space="preserve">
<value>Skipping foreign key '{foreignKeyName}' on table '{tableName}' since it is a duplicate of '{duplicateForeignKeyName}'.</value>
<comment>Warning SqlServerEventId.DuplicateForeignKeyConstraintIgnored string string string</comment>
</data>
<data name="LogSavepointsDisabledBecauseOfMARS" xml:space="preserve">
<value>Savepoints are disabled because Multiple Active Result Sets (MARS) is enabled. If 'SaveChanges' fails, then the transaction cannot be automatically rolled back to a known clean state. Instead, the transaction should be rolled back by the application before retrying 'SaveChanges'. See https://go.microsoft.com/fwlink/?linkid=2149338 for more information. To identify the code which triggers this warning, call 'ConfigureWarnings(w =&gt; w.Throw(SqlServerEventId.SavepointsDisabledBecauseOfMARS))'.</value>
<comment>Warning SqlServerEventId.SavepointsDisabledBecauseOfMARS</comment>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,18 @@ FROM [sys].[foreign_keys] AS [f]
}
else
{
var duplicated = table.ForeignKeys
smitpatel marked this conversation as resolved.
Show resolved Hide resolved
.FirstOrDefault(k => k.Columns.SequenceEqual(foreignKey.Columns)
&& k.PrincipalTable.Equals(foreignKey.PrincipalTable));
if (duplicated != null)
{
_logger.DuplicateForeignKeyConstraintIgnored(
foreignKey.Name!,
DisplayName(table.Schema, table.Name!),
duplicated.Name!);
continue;
}

table.ForeignKeys.Add(foreignKey);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2359,6 +2359,45 @@ CONSTRAINT MYFK FOREIGN KEY (Id) REFERENCES PrincipalTable(Id)
DROP TABLE PrincipalTable;");
}

[ConditionalFact]
public void Skip_duplicate_foreign_key()
{
Test(
@"CREATE TABLE PrincipalTable (
Id int PRIMARY KEY,
);

CREATE TABLE OtherPrincipalTable (
Id int PRIMARY KEY,
);

CREATE TABLE DependentTable (
Id int PRIMARY KEY,
ForeignKeyId int,
CONSTRAINT MYFK1 FOREIGN KEY (ForeignKeyId) REFERENCES PrincipalTable(Id),
CONSTRAINT MYFK2 FOREIGN KEY (ForeignKeyId) REFERENCES PrincipalTable(Id),
CONSTRAINT MYFK3 FOREIGN KEY (ForeignKeyId) REFERENCES OtherPrincipalTable(Id),
);",
Enumerable.Empty<string>(),
Enumerable.Empty<string>(),
dbModel =>
{
var (level, _, message, _, _) = Assert.Single(
Fixture.ListLoggerFactory.Log, t => t.Id == SqlServerEventId.DuplicateForeignKeyConstraintIgnored);
Assert.Equal(LogLevel.Warning, level);
Assert.Equal(
SqlServerResources.DuplicateForeignKeyConstraintIgnored(new TestLogger<SqlServerLoggingDefinitions>())
.GenerateMessage("MYFK2", "dbo.DependentTable", "MYFK1"), message);

var table = dbModel.Tables.Single(t => t.Name == "DependentTable");
Assert.Equal(2, table.ForeignKeys.Count);
},
@"
DROP TABLE DependentTable;
DROP TABLE PrincipalTable;
DROP TABLE OtherPrincipalTable;");
}

#endregion

private void Test(
Expand Down