refactor: unify means of applying `ignoreUndefined` #2589

HanaPearlman · 2020-10-19T15:43:54Z

Description

What changed?
Previously, each operation in collection.ts needed to do the necessary logic to figure out which BSONSerializationOptions values to include in the options. Some operations omitted this logic completely, while others did it incorrectly. This change stores the BSONSerializationOptions as a new inheritable property on MongoClient, Db, Collection, and OperationBase, and makes it accessible via a new bsonOptions getter for each class.

NODE-1561

src/operations/command.ts

HanaPearlman · 2020-10-19T19:14:49Z

src/operations/find.ts

-      promoteValues: options.promoteValues ?? collection.s.promoteValues ?? true,
-      promoteBuffers: options.promoteBuffers ?? collection.s.promoteBuffers ?? false,
-      ignoreUndefined: options.ignoreUndefined ?? collection.s.ignoreUndefined ?? false
-    };
  }


This logic now occurs in CommandOperation. Nothing else needs to change in this file; the execute method here already includes this.bsonOptions in its options.

nbbeeken

just a few nits and 🧅 s, nice work!

nbbeeken · 2020-10-20T17:10:37Z

src/bson.ts

+
+// Merge the given BSONSerializeOptions, preferring options over parentOptions, and substituting
+// defaults for values not set.
+export function inheritOrDefaultBSONSerializableOptions(


The comments above are useful, nice work you can go ahead and make it a docstyle comment

/** * ... * @internal */

Then it will be readable by just hovering it anywhere in the codebase, also you can make it internal.

Also fine with it as is but may I suggest the name: inheritBSONOptions I think its fine to just state they will be defaulted in the docs.

nbbeeken · 2020-10-20T17:13:18Z

src/operations/command.ts

+    const base = Object.assign({}, parent?.s.options, parent?.s);
+    Object.assign(this, inheritOrDefaultBSONSerializableOptions(options, base));


We can inline the base here like so:

Object.assign(this, inheritOrDefaultBSONSerializableOptions(options, {...parent.s.options, ...parent?.s}));

nbbeeken · 2020-10-20T17:15:18Z

src/operations/command.ts

+    // Assign BSON serialize options to OperationBase, preferring options over parent options.
+    // base accounts for the fact that Collection stores bson options in s, while other parents,
+    // like Db, stores bson options in s.options


Maybe this is cause to change that? I wonder how much it would effect to make collection keep its bsonOptions in options, or have db spread them on to s.

That could make this into a lot of work, so total throwaway comment if so, but curious if you have insight on what that would entail?

I don't think it would have a strong effect at all to move the Collection bsonOptions to s.options-- as far as I can tell, very few places were using the Collection bsonOptions on s, and I deleted most of them in this PR. Would be happy to make this change.

As for spreading other parent bsonOptions onto s, Db was just one example. I think we would also have to change cursor too, and possibly others.

I think that would be nice for consistency (the first proposal)
I'm thinking now the original reason that collection might pick off its own copy of these options is to make sure that it doesn't edit its parent's options, I took a quick look around and it seems like collection does get its own separate options object but that could be a bug you run into if you give this a go, the options prop that collections uses shouldn't be the same reference to Db's options object.

nbbeeken · 2020-10-20T17:18:11Z

src/operations/delete.ts

-    const options = this.options;
+    const options = Object.assign({}, this.options, this.bsonOptions);


I won't comment on every instance to spare you the emails, but I think we should prefer the spread operator to assign: const options = {...this.options, ...this.bsonOptions}. But not a strong opinion, and spread doesn't work in every case, like when assigning onto 'this

nbbeeken · 2020-10-20T17:20:46Z

test/functional/insert.test.js

+    // Add a tag that our runner can trigger on
+    // in this case we are setting that node needs to be higher than 0.10.X to run


this comment looks left over? there's nothing about 0.10.x here

reggi · 2020-10-20T20:18:42Z

src/operations/bulk_write.ts

@@ -24,18 +25,15 @@ export class BulkWriteOperation extends OperationBase<BulkWriteOptions, BulkWrit

    this.collection = collection;
    this.operations = operations;
+
+    // Assign BSON serialize options to OperationBase, preferring options over collection options
+    Object.assign(this, inheritBSONOptions(options, collection.s));


Total opinons here:

I'd love some consistency if possible. It seems that collection has it on .s but bulk_write is on the class itself.

Also using Object.assign(this... is super convenient but usually shy away from it to ensure types not being violated.

Here I'd prefer:

this.bsonOptions = inheritBSONOptions(options, collection.s));

or

const bsonOptions = inheritBSONOptions(options, collection.s)); this.raw = bsonOptions.raw; this.promoteLongs = bsonOptions.promoteLongs; this.promoteValues = bsonOptions.promoteValues; this.promoteBuffers = bsonOptions.promoteBuffers; this.ignoreUndefined = bsonOptions.ignoreUndefined; this.serializeFunctions = bsonOptions.serializeFunctions; this.fieldsAsRaw = bsonOptions.fieldsAsRaw;

Your point about consistency is great, I definitely struggled with that for these changes.

It seems to me that for the Collection/Db/Client, the bson options must be somewhere in .s, but classes extending OperationBase don't (currently) have a .s, so unless that changes there can't be perfect consistency...

Here is how I was thinking about it: Collection, Db, and MongoClient could all store the bson options in s.options, while anything extending OperationBase has the bson options on the class itself. As for having a this.bsonOptions property for OperationBase instead of having all of the values directly on the class, I would be Ok with this! The rational for some of this seemed to come out of @nbbeeken's work-- wondering if he has any thoughts on this.

emadum

Had a few minor comments about cleaning up the new tests to our modern style, but this LGTM other than that, nice work!

emadum · 2020-10-21T14:48:11Z

test/functional/ignore_undefined.test.js

+    metadata: { requires: { topology: ['single'] } },
+
+    test: function (done) {
+      var configuration = this.configuration;


Nit: while we don't go through and update old tests systematically, we generally try to change the old var style of declaration to the modern const/let when adding new tests.

emadum · 2020-10-21T14:52:34Z

test/functional/ignore_undefined.test.js

+        poolSize: 1,
+        ignoreUndefined: false
+      });
+


At this point you have a client, so you could use the withClient helper here and avoid needing to connect or close the client, e.g. return withClient.call(this, client, (client, done) => { /* test */ });

emadum · 2020-10-21T14:57:41Z

test/functional/ignore_undefined.test.js

+
+  it(
+    'Should correctly inherit ignore undefined field from collection during insert',
+    withClient(function (client, done) {


It looks like the other tests in this file only run on single topology via the metadata; is there a reason this test and the subsequent ones don't?

Nope, there is no reason! I guess I didn't see exactly why that was needed, so I didn't include it. Will make this change in case there is something I am missing.

I think in some cases we limit very basic tests that don't depend on replica sets or sharded clusters to just run on the single topology, because we have extra tests that we only run on replica/sharded, so those tasks take longer to complete in Evergreen. Limiting tests to single when possible is a way we can cut down on the wait time (and cost!) of running our tests. @mbroadst might be able to provide more background about this.

Oops, I just left a comment about this below. I think the rule of thumb should actually be that we run tests everywhere when possible, so we maximize our coverage. Let's tackle the runtime of the tests at a later date, we can likely make a number of improvements to speed things up while maintaining as much coverage as possible across multiple cluster types

emadum · 2020-10-21T14:59:56Z

test/functional/ignore_undefined.test.js

+
+          // Locate the doument
+          collection.findOne((err, item) => {
+            test.equal(1, item.a);


Similar to using const/let for new tests, we also try to use the chai style of expecations. Here it would be e.g. expect(item).to.have.property('a', 1);

emadum

LGTM 👍

reggi · 2020-10-22T14:12:25Z

src/operations/bulk_write.ts

@@ -24,18 +25,15 @@ export class BulkWriteOperation extends OperationBase<BulkWriteOptions, BulkWrit

    this.collection = collection;
    this.operations = operations;
+
+    // Assign BSON serialize options to OperationBase, preferring options over collection options
+    Object.assign(this, inheritBSONOptions(options, collection.s.options, true));


I don't think we should use Object.assign on this it does not give any information just by looking at this how this is being manipulated.

Alternative examples in #2589 (comment)

src/operations/command.ts

src/bson.ts

mbroadst

LGTM! I left one minor nit about test structure. 👏 great job on this work

mbroadst · 2020-10-23T13:26:28Z

test/functional/ignore_undefined.test.js

@@ -148,4 +149,132 @@ describe('Ignore Undefined', function () {
      });
    }
  });
+
+  it('Should correctly inherit ignore undefined field from db during insert', {
+    metadata: { requires: { topology: ['single'] } },


Is there a reason to only test against a standalone? If you want to test on any topology type/server version then you can omit the metadata completely:

if('should correctly inherit ignoreUndefined from db during insert', function(done) { ... });

reggi

LGTM

HanaPearlman added 7 commits October 16, 2020 10:59

store properly inherited bson options in op base

bc2e9d1

remove ignore undefined checks from deprecated coll ops and db

9ce2c3f

first step to handle all parent impls

6a20101

Merge branch 'master' into NODE-1561/master/unify-ignoreUndefined

39d9d2d

add tests to check bson option inheritance

14e0cdf

include serialize fns and fields as raw in bson opts

f30ca60

small clean up and more tests

2c79f70

HanaPearlman force-pushed the NODE-1561/master/unify-ignoreUndefined branch from a3d7338 to 2c79f70 Compare October 19, 2020 16:54

small cleanup

dcffd13

HanaPearlman commented Oct 19, 2020

View reviewed changes

src/operations/command.ts Outdated Show resolved Hide resolved

HanaPearlman commented Oct 19, 2020

View reviewed changes

HanaPearlman requested review from mbroadst, nbbeeken, emadum and reggi and removed request for mbroadst and nbbeeken October 19, 2020 19:35

HanaPearlman marked this pull request as ready for review October 19, 2020 19:36

HanaPearlman force-pushed the NODE-1561/master/unify-ignoreUndefined branch 2 times, most recently from cedf80c to 38efad2 Compare October 19, 2020 20:48

use withClient for some tests

c582310

HanaPearlman force-pushed the NODE-1561/master/unify-ignoreUndefined branch from 38efad2 to 59a0db7 Compare October 20, 2020 13:21

fix utils bug and test

72f6794

HanaPearlman force-pushed the NODE-1561/master/unify-ignoreUndefined branch from 59a0db7 to 72f6794 Compare October 20, 2020 13:35

nbbeeken suggested changes Oct 20, 2020

View reviewed changes

rename helper and add docs comments

8a75dc8

HanaPearlman force-pushed the NODE-1561/master/unify-ignoreUndefined branch from 7ce8201 to 8a75dc8 Compare October 20, 2020 18:58

reggi reviewed Oct 20, 2020

View reviewed changes

move collection bson opts to s.options

244c846

emadum suggested changes Oct 21, 2020

View reviewed changes

respond to test nits

4baafa6

HanaPearlman requested review from emadum and nbbeeken October 21, 2020 17:44

more idiomatic property test

8db6961

HanaPearlman requested a review from reggi October 21, 2020 17:50

emadum approved these changes Oct 21, 2020

View reviewed changes

reggi suggested changes Oct 22, 2020

View reviewed changes

store bson options in single property

7e111c1

HanaPearlman requested a review from reggi October 22, 2020 17:59

mbroadst reviewed Oct 22, 2020

View reviewed changes

src/operations/command.ts Outdated Show resolved Hide resolved

src/bson.ts Outdated Show resolved Hide resolved

src/bson.ts Outdated Show resolved Hide resolved

put bsonOptions property on parents

e436fba

HanaPearlman force-pushed the NODE-1561/master/unify-ignoreUndefined branch from 5b360ed to e436fba Compare October 22, 2020 22:35

HanaPearlman requested a review from mbroadst October 23, 2020 12:10

mbroadst approved these changes Oct 23, 2020

View reviewed changes

remove metadata from ignore undefined tests

b81c309

reggi approved these changes Oct 26, 2020

View reviewed changes

nbbeeken approved these changes Oct 27, 2020

View reviewed changes

HanaPearlman merged commit b4ec3ed into mongodb:master Oct 27, 2020

HanaPearlman deleted the NODE-1561/master/unify-ignoreUndefined branch October 27, 2020 21:49

HanaPearlman restored the NODE-1561/master/unify-ignoreUndefined branch October 27, 2020 21:52

HanaPearlman deleted the NODE-1561/master/unify-ignoreUndefined branch October 27, 2020 22:03

HanaPearlman mentioned this pull request Oct 27, 2020

fix: apply bson options for bulk operations #2601

Merged

emadum mentioned this pull request Dec 9, 2020

fix: honor ignoreUndefined on findAndModify commands #2671

Merged

		const base = Object.assign({}, parent?.s.options, parent?.s);
		Object.assign(this, inheritOrDefaultBSONSerializableOptions(options, base));

		const options = this.options;
		const options = Object.assign({}, this.options, this.bsonOptions);

		// Add a tag that our runner can trigger on
		// in this case we are setting that node needs to be higher than 0.10.X to run

refactor: unify means of applying ignoreUndefined #2589

refactor: unify means of applying ignoreUndefined #2589

Uh oh!

Conversation

HanaPearlman commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbbeeken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbbeeken Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reggi Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HanaPearlman Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emadum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emadum Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emadum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbroadst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reggi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

refactor: unify means of applying `ignoreUndefined` #2589

refactor: unify means of applying `ignoreUndefined` #2589

HanaPearlman commented Oct 19, 2020 •

edited

Loading

nbbeeken Oct 20, 2020 •

edited

Loading

reggi Oct 20, 2020 •

edited

Loading

HanaPearlman Oct 20, 2020 •

edited

Loading

emadum Oct 21, 2020 •

edited

Loading