jaduff / LabLog

Software for recording physical users of computers in a lab environment.
MIT License
0 stars 1 forks source link

Handling Validation #35

Closed jaduff closed 6 years ago

jaduff commented 6 years ago

Asp.Net mvc handles validation as part of model binding (https://docs.microsoft.com/en-us/aspnet/core/mvc/models/validation). But this involves the read model, not the write model (unless I bind the Write model to the controller?)

Should I be handling validation in the Read Model using model binding? Should I be handling validation in the Write Model using exceptions?

Should I be doing both? Handling more validation in the Read, and then sending exceptions for anything exceptional from the Write? Would this pattern be more similar to the idea of a separate server-side api and a front-end app disconnected from it? The front-end would give immediate validation for simple rules, while the back-end would give validation based on internal consistency.

jaduff commented 6 years ago

The general response seems to be that it all sucks, and no resolution is perfect (amongst others, http://enterprisecraftsmanship.com/2016/09/13/validation-and-ddd/). Is DDD predominantly a thing used on big, complex projects, requiring that the back-end and front-end are coded by different people? I could easily see a situation where each team does what they think is best, the other team be damned! (or is that me projecting experiences in education?)

jaduff commented 6 years ago

Another article was talking about the issue of validation in the Domain. What happens if the validation rules need to be changed? When you next replay events, your domain entity is passing through states which it now considers invalid, and may finish in an invalid state. It advocated for allowing this to happen, and instead handling validation outside of the domain. (http://jeffreypalermo.com/blog/the-fallacy-of-the-always-valid-entity/)

simonjduff commented 6 years ago

The Read Model isn't involved. It has to be valid by definition (otherwise the command wouldn't have been accepted by the write model). Personally I'd consider throwing an exception from the write model if it tries to enter an incorrect state, and then use that to handle feedback to the front end. But try whatever makes sense :) Yes DDD and CQRS are better for large scale projects, but also for good modelling :)

jaduff commented 6 years ago

If the Write Model throws an exception, it would need to include sufficient information that could be used to prompt the user with the nature of the problem (password too short? etc), and ideally, which field it relates to so that it can be highlighted in the UI. To be able to implement that, the Controller would need to be tightly coupled to the WR so it could match exception variables to form fields. Is this something I should be avoiding? I suppose you could have a system of ILabExceptions, with a mirror of the various types of LabEvents. Then they could have a body which contains the same variables expected to create the event. Sounds like a lot of places changes to the Domain would need to be made in though. Throwing an exception saying there is an invalid state would be easy. Handling it sensibly in the UI appears not so simple.

jaduff commented 6 years ago

Or perhaps utilise the events themselves? Return an event, but with an additional ModelState property. Then the domain could include the event in the Exception. And given the ReadModel is already aware of the WM (because it is capable of replaying WM events to generate the RM), it doesn't increase the coupling much more than already has been done.

simonjduff commented 6 years ago

You can create custom exceptions to describe a problem. Eg DuplicateComputerException RoomDoesntExistException

jaduff commented 6 years ago

Ah! Ok I can see how that would work, as long as the exception means I still have access to the original form data to regenerate the view with the user's previous input (nothing worse than a validation error and a blank form!). I'll poke it and see what happens :)

jaduff commented 6 years ago

What would happen with multiple validation errors? Presumably only a single exception can be thrown? It'd be better to notify the user once, rather than have to sequentially correct validation errors one at a time.

jaduff commented 6 years ago

How about a ComputerException, which would include a list of the various possible exceptions, and the controller would just have to iterate through and apply them.

simonjduff commented 6 years ago

normally you'd only have one exception. To have multiple you'd need to catch the first, store it, continue processing, and then raise one at the end with all the others. But that seems weird.

jaduff commented 6 years ago

Its not weird from a UI point of view. If I have 5 fields all of which have failed validation, I'd like to know that all at once, not by fixing one, pressing submit, and moving to the next. The only way to avoid the list of errors would be to have so many exceptions that they cover all the permutations of validation failure! I've almost got something that feels like it might work.

simonjduff commented 6 years ago

Validating the input, and validating the model are two different things. You can definitely validate multiple input fields (does the email address exceed the maximum length of the specification, does the firstname contain the complete works of shakespeare etc), but the model can really only have a single logical failure at a time

jaduff commented 6 years ago

How would I include it in testing though? Surround the whole await CTest thing in a try catch block? Some kind of assert that .then shouldn't ever run, and if it does its a fail? Another assert in the catch that checks the exception type?

jaduff commented 6 years ago

Validating the input separately from the domain validation means duplicating the validation logic. Isn't this something to avoid? Or is it a worthwhile trade-off?

simonjduff commented 6 years ago

You can Assert.Throws()

simonjduff commented 6 years ago

No duplication. One is about invalid state in business logic, the other is about the shape of the input. The two are related but distinct I feel

jaduff commented 6 years ago

See PR #36

jaduff commented 6 years ago

In the following code:

await CTest<WriteSchoolContext>
                .Given(a => a.School())
                .When(i => i.AddARoom("Test Room"))
                .And(i => i.Context.Delayed = () => i.AddARoom("Test Room"))
                .Then(t => 
                {
                    try
                    {
                       t.Context.Delayed();
                    }
                    catch (LabException ex)
                    {
                        Assert.Throws<UniqueRoomNameException>(() => ex.NextException());
                    }
                })
                .ExecuteAsync();

If t.Context.Delayed() doesn't result in an exception, then the assert in the catch will never be run, and therefore there won't be a test failure when there should be. If I add an assert.throw around the t.Context.Delayed();, then it handles the exception, and the assert in the catch still won't run. How can I test the layers of exceptions? Or is the second layer of exceptions totally unnecessary anyway? Would there be any downside to simply returning a text string of the name currently given to the exception and handling it in a switch statement?

jaduff commented 6 years ago

Something to do with InnerException?

jaduff commented 6 years ago

Ok, just shoved an Assert.False(true) after the line that should have caused the exception. If there's no exception, then it asserts that true is false and fails.