PrestaShop / docs

PrestaShop technical documentation
https://devdocs.prestashop-project.org/
Other
117 stars 466 forks source link

CQRS validation #905

Open velorb opened 3 years ago

velorb commented 3 years ago

I have question about your approach to validation. I've discovered validation rules in: forms, commands before dispatch, command handlers and entities (eg. when add product to cart). This includes a lot of repetition.

Can anyone describe how validation should be performed?

I read about future plans to move all backend/frontend presentation to vue. How symfony forms will be connected with vue? Does creating symfony forms make sens with vue in mind? Shouldn't form validation will be performed in commands?

matks commented 3 years ago

I've discovered validation rules in: forms, commands before dispatch, command handlers and entities (eg. when add product to cart). This includes a lot of repetition.

Hello, this is wanted. These are all different levels of validation, and you might want to handle them differently.

Form level

We want to validate the data submitted in the form is correct by itself. For example if a form input expects Yes or No and user submitted A we want to reject the data 😄 . This first level of validity aims to validate "user filled and submitted the form correctly"

If you catch an issue at form level you can tell user using an error message attached to the input used 👍 .

So we dont look at "is the data valid" but rather "is the form submitted correctly, according to its settings and validation rules?"

Before dispatch ; Data validation

At this level we aim to validate, when possible, the data itself. Not the form, we forget about the form. For example if user has submitted a Country ISO code that does not exist, we can detect it here. We also check data type. However we try not to perform too many SQL queries so sometimes we validate a data later if it's very expensive to do it in this level.

We do this validation because, as much as possible, we want to NOT dispatch a command with not valid data. If we can detect data is not valid before dispatch, we try to do it here.

If you catch an issue at data level you can tell user using an error message on the BO page 👍 . You dont know what form input submitted it but at least you can tell merchant "hello ! we did NOT performed your action because some data is not valid"

So we look at "is the data valid" but not the business rules.

Command handlers

At this level we aim to validate business rules, we WANT to perform the action required by the user. But when performing the action, we might find issues, and you can only find them when performing heavy SQL statements. For example I have a Cart ID: 8278. I perform getCartById($myId); but it fails and returns a boolean. So I cannot perform the action.

If you catch an issue at command handler level, you have to throw an Exception. This is less user friendly althought you can display them nicely by catching them at controller level. Also maybe some data has been modified then process was halted so it's bad we found the data is not valid so late.

This is where the business rules should be validated 😊 .

How catching business exceptions

We catch Exceptions at controller level, look at https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Controller/Admin/Sell/Catalog/CategoryController.php#L291

        try {
            $categoryForm->handleRequest($request);

            $handlerResult = $categoryFormHandler->handleFor((int) $categoryId, $categoryForm);

            ...
        } catch (Exception $e) {
            $this->addFlash('error', $this->getErrorMessageForException($e, $this->getErrorMessages()));
        }

In the catch we look at exception type and print the right error message for it

Entities

This last level is there to prevent data integrity issues to happen. Like attempting to store a string into a integer column. This is something that, if all data validations before were done correctly, should almost never happen. It's like the very last protection against nasty data being persisted.

If you catch an issue at entities level, you have to throw an Exception. I dont think we catch them so it just returns a nasty error webpage. Because something very scary happened, so you want to tell user in a very unambiguous way.

The earlier you catch a validation issue, the better it is to show it to user.

forwarding messages from them to symfony form supports async command handling?

We dont do this 😢 we dont map business exceptions to form right now. We display flash messages. It could be improved by parsing business exceptions and mapping them to "which field in the form is related" but that is a work we dont do. You can do it 😉 in a module.

I read about future plans to move all backend/frontend presentation to vue. How symfony forms will be connected with vue? Does creating symfony forms make sens with vue in mind? Shouldn't form validation will be performed in commands?

I will answer later as I already needed 30 minutes to write the above 😄 . Hope you liked the 1st part.

velorb commented 3 years ago

Thanks for the first part, that is what i expected. I'm looking forward for the next part about separate frontend/backend and validation handling.

We catch Exceptions at controller level, look at...

I saw it, but one of CQRS strength is async command handling. I mean, that there can be situation where command is dispatched not immediately after registration (e.g. due performance reasons). Command is registered and after some time another process (eg. through queue) handles command. Then you can't catch exception in controller because request has already finished. Did you think about this type of command execution or maybe you do not plan handling async commands?

Why did you use CQRS? In this scenario (sync handling) cqrs only structure query/command but that is all. What about using standard DDD application services instead of command handlers?

We dont do this cry we dont map business exceptions to form right now. We display flash messages. It could be improved by parsing business exceptions and mapping them to "which field in the form is related"

How do you see that mapping, based on what?

matks commented 3 years ago

Why did you use CQRS? In this scenario (sync handling) cqrs only structure query/command but that is all. What about using standard DDD application services instead of command handlers?

You are right: a lot of people use CQRS to either use a Read model and a Write model that are different, and to use async operations for Commands.

But we did not 😊 . Everything is explained in https://build.prestashop.com/news/from-legacy-to-future-architecture-connecting-the-dots/

If you don't want to read the full blog post, the quick answer is: we use CQRS because it's a pattern that

In the end we use CQRS for the benefits it brings to the architecture (meaning: how we organize the PHP code) rather than the benefits it could give in term of async capabilities.

Legacy PrestaShop architecture was just a big MVC. We want to split it into a domain and an application, a domain that must remain as much as possible isolated and unmodified (only extended) and an application that, on the contrary, we WANT people to override/extend and even throw away to replace it with their own (think about SPA and PWA).

We dont do this cry we dont map business exceptions to form right now. We display flash messages. It could be improved by parsing business exceptions and mapping them to "which field in the form is related"

How do you see that mapping, based on what?

I have not thought about it. I'm afraid the 1st idea that comes to mind is basic map, an array that says "exception of type X, when error code is Y, are injected next to form input Z" 😅 nothing smart, just hardcoded logic.