Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SaveChanges fails for owned entity when owner has PK with default values #23730

Closed
smitpatel opened this issue Dec 18, 2020 · 11 comments
Closed

SaveChanges fails for owned entity when owner has PK with default values #23730

smitpatel opened this issue Dec 18, 2020 · 11 comments

Comments

@smitpatel
Copy link
Member

@smitpatel smitpatel commented Dec 18, 2020

    public class Blog
    {
        public int Id1 { get; set; }
        public int Id2 { get; set; }
        public OwnedType Type { get; set; }
    }

    [Owned]
    public class OwnedType
    {
        public string Value { get; set; }
    }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            // Configure model
            modelBuilder.Entity<Blog>().HasKey(e => new { e.Id1, e.Id2 });
        }
                db.Add(new Blog { Type = new OwnedType { Value = "A" } });

                db.SaveChanges();

Adding only Blog works fine. Adding owned type with blog throws error

An exception occurred in the database while saving changes for context type 'EFSampleApp.MyContext'.
      System.InvalidOperationException: The value of 'OwnedType.BlogId1' is unknown when attempting to save changes. This is because the property is also part of a foreign key for which the principal entity in the relationship is not known.
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.PrepareToSave()
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.GetEntriesToSave(Boolean cascadeChanges)
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(DbContext _, Boolean acceptAllChangesOnSuccess)
         at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChaUnhandled exception.nges(Boolean acceptAllChangesOnSuccess)
         at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
      System.InvalidOperationException: The value of 'OwnedType.BlogId1' is unknown when attempting to save changes . This is because the property is also part of a foreign key for which the principal entity in the relationship is not known.
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.PrepareToSave()
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.GetEntriesToSave(Boolean cascadeChanges)
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(DbContext _, Boolean acceptAllChangesOnSuccess)
         at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
         at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)

Happens for SqlServer/InMemory (provider agnostic.

@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Dec 18, 2020

@smitpatel Regression?

@smitpatel
Copy link
Member Author

@smitpatel smitpatel commented Dec 19, 2020

Did not try on older release. Ran into this when writing test in main branch.

@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Jan 4, 2021

Confirmed regression from 3.1.

@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Jan 5, 2021

Marking for re-triage. This only happens when the key value is not configured as generated, but no key value is set. This is a pseudo-negative case. That is, the first entity saved like this in 3.1 would have succeeded because the key value of zero is not taken yet. The second entity saved will cause a primary key violation.

We should still look at either making this work for the first case, or more likely throw a better exception. But I don't think we need to patch.

@tosatoandrea
Copy link

@tosatoandrea tosatoandrea commented Jan 10, 2021

I ran into this problem when I try to add and save an entity with self relation

@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Jan 15, 2021

@tosatoandrea and others who have thumbs-uped this issue. Are you sure you are running into this problem and not something different that generates the same error message? This particular issue only happens when you try to save entities that are configured not to have store-generated keys and yet do not have a key value set. This exception happens the first time you do this, but even if we fix this exception you will still get an error on the next entity saved, since it will have a conflicting key value.

Therefore, it doesn't seem like fixing this will enable any meaningful scenarios. If you are hitting this, then can you explain what the scenario is so that we can better understand the value of making the fix?

@ajcvickers ajcvickers added this to the 6.0.0 milestone Jan 15, 2021
@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Jan 15, 2021

Note from triage: fix in 6.0 for now; hold off on patching until we get a confirmed customer report that isn't doing something that will always fail later.

ajcvickers added a commit that referenced this issue Jan 15, 2021
…, non-generated, value (#23875)

Fixes #23730
@CampaginAgent
Copy link

@CampaginAgent CampaginAgent commented Jan 23, 2021

@ajcvickers - I think I've hit up against this bug in a real-world case that that won't always fail later.

I'm converting a 3.1 to 5.0 application. This was working perfectly under 3.1 for a long time.

I have a value object defined as:

public class VpaApplicationAgreementSection : ValueObject
   {
       public enum AgreementSectionType
       {
          CoverPage,
          TermsAndConditions,
          Deed,
         ActionLogs
       }

       public AgreementSectionType Type { get; protected set; }
       public string Label { get; protected set; }

       /// <summary>
       /// A Zero-based index 
       /// </summary>
       public int DocumentIndex { get; protected set; }

       public AgreementVersion AgreementVersion { get; protected set; }
   }

This value object is part of an owns-many relationship, defined as

builder.OwnsMany(x => x.AgreementSections,
                           cb =>
                           {
                               cb.ToTable("VpaApplicationAgreementSections", "VpaApply");
                               cb.WithOwner().HasForeignKey("VpaApplicationId");
                               cb.Property(p => p.Type).HasConversion<string>().HasColumnName("Type");
                               cb.Property(p => p.Label).HasColumnName("Label");
                               cb.Property(p => p.DocumentIndex).HasColumnName("DocumentIndex");
                               cb.OwnsOne(x => x.AgreementVersion,
                                          ccb =>
                                          {
                                              ccb.Property(p => p.MergeTemplateAggregateId).HasColumnName("MergeTemplateAggregateId");
                                              ccb.Property(p => p.Version).HasColumnName("Version");
                                          });
                               cb.UsePropertyAccessMode(PropertyAccessMode.Field);
                               cb.HasKey("VpaApplicationId",
                                        "Type", 
                                         "Label",
                                         "DocumentIndex"
                                         
                                         );
                           });

Upon migration of EF5, some of our tests fail with:
System.InvalidOperationException: The value of 'VpaApplicationAgreementSection.AgreementVersion#AgreementVersion.VpaApplicationAgreementSectionType' is unknown when attempting to save changes. This is because the property is also part of a foreign key for which the principal entity in the relationship is not known.

In our value object, DocumentIndex = 0 and Type = AgreementSectionType.CoverPage are both valid values, however, because these also happen to coincide with the C# default values for the int and our enum, we hit up against this bug.

If I change my code so that DocumentIndex starts as one, and my enum is defined as:

        public enum AgreementSectionType
        {
           None,
            CoverPage,
           TermsAndConditions,
           Deed,
          ActionLogs
        }

then my test-suite passes. - This work-around feels a little distasteful, as 'None' is not really a valid value in my domain.

@ajcvickers ajcvickers reopened this Jan 25, 2021
@ajcvickers ajcvickers removed this from the 6.0.0 milestone Jan 25, 2021
@ajcvickers ajcvickers added this to the 5.0.4 milestone Jan 26, 2021
ajcvickers added a commit that referenced this issue Feb 17, 2021
ajcvickers added a commit that referenced this issue Feb 17, 2021
… 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.
@SimonCropp
Copy link
Contributor

@SimonCropp SimonCropp commented Mar 9, 2021

given 5.0.4 is released , should this be closed?

wtgodbe pushed a commit that referenced this issue Mar 10, 2021
… 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.
@smitpatel smitpatel removed this from the 5.0.4 milestone Mar 10, 2021
@smitpatel smitpatel added this to the 5.0.5 milestone Mar 10, 2021
@ajcvickers ajcvickers closed this Mar 11, 2021
@Sayan751
Copy link

@Sayan751 Sayan751 commented Aug 18, 2021

Facing this in 5.0.9. Tried all the versions till 5.0.5. Faced the same issue in all versions. This seems not to be fixed. Please consider reopening this and fixing it in 5.x.

@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Aug 18, 2021

@Sayan751 Please open a new issue and attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment