Removes the default write options.

writeConcern defaults to undefined now

Addresses: https://github.com/digitalbazaar/bedrock-mongodb/issues/51

Additionally it turns out this fixes an issue with node mongo driver 4:

  error: MongoInvalidArgumentError: Option "explain" cannot be used on an aggregate call with writeConcern

You can not use explain if writeConcern was added an option. This was tested and with node mongo driver 3 you can have explain with writeConcern, but you can not with node mongo driver 4. Hence this PR now is a necessary feature in the upgrade.

Codecov Report

Merging #96 (f26185a) into mongo-driver-4-rc (944063f) will decrease coverage by 0.09%. The diff coverage is 100.00%.

@@                  Coverage Diff                  @@
##           mongo-driver-4-rc      #96      +/-   ##
=====================================================
- Coverage              87.08%   86.98%   -0.10%     
=====================================================
  Files                      8        8              
  Lines                    813      807       -6     
=====================================================
- Hits                     708      702       -6     
  Misses                   105      105

Impacted Files	Coverage Δ
lib/config.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Can you add a changelog entry that explains how this doesn't break things? For example, how is the write concern configured / applied if not via this config setting? It's vital that our connections always use the settings that are being removed here.

Docs for writeConcern are here

Starting in MongoDB 4.4, replica sets and sharded clusters support setting a global default write concern. Operations which do not specify an explicit write concern inherit the global default write concern settings. See setDefaultRWConcern for more information.

The implicit writeConcern is basically just w: "majority" unless there is a replica set involved as of Mongo Server 5.

As for the journal option: as long as w is majority j defaults to true. So it looks like removing writeConcertn works because the default writeConcern as of Mongo Server 5 replicates our existing writeConcern in development. When using mongo atlas configs with replica sets with arbiters the default could not be w: majority resulting in a different behavior for both w and j.

I had planned on adding a minor feature which is a new method database.explainOperation. It might be possible to remove all the explain instances with this new method and then when doing that explainOperation we could suspend passing in writeConcern which would allow explain with out having to remove writeConcern as a global. The issue this PR addresses suggested that we wanted to remove global writeConcern.

One additional note: we do not set this option: wtimeout, but the default from mongo seems to have worked for us.

So these are the options

[ ] trust the Mongo's default writeConcern is smart enough to handle our use cases (hence this PR is good)
[ ] add a new method explainOperation that gets a cursor with out writeConcern specified and returns the explain.
[ ] We don't seem to use explain in production so the README could be update with instructions on how to remove writeConcern in the case that explain needs to be used.
[ ] We don't seem to use explain in production so we could detect if bedrock.test is occurring and remove the writeConcern when testing.
[ ] We could add a config option config.mongodb.explain = true; that suspends writeConcern as long as that option is true.

Check your preference(s).

@aljones15,

I think we need to separate what we do with write concern from what we do with any potential problems with explain. The former is important to get right first -- and then we can deal with whatever needs to happen for the explain use cases.

If we are not going to specify write concern config options any more, then we need to determine whether we require a mongodb 5.0 or greater for this major release. That version defaults to a write concern of w: majority. Our changelog will need to be very clear that default write concern must be set prior to upgrading to this next major release (either implicitly or explicitly). We should list what will happen by default if nothing is done (this part depends on the mongodb server versions supported). If we only support 5.0+, we can just say what the default is and that if something else is needed, it must be set prior to upgrading because we do not support a per-driver (per-connection pool) setting anymore.

Also, it looks like setDefaultRWConcern is unavailable in standalone mode -- which is not a problem for the w option (since w: 1 is the same thing as w: majority in standalone mode and these are the two defaults for < 5.0 and >= 5.0, respectively), but it does appear to be a problem for the j option (journal write acknowledgement).

We have applications that run in standalone mode (and most, if not all, dev environments do this) and need journal acknowledgement set to true. In standalone mode, when the write concern is w: 1, j will default to false (in-memory acknowledgement only):

https://www.mongodb.com/docs/manual/reference/write-concern/#standalone

This is not a problem when the default write concern is w: majority as j will default to true ... when journaling is on, which it is by default for the WiredTiger backend at least. This implies we should also say that we only support WiredTiger in standalone mode for this reason. This latter bit isn't a problem for any of our applications to my knowledge.

However, the default write concern prior to mongodb 5.0 is w: 1. AFAICT, this means that using mongodb 4.4 in standalone mode without configuring per-driver/per-connection write concern would result in an unacceptable outcome. Unless you can see some other way that mongodb 4.4 can be run in standalone mode with the proper settings, we will need to say this major release is mongodb 5.0+ only (or that mongodb 4.4 can only work in non-standalone mode) if we disable configuring the write concern options.

I'm fine with that, it seems like the best trade off. I'd rather reduce the number of configurable options, especially those that are dangerous like these. But, please let me know if you see another way that mongodb 4.4 can work in standalone mode.

If we go this way, we should prohibit setting write concern via the config (throw some kind of error) to let people know to remove the options if they have set them. Also, it seems that if we make this choice, the explain issue (whatever it was -- I didn't look into it closely) may become irrelevant.

Just so this is known: There are several ways of passing writeConcern to Mongo.

via initializing MongoClient (we pass writeConcern to MongoClient.connect`)
via an individual operation such as insertOne (this pattern is inconsistent in our codebase)

We could remove the global writeConcern passed to MongoClient and instead just rely on writeConcern being passed to individual operations. Building on this: we could wrap each individual operation, ensuring it passes the default writeConcern unless a newer writeConcern is specified. The later pattern would remove the inconsistency in our codebase (some libraries pass writeConcern to operations and others don't). Additionally using the wrap method would let us omit writeConcern if explain: true.

@aljones15,

via an individual operation such as insertOne (this pattern is inconsistent in our codebase)

The reason this is inconsistent in the code base is twofold:

We are in the process of removing the pattern everywhere we can and may have missed some places.
Some places still required the options to be passed because of bugs in some version of the 3.x driver -- and these may have been resolved.

We could remove the global writeConcern passed to MongoClient and instead just rely on writeConcern being passed to individual operations.

No, I don't think we don't want to do this -- we specifically moved away from it because it's so error prone -- and we (perhaps without exception) don't change the default config options we've set today in any applications.

Building on this: we could wrap each individual operation, ensuring it passes the default writeConcern unless a newer writeConcern is specified. The later pattern would remove the inconsistency in our codebase (some libraries pass writeConcern to operations and others don't). Additionally using the wrap method would let us omit writeConcern if explain: true.

This would be hard to maintain and error prone -- and would introduce another layer of otherwise unnecessary abstraction.

I think the best solution is to remove the need for anyone to mess with these options in applications by having the server provide defaults that are good for all (or nearly all) applications. The next best solution, which we've tried to do today, is to use write concern at the MongoClient level and not anywhere else by default. These two approaches allow us to not mess with writeConcern at all in almost every call in our applications. The only time we'd do something different would be for operations in the code where we explicitly (and we include comments where we do this) need to use a less safe writeConcern. We don't have any of those instances so far, to my knowledge. The explain calls could become an exception to this, but only if we find that it's necessary to both support mongodb 4.4 and that, when using that version, we need to configure MongoClient with writeConcern from the config.

So, if we can just remove the option from the config all together and rely on the server using the right settings, that's best here. This still allows applications to create special-purpose calls that need different options if they really need it, but we haven't had to do that yet.

digitalbazaar / bedrock-mongodb

Remove defaults for writeOptions from config. #96

Codecov Report