Change the Status to Failed when any ValidationError is added to the Result.

[Ewerton]

I'm new using the Result so, first of all: congratulation and thanks for this amazing lib.

I like to develop my methods using the following pattern:

• Instantiate the return of the method, in this case, the Result.
• Run through the method evaluating business rules and adding errors to the Result (if applicable).
• Return the Result at the end of the method

Seems like the Result don't like this approach because it forces me to define the Status at the creation of the Result, like so:

• Result<Customer> result = Result.Success(customer);
• Result<Customer> result = Result.Error("Some Error");
• Result<Customer> result = Result.Invalid(new ValidationError("Some Error"));

Although, I tried to follow the pattern and discovered that I can instantiate the Result without defining the Status using new Result(...) but I found a "strange" behavior, here is the code:

public static void SomeMethod()
{
    Customer customer = new Customer() { Name = "", Email = "some@email.com" };
    Result<Customer> result = new Result<Customer>(customer); // I can instantiate a Result without defining the status and I know that the default is *Ok*
    var valueBefore = result.IsSuccess; // Correct. I didn't added any error yet
    result.ValidationErrors.Add(new ValidationError("Error 1"));
    var valueAfter = result.IsSuccess; // Incorrect. I added an error, so it should be false
}

I don't know if this is a bug but it would be more concise if the Status get changed to something like Invalid ou Error when any ValidationError is added to the Result. Is it possible or plausible?

[ardalis]

Error is meant for exceptions typically, so if you add any validation errors I would expect the status to update to Invalid. That certainly seems like something we could add without too much trouble (and hopefully without breaking things for any existing users). It would be weird (to me) for someone to create a Success result and also add validation errors to it while expecting it to remain a Success, but I obviously can't see how everyone is using this library.

I'd be open to changing the behavior to that when adding a validation error the status changes to invalid, but I'm also not sure the current design will easily support that. I unfortunately broke my own advice and did not encapsulate the validation errors property - it's just an exposed List<ValidationError> which means there is no way as it stands currently to hook into any of the List methods (like Add or Clear).

To implement this change we would need to:

• Refactor ValidationErrors to be a custom ValidationErrorList : List<ValidationError> type and use that for the property
• When creating the custom collection, pass a reference to the Result instance into its constructor
• Hook into the Add method and have it check the parent Result type's status
• Update the status to Invalid if the status is Success

// Result.cs currently
        public List<ValidationError> ValidationErrors { get; protected set; } = new List<ValidationError>();

Does that make sense and do you see a simpler way to do it?

[Ewerton]

I was hard thinking about this in the last days and I always ended-up asking myself: Why I can change the ValidationErros but can't change the Errors property? What if I could? What would be the implications? I always tend to like the conciseness of the libraries, I mean, the library should offer a standardized way of instantiating and interacting with it kind of "guiding" the user and thus, providing some "intuitiveness". It made me think that, for the Result lib, we could think in two scenarios:

1 Only allow the user to instantiate it from the static constructors

• The user would not be able to use new Result() it only would be available static constructors like Result.Success(), Result.Invalid, etc
• The user would not be able to modify properties like Errors, ValidationErrors, Success, etc

PROS

• Very concise and simple way to interact with the lib.

  CONS

• Not flexible, to address my initial requirement user will need to store validation errors in some other variable and pass it to the Result.Invalid(validationErrors) constructor.

2 Allow the user to instantiate it the way he likes

• The user would be able to instantiate it with new Result() or via the static constructors.
• The user would be able to modify properties like Errors, ValidationErrors, Success, and the Status property would be updated accordingly.

PROS

• Very flexible. The user can choose to use the lib the way he likes.

  CONS

• If the user Add Errors and ValidationErrors to the result we will need to use some logic to evaluate the Status
  • My suggestion is to evaluate the Status based on the state of the Errors and ValidationErrors properties like the following:

    if (Errors.Count > 0) 
        return ResultStatus.Error;
    else if (ValidationErrors.Count > 0) 
        return ResultStatus.Invalid;
    else        
        return ResultStatus.Ok;

I really like the option 2 and I'm working to get it work like described here.

PS: As I don't know the library in deep, and I don't know how breaking this second option would be. Right now my implementation does not break any test of the solution.

[KyleMcMaster]

Error is meant for exceptions typically, so if you add any validation errors I would expect the status to update to Invalid. That certainly seems like something we could add without too much trouble (and hopefully without breaking things for any existing users). It would be weird (to me) for someone to create a Success result and also add validation errors to it while expecting it to remain a Success, but I obviously can't see how everyone is using this library.

I'd be open to changing the behavior to that when adding a validation error the status changes to invalid, but I'm also not sure the current design will easily support that. I unfortunately broke my own advice and did not encapsulate the validation errors property - it's just an exposed List<ValidationError> which means there is no way as it stands currently to hook into any of the List methods (like Add or Clear).

To implement this change we would need to:

• Refactor ValidationErrors to be a custom ValidationErrorList : List<ValidationError> type and use that for the property
• When creating the custom collection, pass a reference to the Result instance into its constructor
• Hook into the Add method and have it check the parent Result type's status
• Update the status to Invalid if the status is Success

// Result.cs currently
        public List<ValidationError> ValidationErrors { get; protected set; } = new List<ValidationError>();

Does that make sense and do you see a simpler way to do it?

I believe ValidationErrors shouldn't be exposed as a List but as an IEnumerable<ValidationError> or IReadOnlyCollection<ValidationError>. @ardalis has a great article on this here. This would prevent modifying the collection directly on an instance of a Result, keeping Result immutable and semi-functional (like the Map feature). Instead, users who would like to modify the ValidationErrors collection would be encouraged to instantiate a new Result rather than changing state of an existing result. I'm thinking something like this:

var successResult = Result.Success();

var validationErrors = successResult.ValidationErrors.ToList();
validationErrors.Add(new ValidationError { Identifier = "Foo", ErrorMessage = "Invalid Foo for some reason" });

var invalidResult = Result.Invalid(validationErrors);

This prevents an existing Result from transitioning to a corrupted state on accident, like a successful Result with a ValidationError count greater than zero. Another benefit is this also keeps the library code simple since the Result can stay agnostic of any external manipulation in user code. Just weighing in on some thoughts I had about this.

Edit: Note that in any approach, any changes made to how ValidationErrors are handled should probably be considered a breaking change as well for versioning purposes.

[Ewerton]

Hi @KyleMcMaster

I agree with your opinion, it is somehow what I described in "option 1" above. Like I said, this design is less flexible and restricts the way the user interact with the lib, but it is very straightforward (which is good).

I do not fully agree with you when you say:

This prevents an existing Result from transitioning to a corrupted state on accident, like a successful Result with a ValidationError count greater than zero

We can prevent this corrupted state with proper coding and automated testing like I did in the PR.

Also, I read the article you mentioned and, from my understanding, it is more focused in Domain Driven Design which it is an effective way to design large and complex business rules and, "maybe" not so good to design a utility library as Result.

Current Situation

Right now the lib allows editing the ValidationError but does not allow editing the Errors property. So, we have two options to make the design more concise:

1. Restrict the editing of the ValidationErrors property.
2. Accept my PR to allow editing both ValidationErrors and Errors properties.

Both are breaking changes

[ardalis]

I think it was a design error of mine to expose ValidationErrors as just a List. I would opt for your option 1 as my breaking change preference.

[Ewerton]

Ok. Should I implement and submit a PR to turn List<ValidationError> ValidationErrors into IEnumerable<ValidationError> ValidationErrors ?

Also, please reject the previous PR.

[ardalis]

Oops I forgot about this conversation and took in that PR, but we will make another PR here soon to fix everything up.