Conflict error for compound indexes is not correct

[cdimitroulas]

Steps to reproduce

1. Create a compound index for your collection:

const mongoose = require('mongoose')

const schema = new mongoose.Schema({
  // random fields used as an example
  firstId: { type: mongoose.Types.ObjectId },
  secondId: { type: mongoose.Types.ObjectId }
})

schema.index({ firstId: 1, secondId: 1 }, { unique: true })

2. Create two documents, both with the same firstId and secondId field values and inspect the error message returned from mongoose. It will look something like this:
   firstId: ObjectId(\'5b891cf30d74a9432c0ee036\'), : ObjectId(\'587775b068194a6b3c818dce\') already exists.

Expected behavior

Feathers mongoose should handle a 1100 or 11001 MongoDB error and display a useful error message for the user of the library

Actual behavior

This works fine for regular indexes, but when there is a compound index the error message is rather
obscure.

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working):
feathers-mongoose: 6.1.2
mongoose: 4.13.14

NodeJS version: 8.11.2

Operating System: Ubuntu 16.04

[daffl]

firstId: ObjectId(\'5b891cf30d74a9432c0ee036\'), : ObjectId(\'587775b068194a6b3c818dce\') already exists. as an error message is not really incorrect is it?

Since everybody has a different idea what "useful" means, most adapters don't customize database error messages. The best way to customize errors to your needs is by using Feathers error hooks.

[cdimitroulas]

It is incorrect, it should say firstId: ObjectId("..."), secondId: ObjectId("..."). The key before the second ObjectId is blank

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Apologies if the issue could not be resolved. FeathersJS ecosystem modules are community maintained so there may be a chance that there isn't anybody available to address the issue at the moment. For other ways to get help see here.

[cdimitroulas]

Reminder to self to have a stab at a PR for this. Unless @daffl you really believe this is not an issue and the error message is correct (in which case, could you explain to me how I'm misreading it?)

[daffl]

No, that's fair. Is that a problem in the conflict error handler part (https://github.com/feathersjs-ecosystem/feathers-mongoose/blob/master/lib/error-handler.js#L4)?

[cdimitroulas]

Yes, I believe there's something wrong or incomplete with the regex there.

[cdimitroulas]

@daffl I don't think it's possible to format the error message easily with simple regex. I have written an implementation of how to solve this particular bug that I describe in this issue, but it would not work for compound indexes with more than 2 fields and I believe my implementation would also fail in cases where the fields names contain both underscores and digits (e.g. address_1)

You can have a look at it here https://github.com/cdimitroulas/feathers-mongoose/pull/1
(I didn't want to make a PR to the real feathers-mongoose repo as I don't believe this code is of a good enough quality to make it into your library).

In order to solve this properly, I think we would need to query for the index information (this is the approach that the mongoose-beautiful-unique-validation library takes for formatting E11000 errors)

[daffl]

Might make sense to make the Regex only apply to the simple case that it actually covers then. Parsing the error message probably wasn't a good idea to begin with (strangely it is what Mongoose appears to recommend in the issues related to that) but I don't want to make another breaking change for something that might make things less user friendly.

[cdimitroulas]

OK, that makes sense.

My only other suggestion would be to make the original error message from MongoDB available (maybe a property like .originalMessage on the Error). That way in the cases where the formatted message from feathers-mongoose seems strange, the user can always go and check the raw MongoDB error message.

[cdimitroulas]

@daffl ^ what do you think of my proposed idea?

[daffl]

We probably could do the same thing the new Sequelize adapter is doing and add a hidden property with the original error (https://github.com/feathersjs-ecosystem/feathers-sequelize#migrating).

[daffl]

It basically just adds the original error in a Symbol error property (see https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/utils.js) so it can be accessed and used in the server-side error handler.