Invalid requests should respond with more detailed info

[ritch]

@raymondfeng @Schoonology @bajtos

Currently we have poor support for responding with validation errors. Below is an overview of what we have and what I think we should move to. Any suggestions would be helpful before making any changes. This might require changes in the iOS SDK, strong-remoting, and the datasource juggler.

• Using a simple Todo model (ex 1).
• Making an HTTP request to create a model (ex 2)
• Current response (ex 3)
• Potential / Improved response (ex 4)
  • Status code 422
  • Body should support multiple errors...

ex 1

app.model(
  loopback
    .memory()
    .createModel('todo', {
      task: 'string',
      done: 'boolean',
      owner: {type: 'string', required: true}
    })
)

ex 2

POST /todos HTTP/1.1
Content-Type: application/json

{
  "task": "testing...",
  "done": false
}

ex 3

HTTP/1.1 400 Bad Request

ex 4

HTTP/1.1 422 Unprocessable Entity
Content-Type: application/json

{
  "errors": {
    "owner": ["owner is required"]
  }
}

[raymondfeng]

I prefer ex 3 with JSON payload to report the errors. For SL remoting, we probably need to have a way to find out what http status code to report based on the error object from the callback.

[Schoonology]

@ritch 👍 for sure. I like the specific response code (for all the reasons listed in the linked RFC).

One change: I wouldn't try to support multiple errors. The logic behind it is tricky, most middleware/libraries don't support it (including node itself - think about callback formats!), and at best it requires more processing on both sides. I'd stick to a single "error".

[ritch]

@Schoonology - the juggler is already capable of returning errors in this format. See isValid(). If it is easy / simple to use this to get the list of errors, I think we should just use it.

[Schoonology]

@ritch - Fair. This comes down to two long-term practices, then, in the interest of consistent consistency:

1. All errors follow the Juggler's format, leading to code like the following:

function remotable(data, callback) {
  doSomething(data, function (err, result) {
    if (err) {
      // Generate erroneous result - this can live in a common/util file, etc.
      callback(null, { errors: [err.message || err] })
      return
    }

    callback({ result: result })
  })
}

1. All errors follow the "other" or "node-y" format, and Juggler's results get changed instead:

function remotable(data, callback) {
  doSomethingFromJuggler(data, function (errors, result) {
    if (errors && errors.length) {
      // Generate erroneous result - this can live in a common/util file, etc.
      callback(null, { error: errors[0] })
      return
    }

    callback({ result: result })
  })
}

Both are valid, and it sounds like we're going for option 1. Does that line up?

[ritch]

Juggler doesn't return an array of errors. The only unconventional method is myModel.isValid(). Its function takes a valid boolean. Calling isValid() updates the model with a wierd/magic errors property. errors is an object keyed by properties (eg. username, email, etc). The values of those properties are arrays of error messages.

I think the juggler api should maintain node's convention. This means if it doesn't currently callback with err for validation errors, it should. Once that is done / verified we just need to add error formatting to strong-remoting.

[bajtos]

My two cents:

👍 for the status code 422 and a JSON body containing error details

Ad single vs. multiple errors:

IMO it's more friendly to users to show all errors at once.

Imagine you are entering a password in a change-my-password form. The first time you are told the new password must be at least 6 characters long, then you are told it must contain at least one number. I find it annoying when you have to iterate on the values to find out all constraints.

Implementation (API)

Regardless of the choice we make about singe vs. multiple validation errors, I am strongly against leaking the details how errors are reported in the REST transport to the users of loopback/strong-remoting.

I am proposing:

1. Add a layer that converts JS errors (passed as the first callback parameter) to HTTP responses. By default, we can return status code 500 and the response body can contain subset of error properties (code, message, etc.)
2. Create a new class ValidationError and modify our HTTP error handler to return 422 on ValidationError.

This is how the user code could look like:

function remotable(data, callback) {
  if (!data.isValid()) {
    // validation error. 
    // loopback/strong-remoting translates the error to HTTP 422.
    callback(new ValidationError(data.errors));
    return;
  }

  doSomething(data, function (err, result) {
    if (err) {
      // possibly any kind of error, e.g. database problem or validation errors.
      // we don't care, because loopback/strong-remoting detects error type
      // and returns appropriate HTTP result
      callback(err);
      return;
    }

    callback({ result: result })
  })
}

I think the juggler api should maintain node's convention. This means if it doesn't currently callback with err for validation errors, it should. Once that is done / verified we just need to add error formatting to strong-remoting.

The solution I proposed above should work in both cases. With the current state of validation in Juggler, user has to do the validation manually as shown in the code above. Once Juggler reports validation errors via callback, users can skip the manual validation. However, they still have the option of adding a custom validation step, e.g.

if (data.user === 'admin' && data.password.length < 20) {
  callback(new ValidationError({
    password: 'Admin\'s pasword must be at least 20 characters long.'
  }));
}

Response JSON

The response format should support different error types, each having a different set of properties. We can start with two properties in the top-level object: error and details.

{
  "error": "ValidationError",
  "details": { 
    "owner": ["owner is required"]
  }
}

{
  "error": "Error",
  "details": {
    "message": "read ECONNRESET",
  }
}

alternatively

{
  "error": "Error",
  "details": "read ECONNRESET",
}