Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Custom validator promises and options #1229

Closed
wants to merge 2 commits into from
Closed

WIP: Custom validator promises and options #1229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
529e40a
Pass through options to custom validators (Code only)
PaddyMann Jan 17, 2017
163e3a6
Add support for custom validators to return promises
PaddyMann Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion lib/validations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,20 @@ function validateCustom(attr, conf, err, options, done) {
done = options;
options = {};
}
conf.customValidator.call(this, err, done);

if (conf.customValidator.length <= 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

conf.customValidator.call(this, options)
.then((isValid) => {
if (!isValid) throw new Error();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

done();
})
.catch(() => {
err();
done();
});
} else {
conf.customValidator.call(this, err, done, options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}

/*!
Expand Down
11 changes: 11 additions & 0 deletions test/validations.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,17 @@ describe('validations', function() {
done();
})).should.be.false;
});

it.only('should validate using custom async validation using promises', function(done) {
User.validateAsync('email', function(options) {
return Promise.resolve(true);
}, {});
var u = new User({email: 'hello'});
Boolean(u.isValid(function(valid) {
valid.should.be.true;
done();
})).should.be.false;
});
});

describe('invalid value formatting', function() {
Expand Down