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

Changes to entities ignored while looping through change tracker entries #17997

Closed
nphmuller opened this issue Sep 23, 2019 · 8 comments · Fixed by #18556
Closed

Changes to entities ignored while looping through change tracker entries #17997

nphmuller opened this issue Sep 23, 2019 · 8 comments · Fixed by #18556
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@nphmuller
Copy link

nphmuller commented Sep 23, 2019

I have some code that I run just before SaveChanges is called. This code loops through the entries currently in the change tracker and changes a few values of the underlying entities.

During upgrade to 3.0.0-rc1.19456.14 I noticed that these changes are now ignored. I've also verified this in 3.0.0-rc2.19463.9 where this still happens. Edit: Also still happens on 3.0.0. (Congratulations on the release, btw! 😄)

This happens with the SqlServer provider, and it doesn't happen with the InMemory provider.

Steps to reproduce

The following code ran in 2.2.x, because the foreign key was set to null while looping the entries. Now, because this change is ignored, it throws because an entity with the foreign key 999 doesn't exist.

public class Tests
{
    [Fact]
    public ShouldNotThrow()
    {
        var dbContext = new MyContext();
        dbContext.Database.EnsureDeleted();
        dbContext.Database.EnsureCreated();
        dbContext.Set<Entity>().Add(new Entity() { SubEntityId = 999 });

        foreach (var entry in dbContext.ChangeTracker.Entries())
        {
            if (entry.Entity is Entity entity)
            {
                entity.SubEntityId = null;
            }
        }

        dbContext.SaveChanges();
    }
}

public class MyContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer("Data Source=localhost;Database=Test;Integrated Security=False;User ID=sa;Password={my-pass}");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Entity>();
        modelBuilder.Entity<SubEntity>();
    }
}

public class Entity
{
    public int Id { get; set; }
        
    public int? SubEntityId { get; set; }
    public SubEntity SubEntity { get; set; }
}

public class SubEntity
{
    public int Id { get; set; }
}

Further technical details

EF Core version: 3.0.0-rc1.19456.14 / 3.0.0-rc2.19463.9 / 3.0.0
Database provider: SqlServer
Target framework: netcoreapp3.0
Operating system: Win 10 1903
IDE: Visual Studio 2019 16.3.0

@smitpatel
Copy link
Contributor

Wait for @ajcvickers

@smitpatel
Copy link
Contributor

Verified that this reproduce in 3.0

@smitpatel smitpatel modified the milestones: 3.1, 3.1.0 Sep 27, 2019
@dragnilar
Copy link

I stumbled across this too while testing out EF 3.0 RC and release, it was causing weird things to happen in some spots. I was too focused on other things to report it, but its definitely going to make our codebase stay on 2.6 for a while longer.

I hope this can gets fixed soon.

@nphmuller
Copy link
Author

It seems this is planned to be fixed in 3.1.0. Is there perhaps any known workaround we can use in the meantime? Else we will have to wait till November until we can upgrade to ASP.NET Core 3.x.

ajcvickers added a commit that referenced this issue Oct 23, 2019
Fixes #17997

The problem here was that the temporary values snapshot was copying current values rather than starting with empty values. This then meant that when the current value was set to null, the temp value took over, which was then incorrect. Related to this, when setting a current value to null, the temp value (if it exists) must also be set to null otherwise it will start being used.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 23, 2019
@ajcvickers
Copy link
Member

@nphmuller Based on the root cause, workarounds might be:

  • Set actual values before calling Add
  • When setting a value to null/CLR default, also force the temporary value to the same thing.

ajcvickers added a commit that referenced this issue Oct 23, 2019
Fixes #17997

The problem here was that the temporary values snapshot was copying current values rather than starting with empty values. This then meant that when the current value was set to null, the temp value took over, which was then incorrect. Related to this, when setting a current value to null, the temp value (if it exists) must also be set to null otherwise it will start being used.
ajcvickers added a commit that referenced this issue Oct 24, 2019
Fixes #17997

The problem here was that the temporary values snapshot was copying current values rather than starting with empty values. This then meant that when the current value was set to null, the temp value took over, which was then incorrect. Related to this, when setting a current value to null, the temp value (if it exists) must also be set to null otherwise it will start being used.
ajcvickers added a commit that referenced this issue Oct 24, 2019
Fixes #17997

The problem here was that the temporary values snapshot was copying current values rather than starting with empty values. This then meant that when the current value was set to null, the temp value took over, which was then incorrect. Related to this, when setting a current value to null, the temp value (if it exists) must also be set to null otherwise it will start being used.
@dragnilar
Copy link

@ajcvickers Finally Something Crunchy? 😄
Thanks for fixing this. Do you recommend trying it out in the nightly if we want to test it or do you think it would be better to wait until 3.1 has an RC?

@ajcvickers ajcvickers modified the milestones: 3.1.0, 3.1.0-preview2 Oct 24, 2019
@nphmuller
Copy link
Author

Since 3.1 will mainly contain bug fixes I think it’s worth trying out the nightlies.
Thanks @ajcvickers !

@ajcvickers
Copy link
Member

@dragnilar @nphmuller Always the nightlies, otherwise we won't have enough time to respond if you find issues.

@ajcvickers ajcvickers modified the milestones: 3.1.0-preview3, 3.1.0 Dec 2, 2019
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants