diff --git a/src/EFCore/Diagnostics/CoreEventId.cs b/src/EFCore/Diagnostics/CoreEventId.cs index 1a9e8207b3d..65ebd987b33 100644 --- a/src/EFCore/Diagnostics/CoreEventId.cs +++ b/src/EFCore/Diagnostics/CoreEventId.cs @@ -84,7 +84,9 @@ private enum Id RedundantIndexRemoved, IncompatibleMatchingForeignKeyProperties, RequiredAttributeOnDependent, + NonNullableOnDependent, RequiredAttributeOnBothNavigations, + NonNullableReferenceOnBothNavigations, ConflictingShadowForeignKeysWarning, MultiplePrimaryKeyCandidates, MultipleNavigationProperties, @@ -443,6 +445,20 @@ public static readonly EventId PossibleUnintendedReferenceComparisonWarning /// public static readonly EventId RequiredAttributeOnDependent = MakeModelId(Id.RequiredAttributeOnDependent); + /// + /// + /// The entity type with the navigation property that has non-nullability + /// was configured as the dependent side in the relationship. + /// + /// + /// This event is in the category. + /// + /// + /// This event uses the payload when used with a . + /// + /// + public static readonly EventId NonNullableOnDependent = MakeModelId(Id.NonNullableOnDependent); + /// /// /// Navigations separated into two relationships as was specified on both navigations. @@ -456,6 +472,19 @@ public static readonly EventId PossibleUnintendedReferenceComparisonWarning /// public static readonly EventId RequiredAttributeOnBothNavigations = MakeModelId(Id.RequiredAttributeOnBothNavigations); + /// + /// + /// Navigations separated into two relationships as non-nullability was specified on both navigations. + /// + /// + /// This event is in the category. + /// + /// + /// This event uses the payload when used with a . + /// + /// + public static readonly EventId NonNullableReferenceOnBothNavigations = MakeModelId(Id.NonNullableReferenceOnBothNavigations); + /// /// /// The properties that best match the foreign key convention are already used by a different foreign key. diff --git a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs index d95ebf7a9db..9960b495986 100644 --- a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs +++ b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs @@ -1153,6 +1153,45 @@ private static string RequiredAttributeOnDependent(EventDefinitionBase definitio return d.GenerateMessage(p.Navigation.Name, p.Navigation.DeclaringEntityType.DisplayName()); } + /// + /// 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. + /// + public static void NonNullableOnDependent( + [NotNull] this IDiagnosticsLogger diagnostics, + [NotNull] INavigation navigation) + { + var definition = CoreResources.LogNonNullableOnDependent(diagnostics); + + var warningBehavior = definition.GetLogBehavior(diagnostics); + if (warningBehavior != WarningBehavior.Ignore) + { + definition.Log( + diagnostics, + warningBehavior, + navigation.Name, navigation.DeclaringEntityType.DisplayName()); + } + + if (diagnostics.DiagnosticSource.IsEnabled(definition.EventId.Name)) + { + diagnostics.DiagnosticSource.Write( + definition.EventId.Name, + new NavigationEventData( + definition, + NonNullableOnDependent, + navigation)); + } + } + + private static string NonNullableOnDependent(EventDefinitionBase definition, EventData payload) + { + var d = (EventDefinition)definition; + var p = (NavigationEventData)payload; + return d.GenerateMessage(p.Navigation.Name, p.Navigation.DeclaringEntityType.DisplayName()); + } + /// /// 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 @@ -1203,6 +1242,56 @@ private static string RequiredAttributeOnBothNavigations(EventDefinitionBase def secondNavigation.Name); } + /// + /// 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. + /// + public static void NonNullableReferenceOnBothNavigations( + [NotNull] this IDiagnosticsLogger diagnostics, + [NotNull] INavigation firstNavigation, + [NotNull] INavigation secondNavigation) + { + var definition = CoreResources.LogNonNullableReferenceOnBothNavigations(diagnostics); + + var warningBehavior = definition.GetLogBehavior(diagnostics); + if (warningBehavior != WarningBehavior.Ignore) + { + definition.Log( + diagnostics, + warningBehavior, + firstNavigation.DeclaringEntityType.DisplayName(), + firstNavigation.Name, + secondNavigation.DeclaringEntityType.DisplayName(), + secondNavigation.Name); + } + + if (diagnostics.DiagnosticSource.IsEnabled(definition.EventId.Name)) + { + diagnostics.DiagnosticSource.Write( + definition.EventId.Name, + new TwoPropertyBaseCollectionsEventData( + definition, + NonNullableReferenceOnBothNavigations, + new[] { firstNavigation }, + new[] { secondNavigation })); + } + } + + private static string NonNullableReferenceOnBothNavigations(EventDefinitionBase definition, EventData payload) + { + var d = (EventDefinition)definition; + var p = (TwoPropertyBaseCollectionsEventData)payload; + var firstNavigation = p.FirstPropertyCollection[0]; + var secondNavigation = p.SecondPropertyCollection[0]; + return d.GenerateMessage( + firstNavigation.DeclaringType.DisplayName(), + firstNavigation.Name, + secondNavigation.DeclaringType.DisplayName(), + secondNavigation.Name); + } + /// /// 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 diff --git a/src/EFCore/Diagnostics/LoggingDefinitions.cs b/src/EFCore/Diagnostics/LoggingDefinitions.cs index 66c1bf4ef8f..21f8ecda5f2 100644 --- a/src/EFCore/Diagnostics/LoggingDefinitions.cs +++ b/src/EFCore/Diagnostics/LoggingDefinitions.cs @@ -457,6 +457,15 @@ public abstract class LoggingDefinitions [EntityFrameworkInternal] public EventDefinitionBase LogRequiredAttributeOnDependent; + /// + /// 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 LogNonNullableOnDependent; + /// /// 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 @@ -466,6 +475,15 @@ public abstract class LoggingDefinitions [EntityFrameworkInternal] public EventDefinitionBase LogRequiredAttributeOnBothNavigations; + /// + /// 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 LogNonNullableReferenceOnBothNavigations; + /// /// 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 diff --git a/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs b/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs index 39fd485f91c..53718b14318 100644 --- a/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs +++ b/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs @@ -113,6 +113,7 @@ var servicePropertyDiscoveryConvention var concurrencyCheckAttributeConvention = new ConcurrencyCheckAttributeConvention(logger); var databaseGeneratedAttributeConvention = new DatabaseGeneratedAttributeConvention(logger); var requiredPropertyAttributeConvention = new RequiredPropertyAttributeConvention(logger); + var nonNullableReferencePropertyConvention = new NonNullableReferencePropertyConvention(logger); var maxLengthAttributeConvention = new MaxLengthAttributeConvention(logger); var stringLengthAttributeConvention = new StringLengthAttributeConvention(logger); var timestampAttributeConvention = new TimestampAttributeConvention(logger); @@ -121,6 +122,7 @@ var servicePropertyDiscoveryConvention conventionSet.PropertyAddedConventions.Add(concurrencyCheckAttributeConvention); conventionSet.PropertyAddedConventions.Add(databaseGeneratedAttributeConvention); conventionSet.PropertyAddedConventions.Add(requiredPropertyAttributeConvention); + conventionSet.PropertyAddedConventions.Add(nonNullableReferencePropertyConvention); conventionSet.PropertyAddedConventions.Add(maxLengthAttributeConvention); conventionSet.PropertyAddedConventions.Add(stringLengthAttributeConvention); conventionSet.PropertyAddedConventions.Add(timestampAttributeConvention); @@ -190,6 +192,7 @@ var servicePropertyDiscoveryConvention conventionSet.NavigationAddedConventions.Add(backingFieldConvention); conventionSet.NavigationAddedConventions.Add(new RequiredNavigationAttributeConvention(logger)); + conventionSet.NavigationAddedConventions.Add(new NonNullableNavigationConvention(logger)); conventionSet.NavigationAddedConventions.Add(inversePropertyAttributeConvention); conventionSet.NavigationAddedConventions.Add(foreignKeyPropertyDiscoveryConvention); conventionSet.NavigationAddedConventions.Add(relationshipDiscoveryConvention); @@ -212,6 +215,7 @@ var servicePropertyDiscoveryConvention conventionSet.PropertyFieldChangedConventions.Add(concurrencyCheckAttributeConvention); conventionSet.PropertyFieldChangedConventions.Add(databaseGeneratedAttributeConvention); conventionSet.PropertyFieldChangedConventions.Add(requiredPropertyAttributeConvention); + conventionSet.PropertyFieldChangedConventions.Add(nonNullableReferencePropertyConvention); conventionSet.PropertyFieldChangedConventions.Add(maxLengthAttributeConvention); conventionSet.PropertyFieldChangedConventions.Add(stringLengthAttributeConvention); conventionSet.PropertyFieldChangedConventions.Add(timestampAttributeConvention); diff --git a/src/EFCore/Metadata/Conventions/Internal/NonNullableConvention.cs b/src/EFCore/Metadata/Conventions/Internal/NonNullableConvention.cs new file mode 100644 index 00000000000..5f788b22247 --- /dev/null +++ b/src/EFCore/Metadata/Conventions/Internal/NonNullableConvention.cs @@ -0,0 +1,55 @@ +// 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.Linq; +using System.Reflection; + +namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal +{ + /// + /// 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. + /// + public abstract class NonNullableConvention + { + private const string NullableAttributeFullName = "System.Runtime.CompilerServices.NullableAttribute"; + private Type _nullableAttrType; + private FieldInfo _nullableFlagsFieldInfo; + + /// + /// 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. + /// + protected virtual bool IsNonNullable(MemberInfo memberInfo) + { + // For C# 8.0 nullable types, the C# currently synthesizes a NullableAttribute that expresses nullability into assemblies + // it produces. If the model is spread across more than one assembly, there will be multiple versions of this attribute, + // so look for it by name, caching to avoid reflection on every check. + // Note that this may change - if https://github.com/dotnet/corefx/issues/36222 is done we can remove all of this. + + if (Attribute.GetCustomAttributes(memberInfo, true)?.FirstOrDefault(a => a.GetType().FullName == NullableAttributeFullName) is Attribute attribute) + { + if (attribute.GetType() != _nullableAttrType) + { + _nullableFlagsFieldInfo = attribute.GetType().GetField("NullableFlags"); + _nullableAttrType = attribute.GetType(); + } + + // For the interpretation of NullableFlags, see + // https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-reference-types.md#annotations + if (_nullableFlagsFieldInfo?.GetValue(attribute) is byte[] flags + && flags.FirstOrDefault() == 1) + { + return true; + } + } + + return false; + } + } +} diff --git a/src/EFCore/Metadata/Conventions/Internal/NonNullableNavigationConvention.cs b/src/EFCore/Metadata/Conventions/Internal/NonNullableNavigationConvention.cs new file mode 100644 index 00000000000..5ecb0d0dd39 --- /dev/null +++ b/src/EFCore/Metadata/Conventions/Internal/NonNullableNavigationConvention.cs @@ -0,0 +1,102 @@ +// 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.ComponentModel.DataAnnotations; +using System.Linq; +using System.Reflection; +using JetBrains.Annotations; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Internal; +using Microsoft.EntityFrameworkCore.Metadata.Internal; +using Microsoft.EntityFrameworkCore.Utilities; +using Microsoft.Extensions.Logging; + +namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal +{ + /// + /// 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. + /// + public class NonNullableNavigationConvention : NonNullableConvention, INavigationAddedConvention + { + /// + /// 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. + /// + public NonNullableNavigationConvention([NotNull] IDiagnosticsLogger logger) + { + Logger = logger; + } + + /// + /// 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. + /// + protected virtual IDiagnosticsLogger Logger { get; } + + /// + /// 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. + /// + public virtual InternalRelationshipBuilder Apply( + InternalRelationshipBuilder relationshipBuilder, + Navigation navigation) + { + Check.NotNull(relationshipBuilder, nameof(relationshipBuilder)); + Check.NotNull(navigation, nameof(navigation)); + + if (!IsNonNullable(navigation) || navigation.IsCollection()) + { + return relationshipBuilder; + } + + if (!navigation.IsDependentToPrincipal()) + { + var inverse = navigation.FindInverse(); + if (inverse != null) + { + if (IsNonNullable(inverse)) + { + Logger.NonNullableReferenceOnBothNavigations(navigation, inverse); + return relationshipBuilder; + } + } + + if (!navigation.ForeignKey.IsUnique + || relationshipBuilder.Metadata.GetPrincipalEndConfigurationSource() != null) + { + return relationshipBuilder; + } + + var newRelationshipBuilder = relationshipBuilder.HasEntityTypes( + relationshipBuilder.Metadata.DeclaringEntityType, + relationshipBuilder.Metadata.PrincipalEntityType, + ConfigurationSource.Convention); + + if (newRelationshipBuilder == null) + { + return relationshipBuilder; + } + + Logger.NonNullableOnDependent(newRelationshipBuilder.Metadata.DependentToPrincipal); + relationshipBuilder = newRelationshipBuilder; + } + + return relationshipBuilder.IsRequired(true, ConfigurationSource.Convention) ?? relationshipBuilder; + } + + private bool IsNonNullable(Navigation navigation) + => navigation.DeclaringEntityType.HasClrType() + && navigation.DeclaringEntityType.GetRuntimeProperties().Find(navigation.Name) is PropertyInfo propertyInfo + && IsNonNullable(propertyInfo); + } +} diff --git a/src/EFCore/Metadata/Conventions/Internal/NonNullableReferencePropertyConvention.cs b/src/EFCore/Metadata/Conventions/Internal/NonNullableReferencePropertyConvention.cs new file mode 100644 index 00000000000..78516936322 --- /dev/null +++ b/src/EFCore/Metadata/Conventions/Internal/NonNullableReferencePropertyConvention.cs @@ -0,0 +1,72 @@ +// 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.Linq; +using System.Reflection; +using JetBrains.Annotations; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Metadata.Internal; + +namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal +{ + /// + /// 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. + /// + public class NonNullableReferencePropertyConvention : NonNullableConvention, + IPropertyAddedConvention, IPropertyFieldChangedConvention + { + /// + /// 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. + /// + public NonNullableReferencePropertyConvention([NotNull] IDiagnosticsLogger logger) + { + Logger = logger; + } + + /// + /// 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. + /// + protected virtual IDiagnosticsLogger Logger { get; } + + /// + /// 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. + /// + public virtual InternalPropertyBuilder Apply(InternalPropertyBuilder propertyBuilder) + { + // If the model is spread across multiple assemblies, it may contain different NullableAttribute types as + // the compiler synthesizes them for each assembly. + if (propertyBuilder.Metadata.GetIdentifyingMemberInfo() is MemberInfo memberInfo + && IsNonNullable(memberInfo)) + { + propertyBuilder.IsRequired(true, ConfigurationSource.Convention); + } + + return propertyBuilder; + } + + /// + /// 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. + /// + public virtual bool Apply(InternalPropertyBuilder propertyBuilder, FieldInfo oldFieldInfo) + { + Apply(propertyBuilder); + return true; + } + } +} diff --git a/src/EFCore/Metadata/Conventions/Internal/RequiredNavigationAttributeConvention.cs b/src/EFCore/Metadata/Conventions/Internal/RequiredNavigationAttributeConvention.cs index a210efcef32..93a84443f65 100644 --- a/src/EFCore/Metadata/Conventions/Internal/RequiredNavigationAttributeConvention.cs +++ b/src/EFCore/Metadata/Conventions/Internal/RequiredNavigationAttributeConvention.cs @@ -42,6 +42,11 @@ public override InternalRelationshipBuilder Apply( Check.NotNull(navigation, nameof(navigation)); Check.NotNull(attribute, nameof(attribute)); + if (navigation.IsCollection()) + { + return relationshipBuilder; + } + if (!navigation.IsDependentToPrincipal()) { var inverse = navigation.FindInverse(); diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index 6dff8b8bfc9..15af48bc1fe 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -3222,6 +3222,30 @@ public static EventDefinition LogRequiredAttributeOnDependent([N return (EventDefinition)definition; } + /// + /// The navigation property '{navigation}' is non-nullable, causing the entity type '{entityType}' to be configured as the dependent side in the corresponding relationship. + /// + public static EventDefinition LogNonNullableOnDependent([NotNull] IDiagnosticsLogger logger) + { + var definition = ((LoggingDefinitions)logger.Definitions).LogNonNullableOnDependent; + if (definition == null) + { + definition = LazyInitializer.EnsureInitialized( + ref ((LoggingDefinitions)logger.Definitions).LogNonNullableOnDependent, + () => new EventDefinition( + logger.Options, + CoreEventId.NonNullableOnDependent, + LogLevel.Debug, + "CoreEventId.NonNullableOnDependent", + level => LoggerMessage.Define( + level, + CoreEventId.NonNullableOnDependent, + _resourceManager.GetString("LogNonNullableOnDependent")))); + } + + return (EventDefinition)definition; + } + /// /// The RequiredAttribute on '{principalEntityType}.{principalNavigation}' was ignored because there is also a RequiredAttribute on '{dependentEntityType}.{dependentNavigation}'. RequiredAttribute should only be specified on the dependent side of the relationship. /// @@ -3246,6 +3270,30 @@ public static EventDefinition LogRequiredAttribu return (EventDefinition)definition; } + /// + /// '{principalEntityType}.{principalNavigation}' may still be null at runtime despite being declared as non-nullable since only the navigation to principal '{dependentEntityType}.{dependentNavigation}' can be configured as required. + /// + public static EventDefinition LogNonNullableReferenceOnBothNavigations([NotNull] IDiagnosticsLogger logger) + { + var definition = ((LoggingDefinitions)logger.Definitions).LogNonNullableReferenceOnBothNavigations; + if (definition == null) + { + definition = LazyInitializer.EnsureInitialized( + ref ((LoggingDefinitions)logger.Definitions).LogNonNullableReferenceOnBothNavigations, + () => new EventDefinition( + logger.Options, + CoreEventId.NonNullableReferenceOnBothNavigations, + LogLevel.Debug, + "CoreEventId.NonNullableReferenceOnBothNavigations", + level => LoggerMessage.Define( + level, + CoreEventId.NonNullableReferenceOnBothNavigations, + _resourceManager.GetString("LogNonNullableReferenceOnBothNavigations")))); + } + + return (EventDefinition)definition; + } + /// /// Navigations '{dependentEntityType}.{dependentNavigation}' and '{principalEntityType}.{principalNavigation}' were separated into two relationships as ForeignKeyAttribute was specified on navigations on both sides. /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 6398dbba786..eddbc163fe8 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -995,10 +995,18 @@ The navigation property '{navigation}' has a RequiredAttribute causing the entity type '{entityType}' to be configured as the dependent side in the corresponding relationship. Debug CoreEventId.RequiredAttributeOnDependent string string + + The navigation property '{navigation}' is non-nullable, causing the entity type '{entityType}' to be configured as the dependent side in the corresponding relationship. + Debug CoreEventId.NonNullableOnDependent string string + The RequiredAttribute on '{principalEntityType}.{principalNavigation}' was ignored because there is also a RequiredAttribute on '{dependentEntityType}.{dependentNavigation}'. RequiredAttribute should only be specified on the dependent side of the relationship. Debug CoreEventId.RequiredAttributeOnBothNavigations string string string string + + '{principalEntityType}.{principalNavigation}' may still be null at runtime despite being declared as non-nullable since only the navigation to principal '{dependentEntityType}.{dependentNavigation}' can be configured as required. + Debug CoreEventId.NonNullableReferenceOnBothNavigations string string string string + Navigations '{dependentEntityType}.{dependentNavigation}' and '{principalEntityType}.{principalNavigation}' were separated into two relationships as ForeignKeyAttribute was specified on navigations on both sides. Warning CoreEventId.ForeignKeyAttributesOnBothNavigationsWarning string string string string diff --git a/test/EFCore.Tests/Metadata/Conventions/Internal/NavigationAttributeConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/Internal/NavigationAttributeConventionTest.cs index e3af306414d..2d57a077265 100644 --- a/test/EFCore.Tests/Metadata/Conventions/Internal/NavigationAttributeConventionTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/Internal/NavigationAttributeConventionTest.cs @@ -107,7 +107,7 @@ public void RequiredAttribute_overrides_configuration_from_convention_source() nameof(Blog.Posts), ConfigurationSource.Convention); - var navigation = dependentEntityTypeBuilder.Metadata.FindNavigation(nameof(BlogDetails.Blog)); + var navigation = dependentEntityTypeBuilder.Metadata.FindNavigation(nameof(Post.Blog)); relationshipBuilder.IsRequired(false, ConfigurationSource.Convention); @@ -152,19 +152,21 @@ public void RequiredAttribute_does_not_set_is_required_for_collection_navigation var principalEntityTypeBuilder = dependentEntityTypeBuilder.ModelBuilder.Entity(typeof(Principal), ConfigurationSource.Convention); - var relationshipBuilder = dependentEntityTypeBuilder.HasRelationship( - principalEntityTypeBuilder.Metadata, - nameof(Dependent.Principal), + var relationshipBuilder = principalEntityTypeBuilder.HasRelationship( + dependentEntityTypeBuilder.Metadata, nameof(Principal.Dependents), + nameof(Dependent.Principal), ConfigurationSource.Convention); - var navigation = dependentEntityTypeBuilder.Metadata.FindNavigation(nameof(Dependent.Principal)); + var navigation = principalEntityTypeBuilder.Metadata.FindNavigation(nameof(Principal.Dependents)); Assert.False(relationshipBuilder.Metadata.IsRequired); relationshipBuilder = CreateRequiredNavigationAttributeConvention().Apply(relationshipBuilder, navigation); Assert.False(relationshipBuilder.Metadata.IsRequired); + + Assert.Empty(ListLoggerFactory.Log); } [Fact] diff --git a/test/EFCore.Tests/Metadata/Conventions/Internal/NonNullableNavigationConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/Internal/NonNullableNavigationConventionTest.cs new file mode 100644 index 00000000000..23e8c544beb --- /dev/null +++ b/test/EFCore.Tests/Metadata/Conventions/Internal/NonNullableNavigationConventionTest.cs @@ -0,0 +1,303 @@ +// 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.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using System.ComponentModel.DataAnnotations.Schema; +using System.Diagnostics; +using System.Linq; +using System.Reflection; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Diagnostics.Internal; +using Microsoft.EntityFrameworkCore.InMemory.Storage.Internal; +using Microsoft.EntityFrameworkCore.Internal; +using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure; +using Microsoft.EntityFrameworkCore.Metadata.Internal; +using Microsoft.EntityFrameworkCore.Storage; +using Microsoft.EntityFrameworkCore.TestUtilities; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Xunit; + +namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal +{ + public class NonNullableNavigationConventionTest + { + [Fact] + public void Non_nullability_does_not_override_configuration_from_explicit_source() + { + var dependentEntityTypeBuilder = CreateInternalEntityTypeBuilder(); + var principalEntityTypeBuilder = dependentEntityTypeBuilder.ModelBuilder.Entity(typeof(Blog), ConfigurationSource.Convention); + + var relationshipBuilder = dependentEntityTypeBuilder.HasRelationship( + principalEntityTypeBuilder.Metadata, + nameof(Post.Blog), + nameof(Blog.Posts), + ConfigurationSource.Convention); + + var navigation = dependentEntityTypeBuilder.Metadata.FindNavigation(nameof(Post.Blog)); + + relationshipBuilder.IsRequired(false, ConfigurationSource.Explicit); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + + relationshipBuilder = CreateNotNullNavigationConvention().Apply(relationshipBuilder, navigation); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + Assert.Contains(principalEntityTypeBuilder.Metadata.GetNavigations(), nav => nav.Name == nameof(Blog.Posts)); + Assert.Contains(dependentEntityTypeBuilder.Metadata.GetNavigations(), nav => nav.Name == nameof(Post.Blog)); + } + + [Fact] + public void Non_nullability_does_not_override_configuration_from_data_annotation() + { + var dependentEntityTypeBuilder = CreateInternalEntityTypeBuilder(); + var principalEntityTypeBuilder = dependentEntityTypeBuilder.ModelBuilder.Entity(typeof(Blog), ConfigurationSource.Convention); + + var relationshipBuilder = dependentEntityTypeBuilder.HasRelationship( + principalEntityTypeBuilder.Metadata, + nameof(Post.Blog), + nameof(Blog.Posts), + ConfigurationSource.Convention); + + var navigation = dependentEntityTypeBuilder.Metadata.FindNavigation(nameof(Post.Blog)); + + relationshipBuilder.IsRequired(false, ConfigurationSource.DataAnnotation); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + + relationshipBuilder = CreateNotNullNavigationConvention().Apply(relationshipBuilder, navigation); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + Assert.Contains(principalEntityTypeBuilder.Metadata.GetNavigations(), nav => nav.Name == nameof(Blog.Posts)); + Assert.Contains(dependentEntityTypeBuilder.Metadata.GetNavigations(), nav => nav.Name == nameof(Post.Blog)); + } + + [Fact] + public void Non_nullability_does_not_set_is_required_for_collection_navigation() + { + var dependentEntityTypeBuilder = CreateInternalEntityTypeBuilder(); + var principalEntityTypeBuilder = + dependentEntityTypeBuilder.ModelBuilder.Entity(typeof(Principal), ConfigurationSource.Convention); + + var relationshipBuilder = principalEntityTypeBuilder.HasRelationship( + dependentEntityTypeBuilder.Metadata, + nameof(Principal.Dependents), + nameof(Dependent.Principal), + ConfigurationSource.Convention); + + var navigation = principalEntityTypeBuilder.Metadata.FindNavigation(nameof(Principal.Dependents)); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + + relationshipBuilder = CreateNotNullNavigationConvention().Apply(relationshipBuilder, navigation); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + + Assert.Empty(ListLoggerFactory.Log); + } + + [Fact] + public void Non_nullability_does_not_set_is_required_for_navigation_to_dependent() + { + var dependentEntityTypeBuilder = CreateInternalEntityTypeBuilder(); + var principalEntityTypeBuilder = + dependentEntityTypeBuilder.ModelBuilder.Entity(typeof(Principal), ConfigurationSource.Convention); + + var relationshipBuilder = dependentEntityTypeBuilder.HasRelationship( + principalEntityTypeBuilder.Metadata, + nameof(Dependent.Principal), + nameof(Principal.Dependent), + ConfigurationSource.Convention) + .HasEntityTypes + (principalEntityTypeBuilder.Metadata, dependentEntityTypeBuilder.Metadata, ConfigurationSource.Explicit); + + var navigation = principalEntityTypeBuilder.Metadata.FindNavigation(nameof(Principal.Dependent)); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + + relationshipBuilder = CreateNotNullNavigationConvention().Apply(relationshipBuilder, navigation); + + Assert.False(relationshipBuilder.Metadata.IsRequired); + } + + [Fact] + public void Non_nullability_inverts_when_navigation_to_dependent() + { + var dependentEntityTypeBuilder = CreateInternalEntityTypeBuilder(); + var principalEntityTypeBuilder = + dependentEntityTypeBuilder.ModelBuilder.Entity(typeof(Principal), ConfigurationSource.Convention); + + var relationshipBuilder = dependentEntityTypeBuilder.HasRelationship( + principalEntityTypeBuilder.Metadata, + nameof(Dependent.Principal), + nameof(Principal.Dependent), + ConfigurationSource.Convention); + + Assert.Equal(nameof(Dependent), relationshipBuilder.Metadata.DeclaringEntityType.DisplayName()); + Assert.False(relationshipBuilder.Metadata.IsRequired); + + var navigation = principalEntityTypeBuilder.Metadata.FindNavigation(nameof(Principal.Dependent)); + + relationshipBuilder = CreateNotNullNavigationConvention().Apply(relationshipBuilder, navigation); + + Assert.Equal(nameof(Principal), relationshipBuilder.Metadata.DeclaringEntityType.DisplayName()); + Assert.True(relationshipBuilder.Metadata.IsRequired); + + var logEntry = ListLoggerFactory.Log.Single(); + Assert.Equal(LogLevel.Debug, logEntry.Level); + Assert.Equal( + CoreResources.LogNonNullableOnDependent(new TestLogger()).GenerateMessage( + nameof(Principal.Dependent), nameof(Principal)), logEntry.Message); + } + + [Fact] + public void Non_nullability_sets_is_required_with_conventional_builder() + { + var modelBuilder = CreateModelBuilder(); + var model = (Model)modelBuilder.Model; + modelBuilder.Entity(); + + Assert.True( + model.FindEntityType(typeof(BlogDetails)).GetForeignKeys().Single(fk => fk.PrincipalEntityType?.ClrType == typeof(Blog)) + .IsRequired); + } + + [Fact] + public void Non_nullability_can_be_specified_on_both_navigations() + { + var modelBuilder = CreateModelBuilder(); + var model = (Model)modelBuilder.Model; + modelBuilder.Entity().HasOne(b => b.Blog).WithOne(b => b.BlogDetails); + + Assert.True( + model.FindEntityType(typeof(BlogDetails)).GetForeignKeys() + .Single(fk => fk.PrincipalEntityType?.ClrType == typeof(Blog)).IsRequired); + + var logEntry = ListLoggerFactory.Log.Single(); + Assert.Equal(LogLevel.Debug, logEntry.Level); + Assert.Equal( + CoreResources.LogNonNullableReferenceOnBothNavigations(new TestLogger()).GenerateMessage( + nameof(Blog), nameof(Blog.BlogDetails), nameof(BlogDetails), nameof(BlogDetails.Blog)), logEntry.Message); + } + + private NonNullableNavigationConvention CreateNotNullNavigationConvention() + => new NonNullableNavigationConvention(CreateLogger()); + + public ListLoggerFactory ListLoggerFactory { get; } + = new ListLoggerFactory(l => l == DbLoggerCategory.Model.Name); + + private InternalEntityTypeBuilder CreateInternalEntityTypeBuilder() + { + var conventionSet = new ConventionSet(); + conventionSet.EntityTypeAddedConventions.Add( + new PropertyDiscoveryConvention( + CreateTypeMapper(), + new TestLogger())); + + conventionSet.EntityTypeAddedConventions.Add(new KeyDiscoveryConvention(CreateLogger())); + + var modelBuilder = new InternalModelBuilder(new Model(conventionSet)); + + return modelBuilder.Entity(typeof(T), ConfigurationSource.Explicit); + } + + private static ITypeMappingSource CreateTypeMapper() + => TestServiceFactory.Instance.Create(); + + private ModelBuilder CreateModelBuilder() + { + var contextServices = InMemoryTestHelpers.Instance.CreateContextServices(); + var logger = CreateLogger(); + var dependencies = contextServices.GetRequiredService().With(logger); + + return new ModelBuilder( + new RuntimeConventionSetBuilder( + new ProviderConventionSetBuilder(dependencies), + Enumerable.Empty()) + .CreateConventionSet()); + } + + private DiagnosticsLogger CreateLogger() + { + ListLoggerFactory.Clear(); + var options = new LoggingOptions(); + options.Initialize(new DbContextOptionsBuilder().EnableSensitiveDataLogging(false).Options); + var modelLogger = new DiagnosticsLogger( + ListLoggerFactory, + options, + new DiagnosticListener("Fake"), + new TestLoggingDefinitions()); + return modelLogger; + } + +#nullable enable +#pragma warning disable CS8618 + + private class Blog + { + public int Id { get; set; } + + [NotMapped] + public BlogDetails BlogDetails { get; set; } + + public ICollection Posts { get; set; } + } + + private class BlogDetails + { + public int Id { get; set; } + + public Blog Blog { get; set; } + + private Post Post { get; set; } + } + + private class Post + { + public int Id { get; set; } + + public Blog Blog { get; set; } + } + + private class Principal + { + public static readonly PropertyInfo DependentIdProperty = typeof(Principal).GetProperty("DependentId"); + + public int Id { get; set; } + + public int DependentId { get; set; } + + [ForeignKey("PrincipalFk")] + public ICollection Dependents { get; set; } + + public Dependent Dependent { get; set; } + } + + private class Dependent + { + public static readonly PropertyInfo PrincipalIdProperty = typeof(Dependent).GetProperty("PrincipalId"); + + public int Id { get; set; } + + public int PrincipalId { get; set; } + + public int PrincipalFk { get; set; } + + [ForeignKey("AnotherPrincipal")] + public int PrincipalAnotherFk { get; set; } + + [ForeignKey("PrincipalFk")] + [InverseProperty("Dependent")] + public Principal? Principal { get; set; } + + public Principal? AnotherPrincipal { get; set; } + + [ForeignKey("PrincipalId, PrincipalFk")] + public Principal? CompositePrincipal { get; set; } + } +#pragma warning restore CS8618 +#nullable disable + } +} diff --git a/test/EFCore.Tests/Metadata/Conventions/Internal/NonNullableReferencePropertyConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/Internal/NonNullableReferencePropertyConventionTest.cs new file mode 100644 index 00000000000..551f555529d --- /dev/null +++ b/test/EFCore.Tests/Metadata/Conventions/Internal/NonNullableReferencePropertyConventionTest.cs @@ -0,0 +1,106 @@ +// 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.ComponentModel.DataAnnotations; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.InMemory.Storage.Internal; +using Microsoft.EntityFrameworkCore.Metadata.Internal; +using Microsoft.EntityFrameworkCore.Storage; +using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; + +namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal +{ + public class NonNullableReferencePropertyConventionTest + { + [Fact] + public void Non_nullability_does_not_override_configuration_from_explicit_source() + { + var entityTypeBuilder = CreateInternalEntityTypeBuilder(); + + var propertyBuilder = entityTypeBuilder.Property("Name", typeof(string), ConfigurationSource.Explicit); + + propertyBuilder.IsRequired(false, ConfigurationSource.Explicit); + + new NonNullableReferencePropertyConvention(new TestLogger()).Apply(propertyBuilder); + + Assert.True(propertyBuilder.Metadata.IsNullable); + } + + [Fact] + public void Non_nullability_does_not_override_configuration_from_data_annotation_source() + { + var entityTypeBuilder = CreateInternalEntityTypeBuilder(); + + var propertyBuilder = entityTypeBuilder.Property("Name", typeof(string), ConfigurationSource.Explicit); + + propertyBuilder.IsRequired(false, ConfigurationSource.DataAnnotation); + + new NonNullableReferencePropertyConvention(new TestLogger()).Apply(propertyBuilder); + + Assert.True(propertyBuilder.Metadata.IsNullable); + } + + [Fact] + public void Non_nullability_sets_is_nullable_with_conventional_builder() + { + var modelBuilder = CreateModelBuilder(); + var entityTypeBuilder = modelBuilder.Entity(); + + Assert.False(entityTypeBuilder.Property(e => e.Name).Metadata.IsNullable); + } + + [Theory] + [InlineData(nameof(A.NullAwareNonNullable), false)] + [InlineData(nameof(A.NullAwareNullable), true)] + [InlineData(nameof(A.NullObliviousNonNullable), true)] + [InlineData(nameof(A.NullObliviousNullable), true)] + [InlineData(nameof(A.RequiredAndNullable), false)] + public void Reference_nullability_sets_is_nullable_correctly(string propertyName, bool expectedNullable) + { + var modelBuilder = CreateModelBuilder(); + var entityTypeBuilder = modelBuilder.Entity(); + + Assert.Equal(expectedNullable, entityTypeBuilder.Property(propertyName).Metadata.IsNullable); + } + + private InternalEntityTypeBuilder CreateInternalEntityTypeBuilder() + { + var conventionSet = new ConventionSet(); + conventionSet.EntityTypeAddedConventions.Add( + new PropertyDiscoveryConvention( + CreateTypeMapper(), + new TestLogger())); + + var modelBuilder = new InternalModelBuilder(new Model(conventionSet)); + + return modelBuilder.Entity(typeof(T), ConfigurationSource.Explicit); + } + + private static ITypeMappingSource CreateTypeMapper() + => TestServiceFactory.Instance.Create(); + + private class A + { + public int Id { get; set; } + +#nullable enable + public string Name { get; set; } = ""; + + public string NullAwareNonNullable { get; set; } = ""; + public string? NullAwareNullable { get; set; } + + [Required] + public string? RequiredAndNullable { get; set; } +#nullable disable + +#pragma warning disable CS8632 // The annotation for nullable reference types should only be used in code within a '#nullable' context. + public string NullObliviousNonNullable { get; set; } + public string? NullObliviousNullable { get; set; } +#pragma warning restore CS8632 // The annotation for nullable reference types should only be used in code within a '#nullable' context. + } + + private static ModelBuilder CreateModelBuilder() => InMemoryTestHelpers.Instance.CreateConventionBuilder(); + } +}