async-validator icon indicating copy to clipboard operation
async-validator copied to clipboard

Incorrect operation of the string validator with a restriction on the length of the string

Open mahovich opened this issue 5 years ago • 4 comments

When executing a validator to validate data

const Validator = require('async-validator').default;

const validator = new Validator(
  { name: { type: 'string', max: 30 } },
);

(async () => {
  const errorsData = await validator.validate(
    { name: 55 },
    { firstFields: true },
  ).catch(({ errors }) => errors);
  
  console.log(errorsData);
})();

the result will be like this:

// [
//   { message: 'name is not a string', field: 'name' },
//   { message: 'name cannot be greater than 30', field: 'name' }
// ]

There are 2 errors here:

  1. There should be one error, not two, because the validator is set to output only one error for one field (firstFields: true)
  2. Errors 'name cannot be greater than 30' should not be, because it has nothing to do with string checking (this error text applies to type: 'number')

mahovich avatar Mar 24 '20 20:03 mahovich

You can give another similar example, which displays two errors instead of one:

const Validator = require('async-validator').default;

const validator = new Validator(
  {
    color: {
      type: 'string',
      len: 7,
      pattern: /^#[0-9a-fA-F]+$/,
    },
  },
);

(async () => {
  const errorsData = await validator.validate(
    { color: 'noname' },
    { firstFields: true },
  ).catch(({ errors }) => errors);
  
  console.log(errorsData);
})();

the result will be like this:

// [
//   { message: 'color must be exactly 7 characters', field: 'color' },
//   {
//     message: 'color value noname does not match pattern /^#[0-9a-fA-F]+$/',
//     field: 'color'
//   }
// ]

mahovich avatar Mar 24 '20 20:03 mahovich

  1. There should be one error, not two, because the validator is set to output only one error for one field (firstFields: true)

Maybe you should change the rules to an array, firstFields will take effect, like this:

const validator3 = new Validator(
    // { color: { type: 'string', pattern: /^\d+$/, max: 5 } },
    { color: [{ type: 'string' }, { pattern: /^\d+$/ }, { max: 5 }] },
);

(async () => {
    var errorsData = await validator3.validate(
        { color: 'noname' },
        { firstFields: true },
    ).catch(({ errors }) => {
        console.log(errors)
    });
})();

the result will be like this:

[
  {
    message: 'color value noname does not match pattern /^\\d+$/',
    field: 'color'
  }
]
  1. Errors 'name cannot be greater than 30' should not be, because it has nothing to do with string > checking (this error text applies to type: 'number')

The range check depends on the type of data passed in. Here you pass in a numeric type. I think message is reasonable.

{ message: 'name cannot be greater than 30', field: 'name' }

It is also recommended to use custom messages to avoid ambiguity.

ygj6 avatar Mar 26 '20 07:03 ygj6

Maybe you should change the rules to an array

I believed that with the setting of firstFields for the rule

{ type: 'string', pattern: /^\d+$/, max: 5 } }

an error should occur after the first validation rule type: 'string' i.e. not for all specified restrictions type: 'string', pattern: /^\d+$/, max: 5

In similar validators that I looked earlier, to get one error for one field, the restrictions were set in one object, and not in an array of objects. I suggest @yiminghe to consider adding a new setting to the library, for example, firstFieldError, which will always return one error for one field.

This will allow:

  • easier to set restrictions - do not use an array to set standard restrictions
  • (oddly enough, but according to my measurements) such a check is about one and a half times more perfomance
const validator = new Validator(
  {
    color: {
      type: 'string',
      len: 7,
      pattern: /^#[0-9a-fA-F]+$/,
    },
  },
);

than the same array of checks

const validator = new Validator(
  {
    color: [
      { type: 'string' },
      { len: 7 },
      { pattern: /^#[0-9a-fA-F]+$/ },
    ],
  },
);

mahovich avatar Mar 28 '20 19:03 mahovich

  1. Errors 'name cannot be greater than 30' should not be, because it has nothing to do with string > checking (this error text applies to type: 'number')

The range check depends on the type of data passed in. Here you pass in a numeric type. I think message is reasonable.

{ message: 'name cannot be greater than 30', field: 'name' }

For simple (standard) checks, it is convenient to use standard error texts. Imagine the user’s surprise if he gets such an error (related to a number) for a text field.

The setting suggested above (firstFieldError) would help solve this problem.

mahovich avatar Mar 28 '20 20:03 mahovich