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

Simple Child Entity Add Operations fails with preset IDs #17747

Closed
RickStrahl opened this issue Sep 10, 2019 · 6 comments
Closed

Simple Child Entity Add Operations fails with preset IDs #17747

RickStrahl opened this issue Sep 10, 2019 · 6 comments

Comments

@RickStrahl
Copy link

RickStrahl commented Sep 10, 2019

I have a very simple model of users and roles and when trying to add roles to users the addition of users fails with:

image

The model is set up like this:

    public class UserContext : DbContext
    {
        public string ConnectionString { get; }

        public DbSet<User> Users { get; set; }
        public DbSet<Role> Roles { get; set; }
 
        public UserContext() : base()
        {
        }

        public UserContext(string connectionString) : base()
        {
            ConnectionString = connectionString;
        }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            string connectionString =
                "server=.;database=FTDB_WESTWIND;integrated security=true;MultipleActiveResultSets=true;App=FTDB";

            if (ConnectionString != null)
                connectionString = ConnectionString;
            optionsBuilder.UseSqlServer(connectionString);

            base.OnConfiguring(optionsBuilder);
        }
    }


    public class User
    {
        public Guid Id { get; set; } = Guid.NewGuid();

        [Required]
        [MaxLength(100)]
        public string FirstName { get; set; }

        [Required]
        [MaxLength(100)]
        public string LastName { get; set; }

        [Required]
        [MaxLength(100)]
        public int Department { get; set; } = 0;


        [MaxLength(100)]
        public string UserName { get; set; }

        [MaxLength(100)]
        public string Password { get; set; }

        public DateTime LastAccessed { get; set; } = DateTime.UtcNow;

        public virtual IList<Role> Roles { get; set; } = new List<Role>();

    }

    public class Role
    {
        public Guid Id { get; set; } = Guid.NewGuid();

        [Required]
        [MaxLength(80)]
        public string Name { get; set; }

        public int Level { get; set; } = 1;

        public bool IsUserAdmin { get; set; }

        public bool IsRootAdmin { get; set; }


        public Role()
        {
        }
    }

The code fails if I run the test code like this:

var AppContext = new UserContext();


var user = new User()
{
    FirstName = "Joe",
    LastName = DataUtils.GenerateUniqueId(10)
};
AppContext.Users.Add(user);

var role = new Role { Level = 10, Name = "Administrator" };
user.Roles.Add(role);
role = new Role { Level = 8, Name = "LocalAdmin" };
user.Roles.Add(role);

AppContext.SaveChanges();

foreach (var rle in user.Roles)
    AppContext.Remove(rle);
AppContext.Remove(user);

AppContext.SaveChanges();

Further technical details

The problem appears to be that the Role entity is presetting the Guid in the initialization - ie. the value is preset.

 public Guid Id { get; set; } = Guid.NewGuid();

If I remove initial ID assignment, the code runs fine.

Now, if this is by design that's terrible, because for things like Guids you should be able to create Ids in code and have EF figure out that the ID was preassigned. This code also works no problem in 2.2 - just swapping back to 2.2 assemblies makes this code work as is (ie. with the ID preassignment).

It almost looks like if the ID is assigned it assumes that the record exists. Even if that's assumed it's inconsistent because the same operation works on the non-child insertion of User which also has an ID preset.

EF Core version: 3.0.0-preview9.19423.6
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.0 Preview 9
Operating system: Windows 10
IDE: VS 2019 16.3

@davidroth
Copy link
Contributor

davidroth commented Sep 10, 2019

That's by design in efcore 3.0. You are not adding the Roles via context.Add(), but instead adding them to the user.Roles collection. So they are not set to the added entity state. Instead they are logically untracked.

Change tracker treats untracked entities as modified in this case. This is a breaking change from 2.x and has been documented here:

https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-3.0/breaking-changes#detectchanges-honors-store-generated-key-values

You can configure the Roles.Id property to ValueGeneratedNever and you will get EfCore 2.x semantics.

@RickStrahl
Copy link
Author

RickStrahl commented Sep 10, 2019

Thanks David.

Verified that it works if I explicitly add the entities.

Here's the code that does work:

            var user = new User()
            {
                FirstName = "Joe",
                LastName = DataUtils.GenerateUniqueId(10)
            };
            AppContext.Users.Add(user);
      
            var role = new Role { Level = 10, Name = "Administrator" };
            user.Roles.Add(role);
            AppContext.Roles.Add(role);

            role = new Role { Level = 8, Name = "LocalAdmin" };
            user.Roles.Add(role);
            AppContext.Roles.Add(role);

            AppContext.SaveChanges();

            foreach (var rle in user.Roles)
                AppContext.Remove(rle);
            AppContext.Remove(user);

            AppContext.SaveChanges();

Still doesn't make sense though, in that it works when don't pre-assign the Ids.

But I gotta ask (not you specifically but more generally): Why change this behavior, because this makes EF even more unwieldy than it already was and makes the whole premise of using an entity model/ORM really painfully useless?

I'm sure there are technical reasons that are internal to the behavior of EF, but it sure as heck makes for a worse - and unexpected - developer experience for those that have to use it.

@AndriySvyryd
Copy link
Member

Most of the time entities that have store-generated keys only have the key values set explicitly if they already exist in the database: #14616

But we are listening for feedback and might make this behavior more explicit: #13575

@davidroth
Copy link
Contributor

davidroth commented Sep 11, 2019

But I gotta ask (not you specifically but more generally): Why change this behavior, because this makes EF even more unwieldy than it already was and makes the whole premise of using an entity model/ORM really painfully useless?

Well for me personally the new behavior makes definitely more sense and is a much better default when working with disconnected entity graphs.
It makes no sense to insert untracked-entities with an existing key, if ValueGenerationOnAdd is configured. Of course, someone has to be aware of how ef works in this regards but thats with many features / behaviors.

Why change this behavior, because this makes EF even more unwieldy than it already was and makes the whole premise of using an entity model/ORM really painfully useless

Well, any "real" ORM brings a lot of complexity and implicit behavior which a developer must be aware of. I don`t see why this specific behavior makes it "really painfully useless".

Maybe the documentation around using guids as primary keys can be improved to guide developers to the right usage?

@RickStrahl
Copy link
Author

RickStrahl commented Sep 12, 2019

Yes there are workarounds and once you know what the problems is it's not a big thing to work around it. But the inconsistency here is what the problem is - and figuring what the problem once you hit it - the DB error message certainly had me scratching my head for a while and only by trial and error did I figure out that the ID was the problem.

Re: more consistent

If you want to be 'pure' and explicitly specify everything that's great, but nothing is stopping you from doing that even if the behavior worked as it previously did. You lose nothing, but those that used this - they do.

It seems really somewhat irresponsible to change this behavior in a 3.0 release when it's always worked a different way before. Because you know that this will break any application that gets ported. Worse this is a break that'll break at Runtime and will be a bear to fix because you now have to go hunting for all the places where Add() was called.

The main reason this rubs me wrong is because this is not a difficult problem to solve - heck it's long been solved. When you get an unattached entity and add it to a tracked entity collection you can assume it's a new entity. How else could this possibly be interpreted? So it's not like there should be a major technical reason for this. Just because it has an already assigned ID should make no difference to the entity state.

To be fair, if you're using integer keys you're probably not pre-assigning so you may never see this behavior, but it's still very unexpected and inconsistent.

@smitpatel
Copy link
Member

Closing as the question has been answered and we have a tracking issue to collection feedback.

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

No branches or pull requests

5 participants