lukejagodzinski / meteor-astronomy-validators

https://atmospherejs.com/jagi/astronomy-validators
MIT License
11 stars 13 forks source link

or validator gives wrong error message #16

Open banglashi opened 8 years ago

banglashi commented 8 years ago

Hi @jagi

I added a or validator like you show in your example

Post.addValidator('title', Validators.or([
    Validators.null(),
    Validators.and([
      Validators.string(),
      Validators.minLength(5),
      Validators.maxLength(15)
    ])
  ]));

Problem is, if the field is filled in with less than 5 chars, I get following error message "The "title" field's value has to be null" Is this the intended behavior?

As I see it it should return the error message of minLength.

I created a repo for you to test here: https://github.com/banglashi/meteer-astronomy-validator-bug

Let me know.

lukejagodzinski commented 8 years ago

The problem is with validation order. The first validator it's going to execute is null and it's not null so why the error appears. The solution (for now) is to reverse the order:

Post.addValidator('title', Validators.or([
  Validators.and([
    Validators.string(),
    Validators.minLength(5),
    Validators.maxLength(15)
  ]),
  Validators.null()
]));

Now, everything should work.

Notice, that when working with forms and having validators order changed, there is no way to receive an error from the null validator. Because, even when the input field is empty it contains 0 length string, what causes validation to skip to the and validator. Of course, if you don't set any value on the field, then you would receive an error for the null validator.

Using this kind of nested validators is hard and error prone. Developers asked me to implement required attribute, so that's what I did for the next release. Now, you can forget about those awkward nested validators. However, I'm not removing nested validators, to be precise :).

In the new version your class will look like:

Post = Astronomy.Class({
  name: 'Post',
  collection: Posts,
  fields: {
    title: {
      type: 'string',
      required: false,
      validators: [
        Validators.string(),
        Validators.minLength(5),
        Validators.maxLength(15)
      ]
    }
  }
});

It will only be validated if the field's value is non empty.

One more important thing is that you may want to treat empty string from the form field as a null value (or lack of value). However, I think it's not the job of package to deal with it. I would rather recommend some check in the event.

var val = t.find('.js-title').value;
if (val.length) {
  t.post.title = t.find('.js-title').value;
}

However, I would not recommend doing so. Check for a null value is rather for a safety reason then for informing user that value can be null or a 5-15 characters long string.