Skip to content
This repository

multiple validator errors #1214

Closed
wants to merge 6 commits into from

6 participants

RGBboy Aaron Heckmann Jason Madigan vizo Teddy Chan Michael Succi
RGBboy

Fixes #1031
Fixes #1104

This fix changes the way validator errors are added to the validation error. When multiple errors occur for a property, an array is created and populated with all the validator errors.

A test has also been added to schema.test.js

Let me know if this fits the bill.

Aaron Heckmann
Collaborator

We shouldn't be passing arrays back as errors. The node way is to pass an instance of Error

Any changes to make this happen must be compatible with 3.x (all tests in master must pass). Otherwise we can review the change for release some day far down the road in 4.x.

RGBboy

I have run the changes against the current master tests and they all pass except for one unrelated test:

model console.log hides private props @ ./test/model.test.js:3759:14

The current fix results in a validation error like:

{
  message: 'Validation failed',
  name: 'ValidationError',
  errors: {
    testProperty: [
      {
        message: 'Validator "required" failed for path testProperty',
        name: 'ValidatorError',
        path: 'testProperty',
        type: 'required'
      },
      {
        message: 'Validator "min" failed for path testProperty',
        name: 'ValidatorError',
        path: 'testProperty',
        type: 'min'
      },
      {
        message: 'Validator "max" failed for path testProperty',
        name: 'ValidatorError',
        path: 'testProperty',
        type: 'max'
      }
    ]
  }
}

Is this an acceptable format for multiple errors within the validation error object?

What should an error returned from doValidation() that contains multiple errors look like? Should a new validator error be created (perhaps a multipleValidator error) and returned?

RGBboy

The latest commits added a new type of error called ValidatorCollection that can be used to hold multiple validator errors for a single property. This is the type of error that is called back with when there is more than one error produced when validating a single property. After the change mongoose now produces a ValidationError like:

{
  message: 'Validation failed',
  name: 'ValidationError',
  errors: {
    testProperty: {
      message: 'Validators failed for path testProperty',
      name: 'ValidatorCollectionError',
      path: 'testProperty',
      errors: [
        { message: 'Validator "required" failed for path testProperty',
          name: 'ValidatorError',
          path: 'testProperty',
          type: 'required'
        },
        { message: 'Validator "min" failed for path testProperty',
          name: 'ValidatorError',
          path: 'testProperty',
          type: 'min'
        },
        { message: 'Validator "max" failed for path testProperty',
          name: 'ValidatorError',
          path: 'testProperty',
          type: 'max'
        }
      ]
    }
  }
}

I have also merged the latest changes from the master branch and tested. All tests except for the one mentioned before are passing.

Let me know if this is closer to what you envision or if I need to make further changes.

Thanks

lib/schematype.js
((13 lines not shown))
551  
-    if (err) return;
552  
-    if (val === undefined || val) {
553  
-      --count || fn(null);
554  
-    } else {
555  
-      fn(err = new ValidatorError(path, msg));
  552
+    --count;
  553
+    if (val !== undefined && !val) {
  554
+      if (err) {
  555
+        if (err instanceof ValidatorError) {
  556
+          var errCollection = new ValidatorCollectionError(path);
  557
+          errCollection.errors.push(err);
  558
+          err = errCollection;
  559
+        }
  560
+        err.errors.push(new ValidatorError(path, msg));
  561
+      } else {
  562
+        err = new ValidatorError(path, msg);
1
Aaron Heckmann Collaborator

ValidatorError should always be returned regardless of number of errors. We should probably just modify it to accept multiple errors instead of creating a new type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
RGBboy

Ok, so I have removed the ValidatorCollectionError and re-factored to just use ValidationError. When multiple validators fail for a single property, mongoose produces a validation error like:

{
  message: 'Validation failed',
  name: 'ValidationError',
  errors: {
    testProperty: {
      message: 'Validators failed for path testProperty',
      name: 'ValidatorError',
      path: 'testProperty',
      errors: [
        { message: 'Validator "required" failed for path testProperty',
          name: 'ValidatorError',
          path: 'testProperty',
          type: 'required'
        },
        { message: 'Validator "min" failed for path testProperty',
          name: 'ValidatorError',
          path: 'testProperty',
          type: 'min'
        },
        { message: 'Validator "max" failed for path testProperty',
          name: 'ValidatorError',
          path: 'testProperty',
          type: 'max'
        }
      ]
    }
  }
}

When just a single validator fails for a single property, mongoose creates an error like:

{
  message: 'Validation failed',
  name: 'ValidationError',
  errors: {
    testProperty: {
      message: 'Validator "required" failed for path testProperty',
      name: 'ValidatorError',
      path: 'testProperty',
      type: 'required'
    },
  }
}

This is required to keep backwards compatibility with existing tests.

Let me know if there are any other adjustments that need to be made.

Thanks.

Jason Madigan

Nice patch - would be nice to have.

Aaron Heckmann
Collaborator

generally I like this. after playing around more and hacking on it, its still not fully backward compatible (despite the tests passing) b/c the error messages change when theres more than one error. so this would need to go behind an opt-in option somewhere, probably a schema option, which aren't currently exposed to SchemaTypes which introduces more complexity with regards to child schemas and setting up options correctly when initializing these objects, say the parent schema enables it we'll need to propagate those options to the child schemas etc etc... yuck.

anything I think we do with this in v3 would be too much of a hack. so lets leave this for v4.

vizo

+1
I think it should always return array of errors, no matter how many validations fail.

Teddy Chan

+1, we need multi validation errors too.

Aaron Heckmann
Collaborator

closing until v4

Aaron Heckmann aheckmann closed this March 18, 2013
Michael Succi
kb19 commented April 01, 2013

+1

Does anyone have their own preferred way of working around this for now? I'm trying to work with form input and validate against the schema. For now I have a manual check that puts together a response much like the one above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
25  lib/errors/validator.js
@@ -8,20 +8,25 @@ var MongooseError = require('../error');
8 8
  * Schema validator error
9 9
  *
10 10
  * @param {String} path
11  
- * @param {String} msg
  11
+ * @param {String|Array} type or array of validator errors
12 12
  * @inherits MongooseError
13 13
  * @api private
14 14
  */
15 15
 
16 16
 function ValidatorError (path, type) {
17  
-  var msg = type
18  
-    ? '"' + type + '" '
19  
-    : '';
20  
-  MongooseError.call(this, 'Validator ' + msg + 'failed for path ' + path);
  17
+  if (Array.isArray(type)) {
  18
+    this.errors = type;
  19
+    MongooseError.call(this, 'Validators failed for path ' + path);
  20
+  } else {
  21
+    var msg = type
  22
+      ? '"' + type + '" '
  23
+      : '';
  24
+    MongooseError.call(this, 'Validator ' + msg + 'failed for path ' + path);
  25
+    this.type = type;
  26
+  };
21 27
   Error.captureStackTrace(this, arguments.callee);
22 28
   this.name = 'ValidatorError';
23 29
   this.path = path;
24  
-  this.type = type;
25 30
 };
26 31
 
27 32
 /*!
@@ -29,7 +34,13 @@ function ValidatorError (path, type) {
29 34
  */
30 35
 
31 36
 ValidatorError.prototype.toString = function () {
32  
-  return this.message;
  37
+  if (this.errors) {
  38
+    return this.name + ': ' + this.errors.map(function (value) {
  39
+      return String(value);
  40
+    }, this).join(', ');
  41
+  } else {
  42
+    return this.message;
  43
+  }
33 44
 }
34 45
 
35 46
 /*!
31  lib/schematype.js
@@ -541,20 +541,33 @@ SchemaType.prototype.select = function select (val) {
541 541
  */
542 542
 
543 543
 SchemaType.prototype.doValidate = function (value, fn, scope) {
544  
-  var err = false
  544
+  var err = null
545 545
     , path = this.path
546 546
     , count = this.validators.length;
547 547
 
548  
-  if (!count) return fn(null);
  548
+  if (!count) return fn(err);
549 549
 
550 550
   function validate (val, msg) {
551  
-    if (err) return;
552  
-    if (val === undefined || val) {
553  
-      --count || fn(null);
554  
-    } else {
555  
-      fn(err = new ValidatorError(path, msg));
556  
-    }
557  
-  }
  551
+    --count;
  552
+    if (val !== undefined && !val) {
  553
+      if (err && !Array.isArray(err)) {
  554
+        var errArray = [err];
  555
+        err = errArray;
  556
+      } else if (!err) {
  557
+        err = new ValidatorError(path, msg);
  558
+      };
  559
+      if (Array.isArray(err)) {
  560
+        err.push(new ValidatorError(path, msg));
  561
+      };
  562
+    };
  563
+    if (!count) {
  564
+      if (Array.isArray(err)) {
  565
+        return fn(new ValidatorError(path, err));
  566
+      } else {
  567
+        return fn(err);
  568
+      };
  569
+    };
  570
+  };
558 571
 
559 572
   this.validators.forEach(function (v) {
560 573
     var validator = v[0]
16  test/schema.test.js
@@ -465,6 +465,22 @@ describe('schema', function(){
465 465
       done();
466 466
     });
467 467
 
  468
+    it('multiple errors', function(done){
  469
+      var MultipleTest = new Schema({
  470
+          testProperty: { type: Number, required: true, min: 1, max: 5 } 
  471
+      });
  472
+      
  473
+      MultipleTest.path('testProperty').doValidate(undefined, function (err) {
  474
+        assert.ok(err instanceof ValidatorError);
  475
+        assert.equal(true, Array.isArray(err.errors));
  476
+        assert.equal(err.errors.length, 3);
  477
+        for (var i = 0; i < err.errors.length; i += 1) {
  478
+          assert.ok(err.errors[i] instanceof ValidatorError);
  479
+        };
  480
+        done();
  481
+      });
  482
+    });
  483
+
468 484
     describe('async', function(){
469 485
       it('works', function(done){
470 486
         var executed = 0;
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.