only one configuration should exist per service

[DanielDent]

Follow-up to pull request #3290.

[glasser]

Thanks for the PR! This is fixing a real issue.

I don't think dropDups is a great idea here. After all, if you've gotten your DB into the corrupted state where it matters (multiple registrations) you probably actually want to choose between the two configurations rather than silently losing one of them. I think we'd prefer not having dropDups here. But I also want to make sure that this ends up giving you an error.

Can you remove dropDups from the PR, and then show us what it looks like when you run Meteor with your new PR against a database that has a duplicate service configuration? (Ideally it will stop your app from running and have a message that is at least vaguely comprehensible. If the message is not comprehensible, we could add a try/catch/rethrow around this new line.)

[DanielDent]

While I'm not sure I can come up with a scenario where data of value is lost (this is an interface for specifying service configuration information where the data is supposed to be loaded each time the application is started), I firmly agree with the general sentiment that if there is a way to avoid deleting data silently, that is usually the better choice.

The tradeoff here is that it will create deployment risk for people. The most likely situation where this bug has actually happened is in people's production environments where they have lots of Meteor instances running on a busy site. If they don't have adequate dev/staging/prod parity, they won't catch this issue in testing. Their new release after updating to the latest Meteor will simply fail to start. That was the tradeoff that was in my head when I proposed this approach. But, it's also true that a little downtime is fixable, while data loss maybe wouldn't be. And hopefully if they care about service availability they are using canary instances as part of rolling something into production (which would avoid user-visible service degradation).

The error message that gets generated when there is a duplicate service configuration is decent, as long as your MongoDB foo is strong enough. But then again _ensureIndex is a private call and the entire concept of indexes are outside the scope of the documented Meteor interface to Mongo, even though in practice they may be required for any reasonably sized Meteor deployment. Is it safe to assume our users understand what a MongoDB index is?

W20150127-13:59:29.148(-8)? (STDERR)
W20150127-13:59:29.150(-8)? (STDERR) LONG_METEOR_PATH_HERE/dev_bundle/server-lib/node_modules/fibers/future.js:206
W20150127-13:59:29.150(-8)? (STDERR) throw(ex);
W20150127-13:59:29.151(-8)? (STDERR) ^
W20150127-13:59:29.243(-8)? (STDERR) MongoError: E11000 duplicate key error index: meteor.meteor_accounts_loginServiceConfiguration.$service_1 dup key: { : "twitter" }
W20150127-13:59:29.244(-8)? (STDERR) at Object.Future.wait (LONG_METEOR_PATH_HERE/dev_bundle/server-lib/node_modules/fibers/future.js:326:15)
W20150127-13:59:29.244(-8)? (STDERR) at [object Object].MongoConnection._ensureIndex (packages/mongo/mongo_driver.js:689:1)
W20150127-13:59:29.245(-8)? (STDERR) at [object Object].Mongo.Collection._ensureIndex (packages/mongo/collection.js:619:1)
W20150127-13:59:29.245(-8)? (STDERR) at app/lib/test.js:4:41
W20150127-13:59:29.245(-8)? (STDERR) at app/lib/test.js:9:3

[glasser]

Hmm, yeah, maybe we should wrap that with a throw/catch/rethrow with an extra error explaining "you have more than one configuration for a service in this collection; go to the database and remove one"? Maybe it can even just link to this PR and we can edit the description to be helpful instructions? (It does crash the process, right?)

[DanielDent]

OK, I've updated my patch. Here is how it crashes now:

W20150128-14:48:55.306(-8)? (STDERR) The service-configuration package persists configuration in the meteor_accounts_loginServiceConfiguration collection in MongoDB. As each service should have exactly one configuration, Meteor automatically creates a MongoDB index with a unique constraint on the meteor_accounts_loginServiceConfiguration collection. The _ensureIndex command which creates that index is failing.
W20150128-14:48:55.308(-8)? (STDERR)
W20150128-14:48:55.308(-8)? (STDERR) Meteor versions <= 1.0.3.1 did not create this index. If you recently upgraded and are seeing this error message for the first time, please check your meteor_accounts_loginServiceConfiguration collection for multiple configuration entries for the same service and delete configuration entries until there is no more than one configuration entry per service.
W20150128-14:48:55.308(-8)? (STDERR)
W20150128-14:48:55.309(-8)? (STDERR) If the meteor_accounts_loginServiceConfiguration collection looks fine, the _ensureIndex command is failing for some other reason.
W20150128-14:48:55.309(-8)? (STDERR)
W20150128-14:48:55.309(-8)? (STDERR) For more information on this history of this issue, please see #3514.
W20150128-14:48:55.309(-8)? (STDERR)
W20150128-14:48:55.309(-8)? (STDERR)
W20150128-14:48:55.309(-8)? (STDERR) /path/to/meteor/dev_bundle/server-lib/node_modules/fibers/future.js:206
W20150128-14:48:55.309(-8)? (STDERR) throw(ex);
W20150128-14:48:55.309(-8)? (STDERR) ^
W20150128-14:48:55.310(-8)? (STDERR) MongoError: E11000 duplicate key error index: meteor.meteor_accounts_loginServiceConfiguration.$service_1 dup key: { : "twitter" }

Some notes:
(1) The project guidelines request 80 character wrapping for output from the meteor tool using Console, however the Console object doesn't appear to be available to packages and it doesn't appear like it would be appropriate to require Console in a package.
(2) Minimongo doesn't support indexes and this code is really only relevant on the server. I've moved this code to a new file which only executes on the server.
(3) I bumped the version to a new major release. The API for this package is not especially well documented, but prior versions of this package would have allowed you to do things like inserting a configuration prior to removing an old configuration. Users of the package who do that now will find that their code breaks. As such I don't think this package is API compatible and a new major release is warranted.
(4) The error message makes reference to the current Meteor release. If this patch does not make it to the next stable release, the error message will be invalid.

Kindly point out any errors I may have made in following Meteor's coding conventions. This is my first contribution of code (my last contribution was for documentation on this issue).

[glasser]

Thanks, merged!

I think this is effectively a bugfix so am not bumping the major version number.

[DanielDent]

It was my understanding that it is a goal of Meteor to have packages follow semver.

"Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented."

This bugfix changes the public API of the package in a way which will cause breakage for some users. Somebody that was happily using "add" and then "remove" in their program's startup will find their program now breaks.

[glasser]

Their program was already broken since it was running with a nondeterministic set of credentials.