WIP: Custom validator promises and options

[PaddyMann]

Description

@bajtos recently added support for passing context through to remote methods, hooks, etc.

While the options were passed as far as isValid and even validateCustom, they weren't then being passed onto a user-defined custom validator function.

This PR seeks to:

1. Pass through options (achieved by simply adding the options argument to conf.customValidator.call(this, err, done); in validations.js
2. Add Promise support for custom async validators (as a bonus, though this is a nice-to-have vs. the missing options which is a bug)

I'm not clear on what tests are needed for passing through the options, and would appreciate any help here.

Related issues

• None

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[bajtos]

@slnode ok to test

[bajtos]

@PaddyMann thank you for the pull request. To make the review faster, could you please move the second part (Add Promise support for custom async validators) to a standalone pull request?

[bajtos]

To make the review faster, could you please move the second part (Add Promise support for custom async validators) to a standalone pull request?

Actually, if you don't mind rebasing the changes to preserve two commits, then it's ok to keep all changes in a single pull request.

[bajtos]

Our usual convention in other places where we support both callback/promise based functions, is to check for the return type of the function, instead of relying on the number of arguments.

var result = conf.customValidator(...);
if (result.then && typeof result.then === 'function') {
  // ...
}

My concern is that a custom validator accepting a single argument is a valid use case, see https://github.com/strongloop/loopback-datasource-juggler/pull/1229/files#diff-a1a277b8e694ce811867b2ac3e064ff8R626

       User.validate('email', function(err) {
         if (this.email === 'hello') err();
       }, {code: 'invalid-email'});

[bajtos]

Actually: when defining a validator, user can provide sync flag to tell the framework whether the validator function is sync or async. Perhaps we can add another flag, e.g. promise to distinguish between "old" custom validators using callbacks and the new Promise-based flavour?

Just an idea to consider.

[bajtos]

AFAICT, there are several outcomes of a custom validator:

1. err() is called - an error message is added by the validation framework
2. err(false) is called - the validated object is marked as invalid, but no error message is added by the validation framework. See https://github.com/strongloop/loopback-datasource-juggler/pull/1229/files#diff-a1a277b8e694ce811867b2ac3e064ff8L638
3. done() is called - the property value is valid
4. done(truthy) is called - same as calling err()

I would like to preserve these options in the Promise mode too, i.e.

1. Indicate that the validation failed - items 1 and 4 above
2. Indicate that the validation failed and the error message was already inserted - item 2 above
3. Indicate that the validation passed

I think the cleanest solution is to expect the promise to resolve with a structured object instead of a single true/false value, although we can support boolean resolution to make simple cases simple to implement.

// the value is valid
return Promise.resolve(); 

// the value is invalid, a message should be added by the framework
return Promise.reject();
return Promise.reject(err);
throw unhandledError;

// the value is invalid, a message was already added by the validator
return Promise.reject({ addMessage: false });

Here is a code snippet illustrating a possible implementation of my proposal:

var result = conf.customValidator(...);
if (result.then && typeof result.then === 'function') {
  result.then(
    function onSuccess() { done(); },
    function onFailure(reason) {
      if (reason instanceof Error) {
        done(err);
      } else if (reason && reason.addMessage !== false) {
        err(false);
      } else {
        err();
      }
    })
}

Thoughts?

[bajtos]

I think passing the options after the done callback is rather unusual, since the Node.js convention is to pass the callback function as the last arg.

I am proposing the following change:

if (conf.customValidator.length !== 4) {
  // backwards compatibility with custom validators not accepting options
  conf.customValidator.call(this, err, done);
} else {
  conf.customValidator.call(this, options, err, done);
}

Thoughts?

[bajtos]

I'm not clear on what tests are needed for passing through the options, and would appreciate any help here.

I am proposing to add two tests - one for a sync custom validator, another for an async custom validator.

Please add also few more tests for Promise-based validators, 1) to cover the different validation outcomes I described in my comment above, 2) to verify that options are passed through to the validator.

[PaddyMann]

Thanks @bajtos for the review - really appreciated and a few lessons I'll apply to any future PRs. I'll act on your suggestions :)

[bajtos]

@PaddyMann ping, what's the status of this pull request?

[PaddyMann]

It's on my list for next week - sorry for leaving it untouched for so long.

[PaddyMann]

Hi @bajtos,

Just realized this isn't a requirement for our upcoming release and we're working to tight deadline, so I'm going to have to park this - sorry!

I'll be happy to address it as and when we need the feature implemented.

Paddy

[bajtos]

@PaddyMann no worries! Let's close this pull request for now, feel free to reopen it if/when you will need this feature in the future.

[PaddyMann]

cool - will do

[ebarault]

FYI, I've observed some discussions in the gitter channel of people requiring this fix
In order to get access to context options param in custom validators, dirty hacks like this are required ATM