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

Optional dependent null #24856

Merged
merged 29 commits into from
May 31, 2021
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
fdb07fc
Perf: unset EnableContentResponseOnWrite #22999
umitkavala Nov 13, 2020
a772ec2
Revert "Perf: unset EnableContentResponseOnWrite #22999"
umitkavala Nov 13, 2020
18b340b
Merge remote-tracking branch 'upstream/main' into main
umitkavala Nov 16, 2020
cce3889
Merge remote-tracking branch 'upstream/main' into main
umitkavala Dec 1, 2020
b8ea6fd
Merge remote-tracking branch 'upstream/main' into main
umitkavala Dec 2, 2020
bd2f03d
Merge remote-tracking branch 'upstream/main' into main
umitkavala Apr 3, 2021
ffdfd9d
Merge remote-tracking branch 'upstream/main' into main
kavalau Apr 9, 2021
0e13a04
Merge remote-tracking branch 'upstream/main' into optional-dependent-…
umitkavala May 6, 2021
495e910
Added test to reproduce issue.
umitkavala May 8, 2021
6e613f9
Log optional dependent
umitkavala May 11, 2021
8dfc5e8
Complete method & Add new test
umitkavala May 15, 2021
7ae83fe
refactor to fix existing test case fails
umitkavala May 16, 2021
0248223
Update SharedTableEntryMap.cs
umitkavala May 16, 2021
0156282
Update SharedTableEntryMap.cs
umitkavala May 16, 2021
de2ce07
Merge remote-tracking branch 'upstream/main' into optional-dependent-…
umitkavala May 24, 2021
d4c962f
Move check to ModificationCommand.GenerateColumnModifications
umitkavala May 24, 2021
b90d566
Fix failed test cases.
umitkavala May 24, 2021
cb29589
Update ModificationCommand.cs
umitkavala May 25, 2021
f46926f
log warning instead throw exception
umitkavala May 25, 2021
ca9ff1f
Update ModificationCommand.cs
umitkavala May 25, 2021
b882981
Revert "Update ModificationCommand.cs"
umitkavala May 25, 2021
978befd
Revert "log warning instead throw exception"
umitkavala May 25, 2021
fdda7a7
pass logger to ModificationCommand constructor
umitkavala May 25, 2021
3ad496a
pass new parameter in tests
umitkavala May 25, 2021
de6618e
add sensitive data to event data. typo on warning message
umitkavala May 26, 2021
82046e5
create new eventId & move models to under base test classs
kavalau May 26, 2021
f2579fe
update comments
umitkavala May 27, 2021
c3b64f7
Update RelationalEventId.cs
umitkavala May 28, 2021
5728d14
test fix
umitkavala May 30, 2021
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
17 changes: 16 additions & 1 deletion src/EFCore.Relational/Diagnostics/RelationalEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ private enum Id
IndexPropertiesMappedToNonOverlappingTables,
ForeignKeyPropertiesMappedToUnrelatedTables,
OptionalDependentWithoutIdentifyingPropertyWarning,
OptionalDependentWithAllNullPropertiesWarning,
umitkavala marked this conversation as resolved.
Show resolved Hide resolved

// Update events
BatchReadyForExecution = CoreEventId.RelationalBaseId + 700,
Expand Down Expand Up @@ -726,7 +727,7 @@ private static EventId MakeValidationId(Id id)

/// <summary>
/// <para>
/// A foreign key specifies properties which don't map to the related tables.
/// The entity does not have any property with a non-default value to identify whether the entity exists.
/// </para>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Model.Validation" /> category.
Expand All @@ -738,6 +739,20 @@ private static EventId MakeValidationId(Id id)
public static readonly EventId OptionalDependentWithoutIdentifyingPropertyWarning
= MakeValidationId(Id.OptionalDependentWithoutIdentifyingPropertyWarning);

/// <summary>
/// <para>
/// The entity does not have any property with a non-default value to identify whether the entity exists.
/// </para>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Update" /> category.
/// </para>
/// <para>
/// This event uses the <see cref="UpdateEntryEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
public static readonly EventId OptionalDependentWithAllNullPropertiesWarning
umitkavala marked this conversation as resolved.
Show resolved Hide resolved
= MakeValidationId(Id.OptionalDependentWithAllNullPropertiesWarning);
umitkavala marked this conversation as resolved.
Show resolved Hide resolved

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

private static EventId MakeUpdateId(Id id)
Expand Down
69 changes: 69 additions & 0 deletions src/EFCore.Relational/Diagnostics/RelationalLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2964,5 +2964,74 @@ public static void BatchExecutorFailedToReleaseSavepoint(
diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
}
}

/// <summary>
/// Logs the <see cref="RelationalEventId.OptionalDependentWithAllNullPropertiesWarning" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="entry"> The entry. </param>
public static void OptionalDependentWithAllNullPropertiesWarning(
this IDiagnosticsLogger<DbLoggerCategory.Update> diagnostics,
IUpdateEntry entry)
{
var definition = RelationalResources.LogOptionalDependentWithAllNullProperties(diagnostics);

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

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new UpdateEntryEventData(
definition,
OptionalDependentWithAllNullPropertiesWarning,
entry);

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

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

/// <summary>
/// Logs the <see cref="RelationalEventId.OptionalDependentWithAllNullPropertiesWarning" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="entry"> The entry. </param>
public static void OptionalDependentWithAllNullPropertiesWarningSensitive(
this IDiagnosticsLogger<DbLoggerCategory.Update> diagnostics,
IUpdateEntry entry)
{
var definition = RelationalResources.LogOptionalDependentWithAllNullPropertiesSensitive(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics, entry.EntityType.DisplayName(), entry.BuildCurrentValuesString(entry.EntityType.FindPrimaryKey()!.Properties));
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new UpdateEntryEventData(
definition,
OptionalDependentWithAllNullPropertiesWarningSensitive,
entry
);

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

private static string OptionalDependentWithAllNullPropertiesWarningSensitive(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string>)definition;
var p = (UpdateEntryEventData)payload;
return d.GenerateMessage(p.EntityEntry.EntityType.DisplayName(), p.EntityEntry.BuildCurrentValuesString(p.EntityEntry.EntityType.FindPrimaryKey()!.Properties));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -512,5 +512,23 @@ public abstract class RelationalLoggingDefinitions : LoggingDefinitions
/// </summary>
[EntityFrameworkInternal]
public EventDefinitionBase? LogOptionalDependentWithoutIdentifyingProperty;

/// <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? LogOptionalDependentWithAllNullProperties;

/// <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? LogOptionalDependentWithAllNullPropertiesSensitive;
}
}
50 changes: 50 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.

8 changes: 8 additions & 0 deletions src/EFCore.Relational/Properties/RelationalStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,14 @@
<value>Opening connection to database '{database}' on server '{server}'.</value>
<comment>Debug RelationalEventId.ConnectionOpening string string</comment>
</data>
<data name="LogOptionalDependentWithAllNullProperties" xml:space="preserve">
<value>The entity of type '{entityType}' is an optional dependent using table sharing. The entity does not have any property with a non-default value to identify whether the entity exists. This means that when it is queried no object instance will be created instead of an instance with all properties set to default values. Any nested dependents will also be lost. Either don't save any instance with only default values or mark the incoming navigation as required in the model. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the key values of the entity.</value>
<comment>Warning RelationalEventId.OptionalDependentWithAllNullPropertiesWarning string</comment>
</data>
<data name="LogOptionalDependentWithAllNullPropertiesSensitive" xml:space="preserve">
<value>The entity of type '{entityType}' with primary key values {keyValues} is an optional dependent using table sharing. The entity does not have any property with a non-default value to identify whether the entity exists. This means that when it is queried no object instance will be created instead of an instance with all properties set to default values. Any nested dependents will also be lost. Either don't save any instance with only default values or mark the incoming navigation as required in the model.</value>
<comment>Warning RelationalEventId.OptionalDependentWithAllNullPropertiesWarning string string</comment>
</data>
<data name="LogOptionalDependentWithoutIdentifyingProperty" xml:space="preserve">
<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>Warning RelationalEventId.OptionalDependentWithoutIdentifyingPropertyWarning string</comment>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,13 @@ protected virtual IEnumerable<ModificationCommand> CreateModificationCommands(

command = sharedCommandsMap.GetOrAddValue(
entry,
(n, s, c) => new ModificationCommand(n, s, generateParameterName, _sensitiveLoggingEnabled, c));
(n, s, c) => new ModificationCommand(n, s, generateParameterName, _sensitiveLoggingEnabled, c, Dependencies.UpdateLogger));
isMainEntry = sharedCommandsMap.IsMainEntry(entry);
}
else
{
command = new ModificationCommand(
table.Name, table.Schema, generateParameterName, _sensitiveLoggingEnabled, comparer: null);
table.Name, table.Schema, generateParameterName, _sensitiveLoggingEnabled, comparer: null, Dependencies.UpdateLogger);
}

command.AddEntry(entry, isMainEntry);
Expand Down
32 changes: 31 additions & 1 deletion src/EFCore.Relational/Update/ModificationCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics;
using System.Linq;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Diagnostics.Internal;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
Expand All @@ -31,6 +32,7 @@ public class ModificationCommand
private IReadOnlyList<ColumnModification>? _columnModifications;
private bool _requiresResultPropagation;
private bool _mainEntryAdded;
private readonly IDiagnosticsLogger<DbLoggerCategory.Update>? _logger;

/// <summary>
/// Initializes a new <see cref="ModificationCommand" /> instance.
Expand All @@ -40,12 +42,14 @@ public class ModificationCommand
/// <param name="generateParameterName"> A delegate to generate parameter names. </param>
/// <param name="sensitiveLoggingEnabled"> Indicates whether or not potentially sensitive data (e.g. database values) can be logged. </param>
/// <param name="comparer"> A <see cref="IComparer{T}" /> for <see cref="IUpdateEntry" />s. </param>
/// <param name="logger">A <see cref="IDiagnosticsLogger{T}" /> for <see cref="DbLoggerCategory.Update" />s.</param>
public ModificationCommand(
string name,
string? schema,
Func<string> generateParameterName,
bool sensitiveLoggingEnabled,
IComparer<IUpdateEntry>? comparer)
IComparer<IUpdateEntry>? comparer,
IDiagnosticsLogger<DbLoggerCategory.Update>? logger)
: this(
Check.NotEmpty(name, nameof(name)),
schema,
Expand All @@ -56,6 +60,7 @@ public ModificationCommand(

_generateParameterName = generateParameterName;
_comparer = comparer;
_logger = logger;
}

/// <summary>
Expand Down Expand Up @@ -306,6 +311,12 @@ private IReadOnlyList<ColumnModification> GenerateColumnModifications()
continue;
}

var optionalDependentWithAllNull =
(entry.EntityState == EntityState.Deleted
|| entry.EntityState == EntityState.Added)
&& tableMapping.Table.IsOptional(entry.EntityType)
&& tableMapping.Table.GetRowInternalForeignKeys(entry.EntityType).Any();

foreach (var columnMapping in tableMapping.ColumnMappings)
{
var property = columnMapping.Property;
Expand Down Expand Up @@ -367,6 +378,25 @@ private IReadOnlyList<ColumnModification> GenerateColumnModifications()
}

columnModifications.Add(columnModification);

if (optionalDependentWithAllNull
&& columnModification.IsWrite
&& (entry.EntityState != EntityState.Added || columnModification.Value is not null))
{
optionalDependentWithAllNull = false;
}
}
}

if (optionalDependentWithAllNull && _logger != null)
{
if (_sensitiveLoggingEnabled)
{
_logger.OptionalDependentWithAllNullPropertiesWarningSensitive(entry);
}
else
{
_logger.OptionalDependentWithAllNullPropertiesWarning(entry);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/EFCore/Diagnostics/EntityTypeEventData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Diagnostics;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Update;

namespace Microsoft.EntityFrameworkCore.Diagnostics
{
Expand Down
36 changes: 36 additions & 0 deletions src/EFCore/Diagnostics/UpdateEntryEventData.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.Update;

namespace Microsoft.EntityFrameworkCore.Diagnostics
{
/// <summary>
/// A <see cref="DiagnosticSource" /> event payload class for events that have
/// an entity update entry.
/// </summary>
public class UpdateEntryEventData : 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="entityEntry"> The entry for the entity instance on which the property value has changed. </param>
public UpdateEntryEventData(
EventDefinitionBase eventDefinition,
Func<EventDefinitionBase, EventData, string> messageGenerator,
IUpdateEntry entityEntry)
: base(eventDefinition, messageGenerator)
{
EntityEntry = entityEntry;
}

/// <summary>
/// The entry for the entity instance on which the property value has changed.
/// </summary>
public virtual IUpdateEntry EntityEntry { get; }
}
}
Loading