Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/add controller validation #69

Merged
merged 7 commits into from
Aug 1, 2021
Merged

Feat/add controller validation #69

merged 7 commits into from
Aug 1, 2021

Conversation

gabriellopes00
Copy link
Contributor

@gabriellopes00 gabriellopes00 commented Jul 30, 2021

I've working on controllers validation implementation, as proposed on issue #52. I created a validation module inside infra/http, that contains the validators which can be used by many controllers, once them purpose is general. For now, i'v only implemented the validators in RegisterUserController, so i created two validators. One is the most important for all controllers, is the RequiredFields validator, that receives an object with expected properties (request body expected data), and if any expected property comes with undefined or null value, it returns an error. The second is the CompareFields validator, which receives two fields, and compare if they're equal (in case of RegisterUserController it was used to compre password and password_confirmation).

So was created an interface

interface Validator<T = any> {
  validate(data: T): Either<Error, null>
}

All the validators must implement this interface, once the controllers now will depends of any class that implements it. But once the controller maybe needs more than one validator, i created a validator compositor, which receive others validators, and call the validate method of each of them. For example, in RegistrationUserController is needed to validate required fields in request body, and compare password with password_confirmation, so the validation compositor of RegisterUserController receives an RequiredFieldsValidator and a CompareFieldsValidator, and use this two validators to validate request body.

The validation compositor looks like this

class ValidatorCompositor<T = any> implements Validator<T> {
  constructor(private readonly validators: Validator<T>[]) {}

  validate(input: T): Either<Error, null> {
    for (const validator of this.validators) {
      const error = validator.validate(input)
      if (error !== null) return error
    }

    return null
  }
}

This way, if any of the validators received by the validation compositor returns an error, this error will be passed on to the controller, and finally an error 400 will be returned to the client.

I also created tests for each of the validators, and one more test in RegisterUserController, to verify if the controller actually returns 400 on validation fail.

What do you think ? @diego3g If you approve this implementation, then we can work on implementing the other controllers.

@diego3g
Copy link
Owner

diego3g commented Jul 31, 2021

@gabriellopes00 Thanks a lot for the contribution, I'm taking a deeper look into de implementation but I really think it's great!

@diego3g diego3g self-requested a review July 31, 2021 13:24
@diego3g diego3g linked an issue Jul 31, 2021 that may be closed by this pull request
@diego3g
Copy link
Owner

diego3g commented Jul 31, 2021

Don't you think we could use this validation logic in another places as the Kafka handlers? I think we can move it to outside of the http folder. I think we could put it inside the src/core

@gabriellopes00
Copy link
Contributor Author

Don't you think we could use this validation logic in another places as the Kafka handlers? I think we can move it to outside of the http folder. I think we could put it inside the src/core

Sure, why not ? The main idea is creating a generic validation, and apply to components which requires one... maybe gRPC handlers, GraphQL resolvers... Kafka handlers would be a great use-case ! Once you have the expected handler data format, only inject the validator as a dependency in the handler, and use it to validate the messages (or any other data).

@gabriellopes00
Copy link
Contributor Author

@diego3g Dude, i provided suggested changes on the validator method, changing the verification to:

if ( data[field] === null || data[field] === undefined || (typeof data[field] === 'string' && data[field].trim() === '')) {
        return left(new MissingParamError(field))
}

And i refactored the validation implementation inside RegisterUserController to pass the entire request to validator, and the final result is here:

  async handle(request: RegisterUserControllerRequest): Promise<HttpResponse> {
    try {
      const validationResult = this.validator.validate(request)

      if (validationResult.isLeft()) {
        return clientError(validationResult.value)
      }

      const { name, email, password } = request

      ...

What dou you think ?

@diego3g
Copy link
Owner

diego3g commented Jul 31, 2021

Hey @gabriellopes00, nice job!

So, let's move the generic validator files outside the http folder?

I think they should be at:

  • src/core/validation/* (eg. src/core/validation/Validator.ts)

Besides that, you can go on and create the validators for all the controllers and Kafka handlers :)

@gabriellopes00
Copy link
Contributor Author

Sure, i think we should move the validator interface only to src/core/validation/Validator.ts, and the implementations, we should put in src/infra/validation/*, this way we can share the implementation (if needed) between http controllers and Kafka handlers. Right ?

@diego3g
Copy link
Owner

diego3g commented Jul 31, 2021

I Agree.

@gabriellopes00
Copy link
Contributor Author

Sure.

@gabriellopes00
Copy link
Contributor Author

I refactored the code... now the validator interface is at src/core/infra/Validator.ts and the validators implementation are at src/infra/validation/*. So we won't have many problems to implement them in the Kafka handlers or in others components if needed.

@diego3g
Copy link
Owner

diego3g commented Jul 31, 2021

That's great! Will you create the rest of the validators for the other controllers as well? If not, i'm gonna merge this PR and we create them later.

@gabriellopes00
Copy link
Contributor Author

Allright, in my opinion you can merge this PR for now. And as soon as i implement the rest of the validators in the others controllers, i'll open new PRs (as well as when applying validations to Kafka handlers). Thanks very much 👊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Controller Validations
2 participants