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

[EFC5] Sharing the same .NET type among multiple owned types loses EntityState.Deleted tracking in ChangeTracker #23418

Closed
HaisojYellams opened this issue Nov 20, 2020 · 4 comments · Fixed by #26027
Labels
area-change-tracking 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

@HaisojYellams
Copy link

If the same .NET type (class) is used for two Owned Types on an entity, the DbContext's ChangeTracker no longer detects when those types are Deleted. However, if the Owned Type is used exactly once, it will track it.

In the example below, you can see that the Names property is shown as both Added and Deleted on the second save (which, I believe) is the expected outcome. However, both the CurrentLocation and HomeLocation properties only show Added states - no Deleted.

(This scenario arose for me while attempting to set up an audit - I need to know the values in the deleted type as well as the newly added values.)

Click to see Entities/DbContext
public class Context : DbContext
    {
        public DbSet<Person> Person { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder builder)
        {
            builder.UseSqlServer("Data Source=localhost; User ID=sa; Password=ab4Dpa$$woRd;");
        }

        protected override void OnModelCreating(ModelBuilder builder)
        {
            builder.HasDefaultSchema("bugreport");

            builder.Entity<Person>(
                ebuilder =>
                {
                    ebuilder.OwnsOne(person => person.CurrentLocation);
                    ebuilder.OwnsOne(person => person.HomeLocation);
                    ebuilder.OwnsOne(person => person.Names);
                }
            );
        }

        public override int SaveChanges(bool acceptAllChangesOnSuccess)
        {
            Log();
            return base.SaveChanges(acceptAllChangesOnSuccess);
        }

        public void Log()
        {
            foreach (var entry in ChangeTracker.Entries())
            {
                var propName = "";
                if (entry.Metadata.IsOwned())
                {
                    propName = entry.Metadata.FindOwnership().PrincipalToDependent.Name;
                }

                Console.WriteLine($"Detected entity {entry.Entity.GetType().Name} with state {entry.State}" +
                                  (propName == "" ? "" : $" ({propName})"));
            }

            Console.WriteLine("");
        }
    }

    public class CityState
    {
        public string City { get; private set; }
        public string State { get; private set; }

        private CityState()
        {
        }

        public CityState(string city, string state)
        {
            City = city;
            State = state;
        }
    }

    public class Names
    {
        public string First { get; private set; }
        public string Last { get; private set; }

        private Names()
        {
        }

        public Names(string first, string last)
        {
            First = first;
            Last = last;
        }
    }

    public class Person
    {
        public int Id { get; private set; }
        public CityState CurrentLocation { get; set; }
        public CityState HomeLocation { get; set; }
        
        public Names Names { get; set; }
    }
Click to see driver Program
class Program
    {
        static void Main(string[] args)
        {
            RunOwnedEntityTest();
        }

        private static void RunOwnedEntityTest()
        {
            using (var db = new Context())
            {
                // Create the entities
                var person = new Person()
                {
                    CurrentLocation = new CityState("New York City", "New York"),
                    HomeLocation = new CityState("San Francisco", "California"),
                    Names = new Names("John", "Smith")
                };

                db.Person.Add(person);
                Console.WriteLine("Initial save");
                db.SaveChanges();

                // Update the locations
                person.CurrentLocation = new CityState("Las Vegas", "Nevada");
                person.HomeLocation = new CityState("San Diego", "California");
                person.Names = new Names("Johnathan", "Smith");
                Console.WriteLine("Saving again");
                db.SaveChanges();

                // Delete the person
                db.Person.Remove(person);
                db.SaveChanges();
            }
        }
    }
Click to see the output of running the program
Initial save
Detected entity Person with state Added
Detected entity Names with state Added (Names)
Detected entity CityState with state Added (CurrentLocation)
Detected entity CityState with state Added (HomeLocation)

Saving again
Detected entity Names with state Added (Names)
Detected entity Names with state Deleted (Names)
Detected entity Person with state Unchanged
Detected entity CityState with state Added (CurrentLocation)
Detected entity CityState with state Added (HomeLocation)

Detected entity Person with state Deleted
Detected entity Names with state Deleted (Names)
Detected entity CityState with state Deleted (CurrentLocation)
Detected entity CityState with state Deleted (HomeLocation)
Click to see .csproj file
<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>netcoreapp3.1</TargetFramework>
    </PropertyGroup>

    <ItemGroup>
      <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="5.0.0">
        <PrivateAssets>all</PrivateAssets>
        <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      </PackageReference>
      <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="5.0.0" />
    </ItemGroup>

</Project>

EF Core version: 5.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: netcoreapp3.1
Operating system: Fedora 31
IDE: Rider 2020.2.4

@ajcvickers
Copy link
Member

@AndriySvyryd Why does the Deleted entry show up for non-weak types, but not for weak types? The end result is the same in both cases for owned entities; an update to the database rather than adds and deletes.

@ajcvickers
Copy link
Member

We discussed this in triage and it does seem like a bug, or at least something that could be improved. However, we are currently moving away from weak entity types for shared owned types--see #22378. We will revisit this issue for 6.0 after completing that work.

@HaisojYellams
Copy link
Author

Is there a recommended approach for auditing these weak types in the meantime? This auditing is a strict requirement for the project I am working on. Would it be best to remove the "owning" relationship and turn the "owned" entities into full-fledged entities, having their own table? Or is there a better alternative? Thank you.

@AndriySvyryd
Copy link
Member

@HaisojYellams Yes, you'd need to avoid calling Owns* targeting a particular entity type more than once to avoid it becoming weak.

@ajcvickers ajcvickers modified the milestones: 6.0.0, Backlog Sep 1, 2021
@AndriySvyryd AndriySvyryd modified the milestones: Backlog, 6.0.0 Sep 14, 2021
AndriySvyryd added a commit that referenced this issue Sep 14, 2021
Remove null-checking code for IProperty.GetValueComparer()
Make DebugString terser for Indexes and keys on STETs
Remove extra UsingEntity in ManyToManyQueryFixtureBase

Fixes #23418
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Sep 14, 2021
@AndriySvyryd AndriySvyryd removed their assignment Sep 14, 2021
AndriySvyryd added a commit that referenced this issue Sep 14, 2021
Remove null-checking code for IProperty.GetValueComparer()
Make DebugString terser for Indexes and keys on STETs
Remove extra UsingEntity in ManyToManyQueryFixtureBase

Fixes #23418
AndriySvyryd added a commit that referenced this issue Sep 14, 2021
Remove null-checking code for IProperty.GetValueComparer()
Make DebugString terser for Indexes and keys on STETs
Remove extra UsingEntity in ManyToManyQueryFixtureBase

Fixes #23418
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Sep 15, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc2, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-change-tracking 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.

3 participants