-
Notifications
You must be signed in to change notification settings - Fork 453
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.
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
Feature/op metadata support #586
base: master
Are you sure you want to change the base?
Changes from 9 commits
56c4afb
1581dc6
71f36b1
f845c4f
06b290d
5939b98
6ba9af8
26c71ae
3bcbb05
2cb16ee
7954c44
fb28eed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,7 +171,7 @@ SubmitRequest.prototype.commit = function(callback) { | |
var op = request.op; | ||
op.c = request.collection; | ||
op.d = request.id; | ||
op.m = undefined; | ||
op.m = request._metadataProjection(); | ||
// Needed for agent to detect if it can ignore sending the op back to | ||
// the client that submitted it in subscriptions | ||
if (request.collection !== request.index) op.i = request.index; | ||
|
@@ -185,6 +185,38 @@ SubmitRequest.prototype.commit = function(callback) { | |
}); | ||
}; | ||
|
||
SubmitRequest.prototype._metadataProjection = function() { | ||
var request = this; | ||
|
||
// Default behavior | ||
if (!request.opMetadataProjection) { | ||
return undefined; | ||
} | ||
|
||
// Granular projection | ||
if (typeof request.opMetadataProjection === 'object') { | ||
return request._granularMetadataProjection(); | ||
} | ||
|
||
// Full projection | ||
return request.op.m; | ||
}; | ||
|
||
// Specify top level fields to beincluded in a granular projection | ||
SubmitRequest.prototype._granularMetadataProjection = function() { | ||
var request = this; | ||
var metadataProjection = {}; | ||
for (var key in request.opMetadataProjection) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment, and updated docs with an example. |
||
var doProject = request.opMetadataProjection[key]; | ||
if (doProject ) { | ||
metadataProjection[key] = request.op.m[key]; | ||
} | ||
} | ||
|
||
return metadataProjection; | ||
}; | ||
|
||
|
||
SubmitRequest.prototype.retry = function(callback) { | ||
this.retries++; | ||
if (this.maxRetries != null && this.retries > this.maxRetries) { | ||
|
@@ -233,6 +265,8 @@ SubmitRequest.prototype._addSnapshotMeta = function() { | |
meta.ctime = this.start; | ||
} else if (this.op.del) { | ||
this.op.m.data = this.snapshot.data; | ||
// break circular dependency if snapshot data already contains metadata | ||
delete this.op.m.data._m; | ||
maxfortun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
meta.mtime = this.start; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ var numberType = require('./number-type'); | |
types.register(deserializedType.type); | ||
types.register(deserializedType.type2); | ||
types.register(numberType.type); | ||
var util = require('../util'); | ||
var errorHandler = util.errorHandler; | ||
|
||
module.exports = function() { | ||
describe('client submit', function() { | ||
|
@@ -1210,5 +1212,189 @@ module.exports = function() { | |
}); | ||
}); | ||
}); | ||
|
||
describe('metadata projection', function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 |
||
it('passed metadata to connect', function(done) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this test has anything to do with this PR? |
||
var metadata = {username: 'user'}; | ||
|
||
this.backend.use('connect', function(request, next) { | ||
Object.assign(request.agent.custom, request.req); | ||
next(); | ||
}); | ||
|
||
var connection = this.backend.connect(undefined, metadata); | ||
connection.on('connected', function() { | ||
expect(connection.agent.custom).eql(metadata); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('passed metadata to submit', function(done) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here: I'm not sure this test has anything to do with this PR? |
||
var metadata = {username: 'user'}; | ||
|
||
this.backend.use('connect', function(request, next) { | ||
Object.assign(request.agent.custom, request.req); | ||
next(); | ||
}); | ||
|
||
this.backend.use('submit', function(request) { | ||
expect(request.agent.custom).eql(metadata); | ||
done(); | ||
}); | ||
|
||
var connection = this.backend.connect(undefined, metadata); | ||
var doc = null; | ||
connection.on('connected', function() { | ||
expect(connection.agent.custom).eql(metadata); | ||
doc = connection.get('dogs', 'fido'); | ||
doc.create({name: 'fido'}, function() { | ||
doc.submitOp([{p: ['tricks'], oi: ['fetch']}], {source: 'trainer'}, errorHandler(done)); | ||
}); | ||
}); | ||
}); | ||
|
||
it('received local op without metadata', function(done) { | ||
var metadata = {username: 'user'}; | ||
|
||
this.backend.use('connect', function(request, next) { | ||
Object.assign(request.agent.custom, request.req); | ||
next(); | ||
}); | ||
|
||
this.backend.use('submit', function(request, next) { | ||
expect(request.agent.custom).eql(metadata); | ||
Object.assign(request.op.m, request.agent.custom); | ||
request.opMetadataProjection = {username: true}; | ||
next(); | ||
}); | ||
|
||
var connection = this.backend.connect(undefined, metadata); | ||
var doc = null; | ||
connection.on('connected', function() { | ||
expect(connection.agent.custom).eql(metadata); | ||
doc = connection.get('dogs', 'fido'); | ||
doc.create({name: 'fido'}, function() { | ||
doc.on('op', function(op, source, src, context) { | ||
if (src) { | ||
return; | ||
} | ||
expect(context.op.m).equal(undefined); | ||
done(); | ||
}); | ||
doc.submitOp([{p: ['tricks'], oi: ['fetch']}], {source: 'trainer'}, errorHandler(function() {})); | ||
}); | ||
}); | ||
}); | ||
|
||
it('concurrent changes', function(done) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
this.backend.use('connect', function(request, next) { | ||
expect(request.req).to.have.property('username'); | ||
Object.assign(request.agent.custom, request.req); | ||
next(); | ||
}); | ||
|
||
this.backend.use('submit', function(request, next) { | ||
expect(request.agent.custom).to.have.property('username'); | ||
Object.assign(request.op.m, request.agent.custom); | ||
request.opMetadataProjection = {username: true}; | ||
next(); | ||
}); | ||
|
||
this.backend.use('apply', function(request, next) { | ||
Object.assign(request.snapshot.m, request.op.m); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unneeded? |
||
expect(request.op.m).to.have.property('username'); | ||
next(); | ||
}); | ||
|
||
this.backend.use('commit', function(request, next) { | ||
expect(request.op.m).to.have.property('username'); | ||
next(); | ||
}); | ||
|
||
this.backend.use('afterWrite', function(request, next) { | ||
expect(request.op.m).to.have.property('username'); | ||
next(); | ||
}); | ||
|
||
var subscriberCount = 10; | ||
var subscriberOpCount = 10; | ||
|
||
var metadatas = []; | ||
for (var i = 0; i < subscriberCount; i++) { | ||
metadatas[i] = {username: 'user-'+i}; | ||
} | ||
|
||
var ops = []; | ||
for (var i = 0; i < subscriberCount; i++) { | ||
ops[i] = []; | ||
for (var j = 0; j < subscriberOpCount; j++) { | ||
ops[i].push({p: ['tricks '+i+' '+j], oi: 1}); | ||
} | ||
} | ||
|
||
var docs = []; | ||
|
||
function submitOps() { | ||
for (var j = 0; j < subscriberOpCount; j++) { | ||
for (var i = 0; i < subscriberCount; i++) { | ||
var doc = docs[i]; | ||
doc.submitOp([ops[i][j]], {source: 'src-'+i}, errorHandler(doneAfter)); | ||
} | ||
} | ||
} | ||
|
||
function validateAndDone() { | ||
var firstDoc = docs[0]; | ||
// validate that all documents across connections are in sync | ||
for (var i = 1; i < subscriberCount; i++) { | ||
var doc = docs[i]; | ||
expect(doc.data).eql(firstDoc.data); | ||
} | ||
done(); | ||
}; | ||
|
||
var submitOpsAfter = util.callAfter(subscriberCount - 1, submitOps); | ||
var doneAfter = util.callAfter((subscriberCount * subscriberCount * subscriberOpCount) - 1, validateAndDone); | ||
|
||
function getDoc(callback) { | ||
var thisDoc = this; | ||
thisDoc.fetch(function() { | ||
if (!thisDoc.data) { | ||
return thisDoc.create({}, function() { | ||
thisDoc.subscribe(callback); | ||
}); | ||
} | ||
thisDoc.subscribe(callback); | ||
}); | ||
} | ||
|
||
for (var i = 0; i < subscriberCount; i++) { | ||
var metadata = metadatas[i]; | ||
|
||
var connection = this.backend.connect(undefined, Object.assign({}, metadata)); | ||
connection.__test_metadata = Object.assign({}, metadata); | ||
connection.__test_id = i; | ||
|
||
connection.on('connected', function() { | ||
var thisConnection = this; | ||
|
||
expect(thisConnection.agent.custom).eql(thisConnection.__test_metadata); | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return doneAfter(); | ||
} | ||
var id = op[0].p[0].split(' ')[1]; | ||
expect(context.op.m).eql(metadatas[id]); | ||
doneAfter(); | ||
}); | ||
|
||
getDoc.bind(thisConnection.doc)(submitOpsAfter); | ||
}); | ||
} | ||
}); | ||
}); | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay have discussed with @ericyhwang and we think:
op
middleware, which should handle the cases I asked for on your tests (reiterated here for clarity)Cases we need to handle:
doc.fetch()
— this will fetch ops throughbackend._getSanitizedOps()
doc.submitOp()
— this retrieves ops through another mechanism inSubmitRequest.submit()