jonsequitur / Its.Cqrs

A set of libraries for CQRS and Event Sourcing, with a Domain-Driven Design flavor.
Other
77 stars 21 forks source link

specify throwOnValidationFailure when deriving from Command<T> #202

Open SurajGupta opened 7 years ago

SurajGupta commented 7 years ago

Unless I'm missing it, there doesn't seem to be a great way throw if CommandValidator produces an invalid report without first evaluating Validator (validating the command against the aggregate). It seems that I would need to override ApplyTo and thus have to maintain much of the same code in Command<T>. It would be great if throwOnValidationFailure was an override-able property on Command<T> instead of an optional method parameter on various internal methods. But again, I could just be missing something.

SurajGupta commented 7 years ago

To expand a little bit on my need - I have a command with a property (lets call it Dimensions). If Dimenions is null, then CommandValidator will generate an error.

var dimensionsIsNotNull = Validate.That<MyCommand>(cmd => cmd.Dimensions != null)
                    .WithErrorMessage("Dimensions must be specified.");
...

I also need to validate Dimensions against the aggregate. I believe this requires me to check (again) that that Dimensions is not null? Otherwise, I might get a NullReferenceException as I build-up other validations. It seems I can save the effort and, as a blanket rule, never evaluate Validator if CommandValidator results in errors (particularly if I don't really care about the extra information I get by by evaluating Validator if the command itself is not valid)

jonsequitur commented 7 years ago

I'd like to understand the scenario a little better. Is the target audience for this error the developer or the customer? In other words, would cmd.Dimensions being null be a coding error or an input error?

SurajGupta commented 7 years ago

It would be a coding error. The audience is the developer.

jonsequitur commented 7 years ago

The way I've handled this typically is to make one rule dependent on another using When. The following example shows how you can do this both within rules of the same type as well as across types.

 public class MyCommand : Command<MyAggregate>
    {
        private static readonly ValidationRule<MyCommand> dimensionsIsNotNull =
            Validate.That<MyCommand>(cmd => cmd.Dimensions != null)
                    .WithErrorMessage("Dimensions must be specified.");

        private static readonly ValidationRule<MyCommand> dimensionsCountIsAtLeast1 =
            Validate
                .That<MyCommand>(cmd => cmd.Dimensions.Count > 0)
                .WithErrorMessage("There must be at least one dimension.");

        private readonly ValidationPlan<MyCommand> commandValidator = new ValidationPlan<MyCommand>
        {
            dimensionsIsNotNull,
            dimensionsCountIsAtLeast1.When(dimensionsIsNotNull)
        };

        public Dimensions Dimensions { get; set; }

        public override IValidationRule CommandValidator =>
            commandValidator;

        public override IValidationRule<MyAggregate> Validator =>
            new ValidationPlan<MyAggregate>
            {
                // rules validating the command against the aggregate
            }.When(_ => commandValidator.Check(this));
    }
SurajGupta commented 7 years ago

Hi Jon - That's clever! I like that this approach makes it very clear that the aggregate validation depends on the command validation.

Unfortunately this is a significant amount of work to restructure for this approach in a codebase with lots of commands (breaking-out each command validation rule into a private property, using those properties in the command validator, and then adding When lambda in the aggregate Validator).

It seems nice to have the flexibility to control this at ApplyTo-time. I think the only reasonable ways to do this would be to elevate throwOnValidationFailure to a new parameter on the ApplyTo methods OR create a virtual property on the Command{T} object and use that to drive the decision of whether to throw. I can see the objection to both approaches: it pollutes the object/methods with a low-ish level knob. But I'd like to hear your thoughts...

jonsequitur commented 7 years ago

I noticed while looking more closely that throwOnValidationFailure is actually only ever set to false. I don't remember what refactor left it this way but it might be better to remove it at this point.

One of the ideas earlier in the project was for Its.Domain.Api to source aggregates only when the CommandValidator returns no failures. In your case, would changing to this behavior be a reasonable thing to do?

One possible downside to this would be that user-facing errors might be hidden. For example, a user interacting with a UI backed by Its.Domain.Api might see a validation error for the command, resubmit, and then see a validation error from the aggregate that they might have been able to be shown previously. But this may not be a concern, or could be offset by a behavior change at the Its.Domain.Api layer.

Thoughts?

SurajGupta commented 7 years ago

I think it's a good idea to remove throwOnValidationFailure if it's not a real knob. It tripped me up to see it there because it implied that that was something that could be controlled, but I couldn't see how it was possible to do so.

I'm not familiar with the Api project; thus far the system I've been coding has references to Its.Domain and Its.Domain.Sql only. So I'm not certain what the proposal/earlier-idea would entail. I had thought that the only way to inflate an aggregate (is this what you mean by "source aggregates") was from an event stream. I can say more about my usage pattern and maybe it will help...

The aggregate itself plays a pretty small role overall. The only reason my aggregate gets inflated is for the purpose of applying commands. I don't ever inflate an aggregate for reporting or querying (I rely on events and projections for that). That's not to say that the aggregate isn't important, in fact we have zero tolerance for loss of events - every aggregate that has ever been created must be re-create-able for the lifetime of the company. The majority of my commands are back-end generated and reactive to events that are created as the result of applying other commands. So it's very unlikely that applying a command results in a validation failure (in fact, I've never actually seen it happen). I only validate as a safe-guard against dev/coding-errors and because we have lots of commands with a non-trivial state machine for the aggregate - so just to keep things safe. If the command itself were invalid, it should be apparent what went wrong from the validation messages. Validating the command against the aggregate is unlikely to add any more useful information. In fact, it might just add more noise on the channel because a malformed command means higher probability of a malformed aggregate.

...not sure if all that helped =)

Could you control this in the Api project using the Domain project as-is? Perhaps there are other knobs in Domain that I'm not aware of...