Change the Status to Failed when any ValidationError is added to the Result.
Closed this issue · 7 comments
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?
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 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 likeResult.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
andValidationErrors
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
andValidationErrors
properties like the following:
if (Errors.Count > 0) return ResultStatus.Error; else if (ValidationErrors.Count > 0) return ResultStatus.Invalid; else return ResultStatus.Ok;
- My suggestion is to evaluate the Status based on the state of the
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.
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 theList
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.
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:
- Restrict the editing of the
ValidationErrors
property. - Accept my PR to allow editing both
ValidationErrors
andErrors
properties.
Both are breaking changes
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.
Ok.
Should I implement and submit a PR to turn List<ValidationError> ValidationErrors
into IEnumerable<ValidationError> ValidationErrors
?
Also, please reject the previous PR.
Oops I forgot about this conversation and took in that PR, but we will make another PR here soon to fix everything up.