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

Unable to remove item by its PK only #10180

Closed
silarmani opened this issue Oct 29, 2017 · 6 comments
Closed

Unable to remove item by its PK only #10180

silarmani opened this issue Oct 29, 2017 · 6 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@silarmani
Copy link

If an entity has extra indices then I am unable to delete it by its PK only

Example

    class Program
    {
        static int Main(string[] args)
        {
            var ctx = new MyDbContext();

            **var toBeDeleted = ctx.RiskConsequenceSeverityGroupItems
                .Select(e => new RiskConsequenceSeverityGroupItem { Id = e.Id });**

            ctx.RiskConsequenceSeverityGroupItems.RemoveRange(toBeDeleted);

            ctx.SaveChanges();

            return 0;
        }
    }

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

        public int Order { get; set; }

        public string Name { get; set; }
    }

    public class MyDbContext : DbContext
    {
        public DbSet<RiskConsequenceSeverityGroupItem> RiskConsequenceSeverityGroupItems { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer("Server=localhost,1433;Database=ipl_tux;Integrated Security=True;");            
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<RiskConsequenceSeverityGroupItem>()
                .HasIndex(e => new { e.Order })
                .IsUnique();

            base.OnModelCreating(modelBuilder);
        }
    }

In this case I get the following execption

Unhandled Exception: System.ArgumentException: An item with the same key has already been added. Key: System.Object[]
at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(Object key)
at System.Collections.Generic.Dictionary2.TryInsert(TKey key, TValue value, InsertionBehavior behavior) at System.Collections.Generic.Dictionary2.Add(TKey key, TValue value)
at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.AddUniqueValueEdges(Multigraph2 commandGraph) at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.TopologicalSort(IEnumerable1 commands)
at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.d__8.MoveNext()
at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(Tuple2 parameters) at Microsoft.EntityFrameworkCore.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func3 operation, Func3 verifySucceeded) at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.Execute[TState,TResult](IExecutionStrategy strategy, TState state, Func2 operation)
at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(IEnumerable1 commandBatches, IRelationalConnection connection) at Microsoft.EntityFrameworkCore.Storage.RelationalDatabase.SaveChanges(IReadOnlyList1 entries)
at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(IReadOnlyList`1 entriesToSave)
at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
at Microsoft.EntityFrameworkCore.DbContext.SaveChanges()
at EfTests.Program.Main(String[] args) in D:\code\tuxedo\Tests\EfBugTest.cs:line 17

However, if I specify the extra column which has an index on then it works fine

Example

            var toBeDeleted = ctx.RiskConsequenceSeverityGroupItems
                .Select(e => new RiskConsequenceSeverityGroupItem { Id = e.Id**, Order = e.Order** });

            ctx.RiskConsequenceSeverityGroupItems.RemoveRange(toBeDeleted);

            ctx.SaveChanges();

Having to specify that extra column seems to be irrelevant to me if all want to do is to delete it. It works even if I assign random numbers to it.

Example

            var toBeDeleted = ctx.RiskConsequenceSeverityGroupItems
                .Select(e => new RiskConsequenceSeverityGroupItem { Id = e.Id**, Order = new Random().Next()** });

            ctx.RiskConsequenceSeverityGroupItems.RemoveRange(toBeDeleted);

            ctx.SaveChanges();
@ralmsdeveloper
Copy link
Contributor

Duplicate #10179

@smitpatel
Copy link
Contributor

Not a duplicate.

@ralmsdeveloper
Copy link
Contributor

Alright, @smitpatel, I thought this could be handled in one.

Both are related to deletion and PK in their descriptions.

@smitpatel
Copy link
Contributor

When saving records to database, we sort them by values in unique index properties which allows you to delete an entity and save another one which would have same value of property participating in unique index. If the records are not sorted properly this can cause exception in database while saving. See #7193

To resolve this, we sort the commands based on unique index properties but in above case, 2 entities are having same values so sorting failed. It should not throw exception like that above.

Whether this should work or throw different exception is something we need to discuss. Based on semantics of database (unique index), the currently tracked entity graph is actually incorrect because 2 entities having same value.

@ajcvickers
Copy link
Contributor

@silarmani Thanks for reporting this and also issues #10179 and #10182. There are some situations where EF currently uses more than just the key information for deletes. How much we want to allow stub entities like this to be used in these situations is something we will discuss as a team. Long term, these kinds of operations are probably best done with set-based operations, as tracked by #795. For now, one direction you could go is to use the FromSql method to write a set-based deletes manually.

@ajcvickers
Copy link
Contributor

Notes from triage: we will make the sorting robust to duplicates. This means that if the delete is happening on its own (and therefore does not require any ordering to succeed) then it will not fail. However, in the general case the values for the unique index must be known or the update pipeline could perform operations in the wrong order, and this will fail. It is up to the developer attaching the stub to understand whether attaching a stub without setting unique index values will be safe. If this cannot be determined, then always make sure these values are set.

@ajcvickers ajcvickers added this to the 2.1.0 milestone Nov 2, 2017
@ajcvickers ajcvickers modified the milestones: 2.1.0-preview1, 2.1.0 Jan 17, 2018
@ajcvickers ajcvickers modified the milestones: 2.1.0, Backlog Jan 26, 2018
@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 Apr 23, 2018
@ajcvickers ajcvickers modified the milestones: Backlog, 2.1.0 Apr 23, 2018
@AndriySvyryd AndriySvyryd removed their assignment Jun 6, 2018
@ajcvickers ajcvickers modified the milestones: 2.1.0-rc1, 2.1.0 Nov 11, 2019
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. type-bug
Projects
None yet
Development

No branches or pull requests

5 participants