Skip to content

Conversation

pypmannetjies
Copy link
Contributor

This middleware is similar to the beforeOverwrite, but it does not have a query since it is a brand new document

@coveralls
Copy link

coveralls commented Mar 10, 2021

Coverage Status

Coverage increased (+0.03%) to 91.572% when pulling 7b4e212 on add-create-middleware into e22ef94 on master.

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!

};

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

@pypmannetjies pypmannetjies merged commit 6f2ae85 into master Mar 10, 2021
@pypmannetjies pypmannetjies deleted the add-create-middleware branch March 10, 2021 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants