From c49151c98b32c479336ae410aea89ab4e18e1c53 Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Tue, 26 Nov 2024 20:29:41 -0800 Subject: [PATCH 1/2] Prevent owner from becoming optional Fixes #35110 --- .../ForeignKeyPropertyDiscoveryConvention.cs | 13 +++-- .../Internal/InternalEntityTypeBuilder.cs | 15 ++--- .../Internal/MigrationsModelDifferTest.cs | 57 +++++++++++++++++++ 3 files changed, 69 insertions(+), 16 deletions(-) diff --git a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs index 45194d0a2a1..7fae234a577 100644 --- a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs +++ b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs @@ -81,13 +81,16 @@ private IConventionForeignKeyBuilder ProcessForeignKey( IConventionContext context) { var shouldBeRequired = true; - foreach (var property in relationshipBuilder.Metadata.Properties) + if (!relationshipBuilder.Metadata.IsOwnership) { - if (property.IsNullable) + foreach (var property in relationshipBuilder.Metadata.Properties) { - shouldBeRequired = false; - relationshipBuilder = relationshipBuilder.IsRequired(false) ?? relationshipBuilder; - break; + if (property.IsNullable) + { + shouldBeRequired = false; + relationshipBuilder = relationshipBuilder.IsRequired(false) ?? relationshipBuilder; + break; + } } } diff --git a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs index dfeeb09c785..58c607bc23e 100644 --- a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs @@ -3161,17 +3161,9 @@ public static InternalIndexBuilder DetachIndex(Index indexToDetach) && targetEntityType.Name == existingTargetType.ClrType.DisplayName()))) { relationship = existingNavigation.ForeignKey.Builder; - if (existingNavigation.ForeignKey.IsOwnership) - { - relationship = relationship.IsOwnership(true, configurationSource) - ?.HasNavigations(inverse, navigation, configurationSource); - - relationship?.Metadata.UpdateConfigurationSource(configurationSource); - return relationship; - } - Check.DebugAssert( - !existingTargetType.IsOwned() + existingNavigation.ForeignKey.IsOwnership + || !existingTargetType.IsOwned() || existingNavigation.DeclaringEntityType.IsInOwnershipPath(existingTargetType) || (existingTargetType.IsInOwnershipPath(existingNavigation.DeclaringEntityType) && existingTargetType.FindOwnership()!.PrincipalEntityType != existingNavigation.DeclaringEntityType), @@ -3179,7 +3171,8 @@ public static InternalIndexBuilder DetachIndex(Index indexToDetach) + "Owned types should only have ownership or ownee navigations point at it"); relationship = relationship.IsOwnership(true, configurationSource) - ?.HasNavigations(inverse, navigation, configurationSource); + ?.HasNavigations(inverse, navigation, configurationSource) + ?.IsRequired(true, configurationSource); relationship?.Metadata.UpdateConfigurationSource(configurationSource); return relationship; diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs index 3c00a36a40b..81349f24ddb 100644 --- a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs +++ b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Reflection.Emit; using Microsoft.EntityFrameworkCore.TestUtilities.FakeProvider; // ReSharper disable UnusedAutoPropertyAccessor.Local @@ -9763,6 +9764,62 @@ public void Update_AK_seed_value_with_a_referencing_foreign_key() v => Assert.Equal(4242, v)); })); + [ConditionalFact] + public void Owned_collection_with_explicit_id() + => Execute( + modelBuilder => + { + }, + source => + { + source.Entity("Microsoft.EntityFrameworkCore.Migrations.Internal.Account", b => + { + b.Property("Id"); + b.HasKey("Id"); + b.ToTable("account"); + }); + + source.Entity("Microsoft.EntityFrameworkCore.Migrations.Internal.Account", b => + { + b.OwnsMany("Microsoft.EntityFrameworkCore.Migrations.Internal.AccountHolder", "AccountHolders", b1 => + { + b1.Property("Id"); + b1.Property("account_id"); + b1.HasKey("Id"); + b1.HasIndex("account_id"); + b1.ToTable("account_holder"); + b1.WithOwner().HasForeignKey("account_id"); + }); + }); + }, + target => + { + target.Entity(builder => + { + builder.ToTable("account"); + builder.HasKey("Id"); + builder.OwnsMany(a => a.AccountHolders, navigationBuilder => + { + navigationBuilder.ToTable("account_holder"); + navigationBuilder.Property("Id"); + navigationBuilder.HasKey("Id"); + navigationBuilder.Property("account_id"); + navigationBuilder.WithOwner().HasForeignKey("account_id"); + }); + }); + }, + Assert.Empty); + + public class Account + { + public string Id { get; set; } + public IEnumerable AccountHolders { get; set; } = []; + } + + public class AccountHolder + { + } + [ConditionalFact] public void SeedData_with_guid_AK_and_multiple_owned_types() => Execute( From 7baf361479018408429a6dbb47f6850944fa3e1c Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Tue, 26 Nov 2024 20:37:29 -0800 Subject: [PATCH 2/2] Add quirk mode for #35110 --- .../ForeignKeyPropertyDiscoveryConvention.cs | 6 +++++- .../Metadata/Internal/InternalEntityTypeBuilder.cs | 11 +++++++++-- .../Migrations/Internal/MigrationsModelDifferTest.cs | 1 - 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs index 7fae234a577..beb97f2b1b1 100644 --- a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs +++ b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs @@ -50,6 +50,9 @@ public class ForeignKeyPropertyDiscoveryConvention : IPropertyFieldChangedConvention, IModelFinalizingConvention { + private static readonly bool UseOldBehavior35110 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35110", out var enabled) && enabled; + /// /// Creates a new instance of . /// @@ -81,7 +84,8 @@ private IConventionForeignKeyBuilder ProcessForeignKey( IConventionContext context) { var shouldBeRequired = true; - if (!relationshipBuilder.Metadata.IsOwnership) + if (!relationshipBuilder.Metadata.IsOwnership + || UseOldBehavior35110) { foreach (var property in relationshipBuilder.Metadata.Properties) { diff --git a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs index 58c607bc23e..53d96998377 100644 --- a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs @@ -15,6 +15,9 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Internal; /// public class InternalEntityTypeBuilder : InternalTypeBaseBuilder, IConventionEntityTypeBuilder { + private static readonly bool UseOldBehavior35110 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35110", out var enabled) && enabled; + /// /// 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 @@ -3171,8 +3174,12 @@ public static InternalIndexBuilder DetachIndex(Index indexToDetach) + "Owned types should only have ownership or ownee navigations point at it"); relationship = relationship.IsOwnership(true, configurationSource) - ?.HasNavigations(inverse, navigation, configurationSource) - ?.IsRequired(true, configurationSource); + ?.HasNavigations(inverse, navigation, configurationSource); + + if (!UseOldBehavior35110) + { + relationship = relationship?.IsRequired(true, configurationSource); + } relationship?.Metadata.UpdateConfigurationSource(configurationSource); return relationship; diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs index 81349f24ddb..5ca1a781feb 100644 --- a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs +++ b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Reflection.Emit; using Microsoft.EntityFrameworkCore.TestUtilities.FakeProvider; // ReSharper disable UnusedAutoPropertyAccessor.Local