Handle projections, queries on Mongo doc properties (like _id) and fix comparisons to null #3

zmillman · 2018-02-20T00:24:29Z

These changes bring sharedb-mingo-memory closer to a drop-in replacement for sharedb-mongo

Changes

Add support for sharedb projections (already has test coverage)
Transform mingo query conditions to mimic execution of how they'd perform on Mongo docs persisted by sharedb-mongo
Update mingo dependency to fix <property>: null queries
Add tests for ^

zmillman · 2018-02-20T00:34:46Z

.travis.yml

  - 5
  - 4
-  - 0.10


The newest version of mingo uses Symbol.Iterator, which wasn't supported until Node 0.12: http://node.green/#ES2015-built-ins-typed-arrays--TypedArray--prototype-Symbol-iterator-

ericyhwang · 2018-02-21T01:39:51Z

index.js

+  //   mingo projections.
+  ShareDBMingo.prototype.getSnapshot = function(collection, id, fields, options, callback) {
+    MemoryDB.prototype.getSnapshot.call(this, collection, id, fields, options, function (err, snapshot) {
+      callback(err, projectSnapshot(fields, snapshot));


From testing, snapshot is undefined when err is present, which causes an uncaught exception in projectSnapshot.

ericyhwang · 2018-02-21T01:40:06Z

index.js

+  ShareDBMingo.prototype.query = function(collection, query, fields, options, callback) {
+    MemoryDB.prototype.query.call(this, collection, query, fields, options, function (err, snapshots, extra) {
+      var projectSnapshotWithFields = projectSnapshot.bind(null, fields);
+      callback(err, snapshots.map(projectSnapshotWithFields), extra);


Same deal, snapshots could be undefined.

nateps

Thanks for taking the initiative on this! Eric and I reviewed it together, and spotted a couple of things that we might look into further.

ShareDB is supposed to handle the projections, so ideally we wouldn't need to add anything in this module to do that. Are there any specific things you found that we should fix in ShareDB?
At first glance, I think it might be better to cast the format in which we store the docs rather than cast the query. This is how sharedb-mongo works, so I think it would be easier to make it more consistent. We might also be able to share code by creating another module that this and sharedb-mongo depend on or by at least copy-pasting and cross referencing the code.

nateps · 2018-02-23T19:03:48Z

.travis.yml

@@ -1,8 +1,8 @@
 language: node_js
 node_js:
+  - 8
  - 5


while we are at it, lets remove 5 and add 6 and 9. we should have all LTS versions and the latest stable node version

nateps · 2018-02-23T19:13:13Z

index.js

@@ -8,10 +17,33 @@ function extendMemoryDB(MemoryDB) {

  ShareDBMingo.prototype = Object.create(MemoryDB.prototype);

+  ShareDBMingo.prototype.projectsSnapshots = true;


We shouldn't need to implement projections if the code in ShareDB is working correctly. By default, ShareDB is supposed to implement projections unless the database adapter chooses to implement them natively. Since this is all being done in memory anyway, we might as well leverage the projection code in ShareDB.

Did you discover that projections were not working?

Looks like you're right. Tests relying on projections still pass if all this projection code is removed.

I think I had assumed that projectsSnapshots = true was required in order for projections to work, but didn't realize that it mean that it controlled whether the adapter or ShareDB was responsible for doing the actual field projection.

Removing! 👋

nateps · 2018-02-23T19:25:20Z

index.js

+  // Build a query object that mimics how the query would be executed if it were
+  // made against snapshots persisted with `sharedb-mongo`
+  // FIXME: This doesn't handle nested doc properties with dots like: {'_m.mtime': 12300}
+  function castToSnapshotQuery(query) {


Alternate option to explore would be casting the document data into the same format that sharedb-mongo uses rather than casting the query. Advantage here would be that we'd be more sure it would work like sharedb-mongo for all types of queries. Should possible to make either work, however.

…napshot-queries

nateps · 2018-02-23T21:51:03Z

Glad to hear that removing the projection code still works! Sorry that wasn't more clear up front.

Might you be able to submit a PR to sharedb to add some documentation or comments indicating the purpose of the projectsSnapshots property for future developers?
https://github.com/share/sharedb/blob/master/lib/db/index.js#L10

zmillman · 2018-02-23T22:07:04Z

@nateps share/sharedb#196

I also gave 2. above a try, but had trouble getting it to play nicely with how MemoryDb was written (it doesn't really snapshots and docs particularly separate). Happy to collaborate on getting it working, but casting queries was the much easier (but hackier) approach.

ericyhwang

I talked with Nate, and he's ok with accepting this as is it now, as it brings sharedb-mingo-memory closer to accurately replicating the behavior of sharedb-mongo. We can work on his idea of changing the in-memory doc structure later. I added a skipped test for that case, and added some more comments.

Nate says I can handle the merging and publishing of 1.0.0. Thanks for the PR and for doing the subsequent tweaks!

nateps · 2018-02-27T16:43:31Z

🎉

zmillman added 4 commits February 19, 2018 15:02

Add failing tests for _id and <property>: null queries

3b63906

Upgrade mingo dependency

63590e4

Better test cases for doc query conditions

7566b43

Cast Mongo doc queries to Mingo snapshot queries

7825d2a

zmillman changed the title ~~Handle~~ Handle queries on Mongo doc properties (like _id) and fix comparisons to null Feb 20, 2018

Update Node for CI

288c860

zmillman commented Feb 20, 2018

View reviewed changes

Add basic support for ShareDB projections

6d00006

zmillman changed the title ~~Handle queries on Mongo doc properties (like _id) and fix comparisons to null~~ Handle projections, queries on Mongo doc properties (like _id) and fix comparisons to null Feb 20, 2018

ericyhwang requested changes Feb 21, 2018

View reviewed changes

Handle errors in getSnapshot/query callbacks

8ffbc6d

nateps reviewed Feb 23, 2018

View reviewed changes

zmillman added 3 commits February 23, 2018 13:07

Update Travis.yml to add Node 6 & 9, remove Node 5

c7b540c

Remove redundant snapshot projection code

a878067

Merge remote-tracking branch 'origin/fix/snapshot-queries' into fix/s…

153da3d

…napshot-queries

Add comments to new query tests, add skipped test on _m.mtime

3606619

ericyhwang approved these changes Feb 27, 2018

View reviewed changes

ericyhwang merged commit 7c6228d into share:master Feb 27, 2018

ericyhwang mentioned this pull request Feb 27, 2018

Consider refactor to cast between snapshot and doc like sharedb-mongo #4

Open

zmillman deleted the fix/snapshot-queries branch August 15, 2018 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Handle projections, queries on Mongo doc properties (like _id) and fix comparisons to null #3

Handle projections, queries on Mongo doc properties (like _id) and fix comparisons to null #3

Uh oh!

zmillman commented Feb 20, 2018 •

edited

Loading

Uh oh!

zmillman Feb 20, 2018

Uh oh!

ericyhwang Feb 21, 2018

Uh oh!

ericyhwang Feb 21, 2018

Uh oh!

nateps left a comment

Uh oh!

nateps Feb 23, 2018

Uh oh!

nateps Feb 23, 2018

Uh oh!

zmillman Feb 23, 2018

Uh oh!

nateps Feb 23, 2018

Uh oh!

nateps commented Feb 23, 2018

Uh oh!

zmillman commented Feb 23, 2018

Uh oh!

ericyhwang left a comment

Uh oh!

nateps commented Feb 27, 2018

Uh oh!

Uh oh!

		@@ -8,10 +17,33 @@ function extendMemoryDB(MemoryDB) {

		ShareDBMingo.prototype = Object.create(MemoryDB.prototype);

		ShareDBMingo.prototype.projectsSnapshots = true;

Handle projections, queries on Mongo doc properties (like _id) and fix comparisons to null #3

Handle projections, queries on Mongo doc properties (like _id) and fix comparisons to null #3

Uh oh!

Conversation

zmillman commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

zmillman Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

ericyhwang Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

ericyhwang Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

nateps left a comment

Choose a reason for hiding this comment

Uh oh!

nateps Feb 23, 2018

Choose a reason for hiding this comment

Uh oh!

nateps Feb 23, 2018

Choose a reason for hiding this comment

Uh oh!

zmillman Feb 23, 2018

Choose a reason for hiding this comment

Uh oh!

nateps Feb 23, 2018

Choose a reason for hiding this comment

Uh oh!

nateps commented Feb 23, 2018

Uh oh!

zmillman commented Feb 23, 2018

Uh oh!

ericyhwang left a comment

Choose a reason for hiding this comment

Uh oh!

nateps commented Feb 27, 2018

Uh oh!

Uh oh!

zmillman commented Feb 20, 2018 •

edited

Loading