Where to run a duplicate check for an entity

Question

I'm looking for advice on the "best" place to put validation logic, such as a duplicate check for an entity, when using Entity Framework Code-First, in an MVC application.

To use a simple example:

public class JobRole
{
  public int Id { get; set; }        
  public string Name { get; set; }
}

The rule is that the "Name" field must be unique.

When I add a new JobRole, it is easy to run a check in the Job Role Repository that the Name doesn't already exist.

But if a user edits an existing JobRole, and accidentally sets the Name to one that already exists, how can I check this?

The issue is that there doesn't need to be an "update" method on the Repository, as the Job Role Entity will automatically detect changes, so there isn't a logical place to do this check before attempting to save.

I have considered two options so far:

1. Override the ValidateEntry method on the DbContext, and then when the JobRole entity is being saved with EntityState.Modified, run the duplicate check then.
2. Create some sort of duplicate-checking service, called from the Controller, before attempting a Save.

Neither really seems ideal. Using ValidateEntry seems rather late (just before save) and hard to test. Using a Service leaves the possibility that someone forgets to call it from a Controller, letting duplicate data through.

Is there a better way?

Answer 1

Your problem with ValidateEntity appears to be that the validation occurs on SaveChanges and this is too late for you. But in Entity Framework 5.0 you can call the validation earlier if you wish using DbContext.GetValidationErrors. And of course you could also just call DbContext.ValidateEntity directly. This is how I do it:

1. Override the ValidateEntity method on the DbContext:

   protected override DbEntityValidationResult 
                      ValidateEntity(DbEntityEntry entityEntry,
                      IDictionary<object, object> items)
   {
       //base validation for Data Annotations, IValidatableObject
       var result = base.ValidateEntity(entityEntry, items);
   
       //You can choose to bail out before custom validation
       //if (result.IsValid)
       //    return result;
   
       CustomValidate(result);
       return result;
   }
   
   private void CustomValidate(DbEntityValidationResult result)
   {
       ValidateOrganisation(result);
       ValidateUserProfile(result);
   }
   
   private void ValidateOrganisation(DbEntityValidationResult result)
   {
       var organisation = result.Entry.Entity as Organisation;
       if (organisation == null)
           return;
   
       if (Organisations.Any(o => o.Name == organisation.Name 
                                  && o.ID != organisation.ID))
           result.ValidationErrors
                 .Add(new DbValidationError("Name", "Name already exists"));
   }
   
   private void ValidateUserProfile(DbEntityValidationResult result)
   {
       var userProfile = result.Entry.Entity as UserProfile;
       if (userProfile == null)
           return;
   
       if (UserProfiles.Any(a => a.UserName == userProfile.UserName 
                                 && a.ID != userProfile.ID))
           result.ValidationErrors.Add(new DbValidationError("UserName", 
                                 "Username already exists"));
   }
   

2. Embed Context.SaveChanges in a try catch and create a method to access Context.GetValidationErrors(). This is in my UnitOfWork class:

   public Dictionary<string, string> GetValidationErrors()
   {
       return _context.GetValidationErrors()
                      .SelectMany(x => x.ValidationErrors)
                      .ToDictionary(x => x.PropertyName, x => x.ErrorMessage);
   }
   
   public int Save()
   {
       try
       {
           return _context.SaveChanges();
       }
       catch (DbEntityValidationException e)
       {
           //http://blogs.infosupport.com/improving-dbentityvalidationexception/
           var errors = e.EntityValidationErrors
             .SelectMany(x => x.ValidationErrors)
             .Select(x => x.ErrorMessage);
   
           string message = String.Join("; ", errors);
   
           throw new DataException(message);
       }
   }
   

3. In my controller, call GetValidationErrors() after adding the entity to the context but before SaveChanges():

   [HttpPost]
   public ActionResult Create(Organisation organisation, string returnUrl = null)
   {
       _uow.OrganisationRepository.InsertOrUpdate(organisation);
   
       foreach (var error in _uow.GetValidationErrors())
           ModelState.AddModelError(error.Key, error.Value);
   
       if (!ModelState.IsValid)
           return View();
   
       _uow.Save();
   
       if (string.IsNullOrEmpty(returnUrl))
           return RedirectToAction("Index");
   
       return Redirect(returnUrl);
   }
   

My base repository class implements InsertOrUpdate like this:

    protected virtual void InsertOrUpdate(T e, int id)
    {
        if (id == default(int))
        {
            // New entity
            context.Set<T>().Add(e);
        }
        else
        {
            // Existing entity
            context.Entry(e).State = EntityState.Modified;
        }      
    }

I still recommend adding a unique constraint to the database because that will absolutely guarantee your data integrity and provide an index that can improve the efficiency, but overriding ValidateEntry gives loads of control over how and when validation occurs.

Answer 2

The most reliable place to perform this logic would be the database itself by declaring an unique field constraint on the name column. When someone attempts to insert or update an existing entity and try to set its name to an existing name a constraint violation exception will be thrown that you could catch and interpret in your data access layer.

Answer 3

First, your first option above is acceptable. The fact that another dev might not trap for a failure is not a huge drawback. This is always the case with validation: You catch it as early and elegantly as you can, but the main thing is to keep the integrity of your data. Simpler to just have a constraint in the database and trap for that though, right? But yes, you want to catch it as early as possible.

If you have the scope and time, better would be to have a smarter object that would handle saving, and perhaps other things you need handled consistently. There's many ways of doing this. It might be a wrapper for your entity. (See Decorator pattern, though I prefer to have my object consistently have a Data property for access to the entity). It might require a relevant entity in order to be instantiated. Your controller would give the entity to this smart object to save (again, perhaps instantiating this smart object with your entity.) This smart object would know all the validation logic necessary and make sure it happens.

As an example, you could create JobRole business object. ("busJobRole", bus prefix for "business".) It could have a collection DataExceptions. Your Controller takes the JobRole entity posted back, instantiates a busJobRole, and calls a method SaveIfValid, which would return true if the item was saved successfully and false if there were validation problems. You then check the busJobRoles DataExceptions property for the exact problems, and fill your modelstate, etc. Maybe like this:

// Check ModelState first for more basic errors, like email invalid format, etc., and react accordingly.
var jr = new busJobRole(modelJobRole);
if (jr.SaveIfValid == false) {
     ModelState.AddModelError(jr.DataExceptions.First.GetKey(0), jr.DataExceptions.First.Get(0))
}

We follow this model so consistently, and I made an extension method for ModelState to accept a NameValue collection (returned by business objects) (vb.net version):

<Extension()> _
Public Sub AddModelErrorsFromNameValueCollection(
                        ByVal theModelState As ModelStateDictionary,
                        ByVal collectionOfIssues As NameValueCollection,
                        Optional ByRef prefix As String = "")
    If String.IsNullOrEmpty(prefix) Then
        prefix = ""
    Else
        prefix = prefix & "."
    End If
    For i = 0 To CollectionOfIssues.Count - 1
        theModelState.AddModelError(prefix & CollectionOfIssues.GetKey(i), 
                        CollectionOfIssues.Get(i))
    Next
End Sub

this allows for quick, elegant adding of exceptions (determined by business objects) to the ModelState:

ModelState.AddModelErrorsFromNameValueCollection(NewApp.RuleExceptions, "TrainingRequest")

Your worry that other developers might not follow a plan you set up is very valid and good thinking. That's why your scheme needs to be consistent. For example, in my current project, I have two categories of classes that act as I've described. If they're very light, and just deal with caching and validation, they are "data manager" classes (ex: BureauDataManager). Some are true business domain object, very comprehensive, which I prefix with "bus" (ex: busTrainingRequest). The former all inherit from a common base class, to ensure consistency (and reduce code of course). Consistency allows for true encapsulation, for code discoverability, for the right code being in the right (single) place.