{"action":"created","issue":{"url":"https://api.github.com/repos/dchester/epilogue/issues/45","labels_url":"https://api.github.com/repos/dchester/epilogue/issues/45/labels{/name}","comments_url":"https://api.github.com/repos/dchester/epilogue/issues/45/comments","events_url":"https://api.github.com/repos/dchester/epilogue/issues/45/events","html_url":"https://github.com/dchester/epilogue/issues/45","id":59223220,"number":45,"title":"Error Formatting","user":{"login":"Eluinhost","id":7514501,"avatar_url":"https://avatars.githubusercontent.com/u/7514501?v=3","gravatar_id":"","url":"https://api.github.com/users/Eluinhost","html_url":"https://github.com/Eluinhost","followers_url":"https://api.github.com/users/Eluinhost/followers","following_url":"https://api.github.com/users/Eluinhost/following{/other_user}","gists_url":"https://api.github.com/users/Eluinhost/gists{/gist_id}","starred_url":"https://api.github.com/users/Eluinhost/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Eluinhost/subscriptions","organizations_url":"https://api.github.com/users/Eluinhost/orgs","repos_url":"https://api.github.com/users/Eluinhost/repos","events_url":"https://api.github.com/users/Eluinhost/events{/privacy}","received_events_url":"https://api.github.com/users/Eluinhost/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":13,"created_at":"2015-02-27T11:16:33Z","updated_at":"2015-03-03T20:06:22Z","closed_at":null,"body":"## Defaults formatting\r\nErrors should be in the format:\r\n\r\n```json\r\n{\r\n  \"message\": \"error message\",\r\n  \"errors\": [\"array\", \"of\", \"problems\"]\r\n}\r\n```\r\nThe following show just 'message' when returning an error:\r\n\r\nhttps://github.com/dchester/epilogue/blob/master/lib/Controllers/read.js#L45\r\nhttps://github.com/dchester/epilogue/blob/master/lib/Controllers/delete.js#L30\r\n\r\nI don't think these are a problem, the errors array should be optional.\r\n\r\nThis file is just showing `error` for a 404 though:\r\nhttps://github.com/dchester/epilogue/blob/master/lib/Controllers/update.js#L33\r\n\r\nPlus this line just returns `error` for a 500:\r\nhttps://github.com/dchester/epilogue/blob/master/lib/Controllers/list.js#L120\r\n\r\nThese should probably use a similar system to stop having to worry about fitting the same format each time.\r\n\r\n## Returning Errors\r\nReturning an error is awkward from milestones too, for example I have this middleware:\r\n\r\n```javascript\r\nmodule.exports = function(attributes) {\r\n  if(!_.isArray(attributes)) attributes = [attributes];\r\n\r\n  return function(req, res, context) {\r\n    var intersect = _.intersection(_.keys(req.body), attributes);\r\n    if(intersect.length) {\r\n      res.status(400);\r\n      context.error = {\r\n        message: 'Cannot set fields on this object',\r\n        errors: intersect\r\n      };\r\n      // skip the actual write\r\n      return context.skip();\r\n    }\r\n\r\n    return context.continue();\r\n  };\r\n};\r\n```\r\n\r\nHere I am using the same system used in the default middlewares of doing:\r\n\r\n```javascript\r\nres.staus(400); //set error code\r\ncontext.error = {\r\n  message: 'error message',\r\n  errors: ['array of problems']\r\n}\r\ncontext.skip(); // skip to next\r\n```\r\nThe problem I have with this is it is now required to be ran on write.before so the skip works and then the write function needs to also check there is no errors to `context.continue`. On `create` the default write function doesn't check context.error but the one in `update` does. Every middleware called after this one needs to be aware of the error to avoid processing when there is an error and allow the error to pass through to the deafult .send action (on this note the complete callback says `Run the specified function when the request is complete, regardless of the status of the response.` but as far as I can tell if context.stop() is called it won't run)\r\n\r\n# Potential Solution\r\nI think a proper way to handle this would be to add error to context as a function.\r\n\r\n```javascript\r\ncontext.error({\r\n  message: 'error message',\r\n  status: 400,  // optional and default 500?\r\n  errors: ['array of problems'] // optional\r\n});\r\n```\r\nCalling error should then call a special 'error' function that is outside of the chain (and probably no .before or .after) where the default can be:\r\n\r\n```javascript\r\nfunction(req, res, context, error) {\r\n  res.status(error.status);\r\n  res.json(_.omit(error, ['status']));\r\n  context.stop();\r\n}\r\n```\r\n\r\nThis would make keeping to the default format within epilogue simple as they would just need to call `context.error(...)` and would no longer need to check `context.error` and continue if it is set. It would also allow customization of the default middleware functions error rendering.\r\n\r\nIt should also allow context.continue() and context.skip() to resume processing from within the error milestone.\r\n\r\nIt could also be tier structured:\r\n\r\nRunning create.write.after would check for and run the first of:\r\n\r\n```\r\nmodel.create.write.after.error  \r\nmodel.create.write.error  \r\nmodel.create.error  \r\nmodel.error  \r\ndefault error function  \r\n```\r\n\r\nAnd update.fetch would check for\r\n\r\n```\r\nmodel.update.fetch.action.error\r\nmodel.update.fetch.error\r\nmodel.update.error\r\nmodel.error\r\ndefault error function\r\n```\r\n\r\nIf the tiering isn't possible at minimum a 'previous state' variable should be accessible in the error function to tell what hook errored and allow separate processing based on the state\r\n\r\nOn another side I also see this:\r\nhttps://github.com/dchester/epilogue/blob/master/lib/Controllers/base.js#L122\r\n\r\nWhich should catch any uncaught errors, but just passes them to an empty function"},"comment":{"url":"https://api.github.com/repos/dchester/epilogue/issues/comments/77023529","html_url":"https://github.com/dchester/epilogue/issues/45#issuecomment-77023529","issue_url":"https://api.github.com/repos/dchester/epilogue/issues/45","id":77023529,"user":{"login":"mbroadst","id":312966,"avatar_url":"https://avatars.githubusercontent.com/u/312966?v=3","gravatar_id":"","url":"https://api.github.com/users/mbroadst","html_url":"https://github.com/mbroadst","followers_url":"https://api.github.com/users/mbroadst/followers","following_url":"https://api.github.com/users/mbroadst/following{/other_user}","gists_url":"https://api.github.com/users/mbroadst/gists{/gist_id}","starred_url":"https://api.github.com/users/mbroadst/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mbroadst/subscriptions","organizations_url":"https://api.github.com/users/mbroadst/orgs","repos_url":"https://api.github.com/users/mbroadst/repos","events_url":"https://api.github.com/users/mbroadst/events{/privacy}","received_events_url":"https://api.github.com/users/mbroadst/received_events","type":"User","site_admin":false},"created_at":"2015-03-03T20:06:22Z","updated_at":"2015-03-03T20:06:22Z","body":"@Eluinhost sounds fantastic. I'd say at this point when you feel like you've tested it enough just go ahead and put up a PR"}}