Codit / practical-api-guidelines

Practical guidelines for building & designing APIs with .NET.
MIT License
16 stars 5 forks source link

We shouldn't delete items in this way. #94

Closed MassimoC closed 5 years ago

MassimoC commented 5 years ago

We shouldn't delete items in this way.

The code above will generate a SQL query that will first retrieve the entity, and then you'll use that entity to delete it. This is not a good solution performance-wise as you'll end up with an unnecessary roundtrip to the DB.

A better solution would be something like this:

var entityToDelete = new Customization(id);
_coditoContext.Customizations.Entry[entityToDelete].State = EntityState.Deleted;
await _coditoContext.SaveChangesAsync();

or

var entityToDelete = new Customization(id);
_coditoContext.Customizations.Attach(entityToDelete);
_coditoContext.Customizations.Delete(entityToDelete);
await _coditoContext.SaveChangesAsync();

_Originally posted by @fgheysels

pietersap commented 5 years ago

I will implement it this way, but then the setter Id of Customization will have to be public, even though it's a primary key. Except if I am missing something? @fgheysels

fgheysels commented 5 years ago

Yes true.

There are ways to work around it thought:

  1. The setter of the ID property remains private and you introduce a method that sets the ID. The end-result is the same, but by giving that method an appropriate name you give an indication that it should not be the intention to just (ab)use that method. For instance
public int Id {get; private set;}

public void SetDbIdentifier( int id )
{
    this.Id = id;
}
  1. You can use reflection to set the value of the Id property.

  2. You can choose to bypass the abstraction that EF offers and execute a raw SQL statement that deletes the entity. This offcourse means that you need to write native SQL (nothing wrong with that), but suppose that you have a product where you target multiple databases, you'll need to take care of that.

dbContext.Database.ExecuteSqlCommand("DELETE FROM ...");

I think I like the latter option the most, but, offcourse, n people, n opinions. Maybe @tomkerkhove can drop his 2 cents here as well.

pietersap commented 5 years ago

Personally I think bypassing the EF abstraction is the least flexible. I investigated the reflection method and I think this is best:


var entityToDelete = new Customization();
Type type = entityToDelete.GetType();
PropertyInfo prop = type.GetProperty("Id");
prop.SetValue(entityToDelete, id, null);
_coditoContext.Customizations.Attach(entityToDelete); 
_coditoContext.Customizations.Remove(entityToDelete);

int numberOfChanges = await _coditoContext.SaveChangesAsync();
return numberOfChanges;