Skip to content

Commit

Permalink
[5.0.x] Don't leave unknown FK values when principal is known but has…
Browse files Browse the repository at this point in the history 
… 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 #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.
  • Loading branch information
@ajcvickers
ajcvickers authored Mar 10, 2021
1 parent 09ec021 commit 2cec5f7
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 12 deletions.
36 changes: 24 additions & 12 deletions src/EFCore/ChangeTracking/Internal/KeyPropagator.cs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand All @@ -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)
{
Expand Down Expand Up @@ -93,11 +103,15 @@ public virtual async Task<InternalEntityEntry> 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)
{
Expand All @@ -120,7 +134,7 @@ public virtual async Task<InternalEntityEntry> 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;
Expand Down Expand Up @@ -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))
{
Expand All @@ -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;
}
}
}
67 changes: 67 additions & 0 deletions test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -4211,6 +4212,72 @@ EntityState GetEntryState<TEntity>(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<int>("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<int>("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<ArgumentException>(() => 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<Blog> 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; }
Expand Down

0 comments on commit 2cec5f7

Please sign in to comment.