Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add skipPoll support for json1 ops, instead of throwing errors for them #137

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add skipPoll support for json1 ops, instead of throwing errors for them #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
234384e
Add skipPoll support for json1 ops, instead of throwing errors for them
ericyhwang Jan 9, 2023
67f6f1c
Upgrade to sharedb-mingo-memory@2, which drops Node 12 support
ericyhwang Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 41 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,10 +1044,44 @@ function getInnerFields(params, fields) {
function opContainsAnyField(op, fields) {
for (var i = 0; i < op.length; i++) {
var component = op[i];
if (component.p.length === 0) {
return true;
} else if (fields[component.p[0]]) {
if (Array.isArray(component.p)) {
// json0 op component:
//
// Each op component has its own array of `p` path segments from doc root.
if (component.p.length === 0) {
return true;
} else if (fields[component.p[0]]) {
return true;
}
} else if (typeof component === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check would break with eg text-unicode with a query set up on the root-level _data field (the default field if your snapshot isn't an object)

// json1 field descent from root:
//
// First string in top-level array means all subsequent operations will be
// underneath that top-level field, no need to continue iterating.
return fields[component];
} else if (hasOwnProperty(component, 'p') ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I wonder if we're trying to do too much with heuristics here? It's really hard to determine if an op is a given type, and we may do a bunch of work we don't need to if a certain collection never submits JSON ops (this is the mainline case for our organisation!).

I wonder if this belongs as some sort of helper function plugin exported by this library, which consumers need to enable using ShareDB's skipPoll option.

So given a helper function shouldSkipJson1Poll(), consumer code might look something like:

const backend = new Backend({
  skipPoll: (collection, id, op, query) => {
    // Let's assume this is easy to determine for a consumer. Each
    // collection probably has a set type that never changes,
    // especially if we're querying!
    if (!isJson1Collection(collection)) return false;
    return shouldSkipJson1Poll(op, query);
  },
});

The advantages of this:

  • massively reduces sharedb-mongo's concern leakiness
  • we don't need to use heuristics to determine what types ops are
  • keeps the core of sharedb-mongo type-agnostic
  • we can have a "poll by default" fallback, so polling always works correctly

Disadvantages:

  • effort for consumers
  • obscure optimisation config that many consumers may never find or enable, and pay a performance penalty

hasOwnProperty(component, 'r') ||
hasOwnProperty(component, 'd') ||
hasOwnProperty(component, 'i') ||
hasOwnProperty(component, 'e')) {
// json1 root-level operation:
//
// If we encounter an operation at top level prior to encountering a string,
// (field descent) then the operation affects the entire document.
return true;
} else if (Array.isArray(component)) {
// json1 child operation list:
//
// In a canonical json1 op, if we encounter a child op prior to encountering
// a string (field descent), then the child should start with a field descent.
// If that weren't the case, the op would be pulled up to the top-level array.
var descendant = component;
while (typeof descendant[0] !== 'string') {
descendant = descendant[0];
}
if (fields[descendant[0]]) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add an else statement here that captures types that are neither json0 nor json1 and default to polling.

}
}
return false;
Expand Down Expand Up @@ -1457,6 +1491,10 @@ function isPlainObject(value) {
);
}

function hasOwnProperty(obj, prop) {
return Object.prototype.hasOwnProperty.call(obj, prop);
}

// Convert a simple map of fields that we want into a mongo projection. This
// depends on the data being stored at the top level of the document. It will
// only work properly for json documents--which are the only types for which
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"mongodb4": "npm:mongodb@^4.0.0",
"nyc": "^14.1.1",
"ot-json1": "^1.0.1",
"sharedb-mingo-memory": "^1.1.1",
"sharedb-mingo-memory": "^2.0.0",
"sinon": "^6.1.5",
"sinon-chai": "^3.7.0"
},
Expand Down
46 changes: 46 additions & 0 deletions test/test_skip_poll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var expect = require('chai').expect;
var json1 = require('ot-json1');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for rich-text (and any other types we want to check) please.

var ShareDbMongo = require('../index');

describe('skipPoll', function() {
Expand Down Expand Up @@ -111,6 +112,51 @@ describe('skipPoll', function() {
assertNotSkips({op: [{p: ['a'], dummyOp: 1}, {p: [], dummyOp: 1}]}, query);
assertNotSkips({op: [{p: [], dummyOp: 1}, {p: ['x'], dummyOp: 1}]}, query);
});

it('json1 root-level changes', function() {
// Root-level doc changes should always cause a query poll.
assertNotSkips({op: json1.insertOp('', {brandNew: 'value'})}, query);
assertNotSkips({op: json1.removeOp('')}, query);
});

it('json1 ops not affecting queried fields', function() {
assertSkips({op: json1.insertOp(['notQueried'], 'hello')}, query);
assertSkips({op: json1.moveOp(['notQueried'], ['alsoNotQueried'])}, query);
assertSkips({op: json1.removeOp(['notQueried'], 'hello')}, query);
});

it('json1 insert ops', function() {
assertIfSkips({op: json1.insertOp(['a'], 'hello')}, query, !has(fields, 'a'));
assertIfSkips({op: json1.insertOp(['a', 'b'], 'hello')}, query, !has(fields, 'a'));
assertIfSkips({op: json1.insertOp(['a', 0], 'hello')}, query, !has(fields, 'a'));
});

it('json1 remove ops', function() {
assertIfSkips({op: json1.removeOp(['a'], 'hello')}, query, !has(fields, 'a'));
assertIfSkips({op: json1.removeOp(['a', 'b'], 'hello')}, query, !has(fields, 'a'));
assertIfSkips({op: json1.removeOp(['a', 0], 'hello')}, query, !has(fields, 'a'));
});

it('json1 move ops', function() {
assertIfSkips({op: json1.moveOp(['a'], ['x'])}, query, !has(fields, 'a') && !has(fields, 'x'));
assertIfSkips({op: json1.moveOp(['x'], ['a'])}, query, !has(fields, 'x') && !has(fields, 'a'));
assertIfSkips({op: json1.moveOp(['a'], ['notQueried'])}, query, !has(fields, 'a'));
assertIfSkips({op: json1.moveOp(['notQueried'], ['a'])}, query, !has(fields, 'a'));
assertSkips({op: json1.moveOp(['notQueried'], ['alsoNotQueried'])}, query);
});

it('json1 composed ops', function() {
var compositeNotQueried = json1.type.compose(
json1.insertOp(['notQueried1'], 'hello'),
json1.moveOp(['notQueried2'], ['notQueried3'])
);
assertSkips({op: compositeNotQueried}, query);
var compositeQueried = json1.type.compose(
json1.insertOp(['notQueried1'], 'hello'),
json1.moveOp(['notQueried2'], ['a'])
);
assertIfSkips({op: compositeQueried}, query, !has(fields, 'a'));
});
});
}
});
Expand Down