Should call SaveChanges after deleting a user

[luis-fss]

RavenDB docs states that entities can be marked for deletion by using the Delete method, but will not be removed from the server until SaveChanges is called

Code in question: https://github.com/JudahGabriel/RavenDB.Identity/blob/04bd919a8632e0edfb4496fa04f10453963b15d0/RavenDB.Identity/UserStore.cs#L162

Suggestion:

DbSession.Delete(user.Id); // It's possible user is already saved to the database. If so, delete him.await

DbSession.SaveChangesAsync(cancellationToken);

[JudahGabriel]

We made a deliberate decision to avoid calling .SaveChanges or .SaveChangesAsync, as it breaks composability.

For example, consider a hospital system where you have Patients and Doctors:

public class Patient 
{
   public string Id { get; set; }
}

public class Doctor
{
   public List<string> PatientIds { get; set }
}

If you remove a patient, you'll want to also remove any links to that patient in other documents. We want to do this as a single transaction. Calling .SaveChanges multiple times breaks this into multiple transactions, opening the door for data integrity issues.

This is why you'll find .SaveChanges/.SaveChangesAsync is deliberately left to the app developer. Make your changes, including any business logic changes with adding/removing/updating users, then call .SaveChangesAsync() at the end to have it be a single all-or-nothing transaction.

[luis-fss]

I understand your point, it makes sense, thanks for clarifying.

[jbuedel]

Hi @JudahGabriel. I understand the rational behind giving the controllers responsibility for calling SaveChanges. I don't think I agree with it, but I'm stilling thinking on that. Anyway, I have a question. RoleStore does eagerly save by default, but also exposes AutoSaveChanges which can let RoleStore behave like UserStore and not eagerly save. Is there justification for RoleStore behaving differently by default?

Sort of related: I would suggest that eagerly saving should instead be the default (it's is closer to expected behavior, falling into the pit of the success, etc), but then expose AutoSaveChanges on the options, which could be used to opt in to deferred save. We've implemented something close to this for our project, which I'd clean up and make a pr if it interests you.

Thanks

[JudahGabriel]

I think that RoleStore code existed before from when I inherited the project. I agree it's not consistent.

I'm opened to a PR that makes auto saving the default, with an option to opt-out.