multiple validator errors #1214

Closed
wants to merge 6 commits into
from

Projects

None yet

9 participants

@RGBboy
RGBboy commented Nov 15, 2012

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.

@aheckmann
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
RGBboy commented Nov 15, 2012

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
RGBboy commented Nov 18, 2012

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

@aheckmann aheckmann commented on an outdated diff Nov 19, 2012
lib/schematype.js
- if (err) return;
- if (val === undefined || val) {
- --count || fn(null);
- } else {
- fn(err = new ValidatorError(path, msg));
+ --count;
+ if (val !== undefined && !val) {
+ if (err) {
+ if (err instanceof ValidatorError) {
+ var errCollection = new ValidatorCollectionError(path);
+ errCollection.errors.push(err);
+ err = errCollection;
+ }
+ err.errors.push(new ValidatorError(path, msg));
+ } else {
+ err = new ValidatorError(path, msg);
@aheckmann
aheckmann Nov 19, 2012 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.

RGBboy added some commits Nov 19, 2012
@RGBboy
RGBboy commented Nov 19, 2012

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.

@jasonmadigan

Nice patch - would be nice to have.

@aheckmann
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
vizo commented Jan 29, 2013

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

@teddychan

+1, we need multi validation errors too.

@aheckmann
Collaborator

closing until v4

@aheckmann aheckmann closed this Mar 18, 2013
@kb19
kb19 commented Apr 2, 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.

@vizo
vizo commented Jul 21, 2014

Anything new about this?

@memelet
memelet commented Jan 22, 2015

This is closed because ???

@vkarpov15 vkarpov15 reopened this Jan 22, 2015
@vkarpov15 vkarpov15 closed this Jan 22, 2015
@vkarpov15
Collaborator

Opened #2612 to track

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