I tweeted about a bug that I recently helped fix in System.ComponentModel.DataAnnotations.ValidationAttribute. As I said, it’s a bug resulting from code I wrote for that class years ago. I was honestly surprised there was any interest in this, but there was! Since I piqued your interest, I thought it only fair that I quench your thirst and show you the details of the bug.

History and Backwards Compatibility

The first implementation of ValidationAttribute had a method with the following signature:

public abstract bool IsValid(object value);

 

Any class that inherited from ValidationAttribute had to override the IsValid method and put the validation logic in place.  This was fine and dandy until .NET 4.0 when I worked on a set of features to introduce context-aware validation attributes using new classes called ValidationContext and ValidationResult.  Using ValidationContext, validation attributes could perform complex business logic using application services or even calls into a database.  With this, we wanted to add an overload to IsValid to allow the following signature:

public abstract ValidationResult IsValid(object value, ValidationContext validationContext);
 

Of course we couldn’t add a new abstract method to a class, as that would break existing implementations.  So instead, we looked into adding the following:

public virtual ValidationResult IsValid(object value, ValidationContext validationContext) {
    ValidationResult result = ValidationResult.Success;

  if (!this.IsValid(value)) {
  string[] memberNames = validationContext.MemberName != null ? new string[] {
            validationContext.MemberName
        } : null;

        result = new ValidationResult(
  this.FormatErrorMessage(validationContext.DisplayName),
            memberNames);
    }

   return result;
}

 

This introduced a new problem: new attributes that want to use the ValidationContext must now override both overloads of IsValid, and that would be rather confusing.  We wanted new attributes to only have to override the ValidationContext-based IsValid overload and add documentation that the old boolean-based IsValid method should not be overridden—changing it from abstract to virtual.  We’d change that method to the following:

public virtual bool IsValid(object value) {
  // Call the ValidationContext-based method and if it's successful, return true
  return this.IsValid(value, validationContext: null) == ValidationResult.Success;
}

 

This is where I got in the code before I introduced the bug.  We’ll cover that next.

Ensuring One Overload is Overridden

This is an unusual situation.  We want to introduce a new overload that calls into the original method if it’s overridden.  But we want to make the original method virtual and have it call into the new overload if it’s overridden.

Let’s state that again, because it can be confusing:

  1. If the original method is overridden, have the new overload’s base implementation call into it
  2. If the new overload is overridden, have the original method’s base implementation call into it

A third way of stating it is:

  1. Allow implementers to override either method
  2. Call into the overridden method from whichever base implementation remains

Needless to say, there’s a risk of a circular reference that we need to prevent too.  The way I solved this was to use a private field and a lock to track the state of whether a base implementation was in the middle of making a call to an overridden implementation.  You can see this in the .NET Framework reference source for the System.ComponentModel.DataAnnotations assembly’s ValudationAttribute class.  Here’s the snippet too:

/// <summary>
/// Gets the value indicating whether or not the specified <paramref name="value"/> is valid
/// with respect to the current validation attribute.
/// <para>
/// Derived classes should not override this method as it is only available for backwards compatibility.
/// Instead, implement <see cref="IsValid(object, ValidationContext)"/>.
/// </para>
/// </summary>
/// <remarks>
/// The preferred public entry point for clients requesting validation is the <see cref="GetValidationResult"/> method.
/// </remarks>
/// <param name="value">The value to validate</param>
/// <returns><c>true</c> if the <paramref name="value"/> is acceptable, <c>false</c> if it is not acceptable</returns>
/// <exception cref="InvalidOperationException"> is thrown if the current attribute is malformed.</exception>
/// <exception cref="NotImplementedException"> is thrown when neither overload of IsValid has been implemented
/// by a derived class.
/// </exception>
#if !SILVERLIGHT
public
#else
internal
#endif
virtual bool IsValid(object value) {
  lock (this._syncLock) {
  if (this._isCallingOverload) {
  throw new NotImplementedException(DataAnnotationsResources.ValidationAttribute_IsValid_NotImplemented);
        } else {
  this._isCallingOverload = true;

  try {
  return this.IsValid(value, null) == null;
            } finally {
  this._isCallingOverload = false;
            }
        }
    }
}

#if !SILVERLIGHT
/// <summary>
/// Protected virtual method to override and implement validation logic.
/// <para>
/// Derived classes should override this method instead of <see cref="IsValid(object)"/>, which is deprecated.
/// </para>
/// </summary>
/// <param name="value">The value to validate.</param>
/// <param name="validationContext">A <see cref="ValidationContext"/> instance that provides
/// context about the validation operation, such as the object and member being validated.</param>
/// <returns>
/// When validation is valid, <see cref="ValidationResult.Success"/>.
/// <para>
/// When validation is invalid, an instance of <see cref="ValidationResult"/>.
/// </para>
/// </returns>
/// <exception cref="InvalidOperationException"> is thrown if the current attribute is malformed.</exception>
/// <exception cref="NotImplementedException"> is thrown when <see cref="IsValid(object, ValidationContext)" />
/// has not been implemented by a derived class.
/// </exception>
#else
/// <summary>
/// Protected virtual method to override and implement validation logic.
/// </summary>
/// <param name="value">The value to validate.</param>
/// <param name="validationContext">A <see cref="ValidationContext"/> instance that provides
/// context about the validation operation, such as the object and member being validated.</param>
/// <returns>
/// When validation is valid, <see cref="ValidationResult.Success"/>.
/// <para>
/// When validation is invalid, an instance of <see cref="ValidationResult"/>.
/// </para>
/// </returns>
/// <exception cref="InvalidOperationException"> is thrown if the current attribute is malformed.</exception>
/// <exception cref="NotImplementedException"> is thrown when <see cref="IsValid(object, ValidationContext)" />
/// has not been implemented by a derived class.
/// </exception>
#endif
protected virtual ValidationResult IsValid(object value, ValidationContext validationContext) {
  lock (this._syncLock) {
  if (this._isCallingOverload) {
  throw new NotImplementedException(DataAnnotationsResources.ValidationAttribute_IsValid_NotImplemented);
        } else {
  this._isCallingOverload = true;

  try {
                ValidationResult result = ValidationResult.Success;

  if (!this.IsValid(value)) {
  string[] memberNames = validationContext.MemberName != null ? new string[] { validationContext.MemberName } : null;
                    result = new ValidationResult(this.FormatErrorMessage(validationContext.DisplayName), memberNames);
                }
  return result;
            } finally {
  this._isCallingOverload = false;
            }
        }
    }
}

You’ll notice a fun detail in that for Silverlight code (we cross-compile this code to .NET and Silverlight), we made the original method internal instead of public, because it was a new class for Silverlight—therefore there was no reason to even introduce the method on the public surface area.  Instead, we’d only have the ValidationContext-based approach.

Locks Are Bad

So this is where the code landed.  I had it code reviewed by about a dozen smart people—all much smarter than me in fact.  We all felt sick to our stomachs about it, but we couldn’t think of a better way to accomplish it.  I got code review sign-off, checked in, and this code has been in place for several years now.

Recently though, our team that helps service older code found that this lock I created is quite the bottleneck when validating a lot of attributes.  I don’t know the specific scenario, but it doesn’t really matter—that lock that I used was a bad idea and it’s causing a performance bottleneck for some customers.  It needed to be fixed.

A Cleaner Approach

A couple of weeks ago, Miguel Lacouture worked up a much cleaner approach to solving this problem that doesn’t use locks.  He asked if I would code review his fix since I had written the original code.  I had some review feedback for him, but his approach seems significantly superior to what I had come up with long ago.  With the feedback I sent him, here’s the proposed new implementation for these two methods:

private bool _hasBaseIsValid = false;
private bool _hasBaseIsValidWithContext = false;

virtual bool IsValid(object value) {
  // Track that this overload wasn't overridden
  if (!_hasBaseIsValid) {
        _hasBaseIsValid = true;
    }

  // That means that the other overload must be overridden
  // And if it hasn't been, then throw a NotImplementedException
  if (_hasBaseIsValidWithContext) {
  throw new NotImplementedException(DataAnnotationsResources.ValidationAttribute_IsValid_NotImplemented);
    }

  // We know the other overload was overridden
  // So call it to produce the result
  return this.IsValid(value, null) == null;
}

virtual ValidationResult IsValid(object value, ValidationContext validationContext) {
  // Track that this overload wasn't overridden
  if (!_hasBaseIsValidWithContext) {
        _hasBaseIsValidWithContext = true;
    }

  // That means that the other overload must be overridden
  // And if it hasn't been, then throw a NotImplementedException
  if (_hasBaseIsValid) {
  throw new NotImplementedException(DataAnnotationsResources.ValidationAttribute_IsValid_NotImplemented);
    }

  // We know the other overload was overridden
  // So call it to produce the result
    ValidationResult result = ValidationResult.Success;

  if (!this.IsValid(value)) {
  string[] memberNames = validationContext.MemberName != null ? new string[] {
        validationContext.MemberName
    } : null;

    result = new ValidationResult(
  this.FormatErrorMessage(validationContext.DisplayName),
        memberNames);

  return result;
}

The general idea is that the lock was simply unnecessary because we can easily just record which of the base implementations still exist, and then determine if they both exist.  The lock was guarding against multiple threads calling into one of the IsValid implementations and temporarily flipping a bit to true when we were calling an overload.  But this new approach just turns switches on and if multiple threads come in it won’t cause any problems.

Look Good?

This code is unique—ensuring that 1 of 2 overloads is overridden—and the existing bug has been there for years.  What do you think of the new implementation?  See any holes in it?  Of course, Miguel and the rest of his team will test the heck out of the new implementation, but if you see any issues with it, let me know.

Kudos to Miguel for finding a novel approach to solving the problem.  I wish I’d thought of this approach back when I was writing this code for RIA Services.