poppinss / indicative

Indicative is a simple yet powerful data validator for Node.js and browsers. It makes it so simple to write async validations on nested set of data.
https://indicative.adonisjs.com/
MIT License
417 stars 52 forks source link

Since unset fields are evaluated as null by pope, null should be skippable #225

Closed auloin closed 5 years ago

auloin commented 5 years ago

The prop function from pope evaluates unset fields as null instead of undefined which gives unexpected result in the validation. This happens when in strict mode.

Package version

5.0.8

Node.js and npm version

node: v11.7.0 npm: 6.9.0

Sample Code (to reproduce the issue)

  configure({
     EXISTY_STRICT: true
  });

   const data = {
      email: 'mail'
    };
    const rules = {
      email: 'min:1|email',
      name: `min:1`,
      password: `min:8`
    };
    validateAll(data, rules)
      .then(() => resolve())
      .catch(errors => {
        const formattedErrors = errors.map(error => {
          return { [error.field]: error.message };
        });
        console.log(formattedErrors);
      });

Result

[ { email: 'email validation failed on email' },
  { password: 'min validation failed on password' } ]

At first look you would believe that name is doing what's expected, skipped when undefined but that's not the case. name is evaluated as null which in turn is stringified and becomes "null". This is why password validation is failing without the password being set.

That being said, shouldn't null be considered as skippable in strict mode?

thetutlage commented 5 years ago

I don't see where name is stringified and becomes "null". I understand the point you are making, but not sure where this type conversion is happening?

auloin commented 5 years ago

I don't understand the architecture of the new version yet but here is where it's happening in the old version: https://github.com/poppinss/indicative/blob/5.0.8/src/validations/min.js

get(data, field) => returns null Array.isArray(fieldValue) ? fieldValue : String(fieldValue) => returns "null"

thetutlage commented 5 years ago

Yup, this has been fixed. Also, I have added tests around same to ensure that it is not a problem anymore. https://github.com/poppinss/indicative-rules/commit/b85df9ebb8225eddaf1f0d2c1a415a562b32075e