validate.js icon indicating copy to clipboard operation
validate.js copied to clipboard

Email validation fails on empty or whitespace strings

Open ajenkins-kyr opened this issue 8 years ago • 27 comments

Code example:

var constraints = {
  from: {
    email: true
  }
};
validate({from: ''}, constraints);
// Object {from: Array[1]}
validate({from: ' '}, constraints);
// Object {from: Array[1]}

This seems like a bug because in the documentation it says:

Important! Besides null and undefined being valid empty strings and whitespace only strings are also valid.

My code is also directly from the documentation, but is clearly returning something different.

I'm using [email protected]

ajenkins-kyr avatar Dec 06 '16 20:12 ajenkins-kyr

The reason this matters to me is because I'm trying to build an optional email field. If the user leaves the field blank, that's fine, but if they enter a value, then I want to make sure it's a valid email address.

ajenkins-kyr avatar Dec 06 '16 20:12 ajenkins-kyr

This was working for me correctly as of 0.9.0, but stopped working when I upgraded. Looking at your changelog, I think the issue was introduced in 0.11.0 when you replaced v.isEmpty(value) with !v.isDefined(value). v.isEmpty("") === true, but !v.isDefined("") === false.

ajenkins-kyr avatar Dec 06 '16 20:12 ajenkins-kyr

Also, I tried to get around this by using the constraints:

var constraints = {
  from: {
    format: {pattern: pattern: validate.validators.email.PATTERN}
  }
};

But it seems like many of the validators (not just email) suffer from this bug, including format.

ajenkins-kyr avatar Dec 06 '16 21:12 ajenkins-kyr

I am also stuck on this issue. Not only is this an issue with email, but anytime I define a format which is optional. For example:

var constraints = {
    'street': {
        format: {
            pattern: '^[0-9]+ .+$',
            message: '^The street must be a number followed by a name'
        }
    },
};

If street is an empty string, this validation fails.

Please help.

P.S. I don't know if this has anything to do with the following statement in the docs. I am not sure if I understand what it means or if it is related to the issue that we are seeing:

One thing that is a bit unorthodox is that most validators will consider empty values (null, undefined, whitespace only strings etc) valid values. So for example adding a constraint of at least 6 characters will be like saying If the attribute is given it must be at least 6 characters. This differs from example Ruby on Rails where validators instead have the allow_nil option. I find it quite common that you want to have constraints on an optional attribute.

nareshbhatia avatar Dec 14 '16 00:12 nareshbhatia

Re-reading the docs, I found this:

Changelog - 0.11.0 - Breaking: The presence validator no longer allows empty and whitespace only strings per default. This behaviour can be modified by using the allowEmpty option.

However when I read the the docs on presence, it is not clear what allowEmpty is and how it should be used. In the following examples, presence is set to an object, does it mean that presence is true or false?

validate({input: ""}, {input: {presence: {allowEmpty: true}}});
validate({}, {username: {presence: {message: "is required"}}});

I suggest that a clear example should be provided for the important use case that we are trying to tackle, i.e. if we want a constraint on an optional attribute, how should that be specified.

nareshbhatia avatar Dec 14 '16 05:12 nareshbhatia

Walking through the validate.js code I am arriving at the same conclusion as @ajenkins-kyr. The error we are getting has nothing to do with the presence validator. That validator is not even in the picture. The problem is with other validators like email and format which are not allowing blank strings to pass through with code like this:

email: v.extend(function(value, options) {
  options = v.extend({}, this.options, options);
  var message = options.message || this.message || "is not a valid email";
  // Empty values are fine
  if (!v.isDefined(value)) {
    return;
  }
  if (!v.isString(value)) {
    return message;
  }
  if (!this.PATTERN.exec(value)) {
    return message;
  }
}

This only allows null and undefined to pass through, but empty strings are subject to pattern matching!

@ansman, this is a big blocker for me and many others I am sure. Please respond as soon as possible. I love validate.js and don't mind a workaround until this gets sorted out.

nareshbhatia avatar Dec 14 '16 15:12 nareshbhatia

I would still love to see this fixed, but if this is blocking anyone else, I can share my temporary workaround.

  1. Downgrade to 0.9.0. It seems like that was the last version where this was working as expected.
  2. The reason I upgraded from 0.9.0 to 0.11.1 was to take advantage of a bug-fix related to the equality validator.
  3. Write a custom validator to implement functionality for any features you need from more recent versions.

Obviously this isn't an ideal workaround, but it's what I had to do to get myself unblocked.

ajenkins-kyr avatar Dec 14 '16 15:12 ajenkins-kyr

@ajenkins-kyr thanks for the workaround. Downgrading now.

nareshbhatia avatar Dec 14 '16 15:12 nareshbhatia

Seems like it working at 0.10.0 as expected

irudoy avatar Dec 15 '16 08:12 irudoy

Thanks @irudoy, I will give that a try.

In the meanwhile, I was thinking if it would be better if the presence validator should be considered "special" and not run alongside other validators. Presence check would be done first and based on that check other validators would be run. Other validators would NOT do any presence checks because they are run only when there is something to validate. Here's roughly what I am thinking:

if (isEmpty) {
    if (presenceRequired) {
        mark as presence error
    }
    else {
        // no error
        // do not run other validators
    }
}
else {
    run other validators
}

@ansman and others, what do you think?

nareshbhatia avatar Dec 15 '16 13:12 nareshbhatia

@ansman I am sure you are very busy. Can you give us some idea on when you will be active again on this project/issue?

Thanks again for this great library.

nareshbhatia avatar Jan 02 '17 22:01 nareshbhatia

Sorry, I have very little time for this library and since I'm not working as a web developer any more it my knowledge is a bit outdated.

The presence validator has always been troublesome and I've never found a good solution. The original problem comes from wanting to treat empty strings as null but that should probably not be a part of this library. Instead null should be null and empty string should be a value like any other. I'll see if I can't fix that issue.

ansman avatar Mar 21 '17 20:03 ansman

Ok, this does not directly solve your problem but in 8b07b877882361c026ba01ae473ce4b417a6a345 presence no longer allows empty strings, arrays etc per default.

All other validators will still treat empty strings as values. This is by design. If you want to treat empty strings as null simply replace them with null before validating.

ansman avatar Mar 21 '17 20:03 ansman

@ansman thanks for your response. I will check this out over the weekend and give you my thoughts.

nareshbhatia avatar Mar 22 '17 09:03 nareshbhatia

@ansman, I finally got a chance to look at your latest commit. Unfortunately the workaround you suggested does not work for me (and probably for anyone using React). The trouble is that if I replace empty strings with nulls it upsets React. React requires input fields to have at least an empty string to consider it as a Controlled Component - replacing it with null makes it an uncontrolled component which has a completely different behavior.

I thought of another workaround which kind of works but will require lot of code. I have two use cases:

  1. Validating a single field: In this case I can skip validation entirely if presence is false AND the attribute string is empty:
onInputChanged() {
    let { constraints, errors } = this.props;

    // If presence is false and string is empty, skip validation
    if (constraints.email.presence === false && StringUtils.isEmpty(credentials.email)) {
        errors.email = null;
        return;
    }

    // Must validate!!!
    let localErrors = validate(credentials, constraints) || {};
    errors.email = localErrors.email;
}
  1. Validating an entire form: This gets more complicated. Basically we need to do the if check above for each constraint and remove it from the list of constraints if it needs to be ignored. This is a lot of work!

The only way I can think of solving this issue is to provide an allowEmpty option for each validator.

Thoughts?

nareshbhatia avatar May 30 '17 22:05 nareshbhatia

@ansman I just thought of a neat way to handle this use case as well as others such as #210. This solution does not require any change in validate.js's assumptions about empty strings vs. nulls. How about adding a preValidate option for each attribute? The value of this option will be a user supplied function that returns a boolean:

  • true: skip validations
  • false: continue validations (give an opportunity to add to errors)

This function may also change the attribute's value, e.g. trimming strings, to prepare for real validations.

Example

var constraints = {
  from: {
    preValidate: (attributes) => {
        return isEmpty(attributes.from) ? true : false;
    },
    email: true
  }
};

nareshbhatia avatar Jun 03 '17 14:06 nareshbhatia

I think the treatment of empty strings in this lib is correct (though maybe some options for automatic sanitization would be nice), and the annoyance is that values from html inputs are always strings (rather than null). Just means you'll need to sanitize a bit before validating, but most use-cases are going to need to sanitize anyways (imagine values like ' ' in the email column of your db).

I found a little something like this helpful:

const makeValidator = (constraints) => {
  const options = {
    fullMessages: false,
    format: 'grouped',
  };
  return (params) => {
    const sanitizedParams = _.mapValues(params, (val) => { // lodash alert
      if (typeof val !== 'string') return val;
      return val.trim() || null;
    });
    return validate(sanitizedParams, constraints, options);
  };
};

const validateSignup = makeValidator({
  // contraints here
});

validateSignup({
  // params from signup form
});

tybro0103 avatar Jan 15 '18 21:01 tybro0103

@ansman Would a feature that enables automatic sanitization make a welcome PR, or would that be considered outside the scope of this lib?

tybro0103 avatar Jan 15 '18 22:01 tybro0103

i founf this solution: i changed validation regex for email adding a match for empty string: validate.validators.email.PATTERN = /^[a-z0-9\u007F-\uffff!#$%&'*+\/=?^_{|}~-]+(?:.[a-z0-9\u007F-\uffff!#$%&'*+/=?^_{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z]{2,}$|^$/i;

and with this 2 constraint i check for email required and for empty but always well formed

/*not required*/
var constraints = {
  email : {
    email: true,
  }
};

/*required*/
var constraints = {
  email : {
    presence: {allowEmpty: false},
    email: true,
  }
};

dlombardiniGitHub avatar May 18 '18 11:05 dlombardiniGitHub

Followed @dlombardiniGitHub 's idea but used the following regexp:

validate.validators.email.PATTERN = /(^$|(^([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$)/

... which basically says /(empty string | email)/

ronkot avatar May 21 '18 08:05 ronkot

another workaround:

validate.validators.optionalEmail = (value) => {
  if (value === '') {
    return null;
  }
  return validate.single(value, { email: true });
};

javialon26 avatar Jul 27 '18 02:07 javialon26

I like the custom validator workaround. My only issue is that I cant figure out how to configure a custom message when doing validate.single.

SuneRadich avatar Feb 28 '19 14:02 SuneRadich

Any update on this?

meetDeveloper avatar Jun 19 '19 03:06 meetDeveloper

Are you guys thinking about adding empty string check?

meetDeveloper avatar Jun 19 '19 03:06 meetDeveloper

I suggest the following solution:

validate.validators.optional = (value, options) => {
    return !isEmpty(value) ? validate.single(value, options) : null
}

function isEmpty (v) { ['', null, undefined].includes(v) }

// usage

validate({ email: '[email protected]' }, { email: { optional: { email: true } } })

stsiushkevich avatar Oct 01 '19 11:10 stsiushkevich

A nice solution @stsiushkevich!

However the isEmpty function is missing a return: function isEmpty (v) { return ['', null, undefined].includes(v) }

rudolfs avatar Jan 07 '20 10:01 rudolfs

I like the custom validator workaround. My only issue is that I cant figure out how to configure a custom message when doing validate.single. @SuneRadich

I don't know if this has long since been solved. But validate.single accepts the same constraints as the normal validate method. That is you can specify a message in the constraints. My solution (which might be a bit verbose) looks like this:

validate.validators.validateOptionalEmail = (value, options) => {
  const { message = '^Not a valid e-mail' } = options;

  if (value === '') {
    return null;
  }

  const validationMessages = validate.single(value, {
    email: {
      message: message
    }
  });

  if (validationMessages) {
    return `^${validationMessages[0]}`;
  } else {
    return null;
  }
};

which is then used in the normal constraint object as

const constraints = {
  'my.email.field': {
    validateOptionalEmail: {
      message: '^Custom message can go here'
    }
  }
}

perenstrom avatar May 27 '20 12:05 perenstrom