From 27715956c8f6985afa13fef32ba641ef5ce9e2dc Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Mon, 9 Nov 2020 11:12:21 -0800 Subject: [PATCH] Add more tests for keyless entity types Fixes #11802 --- .../RelationalQueryableExtensions.cs | 2 +- .../Internal/InternalEntityTypeBuilder.cs | 51 ++++++++++++++++- .../Metadata/Internal/SkipNavigation.cs | 5 +- .../CosmosModelBuilderGenericTest.cs | 6 -- .../InMemoryModelBuilderGenericTest.cs | 6 -- .../SqlServerModelBuilderGenericTest.cs | 6 -- .../ModelBuilding/KeylessEntitiesTestBase.cs | 46 --------------- .../ModelBuilding/ModelBuilderGenericTest.cs | 6 -- .../ModelBuilderNonGenericTest.cs | 6 -- .../ModelBuilding/OneToManyTestBase.cs | 56 +++++++++++++++++++ 10 files changed, 111 insertions(+), 79 deletions(-) delete mode 100644 test/EFCore.Tests/ModelBuilding/KeylessEntitiesTestBase.cs diff --git a/src/EFCore.Relational/Extensions/RelationalQueryableExtensions.cs b/src/EFCore.Relational/Extensions/RelationalQueryableExtensions.cs index 019bb3f0bb4..a90a85aa9a8 100644 --- a/src/EFCore.Relational/Extensions/RelationalQueryableExtensions.cs +++ b/src/EFCore.Relational/Extensions/RelationalQueryableExtensions.cs @@ -152,7 +152,7 @@ private static FromSqlQueryRootExpression GenerateFromSqlQueryRoot( IQueryable source, string sql, object[] arguments, - [CallerMemberName] string memberName = null) + [CallerMemberName] string memberName = null!) { var queryRootExpression = (QueryRootExpression)source.Expression; diff --git a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs index cc0d9b699a4..5a7d97a9934 100644 --- a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs @@ -2744,6 +2744,45 @@ private InternalForeignKeyBuilder HasRelationship( Metadata, null, navigationToTarget, !setTargetAsPrincipal, configurationSource, required); } + if (configurationSource == ConfigurationSource.Explicit + && setTargetAsPrincipal.HasValue) + { + if (setTargetAsPrincipal.Value) + { + if (targetEntityType.IsKeyless + && targetEntityType.GetIsKeylessConfigurationSource() == ConfigurationSource.Explicit) + { + throw new InvalidOperationException(CoreStrings.PrincipalKeylessType( + targetEntityType.DisplayName(), + targetEntityType.DisplayName() + + (inverseNavigation == null + ? "" + : "." + inverseNavigation.Value.Name), + Metadata.DisplayName() + + (navigationToTarget == null + ? "" + : "." + navigationToTarget.Value.Name))); + } + } + else + { + if (Metadata.IsKeyless + && Metadata.GetIsKeylessConfigurationSource() == ConfigurationSource.Explicit) + { + throw new InvalidOperationException(CoreStrings.PrincipalKeylessType( + Metadata.DisplayName(), + Metadata.DisplayName() + + (navigationToTarget == null + ? "" + : "." + navigationToTarget.Value.Name), + targetEntityType.DisplayName() + + (inverseNavigation == null + ? "" + : "." + inverseNavigation.Value.Name))); + } + } + } + var existingRelationship = InternalForeignKeyBuilder.FindCurrentForeignKeyBuilder( targetEntityType, Metadata, @@ -2908,6 +2947,11 @@ private InternalForeignKeyBuilder HasRelationship( } relationship = newRelationship; + + if (relationship == null) + { + return null; + } } if (setTargetAsPrincipal == true) @@ -3616,7 +3660,12 @@ private ForeignKey SetOrAddForeignKey( 1, null, new[] { typeof(int) }, new[] { "TempId" }, isRequired: true, baseName: "").Item2; principalKey = principalBaseEntityTypeBuilder.HasKeyInternal( - principalKeyProperties, ConfigurationSource.Convention).Metadata; + principalKeyProperties, ConfigurationSource.Convention)?.Metadata; + + if (principalKey == null) + { + return null; + } } if (foreignKey != null) diff --git a/src/EFCore/Metadata/Internal/SkipNavigation.cs b/src/EFCore/Metadata/Internal/SkipNavigation.cs index 771bdd99640..245d4e81d85 100644 --- a/src/EFCore/Metadata/Internal/SkipNavigation.cs +++ b/src/EFCore/Metadata/Internal/SkipNavigation.cs @@ -166,7 +166,10 @@ public virtual EntityType? JoinEntityType var oldForeignKey = ForeignKey; var isChanging = foreignKey != ForeignKey; - oldForeignKey?.ReferencingSkipNavigations!.Remove(this); + if (oldForeignKey != null) + { + oldForeignKey.ReferencingSkipNavigations!.Remove(this); + } if (foreignKey == null) { diff --git a/test/EFCore.Cosmos.Tests/ModelBuilding/CosmosModelBuilderGenericTest.cs b/test/EFCore.Cosmos.Tests/ModelBuilding/CosmosModelBuilderGenericTest.cs index 11c5b0a8c9b..63107039d59 100644 --- a/test/EFCore.Cosmos.Tests/ModelBuilding/CosmosModelBuilderGenericTest.cs +++ b/test/EFCore.Cosmos.Tests/ModelBuilding/CosmosModelBuilderGenericTest.cs @@ -214,11 +214,5 @@ public class CosmosGenericOwnedTypes : GenericOwnedTypes protected override TestModelBuilder CreateModelBuilder() => CreateTestModelBuilder(CosmosTestHelpers.Instance); } - - public class CosmosGenericKeylessEntities : GenericKeylessEntities - { - protected override TestModelBuilder CreateModelBuilder() - => CreateTestModelBuilder(CosmosTestHelpers.Instance); - } } } diff --git a/test/EFCore.InMemory.Tests/ModelBuilding/InMemoryModelBuilderGenericTest.cs b/test/EFCore.InMemory.Tests/ModelBuilding/InMemoryModelBuilderGenericTest.cs index 66119862d3d..45c563b0405 100644 --- a/test/EFCore.InMemory.Tests/ModelBuilding/InMemoryModelBuilderGenericTest.cs +++ b/test/EFCore.InMemory.Tests/ModelBuilding/InMemoryModelBuilderGenericTest.cs @@ -241,11 +241,5 @@ public class InMemoryGenericOwnedTypes : GenericOwnedTypes protected override TestModelBuilder CreateModelBuilder() => CreateTestModelBuilder(InMemoryTestHelpers.Instance); } - - public class InMemoryGenericKeylessEntities : GenericKeylessEntities - { - protected override TestModelBuilder CreateModelBuilder() - => CreateTestModelBuilder(InMemoryTestHelpers.Instance); - } } } diff --git a/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs b/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs index 0670cdf0c13..8cb5b736f3b 100644 --- a/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs +++ b/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs @@ -671,11 +671,5 @@ public override void Can_configure_owned_type_key() protected override TestModelBuilder CreateModelBuilder() => CreateTestModelBuilder(SqlServerTestHelpers.Instance); } - - public class SqlServerGenericKeylessEntities : GenericKeylessEntities - { - protected override TestModelBuilder CreateModelBuilder() - => CreateTestModelBuilder(SqlServerTestHelpers.Instance); - } } } diff --git a/test/EFCore.Tests/ModelBuilding/KeylessEntitiesTestBase.cs b/test/EFCore.Tests/ModelBuilding/KeylessEntitiesTestBase.cs deleted file mode 100644 index 2bae0fc0f10..00000000000 --- a/test/EFCore.Tests/ModelBuilding/KeylessEntitiesTestBase.cs +++ /dev/null @@ -1,46 +0,0 @@ -// 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 Xunit; - -// ReSharper disable InconsistentNaming -namespace Microsoft.EntityFrameworkCore.ModelBuilding -{ - public abstract partial class ModelBuilderTest - { - public abstract class KeylessEntitiesTestBase : ModelBuilderTestBase - { - [ConditionalFact] - public virtual void Keyless_type_discovered_before_entity_type_does_not_leave_temp_id() - { - var modelBuilder = CreateModelBuilder(); - - modelBuilder.Ignore(); - modelBuilder.Ignore(); - - modelBuilder.Entity().HasNoKey(); - modelBuilder.Entity(); - - modelBuilder.FinalizeModel(); - - Assert.Null(modelBuilder.Model.FindEntityType(typeof(Customer))?.FindProperty("TempId")); - Assert.Null(modelBuilder.Model.FindEntityType(typeof(KeylessEntity)).FindPrimaryKey()); - } - - [ConditionalFact] - public virtual void Keyless_type_with_collection_navigations_does_not_throw() - { - var modelBuilder = CreateModelBuilder(); - - modelBuilder.Ignore(); - - modelBuilder.Entity().HasNoKey(); - - modelBuilder.FinalizeModel(); - - Assert.Empty(modelBuilder.Model.FindEntityType(typeof(Customer)).GetNavigations()); - Assert.Null(modelBuilder.Model.FindEntityType(typeof(Customer)).FindPrimaryKey()); - } - } - } -} diff --git a/test/EFCore.Tests/ModelBuilding/ModelBuilderGenericTest.cs b/test/EFCore.Tests/ModelBuilding/ModelBuilderGenericTest.cs index 336d9ed50ba..20929110674 100644 --- a/test/EFCore.Tests/ModelBuilding/ModelBuilderGenericTest.cs +++ b/test/EFCore.Tests/ModelBuilding/ModelBuilderGenericTest.cs @@ -88,12 +88,6 @@ protected override TestModelBuilder CreateTestModelBuilder(TestHelpers testHelpe => new GenericTestModelBuilder(testHelpers); } - public class GenericKeylessEntities : KeylessEntitiesTestBase - { - protected override TestModelBuilder CreateTestModelBuilder(TestHelpers testHelpers) - => new GenericTestModelBuilder(testHelpers); - } - public class GenericOneToMany : OneToManyTestBase { protected override TestModelBuilder CreateTestModelBuilder(TestHelpers testHelpers) diff --git a/test/EFCore.Tests/ModelBuilding/ModelBuilderNonGenericTest.cs b/test/EFCore.Tests/ModelBuilding/ModelBuilderNonGenericTest.cs index 5190b341dde..1a9a36f9f43 100644 --- a/test/EFCore.Tests/ModelBuilding/ModelBuilderNonGenericTest.cs +++ b/test/EFCore.Tests/ModelBuilding/ModelBuilderNonGenericTest.cs @@ -49,12 +49,6 @@ protected override TestModelBuilder CreateTestModelBuilder(TestHelpers testHelpe => new NonGenericTestModelBuilder(testHelpers); } - public class NonGenericKeylessEntities : KeylessEntitiesTestBase - { - protected override TestModelBuilder CreateTestModelBuilder(TestHelpers testHelpers) - => new NonGenericTestModelBuilder(testHelpers); - } - public class NonGenericOneToMany : OneToManyTestBase { diff --git a/test/EFCore.Tests/ModelBuilding/OneToManyTestBase.cs b/test/EFCore.Tests/ModelBuilding/OneToManyTestBase.cs index 03fbf4cbd13..7037505b673 100644 --- a/test/EFCore.Tests/ModelBuilding/OneToManyTestBase.cs +++ b/test/EFCore.Tests/ModelBuilding/OneToManyTestBase.cs @@ -908,6 +908,62 @@ public virtual void Can_use_non_PK_principal() Assert.Empty(principalType.GetIndexes()); } + [ConditionalFact] + public virtual void Throws_on_keyless_type_as_principal() + { + var modelBuilder = CreateModelBuilder(); + + modelBuilder.Ignore(); + + Assert.Equal( + CoreStrings.PrincipalKeylessType( + nameof(Customer), + nameof(Customer) + "." + nameof(Customer.Orders), + nameof(Order)), + Assert.Throws(() => + modelBuilder.Entity().HasNoKey() + .HasMany(c => c.Orders) + .WithOne(o => o.Customer)).Message); + } + + [ConditionalFact] + public virtual void Keyless_type_with_unmapped_collection_navigations_does_not_throw() + { + var modelBuilder = CreateModelBuilder(); + + modelBuilder.Ignore(); + + modelBuilder.Entity().HasNoKey(); + + var model = modelBuilder.FinalizeModel(); + + var customer = model.FindEntityType(typeof(Customer)); + Assert.Empty(customer.GetNavigations()); + Assert.Null(customer.FindPrimaryKey()); + Assert.Null(model.FindEntityType(typeof(Order))); + } + + [ConditionalFact] + public virtual void Keyless_type_discovered_before_referenced_entity_type_does_not_leave_temp_id() + { + var modelBuilder = CreateModelBuilder(); + + modelBuilder.Ignore(); + modelBuilder.Ignore(); + + modelBuilder.Entity().HasNoKey(); + modelBuilder.Entity(); + + var model = modelBuilder.FinalizeModel(); + + var keyless = model.FindEntityType(typeof(KeylessEntity)); + Assert.Null(model.FindEntityType(typeof(Customer))?.FindProperty("TempId")); + Assert.Null(keyless.FindPrimaryKey()); + + var customerNavigation = keyless.GetNavigations().Single(); + Assert.Same(keyless, customerNavigation.ForeignKey.DeclaringEntityType); + } + [ConditionalFact] public virtual void Can_have_both_convention_properties_specified() {