MongooseAdapter hangs if validation fails

[andrewbranch]

I found another place where the API can hang due to a missing rejection handler: https://github.com/ethanresnick/json-api/blob/master/src/db-adapters/Mongoose/MongooseAdapter.js#L184

The Promise that encapsulates the creation can reject if user’s model validation fails, or if index validation fails (like trying to violate a unique index).

[ethanresnick]

I can't reproduce this so far, and the code looks okay to me. I think the catch function gets called even if the creation promise rejects, because Q.all will propagate that rejection to the catch. Can you post the model and request that triggered this error for you?

[andrewbranch]

Actually, user-defined validation seems to work; but the unique index breaks it. The catch will only catch errors/rejections of the new promise chained on by that then, leaving the rejection of all unhandled.

function PersonSchema() {
  mongoose.Schema.apply(this, arguments);
  this.add({
    name: { type: String, required: true, index: true, unique: true },
    image: { type: mongoose.Schema.Types.ObjectId, ref: 'File', required: true },
    twitterHandle: String,
    birthPlace: String
  });
}

util.inherits(PersonSchema, mongoose.Schema);

module.exports = PersonSchema;

The request that failed was a POST to a discriminator of Person, where another person already had the same name as the one being posted.

[ethanresnick]

Hey Andrew,

To try to reproduce this, I added an index and a unique requirement to Organization model in the json-api-example repo and then POSTed a resource with a name that already existed. Everything seemed to work as expected: I got back a 500 with the correct error. (If anything, the error had a bit too much detail, but that's a separate issue.)

Can you try to reproduce this bug with the example repo, modifying that repo as necessary to show where things break? That way, we're both looking at entirely the same code.

The catch will only catch errors/rejections of the new promise chained on by that then, leaving the rejection of all unhandled.

I'm still pretty sure this isn't right. Try out the following code in Chrome to convince yourself :)

Promise.all([
  new Promise(function(resolve, reject) { setTimeout(reject, 500); })
])
.then(function(it) { return it; }) // some intermediate promise that doesn't matter
.catch(function() { console.log("rejected"); })

[andrewbranch]

I'm still pretty sure this isn't right. Try out the following code in Chrome to convince yourself :)

Huh. I’m convinced. I feel like I’ve run into this before and was surprised to find that this didn’t work the way that, well, it seems to be working now. ¯_(ツ)_/¯

I’ll try to reproduce this soon.

[seawatts]

I'm having this same issue. The response I get back now is:

{
  "errors": [
    {
      "status": "500",
      "title": "An unknown error occurred while trying to process this request."
    }
  ]
}

It would be nice to be able to have the client know what happened so they can show the appropriate error. e.x. "The resource failed validation" or "The resource needs to have the name field be unique"

[ethanresnick]

@seawatts What you're seeing is expected behavior motivated by security concerns. If the JSON API library passed through all errors to the client, that could leak details about the server's implementation and, in the process, make it more vulnerable to an attack. (Granted, security through obscurity—i.e. hiding these details—is never the best method, but it's an extra line of defense.)

If you want to have an error sent to the client, you need to explicitly mark it as "safe" (i.e. as not containing proprietary info), which you can do in two ways. First, you can construct an APIError rather than a generic error. Since all uses of these require an explicit opt-in (i.e. no implementation will throw them out of the box), they're assumed safe. Second, if your error is coming from some existing code, you can attach an isJSONAPIDisplayReady property to it with the value true, and that'll trigger the framework to know that it's safe to display.

Hope that helps! I'm going to close this thread, though, because neither @andrewbranch nor I are able to reproduce the original problem it was about (which is different then yours). Nevertheless, if my fix above doesn't work, feel free to add more comments here.

[seawatts]

Ah yes, sorry I was looking at two similar issues earlier and accidently posted here.

Thanks for the explanation and opening up #86. I'm just curious on how to catch the errors and turn them into APIErrors or mark them as "safe" when they come from a failure in mongoose. i.e. when you POST twice with the same value in a unique property.

[ethanresnick]

I'm just curious on how to catch the errors and turn them into APIErrors or mark them as "safe" when they come from a failure in mongoose... i.e. when you POST twice with the same value in a unique property

Hmmm... When it's a validation error at the Mongoose level, its easy, cause Mongoose lets you specify the error object to use. But when it's an error at the mongo level (like violating a unique constraint), I'm not sure mongoose gives you an easy hook. So I think the solution would be to extend MongooseAdapter with your own subclass, and use that subclass when you initialize the library. I'd also be open to a PR that adds this case into the bundled MongooseAdapter, assuming it outputs an error message that doesn't reference mongo directly (like "The value of the {field} field must be unique across all resources of type {type}, but another resource with its {field} field set to {value} already exists.")

[seawatts]

Interesting, so you are saying you could do something like

require('mongoose').Error = require('json-api').types.Error;

Also that's not a bad idea. I'll look into that.
I'm going to go try to write the delete integration test now.

[ethanresnick]

require('mongoose').Error = require('json-api').types.Error;

Whoops, I didn't mean to imply that! That'll probably break things in Mongoose (like Error instanceof checks). I meant that you could customize the validation error objects that mongoose generates, but it turns out I was misrecollecting on that: you can customize the error message but not the object itself.

The reason mongoose validation errors are output nicely now is because the MongooseAdapter checks for them and let's them through. The same sort of check could be used to let through errors that come from mongo (as opposed to mongoose) constraints, by looking at the error's code.

[seawatts]

Oh, sorry my mistake. I'm pretty new to mongoose, and I saw something similar where someone was trying to override their internal Promise object in that way.

That's interesting about giving more context using the validation property. 👍

I did some looking around and it seems like people are using a plugin to check for this exact case.
Automattic/mongoose#2284
https://www.npmjs.com/package/mongoose-beautiful-unique-validation

This feels kinda weird to me. What are you're thoughts?

[ethanresnick]

I did some looking around and it seems like people are using a plugin to check for this exact case.

Very interesting. Personally, I don't have any issues with people using a plugin to solve this...it's a bit weird, maybe, but it seems to fit with Mongoose's general approach. I'm not sure it'd work with this library, though, because it seems like that plugin requires not using the standard save method?

Regardless, it'd be nice if users of this library didn't have to use another plugin too, so I'd say it'd be preferable to add the check in MongooseAdapter (next to the other check I linked earlier), but it's a low priority for me.

[EmirGluhbegovic]

@ethanresnick

Re the following comment:
"First, you can construct an APIError rather than a generic error. Since all uses of these require an explicit opt-in (i.e. no implementation will throw them out of the box), they're assumed safe. Second, if your error is coming from some existing code, you can attach an isJSONAPIDisplayReady property to it with the value true, and that'll trigger the framework to know that it's safe to display."

The error that is returning 500 is from mongoose, due to dup_email example of one. I want to pass that error message up to the client. It is not a error message I am creating, how do I set isJSONAPIDisplayReady to the APIError class for the requestHandler?

Thank you and regards
Emir