From 2cec5f79e706b7c2beedd1162c84c3bcf0d04e96 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Wed, 10 Mar 2021 09:51:10 -0800 Subject: [PATCH] [5.0.x] Don't leave unknown FK values when principal is known but has not-set, non-generated, value (#24178) * Don't leave unknown FK values when principal is known but has not-set, non-generated, value (#23875) Fixes #23730 * [5.0.x] Don't leave unknown FK values when principal is known but has not-set, non-generated, value Port of 6.0 fix https://github.com/dotnet/efcore/pull/23875 to 5.0 release. Fixes #23730 **Description** An exception is thrown when no key value is set for a non-generated key of an owned type. Normally this is a negative case since non-generated key values must be explicitly set. However, this can works when the non-generated value is part of a composite key for which other parts of the key are generated. In this case, the non-generated part can have the same default value for multiple inserts without violating the primary key constraint. **Customer Impact** This is a regression for the case described above. There is no reasonable workaround. (We already fixed this for EF Core 6.0, but decided not to patch since it seemed to be a regression only in a negative case. Since then other customers have reported the issue and one customer outlined the scenario above where it is a regression in working code.) **How found** Reported by multiple customers. **Test coverage** Test coverage for this case has been added in this PR. **Regression?** Yes, from EF Core 3.1. **Risk** Low. The fix is already in EF Core 6.0 and is targetted to this case. Also quirked. --- .../ChangeTracking/Internal/KeyPropagator.cs | 36 ++++++---- .../ChangeTracking/Internal/OwnedFixupTest.cs | 67 +++++++++++++++++++ 2 files changed, 91 insertions(+), 12 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs b/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs index 542c37e1299..016cbf6e05e 100644 --- a/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs +++ b/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs @@ -1,6 +1,7 @@ // 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.Threading; using System.Threading.Tasks; using JetBrains.Annotations; @@ -30,6 +31,8 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal public class KeyPropagator : IKeyPropagator { private readonly IValueGeneratorSelector _valueGeneratorSelector; + private readonly bool _useOldBehavior + = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23730", out var enabled) && enabled; /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -53,12 +56,19 @@ public virtual InternalEntityEntry PropagateValue(InternalEntityEntry entry, IPr { Check.DebugAssert(property.IsForeignKey(), $"property {property} is not part of an FK"); - var principalEntry = TryPropagateValue(entry, property); + var generationProperty = _useOldBehavior + ? null + : property.GetGenerationProperty(); + + var principalEntry = TryPropagateValue(entry, property, generationProperty); + if (principalEntry == null && property.IsKey() && !property.IsForeignKeyToSelf()) { - var valueGenerator = TryGetValueGenerator(property); + var valueGenerator = _useOldBehavior + ? TryGetValueGenerator(property.GetGenerationProperty()) + : TryGetValueGenerator(generationProperty); if (valueGenerator != null) { @@ -93,11 +103,15 @@ public virtual async Task PropagateValueAsync( { Check.DebugAssert(property.IsForeignKey(), $"property {property} is not part of an FK"); - var principalEntry = TryPropagateValue(entry, property); + var generationProperty = property.GetGenerationProperty(); + var principalEntry = TryPropagateValue(entry, property, generationProperty); + if (principalEntry == null && property.IsKey()) { - var valueGenerator = TryGetValueGenerator(property); + var valueGenerator = _useOldBehavior + ? TryGetValueGenerator(property.GetGenerationProperty()) + : TryGetValueGenerator(generationProperty); if (valueGenerator != null) { @@ -120,7 +134,7 @@ public virtual async Task PropagateValueAsync( return principalEntry; } - private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, IProperty property) + private InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, IProperty property, IProperty generationProperty) { var entityType = entry.EntityType; var stateManager = entry.StateManager; @@ -158,7 +172,9 @@ private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, if (principalProperty != property) { var principalValue = principalEntry[principalProperty]; - if (!principalProperty.ClrType.IsDefaultValue(principalValue)) + + if ((generationProperty == null && !_useOldBehavior) + || !principalProperty.ClrType.IsDefaultValue(principalValue)) { if (principalEntry.HasTemporaryValue(principalProperty)) { @@ -182,13 +198,9 @@ private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, return null; } - private ValueGenerator TryGetValueGenerator(IProperty property) - { - var generationProperty = property.GetGenerationProperty(); - - return generationProperty != null + private ValueGenerator TryGetValueGenerator(IProperty generationProperty) + => generationProperty != null ? _valueGeneratorSelector.Select(generationProperty, generationProperty.DeclaringEntityType) : null; - } } } diff --git a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs index cbfe5b43024..e592e96c38e 100644 --- a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs +++ b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.ComponentModel.DataAnnotations.Schema; using System.Linq; using System.Threading.Tasks; using Microsoft.EntityFrameworkCore.Diagnostics; @@ -4211,6 +4212,72 @@ EntityState GetEntryState(EquatableEntitiesContext context, string role } } + [ConditionalTheory] + [InlineData(false)] + [InlineData(true)] + public async Task SaveChanges_when_owner_has_PK_with_default_values(bool async) + { + using (var context = new OneRowContext(async)) + { + var blog = new Blog { Type = new OwnedType { Value = "A" } }; + + _ = async + ? await context.AddAsync(blog) + : context.Add(blog); + + Assert.Equal(EntityState.Added, context.Entry(blog).State); + Assert.Equal(EntityState.Added, context.Entry(blog.Type).State); + Assert.Equal(0, blog.Id); + Assert.Equal(0, context.Entry(blog.Type).Property("BlogId").CurrentValue); + + _ = async + ? await context.SaveChangesAsync() + : context.SaveChanges(); + + Assert.Equal(EntityState.Unchanged, context.Entry(blog).State); + Assert.Equal(EntityState.Unchanged, context.Entry(blog.Type).State); + Assert.Equal(0, blog.Id); + Assert.Equal(0, context.Entry(blog.Type).Property("BlogId").CurrentValue); + } + + using (var context = new OneRowContext(async)) + { + // Trying to do the same thing again will throw since only one row can have ID zero. + + context.Add(new Blog { Type = new OwnedType { Value = "A" } }); + Assert.Throws(() => context.SaveChanges()); + } + } + + private class OneRowContext : DbContext + { + private readonly bool _async; + + public OneRowContext(bool async) + { + _async = async; + } + + protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) + => optionsBuilder.UseInMemoryDatabase(nameof(OneRowContext) + _async); + + public DbSet Blogs { get; set; } + } + + public class Blog + { + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id { get; set; } + + public OwnedType Type { get; set; } + } + + [Owned] + public class OwnedType + { + public string Value { get; set; } + } + private class User { public Guid UserId { get; set; }