Skip to content

Conversation

@bitmage
Copy link
Contributor

@bitmage bitmage commented Dec 9, 2014

I modified the formatErrors() function to include the current value of the fields which have failed validation.

The format looks like this:

The `Person` instance is not valid. Details: `id: 123456` can\'t be set.

Please let me know if that format is good, and I will update the test expectations to match.

@raymondfeng
Copy link
Contributor

The proposed change breaks some test cases. Can you run npm test to check?

Choose a reason for hiding this comment

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

@bitmage , I think this shouldn't be logged by default to server console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will remove this.

@bitmage
Copy link
Contributor Author

bitmage commented Dec 11, 2014

Is the format of the message good? I don't want to waste time correcting expectations in a dozen tests until I get confirmation on that.

@bajtos
Copy link
Member

bajtos commented Dec 15, 2014

My two cents: the proposed format does not distinguish between different types, it's hard to tell whether the value was a string or a number. It also does not work for object types and when the property value is large (e.g. a 1kb text), the error message will be unreadable.

I am proposing:

  • run the value through JSON.stringify, handle "undefined", truncate values longer than let's say 32 characters
  • if you thing the full value is important, then include it in the response body. ATM there is a code and a message, it should be reasonably easy to add a value field too.

@bajtos bajtos self-assigned this Dec 15, 2014
@bajtos bajtos added the waiting label Jan 5, 2015
@bajtos
Copy link
Member

bajtos commented Jan 5, 2015

What's the status of this work? Are you @bitmage still willing to finish it?

@bitmage
Copy link
Contributor Author

bitmage commented Jan 5, 2015

Oh yeah, sorry! I will take a look in the next day or two and try to get things wrapped up.

@bitmage bitmage force-pushed the clarify-validations branch from 451c6fa to 385a0eb Compare January 7, 2015 22:36
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of this? I noticed we were producing some strange output in the case of nested documents (which have a type of function). I don't know how to output a sensible output in that case... but I think it's an edge case that can be left for another day.

For strings, objects, and undefined/null we now have some pretty clear output.

Copy link
Member

Choose a reason for hiding this comment

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

Handling non-scalar values (objects, arrays, functions) will be always tricky. You can take a look at chai.js which basically prints only top-level keys of the object.

As for function values, JSON.stringify removes them from the output, which solves the problem. On the other hand, it does not handle circular references, which may be a problem.

@bitmage
Copy link
Contributor Author

bitmage commented Jan 10, 2015

Your Model prototype is implementing the inspect method and configuring it to output colors.

Any time a model is console.log'd or inspected it will run this method. This overrides whatever top-level configuration is used, so inspect(value, {colors: false}) in the validation code doesn't work.

I can fall back to JSON.stringify as you originally suggested, but it's a blocking operation, and I'm still concerned about the lack of a depth limit.

@bajtos
Copy link
Member

bajtos commented Jan 12, 2015

Closing in favour of #389.

@bajtos bajtos closed this Jan 12, 2015
@bajtos bajtos removed the #wip label Jan 12, 2015
@bajtos
Copy link
Member

bajtos commented Jan 12, 2015

Any time a model is console.log'd or inspected it will run this method. This overrides whatever top-level configuration is used, so inspect(value, {colors: false}) in the validation code doesn't work.

Good catch! There are several ways how to fix the problem, e.g. by passing customInspect:false to util.inspect or calling toObject() on Model instances before calling util.inspect. I'll check possible solutions and fix the problem in #389.

@bajtos
Copy link
Member

bajtos commented Jan 12, 2015

On Node v0.11.14+, one can return an object from the custom inspect function. That way all options are honoured. For Node v0.10 and older, I have disabled color output.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants