Skip to content

Commit

Permalink
Allow RequiredAttribute and NRT to configure ToDependent navigation r…
Browse files Browse the repository at this point in the history 
…equiredness
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Aug 1, 2020
1 parent f53813d commit 946998a
Show file tree
Hide file tree
Showing 20 changed files with 370 additions and 238 deletions.
26 changes: 22 additions & 4 deletions src/EFCore/Diagnostics/CoreEventId.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ private enum Id
ShadowPropertyCreated = CoreBaseId + 600,
RedundantIndexRemoved,
IncompatibleMatchingForeignKeyProperties,
RequiredAttributeOnDependent,
RequiredAttributeOnBothNavigations,
RequiredAttributeOnDependent, // Unused
RequiredAttributeOnBothNavigations, // Unused
ConflictingShadowForeignKeysWarning,
MultiplePrimaryKeyCandidates,
MultipleNavigationProperties,
Expand All @@ -99,13 +99,14 @@ private enum Id
ConflictingForeignKeyAttributesOnNavigationAndPropertyWarning,
RedundantForeignKeyWarning,
NonNullableInverted, // Unused
NonNullableReferenceOnBothNavigations,
NonNullableReferenceOnDependent,
NonNullableReferenceOnBothNavigations, // Unused
NonNullableReferenceOnDependent, // Unused
RequiredAttributeInverted, // Unused
RequiredAttributeOnCollection,
CollectionWithoutComparer,
ConflictingKeylessAndKeyAttributesWarning,
PossibleIncorrectRequiredNavigationWithQueryFilterInteractionWarning,
RequiredAttributeOnSkipNavigation,

// ChangeTracking events
DetectChangesStarting = CoreBaseId + 800,
Expand Down Expand Up @@ -435,6 +436,7 @@ public static readonly EventId InvalidIncludePathError
/// This event uses the <see cref="TwoPropertyBaseCollectionsEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
[Obsolete]
public static readonly EventId RequiredAttributeOnBothNavigations = MakeModelId(Id.RequiredAttributeOnBothNavigations);

/// <summary>
Expand All @@ -448,6 +450,7 @@ public static readonly EventId InvalidIncludePathError
/// This event uses the <see cref="TwoPropertyBaseCollectionsEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
[Obsolete]
public static readonly EventId NonNullableReferenceOnBothNavigations = MakeModelId(Id.NonNullableReferenceOnBothNavigations);

/// <summary>
Expand All @@ -461,6 +464,7 @@ public static readonly EventId InvalidIncludePathError
/// This event uses the <see cref="NavigationEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
[Obsolete]
public static readonly EventId RequiredAttributeOnDependent = MakeModelId(Id.RequiredAttributeOnDependent);

/// <summary>
Expand All @@ -474,6 +478,7 @@ public static readonly EventId InvalidIncludePathError
/// This event uses the <see cref="NavigationEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
[Obsolete]
public static readonly EventId NonNullableReferenceOnDependent = MakeModelId(Id.NonNullableReferenceOnDependent);

/// <summary>
Expand All @@ -489,6 +494,19 @@ public static readonly EventId InvalidIncludePathError
/// </summary>
public static readonly EventId RequiredAttributeOnCollection = MakeModelId(Id.RequiredAttributeOnCollection);

/// <summary>
/// <para>
/// The <see cref="RequiredAttribute" /> on the skip navigation property was ignored.
/// </para>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Model" /> category.
/// </para>
/// <para>
/// This event uses the <see cref="SkipNavigationEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
public static readonly EventId RequiredAttributeOnSkipNavigation = MakeModelId(Id.RequiredAttributeOnSkipNavigation);

/// <summary>
/// <para>
/// The properties that best match the foreign key convention are already used by a different foreign key.
Expand Down
44 changes: 44 additions & 0 deletions src/EFCore/Diagnostics/CoreLoggerExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1284,6 +1284,7 @@ public static void RequiredAttributeInverted(
}
}

[Obsolete]
private static string RequiredAttributeInverted(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string>)definition;
Expand Down Expand Up @@ -1319,6 +1320,7 @@ public static void NonNullableInverted(
}
}

[Obsolete]
private static string NonNullableInverted(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string>)definition;
Expand All @@ -1332,6 +1334,7 @@ private static string NonNullableInverted(EventDefinitionBase definition, EventD
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="firstNavigation"> The first navigation property. </param>
/// <param name="secondNavigation"> The second navigation property. </param>
[Obsolete]
public static void RequiredAttributeOnBothNavigations(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model> diagnostics,
[NotNull] INavigation firstNavigation,
Expand Down Expand Up @@ -1361,6 +1364,7 @@ public static void RequiredAttributeOnBothNavigations(
}
}

[Obsolete]
private static string RequiredAttributeOnBothNavigations(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string, string, string>)definition;
Expand All @@ -1380,6 +1384,7 @@ private static string RequiredAttributeOnBothNavigations(EventDefinitionBase def
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="firstNavigation"> The first navigation property. </param>
/// <param name="secondNavigation"> The second navigation property. </param>
[Obsolete]
public static void NonNullableReferenceOnBothNavigations(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model> diagnostics,
[NotNull] INavigation firstNavigation,
Expand Down Expand Up @@ -1409,6 +1414,7 @@ public static void NonNullableReferenceOnBothNavigations(
}
}

[Obsolete]
private static string NonNullableReferenceOnBothNavigations(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string, string, string>)definition;
Expand All @@ -1427,6 +1433,7 @@ private static string NonNullableReferenceOnBothNavigations(EventDefinitionBase
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="navigation"> The navigation property. </param>
[Obsolete]
public static void RequiredAttributeOnDependent(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model> diagnostics,
[NotNull] INavigation navigation)
Expand All @@ -1451,6 +1458,7 @@ public static void RequiredAttributeOnDependent(
}
}

[Obsolete]
private static string RequiredAttributeOnDependent(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string>)definition;
Expand All @@ -1463,6 +1471,7 @@ private static string RequiredAttributeOnDependent(EventDefinitionBase definitio
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="navigation"> The navigation property. </param>
[Obsolete]
public static void NonNullableReferenceOnDependent(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model> diagnostics,
[NotNull] INavigation navigation)
Expand All @@ -1488,6 +1497,7 @@ public static void NonNullableReferenceOnDependent(
}
}

[Obsolete]
private static string NonNullableReferenceOnDependent(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string>)definition;
Expand Down Expand Up @@ -1529,6 +1539,40 @@ private static string RequiredAttributeOnCollection(EventDefinitionBase definiti
return d.GenerateMessage(p.Navigation.DeclaringEntityType.DisplayName(), p.Navigation.Name);
}

/// <summary>
/// Logs for the <see cref="CoreEventId.RequiredAttributeOnSkipNavigation" /> event.
/// </summary>
/// <param name="diagnostics"> The diagnostics logger to use. </param>
/// <param name="navigation"> The navigation property. </param>
public static void RequiredAttributeOnSkipNavigation(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model> diagnostics,
[NotNull] ISkipNavigation navigation)
{
var definition = CoreResources.LogRequiredAttributeOnSkipNavigation(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics, navigation.DeclaringEntityType.DisplayName(), navigation.Name);
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new SkipNavigationEventData(
definition,
RequiredAttributeOnSkipNavigation,
navigation);

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

private static string RequiredAttributeOnSkipNavigation(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string>)definition;
var p = (SkipNavigationEventData)payload;
return d.GenerateMessage(p.Navigation.DeclaringEntityType.DisplayName(), p.Navigation.Name);
}

/// <summary>
/// Logs for the <see cref="CoreEventId.ConflictingShadowForeignKeysWarning" /> event.
/// </summary>
Expand Down
13 changes: 13 additions & 0 deletions src/EFCore/Diagnostics/LoggingDefinitions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,7 @@ public abstract class LoggingDefinitions
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
[Obsolete]
public EventDefinitionBase LogRequiredAttributeOnDependent;

/// <summary>
Expand All @@ -530,6 +531,7 @@ public abstract class LoggingDefinitions
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
[Obsolete]
public EventDefinitionBase LogNonNullableReferenceOnDependent;

/// <summary>
Expand All @@ -539,6 +541,7 @@ public abstract class LoggingDefinitions
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
[Obsolete]
public EventDefinitionBase LogRequiredAttributeOnBothNavigations;

/// <summary>
Expand All @@ -548,6 +551,7 @@ public abstract class LoggingDefinitions
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
[Obsolete]
public EventDefinitionBase LogNonNullableReferenceOnBothNavigations;

/// <summary>
Expand All @@ -568,6 +572,15 @@ public abstract class LoggingDefinitions
[EntityFrameworkInternal]
public EventDefinitionBase LogRequiredAttributeOnCollection;

/// <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 LogRequiredAttributeOnSkipNavigation;

/// <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
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore/Infrastructure/CoreOptionsExtension.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ private WarningsConfiguration _warningsConfiguration
.TryWithExplicit(CoreEventId.ManyServiceProvidersCreatedWarning, WarningBehavior.Throw)
.TryWithExplicit(CoreEventId.LazyLoadOnDisposedContextWarning, WarningBehavior.Throw)
.TryWithExplicit(CoreEventId.DetachedLazyLoadingWarning, WarningBehavior.Throw)
.TryWithExplicit(CoreEventId.InvalidIncludePathError, WarningBehavior.Throw)
.TryWithExplicit(CoreEventId.RequiredAttributeOnDependent, WarningBehavior.Throw);
.TryWithExplicit(CoreEventId.InvalidIncludePathError, WarningBehavior.Throw);

/// <summary>
/// Creates a new set of options with everything set to default values.
Expand Down
22 changes: 5 additions & 17 deletions src/EFCore/Metadata/Conventions/NonNullableNavigationConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ private void ProcessNavigation(IConventionNavigationBuilder navigationBuilder)
{
var navigation = navigationBuilder.Metadata;
var foreignKey = navigation.ForeignKey;
var relationshipBuilder = foreignKey.Builder;
var modelBuilder = navigationBuilder.ModelBuilder;

if (!IsNonNullable(modelBuilder, navigation)
Expand All @@ -69,28 +68,17 @@ private void ProcessNavigation(IConventionNavigationBuilder navigationBuilder)
return;
}

if (!navigation.IsOnDependent)
if (foreignKey.GetPrincipalEndConfigurationSource() != null)
{
var inverse = navigation.Inverse;
if (inverse != null)
if (navigation.IsOnDependent)
{
if (IsNonNullable(modelBuilder, inverse))
{
Dependencies.Logger.NonNullableReferenceOnBothNavigations(navigation, inverse);
return;
}
foreignKey.Builder.IsRequired(true);
}

if (foreignKey.GetPrincipalEndConfigurationSource() != null)
else
{
Dependencies.Logger.NonNullableReferenceOnDependent(navigation.ForeignKey.PrincipalToDependent);
return;
foreignKey.Builder.IsRequiredDependent(true);
}

return;
}

relationshipBuilder.IsRequired(true);
}

private bool IsNonNullable(IConventionModelBuilder modelBuilder, IConventionNavigation navigation)
Expand Down
30 changes: 13 additions & 17 deletions src/EFCore/Metadata/Conventions/RequiredNavigationAttributeConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,30 +69,26 @@ private void ProcessNavigation(IConventionNavigationBuilder navigationBuilder)
return;
}

var relationshipBuilder = foreignKey.Builder;
if (!navigation.IsOnDependent)
if (foreignKey.GetPrincipalEndConfigurationSource() != null)
{
var inverse = navigation.Inverse;
if (inverse != null)
if (navigation.IsOnDependent)
{
var attributes = GetAttributes<RequiredAttribute>(inverse.DeclaringEntityType, inverse);
if (attributes.Any())
{
Dependencies.Logger.RequiredAttributeOnBothNavigations(navigation, inverse);
return;
}
foreignKey.Builder.IsRequired(true, fromDataAnnotation: true);
}

if (foreignKey.GetPrincipalEndConfigurationSource() != null)
else
{
Dependencies.Logger.RequiredAttributeOnDependent(foreignKey.PrincipalToDependent);
return;
foreignKey.Builder.IsRequiredDependent(true, fromDataAnnotation: true);
}

return;
}
}

relationshipBuilder.IsRequired(true, fromDataAnnotation: true);
/// <inheritdoc />
public override void ProcessSkipNavigationAdded(
IConventionSkipNavigationBuilder skipNavigationBuilder,
RequiredAttribute attribute,
IConventionContext<IConventionSkipNavigationBuilder> context)
{
Dependencies.Logger.RequiredAttributeOnSkipNavigation(skipNavigationBuilder.Metadata);
}
}
}
20 changes: 3 additions & 17 deletions src/EFCore/Metadata/Internal/ForeignKey.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,14 +580,9 @@ public virtual bool IsRequired
var oldRequired = IsRequired;
_isRequired = required ?? DefaultIsRequired;

if (required == null)
{
_isRequiredConfigurationSource = null;
}
else
{
UpdateIsRequiredConfigurationSource(configurationSource);
}
_isRequiredConfigurationSource = required == null
? null
: (ConfigurationSource?)configurationSource.Max(_isRequiredConfigurationSource);

return IsRequired != oldRequired
? DeclaringEntityType.Model.ConventionDispatcher.OnForeignKeyRequirednessChanged(Builder)
Expand All @@ -614,15 +609,6 @@ public virtual bool IsRequired
public virtual void SetIsRequiredConfigurationSource(ConfigurationSource? configurationSource)
=> _isRequiredConfigurationSource = configurationSource;

/// <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>
private void UpdateIsRequiredConfigurationSource(ConfigurationSource configurationSource)
=> _isRequiredConfigurationSource = configurationSource.Max(_isRequiredConfigurationSource);

/// <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
Expand Down
4 changes: 2 additions & 2 deletions src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3085,10 +3085,10 @@ private InternalForeignKeyBuilder HasOwnership(

var ownershipBuilder = existingNavigation.ForeignKey.Builder;
ownershipBuilder = ownershipBuilder
.IsRequired(true, configurationSource)
?.HasEntityTypes(
.HasEntityTypes(
Metadata, ownershipBuilder.Metadata.FindNavigationsFromInHierarchy(Metadata).Single().TargetEntityType,
configurationSource)
?.IsRequired(true, configurationSource)
?.HasNavigations(inverse, navigation, configurationSource)
?.IsOwnership(true, configurationSource);

Expand Down
Loading

0 comments on commit 946998a

Please sign in to comment.