From 5ea1743675749d924c7dfbf28d106cccb7c93178 Mon Sep 17 00:00:00 2001 From: maumar Date: Thu, 21 May 2020 18:43:30 -0700 Subject: [PATCH] Fix to #19801 - Query: add validation to warn about required navigations pointing to entity with query filter Warning is issued for required navigations for which required side has query filter and the optional side doesn't. When both sides define query filters we assume they are correct, i.e. we don't peek inside the filters to make sure they are consistent. Fixes #19801 --- src/EFCore/Diagnostics/CoreEventId.cs | 17 ++++++++ .../Diagnostics/CoreLoggerExtensions.cs | 39 ++++++++++++++++++ src/EFCore/Diagnostics/LoggingDefinitions.cs | 9 +++++ src/EFCore/Infrastructure/ModelValidator.cs | 12 ++++++ src/EFCore/Properties/CoreStrings.Designer.cs | 24 +++++++++++ src/EFCore/Properties/CoreStrings.resx | 4 ++ .../Infrastructure/ModelValidatorTest.cs | 40 +++++++++++++++++++ 7 files changed, 145 insertions(+) diff --git a/src/EFCore/Diagnostics/CoreEventId.cs b/src/EFCore/Diagnostics/CoreEventId.cs index 09c7fa839f8..f6cf266b3dd 100644 --- a/src/EFCore/Diagnostics/CoreEventId.cs +++ b/src/EFCore/Diagnostics/CoreEventId.cs @@ -104,6 +104,7 @@ private enum Id RequiredAttributeOnCollection, CollectionWithoutComparer, ConflictingKeylessAndKeyAttributesWarning, + PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning, // ChangeTracking events DetectChangesStarting = CoreBaseId + 800, @@ -646,6 +647,22 @@ public static readonly EventId InvalidIncludePathError /// public static readonly EventId ConflictingKeylessAndKeyAttributesWarning = MakeModelId(Id.ConflictingKeylessAndKeyAttributesWarning); + /// + /// + /// Required navigation with principal entity having global query filter defined + /// and the principal entity not having a matching filter + /// + /// + /// This event is in the category. + /// + /// + /// This event uses the payload when used with a + /// . + /// + /// + 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); diff --git a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs index 41ba14f9b02..828635fc1ce 100644 --- a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs +++ b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs @@ -2996,5 +2996,44 @@ private static string ConflictingKeylessAndKeyAttributesWarning(EventDefinitionB p.Property.Name, p.Property.DeclaringEntityType.DisplayName()); } + + /// + /// Logs for the event. + /// + /// The diagnostics logger to use. + /// Foreign key which is used in the incorrectly setup navigation. + public static void PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning( + [NotNull] this IDiagnosticsLogger 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)definition; + var p = (ForeignKeyEventData)payload; + return d.GenerateMessage( + p.ForeignKey.PrincipalEntityType.DisplayName(), + p.ForeignKey.DeclaringEntityType.DisplayName()); + } } } diff --git a/src/EFCore/Diagnostics/LoggingDefinitions.cs b/src/EFCore/Diagnostics/LoggingDefinitions.cs index 82a3f4de886..149a7606de6 100644 --- a/src/EFCore/Diagnostics/LoggingDefinitions.cs +++ b/src/EFCore/Diagnostics/LoggingDefinitions.cs @@ -654,5 +654,14 @@ public abstract class LoggingDefinitions /// [EntityFrameworkInternal] public EventDefinitionBase LogConflictingKeylessAndKeyAttributes; + + /// + /// 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. + /// + [EntityFrameworkInternal] + public EventDefinitionBase LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction; } } diff --git a/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs index 1102505715b..e69c8946f76 100644 --- a/src/EFCore/Infrastructure/ModelValidator.cs +++ b/src/EFCore/Infrastructure/ModelValidator.cs @@ -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); + } } } diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index a2e9c58c9b8..ddd7f679513 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -4243,5 +4243,29 @@ public static EventDefinition LogConflictingKeylessAndKeyAttribu return (EventDefinition)definition; } + + /// + /// Entitiy '{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. + /// + public static EventDefinition LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction([NotNull] IDiagnosticsLogger logger) + { + var definition = ((LoggingDefinitions)logger.Definitions).LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction; + if (definition == null) + { + definition = LazyInitializer.EnsureInitialized( + ref ((LoggingDefinitions)logger.Definitions).LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction, + () => new EventDefinition( + logger.Options, + CoreEventId.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning, + LogLevel.Warning, + "CoreEventId.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning", + level => LoggerMessage.Define( + level, + CoreEventId.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning, + _resourceManager.GetString("LogPossibleIncorrectRequiredNavigationWithQueryFilterInteraction")))); + } + + return (EventDefinition)definition; + } } } diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 0072be98d56..80b3f60500a 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -1375,6 +1375,10 @@ 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. Warning CoreEventId.ConflictingKeylessAndKeyAttributesWarning string string + + Entitiy '{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. + Warning CoreEventId.PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning string string + Invalid {name}: {value} diff --git a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs index 8c52c04a70d..7f205736d37 100644 --- a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs +++ b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs @@ -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().HasMany(x => x.Orders).WithOne(x => x.Customer).IsRequired(); + modelBuilder.Entity().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().HasMany(x => x.Orders).WithOne(x => x.Customer).IsRequired(false); + modelBuilder.Entity().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().HasMany(x => x.Orders).WithOne(x => x.Customer).IsRequired(); + modelBuilder.Entity().HasQueryFilter(x => x.Id > 5); + modelBuilder.Entity().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 {