From f16d6b4bf83ec6fcceee13b2052b8dc669e13b58 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 25 May 2020 15:22:52 -0700 Subject: [PATCH] Consistent fixup from query when reference navigation explicitly set Fixes #18007 Previously it was possible for the existing instance to be replaced when a query brings back a new instance, but only for the first time the new instance is returned. This change prevents the new instance from being used in all cases, which is the agreed behavior here, and also now matches what EF6 does consistently. --- .../Internal/NavigationFixer.cs | 9 +- .../OverzealousInitializationCosmosTest.cs | 28 +++++ .../OverzealousInitializationInMemoryTest.cs | 21 ++++ .../OverzealousInitializationTestBase.cs | 118 ++++++++++++++++++ .../OverzealousInitializationSqlServerTest.cs | 21 ++++ .../OverzealousInitializationSqliteTest.cs | 21 ++++ 6 files changed, 215 insertions(+), 3 deletions(-) create mode 100644 test/EFCore.Cosmos.FunctionalTests/OverzealousInitializationCosmosTest.cs create mode 100644 test/EFCore.InMemory.FunctionalTests/OverzealousInitializationInMemoryTest.cs create mode 100644 test/EFCore.Specification.Tests/OverzealousInitializationTestBase.cs create mode 100644 test/EFCore.SqlServer.FunctionalTests/OverzealousInitializationSqlServerTest.cs create mode 100644 test/EFCore.Sqlite.FunctionalTests/OverzealousInitializationSqliteTest.cs diff --git a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs index b9f66117f17..21bcf4848e3 100644 --- a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs +++ b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs @@ -626,9 +626,12 @@ private void InitialFixup( { foreach (InternalEntityEntry dependentEntry in dependents) { - if (!foreignKey.IsOwnership - || (dependentEntry.EntityState != EntityState.Deleted - && dependentEntry.EntityState != EntityState.Detached)) + if ((!foreignKey.IsOwnership + || (dependentEntry.EntityState != EntityState.Deleted + && dependentEntry.EntityState != EntityState.Detached)) + && (!fromQuery + || foreignKey.DependentToPrincipal == null + || dependentEntry.GetCurrentValue(foreignKey.DependentToPrincipal) == null)) { // Add to collection on principal indicated by FK and set inverse navigation AddToCollection(entry, foreignKey.PrincipalToDependent, dependentEntry, fromQuery); diff --git a/test/EFCore.Cosmos.FunctionalTests/OverzealousInitializationCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/OverzealousInitializationCosmosTest.cs new file mode 100644 index 00000000000..3f44f654716 --- /dev/null +++ b/test/EFCore.Cosmos.FunctionalTests/OverzealousInitializationCosmosTest.cs @@ -0,0 +1,28 @@ +// 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 Microsoft.EntityFrameworkCore.Cosmos.TestUtilities; +using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; + +namespace Microsoft.EntityFrameworkCore.Cosmos +{ + public class OverzealousInitializationCosmosTest + : OverzealousInitializationTestBase + { + public OverzealousInitializationCosmosTest(OverzealousInitializationCosmosFixture fixture) + : base(fixture) + { + } + + [ConditionalFact(Skip = "Issue #17246")] + public override void Fixup_does_not_ignore_eagerly_initialized_reference_navs() + { + } + + public class OverzealousInitializationCosmosFixture : OverzealousInitializationFixtureBase + { + protected override ITestStoreFactory TestStoreFactory => CosmosTestStoreFactory.Instance; + } + } +} diff --git a/test/EFCore.InMemory.FunctionalTests/OverzealousInitializationInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/OverzealousInitializationInMemoryTest.cs new file mode 100644 index 00000000000..035e9858cbd --- /dev/null +++ b/test/EFCore.InMemory.FunctionalTests/OverzealousInitializationInMemoryTest.cs @@ -0,0 +1,21 @@ +// 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 Microsoft.EntityFrameworkCore.TestUtilities; + +namespace Microsoft.EntityFrameworkCore +{ + public class OverzealousInitializationInMemoryTest + : OverzealousInitializationTestBase + { + public OverzealousInitializationInMemoryTest(OverzealousInitializationInMemoryFixture fixture) + : base(fixture) + { + } + + public class OverzealousInitializationInMemoryFixture : OverzealousInitializationFixtureBase + { + protected override ITestStoreFactory TestStoreFactory => InMemoryTestStoreFactory.Instance; + } + } +} diff --git a/test/EFCore.Specification.Tests/OverzealousInitializationTestBase.cs b/test/EFCore.Specification.Tests/OverzealousInitializationTestBase.cs new file mode 100644 index 00000000000..1962f5684a4 --- /dev/null +++ b/test/EFCore.Specification.Tests/OverzealousInitializationTestBase.cs @@ -0,0 +1,118 @@ +// 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.Schema; +using System.Linq; +using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; + +namespace Microsoft.EntityFrameworkCore +{ + public abstract class OverzealousInitializationTestBase : IClassFixture + where TFixture : OverzealousInitializationTestBase.OverzealousInitializationFixtureBase, new() + { + protected OverzealousInitializationTestBase(TFixture fixture) => Fixture = fixture; + + [ConditionalFact] + public virtual void Fixup_does_not_ignore_eagerly_initialized_reference_navs() + { + using var context = CreateContext(); + + var albums = context.Set() + .Include(e => e.Tracks) + .Include(e => e.Artist) + .OrderBy(e => e.Artist) + .ToList(); + + foreach (var album in albums) + { + Assert.Equal(0, album.Artist.Id); + Assert.Null(album.Artist.Name); + } + } + + protected class Album + { + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id { get; set; } + public int ArtistId { get; set; } + + public virtual Artist Artist { get; set; } + public virtual IList Tracks { get; set; } + + public Album() + { + Artist = new Artist(); + Tracks = new List(); + } + } + + protected class Artist + { + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id { get; set; } + public string Name { get; set; } + } + + public class Track + { + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id { get; set; } + public int AlbumId { get; set; } + } + + public class AlbumViewerContext : PoolableDbContext + { + public AlbumViewerContext(DbContextOptions options) + : base(options) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity(); + modelBuilder.Entity(); + modelBuilder.Entity(); + } + } + + protected TFixture Fixture { get; } + + protected AlbumViewerContext CreateContext() => Fixture.CreateContext(); + + public abstract class OverzealousInitializationFixtureBase : SharedStoreFixtureBase + { + public virtual IDisposable BeginTransaction(DbContext context) => context.Database.BeginTransaction(); + + protected override string StoreName { get; } = "OverzealousInitialization"; + + protected override void Seed(AlbumViewerContext context) + { + var artists = new[] + { + new Artist { Id = 1, Name = "Freddie" }, + new Artist { Id = 2, Name = "Kendrick"}, + new Artist { Id = 3, Name = "Jarvis" } + }; + + for (var i = 1; i <= 10; i++) + { + context.Add(new Album + { + Id = i, + Artist = artists[i % 3], + Tracks = new List + { + new Track { Id = i * 2 }, + new Track { Id = i * 2 + 1} + } + }); + } + + context.SaveChanges(); + } + } + } +} diff --git a/test/EFCore.SqlServer.FunctionalTests/OverzealousInitializationSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/OverzealousInitializationSqlServerTest.cs new file mode 100644 index 00000000000..af1a8560503 --- /dev/null +++ b/test/EFCore.SqlServer.FunctionalTests/OverzealousInitializationSqlServerTest.cs @@ -0,0 +1,21 @@ +// 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 Microsoft.EntityFrameworkCore.TestUtilities; + +namespace Microsoft.EntityFrameworkCore +{ + public class OverzealousInitializationSqlServerTest + : OverzealousInitializationTestBase + { + public OverzealousInitializationSqlServerTest(OverzealousInitializationSqlServerFixture fixture) + : base(fixture) + { + } + + public class OverzealousInitializationSqlServerFixture : OverzealousInitializationFixtureBase + { + protected override ITestStoreFactory TestStoreFactory => SqlServerTestStoreFactory.Instance; + } + } +} diff --git a/test/EFCore.Sqlite.FunctionalTests/OverzealousInitializationSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/OverzealousInitializationSqliteTest.cs new file mode 100644 index 00000000000..16657aafe74 --- /dev/null +++ b/test/EFCore.Sqlite.FunctionalTests/OverzealousInitializationSqliteTest.cs @@ -0,0 +1,21 @@ +// 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 Microsoft.EntityFrameworkCore.TestUtilities; + +namespace Microsoft.EntityFrameworkCore +{ + public class OverzealousInitializationSqliteTest + : OverzealousInitializationTestBase + { + public OverzealousInitializationSqliteTest(OverzealousInitializationSqliteFixture fixture) + : base(fixture) + { + } + + public class OverzealousInitializationSqliteFixture : OverzealousInitializationFixtureBase + { + protected override ITestStoreFactory TestStoreFactory => SqliteTestStoreFactory.Instance; + } + } +}