feat(collection): add colleciton level document mapping/unmapping #1698

j · 2018-05-03T23:34:27Z

This is a WIP to add mapping / unmapping on a collection level.

This can give document-mapper libraries a quicker way to map documents so that they don't have to wrap every collection's methods. (i.e. type-mongo-mapper)

Features to be implemented:

Mapping: Allow all collection methods that return plain documents (ignoring aggregations) to be "mapped" (i.e. converting a document to a class).
Unmapping: Allow all collection methods that insert or update plain documents (without update modifiers) to be unmapped (i.e. converting a class to a document).

Basic Example:

class User {
    public _id;
    public firstName;
    public lastName;
    public createdAt;

    constructor(doc) {
        this._id = doc._id || new ObjectID();
        this.firstName = doc.firstName;
        this.lastName = doc.lastName;
        this.createdAt = doc.createdAt || new Date();
    }

    get id() {
        return this._id.toString();
    }

    get fullName() {
        return `${this.firstName} ${this.lastName}`;
    }
}

const collection = db.collection('users', {
    map: (doc) => new User(doc),
    unmap: (user) => ({
        _id: user._id,
        firstName: doc.firstName,
        lastName: doc.lastName,
        createdAt: doc.createdAt,
    })
});

const user = await collection.findOne({ /* ... */ });
assert.true(user instanceof User); // true

const cursor = await collection.find({ /* ... */ }).toArray();
assert.true(user[0] instanceof User); // true

const { ok, value } = await collection.findOneAndUpdate({ /* ... */ });
assert.true(value instanceof User); // true

const user = new User({ firstName: 'John', lastName: 'Doe' });
await collection.insertOne(user); // calls unmap option.. createdAt & the ID were created on the constructor, so it'd already exist.

Discussion: https://jira.mongodb.org/projects/NODE/issues/NODE-1450?filter=allissues
Related: https://github.com/mongodb/js-bson/issues/211

Also, my heart wants this to belong in the bson library, but right now BSON serialization seems to be handed over any javascript object instead of just the documents, so it's (de)serializing more than it should? (I'm unsure of how the internals work exactly). I know the C# BSON driver has advanced class mapping, so that's what made me believe this type of thing can go there. And I believe the serialization option above can be done through a toBSON() function on the class., but it'd still be nice to be more explicit on the collection level and let mapping libraries handle it for you.

j · 2018-05-04T17:37:10Z

/ping @mbroadst thoughts?

daprahamian · 2018-05-07T15:04:11Z

@j heads up: @mbroadst is on vacation for the next few weeks, so it may take a while to circle around to this.

daprahamian

Good implementation. Only a few items I noted below.

Additionally, can you make sure to update the jsdoc descriptions for any relevant methods that this applies to?

As for the naming conventions, I am partial to map/unmap.

daprahamian · 2018-05-07T15:29:18Z

test/functional/collection_deserialization_tests.js

@@ -0,0 +1,170 @@
+'use strict';
+var test = require('./shared').assert;


Thank you for writing tests!

Since we are testing against Node >= 4, please use ES6 syntax where possible. That includes

const where possible, let otherwise.

class instead of raw functions and prototypes.

template strings (` `) instead of string concatenation

instead of using our old assert library (which we mostly keep around for legacy reason), please use chai.expect for your assertions.

daprahamian · 2018-05-07T15:39:37Z

lib/collection.js

+  const cursor = this.s.topology.cursor(this.s.namespace, findCommand, newOptions);
+
+  // add a map function to the cursor if "deserialize" option is set
+  if (this.s.options.deserialize && typeof this.s.options.deserialize === 'function') {


this if condition can be condensed into just typeof this.s.options.deserialize === 'function'.

daprahamian · 2018-05-07T15:41:22Z

lib/collection.js

@@ -2253,6 +2253,11 @@ var findAndModify = function(self, query, sort, doc, options, callback) {
  // Execute the command
  self.s.db.command(queryObject, finalOptions, function(err, result) {
    if (err) return handleCallback(callback, err, null);
+
+    if (result && result.value) {


Like above, we can condense the if statement and function call to:

if (result && result.value && typeof self.s.options.deserialize === 'function') { result.value = self.s.options.deserialize(result.value); }

And we can eliminate the use of deserializeDocument

daprahamian · 2018-05-07T15:46:05Z

test/functional/collection_deserialization_tests.js

+      });
+
+      client.connect(function(err, client) {
+        var db = client.db(configuration.db);


This line should be switched with the next one as follows:

client.connect(function(err, client) { expect(err).to.eql(null); const db = client.db(configuration.db); ...

If there is an error, client will be null|undefined, and the test will error out with the message TypeError: Cannot read property 'db' of undefined. By switching the lines, the error message will be clearer.

j · 2018-05-07T17:18:33Z

@daprahamian Okay, I'd happily make these changes.

And It's good news to be told to use more modern syntax. Are the drivers going to be rewritten using ES6 classes? It hurt my brain sometimes navigating through this code.

And I'm definitely down with map / unmap. I'd be for serialize/unserialize if it was BSON specific, but currently, it's too hard to get this into BSON. I just ended up committing to what I had already prototyped.

Another option I thought of, which is to be like another one of the drivers (I don't have time to look at which one at the moment), but it can look something like this:

const collection = db.collection('users');
const mappedCollection = collection.asMapCollection(mapFn, unmapFn);

// ... or

const mappedCollection = db.mappedCollection('users', mapFn, unmapFn);

And I can create a new MappedCollection class. I think for the most part, I'd just have to wrap most functions.

What are your thoughts or preferences?

daprahamian · 2018-05-07T18:10:15Z

@j we're currently taking a piecemeal approach to rewriting the driver in ES6; when we touch a file or do a major refactor, we try to bring it as up to date as possible. There are already some portions of the code rewritten with ES6 classes, and we will likely continue in the future.

I'd hold off on the additional helpers you mentioned for right now, since it's basically sugar for db.collection('users', { map, unmap});

j · 2018-05-07T18:23:30Z

@daprahamian sounds good, I'll try to get the unmap portion done by EOD tomorrow (hopefully today if I get time), and do more refactoring.

Can you think of other areas I didn't touch that can possibly be mapped to a class on the top of your head? I want to make sure if you have "map/unmap" set, it's mapping everywhere a document ever gets returned (minus aggregations).

daprahamian · 2018-05-07T18:35:23Z

@j I can't think of any other places that need the mapping.

j · 2018-05-07T21:56:20Z

@daprahamian should I stick to await/async (in tests) or keep it vanilla callback style?

daprahamian · 2018-05-07T21:58:25Z

@j Do not use async/await. Our tests are run against Node 4, Node 6, Node 8, and Node 10, and async/await can't be parsed by Node 6 or Node 4.

Other than that, you can use either callbacks or Promises.

j · 2018-05-07T23:08:13Z

@daprahamian I added unmap as well. I kind of want to add an option on the collection method so that you can opt out of the collection mapping if you want.

I ended up doing an "instanceof" check on the unmap function as you can see in the tests to allow me to sort of over-ride it.

I have to head out, so I'll get back on it tomorrow.

daprahamian

Looks good, just a few styling issues.

daprahamian · 2018-05-08T18:52:33Z

lib/collection.js

+      doc._id = self.s.pkFactory.createPk();
+    }
+
+    return unmap !== false ? unmap(doc) : doc;


This can just be an implicit boolean return unmap ? unmap(doc) : doc

daprahamian · 2018-05-08T18:55:22Z

lib/collection.js

+  }
+
+  return docs.map(function(doc) {
+    if (forceServerObjectId !== true && doc._id === void 0) {


lets make this if (forceServerObjectId !== true && doc._id == null) {. I'd like to remove as much void 0 from the codebase as possible.

Thank goodnes ;)

daprahamian · 2018-05-08T18:56:08Z

lib/collection.js

+
+  // no need to modify the docs if server sets the ObjectId
+  // and unmap collection option is unset
+  if (forceServerObjectId === true && unmap === false) {


if (forceServerObjectId === true && !unmap)

daprahamian · 2018-05-08T19:01:14Z

test/functional/collection_mapping_tests.js

+  }
+
+  getFullName() {
+    return this.firstName + ' ' + this.lastName;


Make this a string template:

getFullName() { return `${this.firstName} ${this.lastName}`; }

daprahamian · 2018-05-08T19:05:30Z

test/functional/collection_mapping_tests.js

+          collection.findOne({}, function(err, doc) {
+            expect(err).to.be.null;
+
+            expect(doc).to.eql({


When testing against objects, we prefer to use .to.deep.equal instead of to.eql. It is clearer what is going on here.

j · 2018-05-08T21:01:48Z

@daprahamian I added unmap for over-riding or disabling collection level mapping. I didn't add an over-ride for finding but I can. If unmapping is really needed on a collection level, I figure you can just use the cursor directly. But if you want me to add that for consistency I can!

Let me know what you think.

j · 2018-05-08T21:18:00Z

Now that I think about it, should I remove the ability to over-ride on the collection method level? If someone wants an unmapped collection, they can do:

// collection with mapping
const mappedCollection = db.collection('users', {
    map: /* ... */,
    unmap: /* ... */,
});

// collection without mapping if they want to do things with raw documents
const collection = db.collection('users');

j · 2018-05-08T21:34:21Z

@daprahamian I went ahead and removed the unmap option from the cursor methods since you can just create another unmapped collection. I can bring it back.

If these changes all look good, I can clean up the commits into one and be good to go!

j · 2018-05-09T17:15:37Z

@daprahamian i have some time to finalize any changes today if you feel there are any, otherwise I can squash the commits and clean it up. Let me know.

daprahamian

👍 LGTM. Squash the commits and I can add it into the next beta release.

j · 2018-05-09T18:49:41Z

🤘 done!

j · 2018-05-09T18:59:31Z

When the beta version that contains this is pushed, I'll work on top of it with my typescript mapping library and get a real feel of it. :)

j · 2018-05-09T19:49:17Z

@daprahamian sorry forgot to make the mention.

daprahamian · 2018-06-25T17:29:09Z

@j we unfortunately have to back this change out of the 3.1.0-beta branch before our next release. With mongodb/js-bson#253 and other approaches to document-class mapping on the table, we'd like to have further discussion before committing to any one API approach.

My plan right now is as follows:

~~Fork a branch with your changes~~ branch here
Remove the changes from master
Create a new PR between your branch and master

Reverts #1698. With mongodb/js-bson#253 and other approaches to document-class mapping on the table, we'd like to have further discussion before committing to any one API approach. Reopens NODE-1450

julien-c · 2018-06-27T22:20:30Z

This is really cool! We use something similar but it's sometimes a pain to maintain. Love this implementation @j <3

mbroadst · 2018-06-27T22:41:32Z

@julien-c it’s coming! We just wanted a little more time to make a measured decision about it, sorry for the wait 😄

j · 2018-07-09T21:04:51Z

@daprahamian @mbroadst Quick question... I was working on top of the beta branch for a while and everything got borked and didn't realize it's because my feature was removed. I saw that PR in BSSON, which is an OK start? I just can't get the "target" option in the collection commands to be passed down to the BSON deserialize. Also, that PR didn't serialize either, so that might be a good one to add. I'd like to help with this, but I'm finding it a pain to figure out how to pass a client option down to the BSON methods.

Any clue where I should look? Code bounces around quite a lot and I want to put it in the right place.

PS. After working on top of my merged/reverted feature, everything was working pretty well :)

mbroadst · 2018-07-09T21:16:10Z

@j we are not actively working on the feature, but will be focusing on it next quarter (read: later this summer). We will make sure to keep you apprised, as we want feedback from those who it will be useful to, but in the meantime I recommend making an intermediary module to monkey patch your support into the driver. Sorry for the delay, but we want to make sure this is done right the first time, as we are on the hook for maintaining the feature in the future.

j · 2018-07-09T21:35:18Z

Sounds good. The PR right now in js-bson, he's using firstBatch and nextBatch to determine that it's a document. Is this the best way or are there other thoughts on how you'll want to do the class mapping? (mongodb/js-bson#253)

Reverts mongodb#1698. With mongodb/js-bson#253 and other approaches to document-class mapping on the table, we'd like to have further discussion before committing to any one API approach. Reopens NODE-1450

daprahamian suggested changes May 7, 2018

View reviewed changes

j changed the title ~~[WIP] Collection level (de)serialization~~ [WIP] Collection level (un)mapping May 7, 2018

daprahamian reviewed May 8, 2018

View reviewed changes

daprahamian approved these changes May 9, 2018

View reviewed changes

feat(collection): add colleciton level document mapping/unmapping

7550f21

j changed the title ~~[WIP] Collection level (un)mapping~~ feat(collection): add colleciton level document mapping/unmapping May 9, 2018

daprahamian merged commit d03335e into mongodb:master May 10, 2018

daprahamian mentioned this pull request Jun 25, 2018

revert(collection): reverting collection-mapping features #1750

Merged

daprahamian mentioned this pull request Jun 25, 2018

WIP: Revert "revert(collection): reverting collection-mapping features" #1755

Closed

		@@ -0,0 +1,170 @@
		'use strict';
		var test = require('./shared').assert;

feat(collection): add colleciton level document mapping/unmapping #1698

feat(collection): add colleciton level document mapping/unmapping #1698

Conversation

j commented May 3, 2018 • edited Loading

j commented May 4, 2018

daprahamian commented May 7, 2018

daprahamian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daprahamian May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j commented May 7, 2018

daprahamian commented May 7, 2018

j commented May 7, 2018

daprahamian commented May 7, 2018

j commented May 7, 2018

daprahamian commented May 7, 2018

j commented May 7, 2018

daprahamian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j commented May 8, 2018

j commented May 8, 2018

j commented May 8, 2018

j commented May 9, 2018

daprahamian left a comment

Choose a reason for hiding this comment

j commented May 9, 2018

j commented May 9, 2018

j commented May 9, 2018

daprahamian commented Jun 25, 2018 • edited Loading

julien-c commented Jun 27, 2018

mbroadst commented Jun 27, 2018

j commented Jul 9, 2018

mbroadst commented Jul 9, 2018

j commented Jul 9, 2018

j commented May 3, 2018 •

edited

Loading

daprahamian May 7, 2018 •

edited

Loading

daprahamian commented Jun 25, 2018 •

edited

Loading