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

Error for optional dependents sharing table without identifying column #24573

Merged
1 commit merged into from
Apr 6, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/EFCore.Relational/Diagnostics/RelationalEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ private enum Id
IndexPropertiesBothMappedAndNotMappedToTable,
IndexPropertiesMappedToNonOverlappingTables,
ForeignKeyPropertiesMappedToUnrelatedTables,
OptionalDependentWithoutIdentifyingPropertyWarning,

// Update events
BatchReadyForExecution = CoreEventId.RelationalBaseId + 700,
Expand Down Expand Up @@ -723,6 +724,20 @@ private static EventId MakeValidationId(Id id)
/// </summary>
public static readonly EventId ForeignKeyPropertiesMappedToUnrelatedTables = MakeValidationId(Id.ForeignKeyPropertiesMappedToUnrelatedTables);

/// <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="EntityTypeEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
public static readonly EventId OptionalDependentWithoutIdentifyingPropertyWarning
= MakeValidationId(Id.OptionalDependentWithoutIdentifyingPropertyWarning);

private static readonly string _updatePrefix = DbLoggerCategory.Update.Name + ".";

private static EventId MakeUpdateId(Id id)
Expand Down
36 changes: 35 additions & 1 deletion src/EFCore.Relational/Diagnostics/RelationalLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4674,7 +4674,7 @@ private static string NamedIndexPropertiesMappedToNonOverlappingTables(EventDefi
}

/// <summary>
/// Logs the <see cref="RelationalEventId.IndexPropertiesMappedToNonOverlappingTables" /> event.
/// Logs the <see cref="RelationalEventId.ForeignKeyPropertiesMappedToUnrelatedTables" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="foreignKey"> The foreign key. </param>
Expand Down Expand Up @@ -4729,6 +4729,40 @@ private static string ForeignKeyPropertiesMappedToUnrelatedTables(EventDefinitio
p.ForeignKey.PrincipalEntityType.GetSchemaQualifiedTableName()));
}

/// <summary>
/// Logs the <see cref="RelationalEventId.OptionalDependentWithoutIdentifyingPropertyWarning" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="entityType"> The entity type. </param>
public static void OptionalDependentWithoutIdentifyingPropertyWarning(
this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
IEntityType entityType)
{
var definition = RelationalResources.LogOptionalDependentWithoutIdentifyingProperty(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics, entityType.DisplayName());
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new EntityTypeEventData(
definition,
OptionalDependentWithoutIdentifyingPropertyWarning,
entityType);

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

private static string OptionalDependentWithoutIdentifyingPropertyWarning(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string>)definition;
var p = (EntityTypeEventData)payload;
return d.GenerateMessage(p.EntityType.DisplayName());
}

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

/// <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? LogOptionalDependentWithoutIdentifyingProperty;
}
}
83 changes: 83 additions & 0 deletions src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,89 @@ static bool IsNotNullAndFalse(object? value)
ValidateSharedKeysCompatibility(mappedTypes, table, logger);
ValidateSharedForeignKeysCompatibility(mappedTypes, table, logger);
ValidateSharedIndexesCompatibility(mappedTypes, table, logger);

// Validate optional dependents
if (mappedTypes.Count == 1)
{
continue;
}

var principalEntityTypesMap = new Dictionary<IEntityType, (List<IEntityType> EntityTypes, bool Optional)>();
foreach (var entityType in mappedTypes)
{
if (entityType.BaseType != null
|| entityType.FindPrimaryKey() == null)
{
continue;
}

var (principalEntityTypes, optional) = GetPrincipalEntityTypes(entityType);
if (!optional)
{
continue;
}

var principalColumns = principalEntityTypes.SelectMany(e => e.GetProperties())
.Select(e => e.GetColumnName(table))
.Where(e => e != null)
.ToList();
var requiredNonSharedColumnFound = false;
foreach (var property in entityType.GetProperties())
{
if (property.IsPrimaryKey()
|| property.IsNullable)
{
continue;
}

var columnName = property.GetColumnName(table);
if (columnName != null)
{
if (!principalColumns.Contains(columnName))
{
requiredNonSharedColumnFound = true;
break;
}
}
}
if (!requiredNonSharedColumnFound)
{
if (entityType.GetReferencingForeignKeys().Select(e => e.DeclaringEntityType).Any(t => mappedTypes.Contains(t)))
{
throw new InvalidOperationException(
RelationalStrings.OptionalDependentWithDependentWithoutIdentifyingProperty(entityType.DisplayName()));
}

logger.OptionalDependentWithoutIdentifyingPropertyWarning(entityType);
}
}

(List<IEntityType> EntityTypes, bool Optional) GetPrincipalEntityTypes(IEntityType entityType)
{
if (!principalEntityTypesMap.TryGetValue(entityType, out var tuple))
{
var list = new List<IEntityType>();
var optional = false;
foreach (var foreignKey in entityType.FindForeignKeys(entityType.FindPrimaryKey()!.Properties))
{
var principalEntityType = foreignKey.PrincipalEntityType;
if (!mappedTypes.Contains(principalEntityType))
{
continue;
}
list.Add(principalEntityType);
var (entityTypes, innerOptional) = GetPrincipalEntityTypes(principalEntityType.GetRootType());
list.AddRange(entityTypes);

optional |= !foreignKey.IsRequiredDependent | innerOptional;
}

tuple = (list, optional);
principalEntityTypesMap.Add(entityType, tuple);
}

return tuple;
}
}
}

Expand Down
33 changes: 33 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.

7 changes: 7 additions & 0 deletions src/EFCore.Relational/Properties/RelationalStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,10 @@
<value>Opening connection to database '{database}' on server '{server}'.</value>
<comment>Debug RelationalEventId.ConnectionOpening string string</comment>
</data>
<data name="LogOptionalDependentWithoutIdentifyingProperty" xml:space="preserve">
<value>Entity type '{entityType}' is an optional dependent in table sharing without any required non shared property to identify if the entity type exist. If all nullable properties contain null value in database then an object instance won't be materialized in the query.</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<value>Entity type '{entityType}' is an optional dependent in table sharing without any required non shared property to identify if the entity type exist. If all nullable properties contain null value in database then an object instance won't be materialized in the query.</value>
<value>The entity type '{entityType}' is an optional dependent using table sharing without any required non shared property that could be used to identify whether the entity exists. If all nullable properties contain a null value in database then an object instance won't be created in the query. Add a required property to create instances with null values for other properties or mark the incoming navigation as required to always create an instance. </value>

<comment>Error RelationalEventId.ModelValidationOptionalDependentWithoutIdentifyingPropertyWarning string</comment>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<comment>Error RelationalEventId.ModelValidationOptionalDependentWithoutIdentifyingPropertyWarning string</comment>
<comment>Warning RelationalEventId.ModelValidationOptionalDependentWithoutIdentifyingPropertyWarning string</comment>

</data>
<data name="LogPossibleUnintendedUseOfEquals" xml:space="preserve">
<value>Possible unintended use of method 'Equals' for arguments '{left}' and '{right}' of different types in a query. This comparison will always return false.</value>
<comment>Warning RelationalEventId.QueryPossibleUnintendedUseOfEqualsWarning string string</comment>
Expand Down Expand Up @@ -675,6 +679,9 @@
<data name="NullTypeMappingInSqlTree" xml:space="preserve">
<value>Expression '{sqlExpression}' in the SQL tree does not have a type mapping assigned.</value>
</data>
<data name="OptionalDependentWithDependentWithoutIdentifyingProperty" xml:space="preserve">
<value>Entity type '{entityType}' is an optional dependent containing other dependents in table sharing without any required non shared property to identify if the entity type exist. If all nullable properties contain null value in database then an object instance won't be materialized in the query causing nested dependent's values to be lost.</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<value>Entity type '{entityType}' is an optional dependent containing other dependents in table sharing without any required non shared property to identify if the entity type exist. If all nullable properties contain null value in database then an object instance won't be materialized in the query causing nested dependent's values to be lost.</value>
<value>Entity type '{entityType}' is an optional dependent using table sharing and containing other dependents without any required non shared property to identify whether the entity exists. If all nullable properties contain a null value in database then an object instance won't be created in the query causing nested dependent's values to be lost. Add a required property to create instances with null values for other properties or mark the incoming navigation as required to always create an instance. </value>

We might also need to add a fwlink

</data>
<data name="ParameterNotObjectArray" xml:space="preserve">
<value>Cannot use the value provided for parameter '{parameter}' because it isn't assignable to type object[].</value>
</data>
Expand Down
36 changes: 36 additions & 0 deletions src/EFCore/Diagnostics/EntityTypeEventData.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

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
/// an entity type.
/// </summary>
public class EntityTypeEventData : EventData
{
/// <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="entityType"> The entityType. </param>
public EntityTypeEventData(
EventDefinitionBase eventDefinition,
Func<EventDefinitionBase, EventData, string> messageGenerator,
IReadOnlyEntityType entityType)
: base(eventDefinition, messageGenerator)
{
EntityType = entityType;
}

/// <summary>
/// The entity type.
/// </summary>
public virtual IReadOnlyEntityType EntityType { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
eb.OwnsOne(
p => p.BranchAddress, ab =>
{
ab.IndexerProperty<string>("BranchName");
ab.IndexerProperty<string>("BranchName").IsRequired();
ab.HasData(
new
{
Expand Down Expand Up @@ -760,7 +760,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
eb.OwnsOne(
p => p.LeafBAddress, ab =>
{
ab.IndexerProperty<string>("LeafBType");
ab.IndexerProperty<string>("LeafBType").IsRequired();
ab.HasData(
new
{
Expand Down