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

[SMALL] Fix to #19801 - Query: add validation to warn about required navigations pointing to entity with query filter #21018

Merged
merged 1 commit into from
May 27, 2020
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
17 changes: 17 additions & 0 deletions src/EFCore/Diagnostics/CoreEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ private enum Id
RequiredAttributeOnCollection,
CollectionWithoutComparer,
ConflictingKeylessAndKeyAttributesWarning,
PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning,

// ChangeTracking events
DetectChangesStarting = CoreBaseId + 800,
Expand Down Expand Up @@ -646,6 +647,22 @@ public static readonly EventId InvalidIncludePathError
/// </summary>
public static readonly EventId ConflictingKeylessAndKeyAttributesWarning = MakeModelId(Id.ConflictingKeylessAndKeyAttributesWarning);

/// <summary>
/// <para>
/// Required navigation with principal entity having global query filter defined
/// and the declaring entity not having a matching filter
/// </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 PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning
= MakeModelValidationId(Id.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning);

private static readonly string _changeTrackingPrefix = DbLoggerCategory.ChangeTracking.Name + ".";
private static EventId MakeChangeTrackingId(Id id) => new EventId((int)id, _changeTrackingPrefix + id);

Expand Down
39 changes: 39 additions & 0 deletions src/EFCore/Diagnostics/CoreLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2996,5 +2996,44 @@ private static string ConflictingKeylessAndKeyAttributesWarning(EventDefinitionB
p.Property.Name,
p.Property.DeclaringEntityType.DisplayName());
}

/// <summary>
/// Logs for the <see cref="CoreEventId.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="foreignKey"> Foreign key which is used in the incorrectly setup navigation. </param>
public static void PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
[NotNull] IForeignKey foreignKey)
{
var definition = CoreResources.LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(
diagnostics,
foreignKey.PrincipalEntityType.DisplayName(),
foreignKey.DeclaringEntityType.DisplayName());
}

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

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

private static string PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string>)definition;
var p = (ForeignKeyEventData)payload;
return d.GenerateMessage(
p.ForeignKey.PrincipalEntityType.DisplayName(),
p.ForeignKey.DeclaringEntityType.DisplayName());
}
}
}
9 changes: 9 additions & 0 deletions src/EFCore/Diagnostics/LoggingDefinitions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -654,5 +654,14 @@ public abstract class LoggingDefinitions
/// </summary>
[EntityFrameworkInternal]
public EventDefinitionBase LogConflictingKeylessAndKeyAttributes;

/// <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 LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction;
}
}
12 changes: 12 additions & 0 deletions src/EFCore/Infrastructure/ModelValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,18 @@ protected virtual void ValidateQueryFilters(
throw new InvalidOperationException(
CoreStrings.BadFilterDerivedType(entityType.GetQueryFilter(), entityType.DisplayName()));
}

var requiredNavigationWithQueryFilter = entityType.GetNavigations()
.Where(n => !n.IsCollection
&& n.ForeignKey.IsRequired
&& n.IsOnDependent
&& n.ForeignKey.PrincipalEntityType.GetQueryFilter() != null
&& n.ForeignKey.DeclaringEntityType.GetQueryFilter() == null).FirstOrDefault();

if (requiredNavigationWithQueryFilter != null)
{
logger.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning(requiredNavigationWithQueryFilter.ForeignKey);
}
}
}

Expand Down
24 changes: 24 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 @@ -1375,6 +1375,10 @@
<value>Conflicting attributes have been applied: the 'Key' attribute on property '{property}' and the 'Keyless' attribute on its entity '{entity}'. Note that the entity will have no key unless you use fluent API to override this.</value>
<comment>Warning CoreEventId.ConflictingKeylessAndKeyAttributesWarning string string</comment>
</data>
<data name="LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction" xml:space="preserve">
<value>Entity '{principalEntityType}' has global query filter defined and is a required end of a relationship with the entity '{declaringEntityType}'. This may lead to unexpected results when the required entity is filtered out. Either use optional navigation or define matching query filters for both entities in the navigation. See https://go.microsoft.com/fwlink/?linkid=2131316 for more information.</value>
<comment>Warning CoreEventId.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning string string</comment>
</data>
<data name="InvalidSwitch" xml:space="preserve">
<value>Invalid {name}: {value}</value>
</data>
Expand Down
40 changes: 40 additions & 0 deletions test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1300,6 +1300,46 @@ public virtual void Detects_duplicate_discriminator_values()
VerifyError(CoreStrings.DuplicateDiscriminatorValue(typeof(C).Name, 1, typeof(A).Name), model);
}

[ConditionalFact]
public virtual void Required_navigation_with_query_filter_on_one_side_issues_a_warning()
{
var modelBuilder = CreateConventionalModelBuilder();
modelBuilder.Entity<Customer>().HasMany(x => x.Orders).WithOne(x => x.Customer).IsRequired();
modelBuilder.Entity<Customer>().HasQueryFilter(x => x.Id > 5);

var message = CoreResources.LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction(
CreateValidationLogger()).GenerateMessage(nameof(Customer), nameof(Order));

VerifyWarning(message, modelBuilder.Model);
}

[ConditionalFact]
public virtual void Optional_navigation_with_query_filter_on_one_side_doesnt_issue_a_warning()
{
var modelBuilder = CreateConventionalModelBuilder();
modelBuilder.Entity<Customer>().HasMany(x => x.Orders).WithOne(x => x.Customer).IsRequired(false);
modelBuilder.Entity<Customer>().HasQueryFilter(x => x.Id > 5);

var message = CoreResources.LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction(
CreateValidationLogger()).GenerateMessage(nameof(Customer), nameof(Order));

VerifyLogDoesNotContain(message, modelBuilder.Model);
}

[ConditionalFact]
public virtual void Required_navigation_with_query_filter_on_both_sides_doesnt_issue_a_warning()
{
var modelBuilder = CreateConventionalModelBuilder();
modelBuilder.Entity<Customer>().HasMany(x => x.Orders).WithOne(x => x.Customer).IsRequired();
modelBuilder.Entity<Customer>().HasQueryFilter(x => x.Id > 5);
modelBuilder.Entity<Order>().HasQueryFilter(x => x.Customer.Id > 5);

var message = CoreResources.LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction(
CreateValidationLogger()).GenerateMessage(nameof(Customer), nameof(Order));

VerifyLogDoesNotContain(message, modelBuilder.Model);
}

// INotify interfaces not really implemented; just marking the classes to test metadata construction
private class FullNotificationEntity : INotifyPropertyChanging, INotifyPropertyChanged
{
Expand Down