1

I have these entities in my business:

class Team
{
    public int Id { get; set; }
    public string Name { get; set; }
    public List<Member> Members { get; set; }
}

class Member
{
    public int TeamId { get; set; }
    public int UserId { get; set; }
}

The business rule is that a team cannot have more than 10 members.

So you can add a constraint in your controller or handler like this: AddTeamMemberHandler.cs:

var team = dbContext.Teams
                    .Include(x => x.Members)
                    .FirstOrDefault(x => x.Id == 123);

if (team.Members.Count >= 10) 
    throw new Exception("Cannot add more members");

However, when you have multiple users trying to add users at the same time, and the current number of members in the team is 9, all of the request will pass the validation. This can lead to the number of members in the team exceeding 10.

I think it is possible to add a custom SQL constraint in the EF model configuration like this:

modelBuilder.Entity<Team>()
    .HasCheckConstraint("Team_MaxMembers", $"COUNT(\"Members\") <= 10");

But I'm a little bit hesitant to add the business logic validation in the database layer as the constraint can be dynamic depending on the type of the team or the business rule has changed.

Another solution I can think of is using optimistic concurrency control and adding a concurrency token on team. This can prevent multiples users trying to add members at the same time.

Is there a different way to mitigate this sort of issue?

Thanks

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Daniel Jee
  • 664
  • 5
  • 10
  • 1
    IMHO add optimistic concurrency columns to both tables. Force an update of the `Team` entity when you add a member, even if no other changes are made to that record. – Jeremy Lakeman Aug 28 '23 at 01:51
  • @JeremyLakeman Presumably you mean a `rowversion` column? – Charlieface Aug 28 '23 at 02:00
  • Native or application managed, either would work. https://learn.microsoft.com/en-us/ef/core/saving/concurrency?tabs=data-annotations#native-database-generated-concurrency-tokens That way the `.SaveChanges` would only succeed if nobody had changed the selected `team` between the query and update. – Jeremy Lakeman Aug 28 '23 at 02:04

1 Answers1

2

Your proposal of a CHECK constraint does not work. For a start, the syntax isn't even valid. Even if you put it into a scalar SQL function, you would still be left with a host of concurrency and performance issues at least as serious as what you have already.

A much better solution is to use your existing code, but utilize a transaction at a high isolation level, preferably Serializable, which would place a lock preventing any modifications to those rows being read.

using var transaction = await context.Database.BeginTransaction(IsolationLevel.Serializable);

var teamCount = await dbContext.Teams
    .Where(t => t.Id == 123)
    .SelectMany(t => t.Members)
    .CountAsync();
if (teamCount >= 10)
    throw new Exception("Cannot add more members");  // rollback happens automatically with "using"

// etc

await transaction.CommitAsync();

For this to work properly you are going to want indexes on Team.Id (presumably it's the PK anyway), as well as Member.TeamId (you should probably also have this anyway due to FK cascading).

Do note that if your indexing is not set up correctly then you may get severe blocking and deadlocking issues on the table, as well as the locking order not working correctly.


You may still get deadlocking issues, which in SQL Server is normally solved by using a UPDLOCK hint. Locking the Team table should be sufficient. In other DBMSs, a SELECT FOR UPDATE or similar is done.

You can do this in the following way, by creating a custom IQueryable

var teamsLocked = dbContext.Teams.FromSqlRaw(@"
SELECT * FROM dbo.Team WITH (UPDLOCK)
");
var teamCount = await teamsLocked
    .Where(t => t.Id == 123)
    .SelectMany(t => t.Members)
    .CountAsync();
Charlieface
  • 52,284
  • 6
  • 19
  • 43
  • This is definitely a valid solution. However, I prefer optimistic concurrency over placing DB locks. Not only for performance but I'm initializing transaction outside the handler (I'm doing it inside MediatR behavior). – Daniel Jee Aug 28 '23 at 03:17