Inconsistent error message creation process between `validate(val, type)` and form validation

[th0r]

Lets say you have this refinement:

const t = require('tcomb-validation');

function intl(locale, msg) {
    return `${msg} (${locale} locale)`;
}

const Login = t.refinement(t.String, str => str.length >= 3, 'Login');

Login.getValidationErrorMessage = function (value, path, context) {
    if (!value.length) {
        return intl(context.locale, "Login can't be empty");
    }

    if (value.length < 3) {
        return intl(context.locale, 'Login must contain at least 3 chars');
    }
};

Then this validation won't call getValidationErrorMessage because null won't pass t.String validation:

const validationResult = t.validate(null, Login, { context: { locale: 'ru' } });

The error here will be Invalid value null supplied to String and, as I understand, this error message can't be configured.

But if you'll make a simple form with this type:

const LoginForm = t.struct({ login: Login });

Then getValidationErrorMessage is being called with null on initial form render and on submit if login field remains empty.

The problem is I don't know what is the proper behavior should be:

• In one case, if we will pass to getValidationErrorMessage only values, already validated by the parent type, then there is no way to change default error messages for built-in primitive types (and I don't think that implementing this will help, because showing Value should be string under empty login field is not very cool).
• On the other case, if we will pass any invalid value to the topmost refinement's getValidationErrorMessage, then it should handle any kind of value. It complicates things a little, but the error message will be relevant.

What do you think?

P.S. Maybe create gitter room for such conversations?

[gcanti]

Yes the process is inconsistent:

with tcomb-validation

const Login = t.refinement(t.String, str => str.length >= 3, 'Login');

t.String.getValidationErrorMessage = function (value) {
  if (!value) {
    return 'Login can\'t be empty';
  }
};

Login.getValidationErrorMessage = function (value) {
  if (value.length < 3) {
    return 'Login must contain at least 3 chars';
  }
};

let validationResult = t.validate(null, Login);
console.log(validationResult.errors[0].message); // => "Login can't be empty"
validationResult = t.validate('a', Login);
console.log(validationResult.errors[0].message); // => "Login must contain at least 3 chars"

with tcomb-form

const Login = t.refinement(t.String, str => str.length >= 3, 'Login');

// this is not called
t.String.getValidationErrorMessage = function (value) {
  if (!value) {
    return 'Login can\'t be empty';
  }
};

Login.getValidationErrorMessage = function (value) {
  if (!value) { // <= this must be duplicated
    return 'Login can\'t be empty';
  }
  if (value.length < 3) {
    return 'Login must contain at least 3 chars';
  }
};

const Type = t.struct({
  login: Login
});

[th0r]

Yes the process is inconsistent

So, is it ok?

t.String.getValidationErrorMessage = function (value) {...}

Looks like overriding t.String.getValidationErrorMessage is an ugly hack. There may be a lot of refinements based on t.String (Password, PostTitle, PostDescription etc.) and it seems wrong supporting all of them in t.String.getValidationErrorMessage.

[gcanti]

So, is it ok?

No, I must find a solution if possible.

Looks like overriding t.String.getValidationErrorMessage is an ugly hack

I think it's ok if it handles just the "required string" part.

[th0r]

I think it's ok if it handles just the "required string" part.

But, theoretically, if some other module uses tcomb, this change will affect it as t.String type is common for everyone.

[gcanti]

this change will affect it as t.String type is common for everyone.

Yes but it could be intentional. Say you write something general like this:

t.String.getValidationErrorMessage = function (value) {
  if (!value) {
    return 'This field is required'; // <= a general message, it's ok for all required strings
  }
};

Otherwise if you want a specific message like "Name can't be empty" you can define an alias for t.String (it can even be thought as a best practice: "always define specific types for your models!"):

const Name = t.refinement(t.String, () => true);

Name.getValidationErrorMessage = function (value) {
  if (!value) {
    return 'Name can\'t be empty';
  }
};

const Person = t.struct({
  name: Name
});

[th0r]

So, what is the proper validation flow should be? The topmost getValidationErrorMessage called for any invalid value? Or waterfall flow?

[gcanti]

Waterfall flow would be great. Actually I want to enforce the same flow of t.validate

[gcanti]

@th0r Just pushed a possible implementation on branch 0.6.x and master if you want to give it a spin or simply discuss the implementation

[gcanti]

Released in v0.6.8 and v0.7.4