Skip to content

Conversation

pypmannetjies
Copy link
Contributor

@pypmannetjies pypmannetjies commented Feb 3, 2021

The addition of middleware is done in order to support sharding, in order for consumers of this library to satisfy any requirements for shard keys as defined by the MongoDB docs: https://docs.mongodb.com/manual/core/sharding-shard-key

Usage follows the same pattern as middlewares on ShareDB with differences on what the middleware context looks like depending on the actions.

Currently only adding a limited set of actions and properties on the context in order to reduce scope of this work.

@coveralls
Copy link

coveralls commented Feb 3, 2021

Coverage Status

Coverage increased (+0.5%) to 91.54% when pulling d82a7dc on add-middleware into 67d0728 on master.

@pypmannetjies pypmannetjies marked this pull request as ready for review February 3, 2021 21:48
[![NPM Version](https://img.shields.io/npm/v/sharedb-mongo.svg)](https://npmjs.org/package/sharedb-mongo)
[![Build Status](https://travis-ci.org/share/sharedb-mongo.svg?branch=master)](https://travis-ci.org/share/sharedb-mongo)
[![Coverage Status](https://coveralls.io/repos/github/share/sharedb-mongo/badge.svg?branch=master)](https://coveralls.io/github/share/sharedb-mongo?branch=master)
[![NPM Version](https://img.shields.io/npm/v/sharedb-mongo.svg)](https://npmjs.org/package/sharedb-mongo)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My linter kicked in on this README

index.js Outdated
var succeeded = !!result.modifiedCount;
callback(null, succeeded);
request.query = {_id: id, _v: request.doc._v - 1};
self.trigger(self.MIDDLEWARE_ACTIONS.beforeEdit, 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.

Just so I'm clear: this doesn't need to be triggered on create?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it does not at the moment, and I believe that if we wanted to, we should make another middleware for beforeCreate. They are different because beforeEdit has a query (for an existing document) but beforeCreate would not.

Now that you mention it though, we probably do want to add the beforeCreate for our purposes to ensure we never create documents with missing shard keys 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I would suggest let's leave the beforeCreate for now - I will add it in another PR if we need it

fall back to fetching all ops and working backwards from the
current version.

### Middlewares
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only real change to the docs

README.md Outdated
- `'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
- `query` - A filter that will be used to lookup the document that is about to be edited
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
- `query` - A filter that will be used to lookup the document that is about to be edited
- `query` - A filter that will be used to lookup the document that is about to be edited, which should always include an ID and snapshot version e.g. `{_id: 'uuid', _v: 1}`

Copy link
Contributor

@ericyhwang ericyhwang left a comment

Choose a reason for hiding this comment

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

LGTM

@pypmannetjies pypmannetjies merged commit 5ede68f into master Feb 10, 2021
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