Replace `RequiredFieldsValidator` with per field validation

[diego3g]

Hey @gabriellopes00, I merged the PR #69 but I think that RequiredFieldsValidator is not a good idea. I think we should add validation per field instead a validator that check for empty values within all request data. Can we fix that?

That's causing our tests to fail as userId is present in the request due to the EnsureAuthenticatedMiddleware but in the register route, this value is null so the tests are failing.

[gabriellopes00]

Hii @diego3g. Sure, now i understand why the tests were failing... and we do have a bug here, but of course we can fix it. Can you explain me better what this validation per field would be ? Will this be useful when applying the validator to Kafka handlers ?

[gabriellopes00]

I was thinking, and this error start happening when we refactored the code to pass the entire request to validator. Because before, we were passing only the fields we expected to be filled { name, email, password, password_confirmation }, and the userId put by the middleware wasn't included . It could be fixed by reverting the changes in the controller and de-structuring the fields before pass them to the validator. What do you think ? But tell me more about the validator you proposed...

[diego3g]

I think we should pass exactly what are the required fields instead of iterating through each request field. Like this:

const validator = new ValidatorCompositor([
  new RequiredFieldValidator('name'),
  new RequiredFieldValidator('email'),
  new RequiredFieldValidator('password'),
  new CompareFieldsValidator('password', 'password_confirmation'),
])

[gabriellopes00]

Sure, this is a good approach, i can implement this... Just a suggestion, what if we create only one RequiredFieldsValidator and pass for it an array with all required fields ? Something like this:

const validator = new ValidatorCompositor([
  new RequiredFieldValidator(['name', 'email', 'password', 'password_confirmation'])
])

This way we don't need to instantiate one validator per field. Imagine a request with a lot of them 🤣 What do you think ?

[diego3g]

I think it's better to have one validator per field so we can pass additional options for each validator, for example:

new RequiredFieldValidator('name'),
new RequiredFieldValidator('email', { message: 'Email is required' }),
new RequiredFieldValidator('password'),
new CompareFieldsValidator('password', 'password_confirmation'),

Or maybe we could change it to work like a schema validation:

const validator = new ValidatorCompositor({
  name: [new RequiredFieldValidator()],
  email: [new RequiredFieldValidator({ message: 'Email is required' })],
  password: [
    RequiredFieldValidator(), 
    CompareFieldsValidator({ compare_with: 'password_confirmation' })
  ],
})

[diego3g]

My only worries here in the validation are about deep nested data like complex objects or arrays, how we would validate like the property street inside this object?

const user = {
  address: {
    street: string;
  }
}

Maybe we could nest ValidatorCompositors?

const validator = new ValidatorCompositor({
  address: new ValidatorCompositor({
    street: new RequiredFieldValidator()
  })
})

Just some thoughts so we can improve this feature.

[gabriellopes00]

Ok, we can create one RequiredFieldValidator per field. But i liked so much the idea of a schema validator. And we could use it to solve the problem of nested fields, doing something like this:

const validator = new ValidatorCompositor({
  name: [new RequiredFieldValidator()],
  email: [new RequiredFieldValidator({ message: 'Email is required' })],
  address: [
    new RequiredFieldValidator({ 
      message: 'Email is required', 
      nested_fields: [  // optional property 
        { name: 'street' } 
      ]
    })
  ],
  password: [
    RequiredFieldValidator(), 
    CompareFieldsValidator({ compare_with: 'password_confirmation' })
  ],
  ...
...
})

In this example, for the RequiredFieldsValidator we pass an array, with the nested fields, in the constructor. If this array isn't empty, we iterate on it, and for each nested field, we validate if it is empty or not.

But the problem is actually solved when we make possible that each nested field can have others nested fields, and we go iterating throught every one validating if it is empty or not. In my mind, each nested field would be something like this:

interface NestedField {
  name: string
  nested_fields?: NestedField[]
}

If we have more than one nested field, we just pass them inside "parent fields":

{
  field: [
    new RequiredFieldValidator({ 
      message: 'This field is required', 
      nested_fields: [ 
        {
          name: 'field2',
          nested_fields:  [
            {
              name: 'field3'
            },
          ]
        } ,
        ...
      ]
    })
  ],
}

It's complex but works 🤣 I tested.

[gabriellopes00]

I created a script, of course isn't ready yet, but take a look:

export class RequiredFieldValidator implements Validator {
  constructor(
    private readonly props: { message: string; nested_fields: NestedField[] } // we pass the properties where when we instantiate a new validator
  ) {}

  public validate(obj: any, field: string): Error { // the validator compositor will pass these properties to the method
    if (this.isEmpty(obj[field])) return new Error(this.props.message) // first we validate if the field is empty or not
    if (this.props.nested_fields) { // if this field has nested fields, them we iterate through each one
      return this.iterate(this.props.nested_fields, obj[field])
    }

    return null
  }

  private iterate(fields: NestedField[], obj: any): Error {
    for (const field of fields) {
      if (this.isEmpty(obj[field.name])) return new Error(this.props.message)
      else if (field.nested_fields) {
        return this.iterate(field.nested_fields, obj[field.name]) // here is the "magic"... if the nested field has others nested fields, we use recursion to iterate through them too
      }
    }

    return null
  }

  private isEmpty(field: any): boolean { // validation logic
    return (
      field === null ||
      field === undefined ||
      (typeof field === 'string' && field.trim() === '')
    )
  }
}

[gabriellopes00]

It's a little bit complex, but works. Is a possible solution to the problem of nested fields. What dou you think ? If you aprove this, i can implement together to the refactor of the RequiredFieldsValidator. But if you think isn't a good idea, we can just solve the bug by using your suggestion of creating a validator per field, and think more about it later.

[diego3g]

I think it's somehow too much complex for now. I prefer the first approach I sent before or if we try using a schema validation library like Joi or Yup.

Maybe it would be better now to use one library like that instead of reimplementing the whole wheel.

[gabriellopes00]

Sure, the first approach is good, solve our problem. I'm going to be working in the implementation ✌

[diego3g]

@gabriellopes00 have u tried using one library like a mentioned before? As these libraries are all only JavaScript i think they would not break our domain layer and we can focus on new features instead of investing so much time in validation.

I think we could have something like:

type FieldValidationError = Record<string, string>

class RegisterUserValidator implements Validator {
  validate(): Either<FieldValidationError, null> {
    // validate using Joi or Yup
    // parse validation error messages to FieldValidationError type
  }
}

[gabriellopes00]

All right, we can use an external library... I've already used yup and validator.js. But do you think we should create one validator by controller ? or handler. Because if we use yup for example, we need to create a data shape by expected request body, right ?

Is a good approach using a external library, but i think we should create generic validators to reuse them in more than one place.

Don't worry, the validators are only applied in external layers, the domain is completely safe.

[diego3g]

@gabriellopes00 I think that for now we should not care about reusability of validators, we can create one validator per controller and in the future, if we see too many repetitions, we try to refactor it. This way we can move on with the validation and work on other parts of the application. What do you think?

I have some other big challenges in the application that I would be glad to have your help 😊

[gabriellopes00]

@diego3g Sure, perfect... let's create the validators per controller (and handlers). I'm thinking about using either yup or validator.js, what do you think ? Both of them have many features that would be very helpful for us. Then we could use the libraries to validate only the fields by controllers, and we don't need to worry about a generic script to handle nested fields, to compare fields, or anything else. What do you think ?

I would be very happy to be able to help with future changes.

[diego3g]

I like Yup or Joi. That's great for me.

[diego3g]

@gabriellopes00 It would be good if you could reach me on Discord so we can keep our conversation more dynamic, what u think?

[gabriellopes00]

@diego3g Yess, of course, that is a great idea. What is your profile in discord ?? If you want to add me, my nickname is gabriellopes#8871