Feature/op metadata support by maxfortun · Pull Request #586 · share/sharedb

maxfortun · 2023-01-18T20:41:03Z

Reworked the PR(#513) for issue(#512).
Hoping it will make it into the main branch this time around.

alecgibson · 2023-01-25T10:09:47Z

Hi @maxfortun just a quick note to say thanks for the PR and I promise I haven't forgotten about it! It's on my todo list, just very busy right now unfortunately.

coveralls · 2023-01-26T08:50:09Z

Coverage: 97.419% (-0.02%) from 97.437% when pulling 2cb16ee on maxfortun:feature/op_metadata_support into cc0e338 on share:master.

alecgibson · 2023-01-31T10:58:15Z

lib/backend.js

  var db = this.db;
  var backend = this;

+  var options = {metadata: true};


I think we should potentially consider snapshot metadata in a separate PR. It's more complicated than op metadata, because there's not currently a mechanism to propagate updated snapshot metadata to remote clients (which only receive ops to transform their local data, and don't receive the full snapshot — this is sort of the point of OT in general).

For example, I'd expect a passing test case for this case:

it('updates remote metadata', function(done) { backend.use('commit', (context, next) => { context.snapshot.m.updated = Date.now(); next(); }); var connection1 = backend.connect(); var connection2 = backend.connect(); var doc1 = connection1.get(...); var doc2 = connection2.get(...); async.series([ doc1.subscribe.bind(doc1), doc1.submitOp.bind(doc1, [{p: ['foo'], oi: 'bar'}]), doc2.subscribe.bind(doc2), function(next) { expect(doc1.metadata.updated).to.equal(doc2.metadata.updated); next(); }, function(next) { doc2.once('op', function() { expect(doc1.data).to.eql(doc2.data); expect(doc1.metadata.updated).to.equal(doc2.metadata.updated); next(); }); doc1.submitOp([{p: ['foo'], od: 'bar', oi: 'baz'}], errorHandler(next)); }, ], done); });

lib/client/doc.js

lib/submit-request.js

alecgibson · 2023-01-31T11:03:09Z

lib/submit-request.js

+SubmitRequest.prototype._granularMetadataProjection = function() {
+  var request = this;
+  var metadataProjection = {};
+  for (var key in request.opMetadataProjection) {


We should at least add a code comment here that we only support a single level of projection.

Would be extra nice if we updated the docs to include this flag and notes on its use.

Added a comment, and updated docs with an example.

alecgibson · 2023-01-31T11:08:45Z

lib/backend.js

    // - we handle the projection in _sanitizeSnapshots
    var from = milestoneSnapshot ? milestoneSnapshot.v : 0;
-    db.getOps(collection, id, from, version, null, function(error, ops) {
+    db.getOps(collection, id, from, version, options, function(error, ops) {


I'm not sure we've put this in the right place? Shouldn't we be fetching op metadata in _getSanitizedOps() (and maybe its bulk counterpart)?

It seems that if a snapshot has some saved metadata and we do not pass fields and options to getSnapshot , the metadata will not be retrieved at all. Same with _getSanitizedOps, if we do not pass the options to getOps , the database layer will not retrieve the metadata and there will be nothing to sanitize. I have a very shallow understanding of the intent here, and maybe I am missing something fundamental. I could definitely use your help to understand this better. Thank you!

I think we should basically not touch snapshots at all in this PR. As I mentioned in my other comment, snapshot metadata is a very different beast, and I think we should consider it separately to ops.

I think this belongs in _getSanitizedOps() with the call to backend.db.getOps().

lib/submit-request.js

alecgibson · 2023-01-31T17:11:18Z

test/client/submit.js

    });
+
+    describe('metadata projection', function() {
+      it('passed metadata to connect', function(done) {


I'm not sure this test has anything to do with this PR?

alecgibson · 2023-01-31T17:11:26Z

test/client/submit.js

+        });
+      });
+
+      it('passed metadata to submit', function(done) {


Same here: I'm not sure this test has anything to do with this PR?

alecgibson · 2023-01-31T17:15:29Z

test/client/submit.js

+            thisConnection.doc = docs[thisConnection.__test_id] = thisConnection.get('dogs', 'fido');
+
+            thisConnection.doc.on('op', function(op, source, src, context) {
+              if (!src || !context) { // If I am the source there is no metadata to check


Why don't we send metadata back to the original client? The server might add context the sender wants (and which remote clients will get).

What's your use-case? Does it work without the original op submitter getting their metadata?

We could add the meta to the op acknowledgement.

alecgibson · 2023-01-31T17:16:38Z

test/client/submit.js

+        });
+      });
+
+      it('concurrent changes', function(done) {


Can we please simplify this test case to just have two clients? I'm not sure there's a vast amount of benefit in having so many clients; it just makes the test harder to read. Would be nice to get rid of the for loops.

alecgibson · 2023-01-31T17:19:06Z

test/client/submit.js

+        });
+
+        this.backend.use('apply', function(request, next) {
+          Object.assign(request.snapshot.m, request.op.m);


alecgibson · 2023-01-31T17:21:50Z

test/client/submit.js

      });
    });
+
+    describe('metadata projection', function() {


We've flexed metadata on ops that were broadcast over pubsub, but ops can also be fetched when submitting an op from a stale document (that isn't subscribed, and is behind the server); can we test that the ops we get in that case also have their metadata correctly set?

This would need us to tweak SubmitRequest.submit().

Also, can we add a test case for an unsubscribed client calling fetch after another remote client has changed the doc: this is where you'll need the backend._getSanitizedOps() to handle metadata projection for us.

alecgibson · 2023-01-31T17:45:29Z

docs/middleware/op-submission.md

  }
+
+  // Add op metadata to snapshot before snapshot is stored
+  Object.assign(context.snapshot.m, context.op.m);


Okay have discussed with @ericyhwang and we think:

we should go back to having a feature flag (sorry about the U-turn!)

if the feature flag is enabled, we send all metadata across pubsub

if the feature flag is enabled, we also apply the metadata projection in the op middleware, which should handle the cases I asked for on your tests (reiterated here for clarity)

Cases we need to handle:

Op submission (the case you're already testing): ops broadcast over pubsub to other subscribers

Op fetching: have a stale document that calls doc.fetch() — this will fetch ops through backend._getSanitizedOps()

Ops sent back when submitting: have a stale document that calls doc.submitOp() — this retrieves ops through another mechanism in SubmitRequest.submit()

maxfortun · 2023-08-14T20:07:12Z

Ping?

alecgibson · 2023-08-15T06:28:22Z

@maxfortun sorry you hadn't asked for re-review, so didn't know it was ready to look at again. I'll try to look today.

alecgibson

As discussed inline, we shouldn't be using agent.custom. We've also not got a feature flag as we previously requested.

I think I've lost sight of what it is this PR was trying to achieve? You wish to attach metadata from a client and broadcast it to other clients? Or you just want to broadcast server-side metadata from the server to receiving clients?

alecgibson · 2023-08-15T07:04:31Z

lib/client/doc.js

  }
+
+  // If agent has metadata, append on client level so that both client and backend have access to it
+  if(this.connection.agent && this.connection.agent.custom && typeof this.connection.agent.custom.metadata === "object") {


I'm not sure I understand why we're doing this? The custom object is meant to be a generic data blob for clients to use. Setting a magical property here is surprising and may clash with clients' existing usage.

Also this.connection is only ever available in setups where the client is running on the same machine as the server (as opposed to, say, a client connected remotely over websocket).

alecgibson · 2023-08-15T07:05:41Z

lib/submit-request.js

          op.c = request.collection;
          op.d = request.id;
-          op.m = undefined;
+          op.m = request.agent.custom.metadata ? request.op.m : undefined;


As discussed above, we shouldn't be using agent.custom.

maxfortun added 2 commits January 18, 2023 14:59

Added metadata projection

56c4afb

Added metadata projection

1581dc6

maxfortun added 2 commits January 25, 2023 14:23

Copy metadata from op to snapshot on apply middleware

71f36b1

Include metadata when fetching snapshot

f845c4f

maxfortun mentioned this pull request Jan 25, 2023

Retain metadata in snapshot if needed share/sharedb-mongo#138

Open

maxfortun added 2 commits January 27, 2023 10:06

Include metadata when fetching snapshot

06b290d

Break circular dependency if snapshot data already contains meta

5939b98

alecgibson reviewed Jan 31, 2023

View reviewed changes

maxfortun added 3 commits January 31, 2023 10:05

Cleaning up and adding documentation

6ba9af8

Replacing tabs with spaces

26c71ae

Reverting. Will work on this in a separate PR.

3bcbb05

alecgibson reviewed Jan 31, 2023

View reviewed changes

maxfortun added 3 commits January 31, 2023 13:00

Reverting. Will work on this in a separate PR.

2cb16ee

Limiting changes to ops only. Snapshots to be addressed in own pr

7954c44

Rework tests

fb28eed

alecgibson requested changes Aug 15, 2023

View reviewed changes

Conversation

maxfortun commented Jan 18, 2023

Uh oh!

alecgibson commented Jan 25, 2023

Uh oh!

coveralls commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxfortun commented Aug 14, 2023

Uh oh!

alecgibson commented Aug 15, 2023

Uh oh!

alecgibson 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jan 26, 2023 •

edited

Loading