Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
921 stars 168 forks source link

createRole : E11000 uncatched with mongo v3.2.4 results in a failing unlessExists option #185

Closed jsamr closed 8 years ago

jsamr commented 8 years ago

Reason is the matching expression /E11000 duplicate key error index: ([^ ]+)/ missing the message returned by this 3.2.4 mongo version : "E11000 duplicate key error collection: XXX.roles index: _id_ dup key: { : "ADMIN" }"

mitar commented 8 years ago

Yes, it seems there are some changes between versions on the error string for duplicate keys. I had to fix this in one other my package: https://github.com/peerlibrary/meteor-packages/commit/3c937bd5465e2edcf36e33c741b00e630a3c10f5

jsamr commented 8 years ago

How about just checking for a /^E11000 .*/ match? This error code should be constant across all versions.

mitar commented 8 years ago

I think we also want to know that it is for an expected index (field) name. There might be some other unique indexes on the collection. We do not want to hide errors we should not be hiding.

jsamr commented 8 years ago

Ok. The expression /^E11000 duplicate key error.* index: .*_id_/ works for 3.0 and 3.2, and we do match the expected index exclusively (_id_), but I don't know for bellow versions.

mitar commented 8 years ago

I fixed this in v2.0 branch. But this should probably be fixed also in the 1.0 branch.

jedwards1211 commented 8 years ago

Ah, just as I suspected: https://github.com/alanning/meteor-roles/issues/193

I really think we should use \bE?11000\b in place of E11000 in case 11000 occurs in the middle of some other number or they drop the E. (I've noticed the error messages contain both "11000" and "E11000")

jedwards1211 commented 8 years ago

@mitar what other unique indexes might there be, and wouldn't they only be violated by adding a role that already exists anyway?

jedwards1211 commented 8 years ago

Actually the error has a code property with value 11000, so it would be probably better to check that err.code === 11000.

jedwards1211 commented 8 years ago

The properties of the error when using 1.2.13 with Meteor 1.0.5 (I know, old project) are:

name: "MongoError"
code: 11000
err: "insertDocument :: caused by :: 11000 E11000 duplicate key error index: meteor.roles.$name_1  dup key: { : "admin" }"

So I think the best test would be:

if (err.code === 11000 && 
    new RegExp("index:\\W+meteor\\.roles\\b.*dup(licate)?\\s+key:\\W+" + role + "\\b").test(e.err || e.errmsg)) {
  ...
}
mitar commented 8 years ago

Are you reporting the issue with v1.0 or v2.0? Because in v2.0 this was fixed to my knowledge.

mitar commented 8 years ago

Fixed on v1.0 with 79250d651613fb9868a0c5b1b343f63294e32c61.

To my knowledge this now works on both v1.0 and v2.0 on all MongoDB versions. If this is not so, please tell me.

jedwards1211 commented 8 years ago

Honestly I've been using 1.0 in some projects and 2.0 in others, so it's possible I got the error from 1.0. In any case I was trying to recommend something that would be more immune to future changes in the error message.

mitar commented 8 years ago

What I think this should be a package which would extend Meteor MongoDB collection to add one more method which would allow you do "insert if not duplicate" insert, and then all packages could just use that one.

jedwards1211 commented 8 years ago

You know what? We should probably be using upsert with a $setOnInsert clause instead of insert. That way we wouldn't even get an error if something already exists. That's the top answer for a similar question on SO: http://stackoverflow.com/questions/2801008/mongodb-insert-if-not-exists

jedwards1211 commented 8 years ago

@mitar I'm putting together a PR that uses upsert

mitar commented 8 years ago

Hm, you are right! I am not really sure why we were using this pattern. Maybe old Meteor does not have upsert? When was upsert introduced?

jedwards1211 commented 8 years ago

No idea, obviously I went for several days without realizing upsert was the best way to do this, it wasn't until I read that SO post that I realized it's the best pattern

jedwards1211 commented 8 years ago

Aha, according to https://docs.mongodb.org/manual/reference/operator/update/setOnInsert/, $setOnInsert was introduced in 2.4, so that's one essential feature that used to be missing

mitar commented 8 years ago

Should we then backport this to 1.0 as well?

I am not sure which version of Meteor we want to support with 1.0.

(And thanks for finding it. I then changed this in all my packages yesterday. :-) (Sadly, it was not possible for all.)

jedwards1211 commented 8 years ago

The lowest version I've ever used is 1.0.5, and I know it has upsert. Haven't double checked with 1.0.0 yet

Floriferous commented 5 years ago

I started seeing this issue again, the error is the following:

I20190117-18:02:01.474(1)? { BulkWriteError: E11000 duplicate key error collection: meteor.roles index: name_1 dup key: { : "user" }
I20190117-18:02:01.474(1)?     at OrderedBulkOperation.handleWriteError (/Users/Florian/.meteor/packages/npm-mongo/.3.1.1.1s4wr2v.l5o9++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb/lib/bulk/common.js:1048:11)
I20190117-18:02:01.474(1)?     at resultHandler (/Users/Florian/.meteor/packages/npm-mongo/.3.1.1.1s4wr2v.l5o9++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb/lib/bulk/ordered.js:159:23)
I20190117-18:02:01.474(1)?     at /Users/Florian/.meteor/packages/npm-mongo/.3.1.1.1s4wr2v.l5o9++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb-core/lib/connection/pool.js:532:18
I20190117-18:02:01.499(1)?     at _combinedTickCallback (internal/process/next_tick.js:131:7)
I20190117-18:02:01.499(1)?     at process._tickDomainCallback (internal/process/next_tick.js:218:9)
I20190117-18:02:01.499(1)?   index: 0,
I20190117-18:02:01.499(1)?   code: 11000,
I20190117-18:02:01.499(1)?   errmsg: 'E11000 duplicate key error collection: meteor.roles index: name_1 dup key: { : "user" }',
I20190117-18:02:01.499(1)?   op: { name: 'user', _id: 'vg578ZwJP9hMir2Zo' },
I20190117-18:02:01.500(1)?   name: 'BulkWriteError',
I20190117-18:02:01.500(1)?   driver: true,
I20190117-18:02:01.500(1)?   err:
I20190117-18:02:01.500(1)?    { index: 0,
I20190117-18:02:01.500(1)?      code: 11000,
I20190117-18:02:01.500(1)?      errmsg: 'E11000 duplicate key error collection: meteor.roles index: name_1 dup key: { : "user" }',
I20190117-18:02:01.500(1)?      op: { name: 'user', _id: 'vg578ZwJP9hMir2Zo' } },
I20190117-18:02:01.502(1)?   result:
I20190117-18:02:01.502(1)?    BulkWriteResult {
I20190117-18:02:01.502(1)?      result:
I20190117-18:02:01.502(1)?       { ok: 1,
I20190117-18:02:01.502(1)?         writeErrors: [Array],
I20190117-18:02:01.502(1)?         writeConcernErrors: [],
I20190117-18:02:01.503(1)?         insertedIds: [Array],
I20190117-18:02:01.503(1)?         nInserted: 0,
I20190117-18:02:01.503(1)?         nUpserted: 0,
I20190117-18:02:01.503(1)?         nMatched: 0,
I20190117-18:02:01.503(1)?         nModified: 0,
I20190117-18:02:01.503(1)?         nRemoved: 0,
I20190117-18:02:01.503(1)?         upserted: [],
I20190117-18:02:01.503(1)?         lastOp: [Object] } },
I20190117-18:02:01.503(1)?   [Symbol(mongoErrorContextSymbol)]: {} }

I think the issue here is that e.err is an object, so the regex fails to do the additional test on e.errmsg which would prevent the error from being thrown further.

Could we inverse the order:

if (/E11000 duplicate key error.*(index.*roles|roles.*index).*_id/.test(e.errmsg || e.err))

or

if (/E11000 duplicate key error.*(index.*roles|roles.*index).*_id/.test(e.err) || 
/E11000 duplicate key error.*(index.*roles|roles.*index).*_id/.test(e.errmsg)
) {

}