Validations error returns 500 code
Closed this issue · 6 comments
templates/actions/create.js
Model.create(data).exec((err, newInstance) => {
// Differentiate between waterline-originated validation errors
// and serious underlying issues. Respond with badRequest if a
// validation error is encountered, w/ validation info.
if (err) return res.serverError(err, actionUtil.parseLocals(req));
It seems the comment discusses handling this but it is not handled.
Yes, this is indeed an issue!
So here is the long version of how this came to be:
In the older versions of sails.js (0.11.x, 0.12.x) there was a nice response method called res.negotiate, and it was used in the old create action:
This method would handle exactly what is mentioned in the comments.
In sails 1.0.x+ The res.negotiate response method no longer exists, so this library took a shortcut to make things functional by simply replacing negotiate() with serverError()
Going forward, I have been debating with myself and the rest of my team whether or not this library should automatically re-insert the negotiate() response back into a sails 1.0 project, or whether we should abstract the negotiate() logic into its own utility function that can be used throughout sails-ember-rest without adding a new response method to a dependent application's api/response folder.
Seeing as you are the first person to comment on this particular issue, I would say that means it would prudent to gather your opinion on how you as a consumer of this library would like the problem solved.
Would you like to see the negotiate response added back into a sails 1.0 app running this library or do you think a utility method as part of actionUtil.js makes more sense?
(Also, I am very open to pull requests to solve this one)
Thanks for the fast response. Great work on this library! I am working on my first Sails 1.x.x app, I used sails-generate-ember-blueprints
in previous apps for Sails 0.x.x, this library is crucial to the upgrade.
I see what you are saying. I kind of think the best solution might just be to copy what Sails 1.x.x does. Meaning change this:
if (err) return res.serverError(err, actionUtil.parseLocals(req));
To this:
if (err) {
switch (err.name) {
case 'AdapterError':
switch (err.code) {
case 'E_UNIQUE': return res.badRequest(err, actionUtil.parseLocals(req));
default: return res.serverError(err, actionUtil.parseLocals(req));
} return;
case 'UsageError': return res.badRequest(err, actionUtil.parseLocals(req));
default: return res.serverError(err, actionUtil.parseLocals(req));
}
}
This would be done in every action so there is some code duplication but it isn't too much. I think keeping it as similar to the Sails approach as possible helps make maintenance easier. I copied this from here (https://github.com/balderdashy/sails/blob/master/lib/hooks/blueprints/actions/create.js) and just added actionUtil.parseLocals(req)
to the response calls. Personally I would have used an if
block instead of switch
but I digress.
What do you think of this solution?
We (@TransNexus) use Sails/Ember a lot. I am happy to help contribute to this library.
@mdconaway I like it!
Great! Well 1.0.8 has been pushed to NPM, so I will close this issue. Let me know if you guys have any good ideas you would like to see implemented in this library, as I always enjoy making it better. Also, you may be interested in the new README section added earlier today:
https://github.com/mdconaway/sails-ember-rest#controller
It talks a bit about a whole new lifecycle pattern that only exists in this library.
@mdconaway The readme looks good! I read through the code last week to figure it out. I really like the interrupts!