Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ Register a new middleware.

- `action` _(String)_
One of:
- `'beforeCreate'`: directly before the call to write a new document
- `'beforeOverwrite'`: directly before the call to replace a document, can include edits as well as deletions
- `'beforeSnapshotLookup'`: directly before the call to issue a query for one or more snapshots by ID
- `fn` _(Function(context, callback))_
Expand All @@ -209,6 +210,9 @@ Register a new middleware.
- `action`: The action this middleware is handling
- `collectionName`: The collection name being handled
- `options`: Original options as they were passed into the relevant function that triggered the action
- `'beforeCreate'` actions have additional context properties:
- `documentToWrite` - The document to be written
- `op` - The op that represents the changes that will be made to the document
- `'beforeOverwrite'` actions have additional context properties:
- `documentToWrite` - The document to be written
- `op` - The op that represents the changes that will be made to the document
Expand Down
23 changes: 14 additions & 9 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,16 +270,21 @@ ShareDbMongo.prototype._writeSnapshot = function(request, id, snapshot, opId, ca
if (err) return callback(err);
request.documentToWrite = castToDoc(id, snapshot, opId);
if (request.documentToWrite._v === 1) {
collection.insertOne(request.documentToWrite, function(err) {
if (err) {
// Return non-success instead of duplicate key error, since this is
// expected to occur during simultaneous creates on the same id
if (err.code === 11000 && /\b_id_\b/.test(err.message)) {
return callback(null, false);
}
return callback(err);
self._middleware.trigger(MiddlewareHandler.Actions.beforeCreate, request, function(middlewareErr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we can clarify that this is talking about creating a MongoDB document (not a ShareDB document, which might be "created" with a version > 1).

I guess it's probably fine, so long as all of our actions are talking specifically about MongoDB operations (which I guess is already implied with beforeOverwrite, and differentiated in beforeSnapshotLookup)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I also can't help but wonder if this could have just been a "special case" of beforeOverwrite (maybe renamed beforeWrite), where query happens to be null, but I guess it's a bit late for that now anyway.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, as discussed, this is probably all fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in the weekly meeting, I do still like it this way but open to adding another middleware for both at some point!

if (middlewareErr) {
return callback(middlewareErr);
}
callback(null, true);
collection.insertOne(request.documentToWrite, function(err) {
if (err) {
// Return non-success instead of duplicate key error, since this is
// expected to occur during simultaneous creates on the same id
if (err.code === 11000 && /\b_id_\b/.test(err.message)) {
return callback(null, false);
}
return callback(err);
}
callback(null, true);
});
});
} else {
request.query = {_id: id, _v: request.documentToWrite._v - 1};
Expand Down
2 changes: 2 additions & 0 deletions src/middleware/actions.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
module.exports = {
// Triggers before the call to write a new document is made
beforeCreate: 'beforeCreate',
// Triggers before the call to replace a document is made
beforeOverwrite: 'beforeOverwrite',
// Triggers directly before the call to issue a query for snapshots
Expand Down
68 changes: 68 additions & 0 deletions test/test_mongo_middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var ShareDbMongo = require('..');

var mongoUrl = process.env.TEST_MONGO_URL || 'mongodb://localhost:27017/test';
var BEFORE_EDIT = ShareDbMongo.MiddlewareActions.beforeOverwrite;
var BEFORE_CREATE = ShareDbMongo.MiddlewareActions.beforeCreate;
var BEFORE_SNAPSHOT_LOOKUP = ShareDbMongo.MiddlewareActions.beforeSnapshotLookup;

function create(callback) {
Expand Down Expand Up @@ -155,6 +156,63 @@ describe('mongo db middleware', function() {
});
});

describe(BEFORE_CREATE, function() {
it('has the expected properties on the request object', function(done) {
db.use(BEFORE_CREATE, function(request, next) {
expect(request).to.have.all.keys([
'action',
'collectionName',
'documentToWrite',
'op',
'options'
]);
expect(request.action).to.equal(BEFORE_CREATE);
expect(request.collectionName).to.equal('testcollection');
expect(request.documentToWrite.foo).to.equal('bar');
expect(request.op).to.exist;
expect(request.options.testOptions).to.equal('baz');
next();
done();
});

var snapshot = {type: 'json0', id: 'test1', v: 1, data: {foo: 'bar'}};

db.commit('testcollection', snapshot.id, {v: 0, create: {}}, snapshot, {testOptions: 'baz'}, function(err) {
if (err) return done(err);
});
});

it('should augment the written document when commit is called', function(done) {
// Change the written value of foo to be `fuzz`
db.use(BEFORE_CREATE, function(request, next) {
request.documentToWrite.foo = 'fuzz';
next();
});

var snapshot = {type: 'json0', id: 'test1', v: 1, data: {foo: 'bar'}};

db.commit('testcollection', snapshot.id, {v: 0, create: {}}, snapshot, null, function(err) {
if (err) return done(err);
expectDocumentToContainFoo('fuzz', done);
});
});

it('returns without writing when there was a middleware error', function(done) {
db.use(BEFORE_CREATE, function(_, next) {
next(new Error('Oh no!'));
});

var snapshot = {type: 'json0', id: 'test1', v: 1, data: {foo: 'bar'}};

db.commit('testcollection', snapshot.id, {v: 0, create: {}}, snapshot, null, function(err) {
expectDocumentNotToExist(function() {
if (err) return done();
done('Expected an error, did not get one');
});
});
});
});

describe(BEFORE_SNAPSHOT_LOOKUP, function() {
it('has the expected properties on the request object before getting a single snapshot', function(done) {
db.use(BEFORE_SNAPSHOT_LOOKUP, function(request, next) {
Expand Down Expand Up @@ -284,4 +342,14 @@ describe('mongo db middleware', function() {
cb();
});
};

function expectDocumentNotToExist(cb) {
var query = {_id: 'test1'};
Copy link
Contributor

Choose a reason for hiding this comment

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

This function feels weird to me:

  • it uses magic strings
  • it assumes it's the only assertion, which is always run at the end (what if I want to assert many things?)

I'd normally prefer something like:

describe('', () => {
  var docId;

  beforeEach(() => {
    docId = 'test1';
  });

  function expectStuff() {
    // has access to docId
  }
});

But better yet, can't we just make a helper method:

function docCount(collection, id, cb) {
  var query = {_id: id};
  db.query('collection', query, null, null, function(err, results) {
    var count = null;
    if (!err) count = results.length;
    cb(err, count);
  }
}

Still has some boilerplate in the code itself, but that's the sad fact of callbacks, I guess:

docCount(collection, id, function(error, count) {
  if (error) return done(error);
  expect(count).to.equal(0);
  return done();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, will fix this in another PR


db.query('testcollection', query, null, null, function(err, results) {
if (err) return cb(err);
expect(results).to.be.empty;
cb();
});
};
});