For readSnapshots middleware, add method and parameter properties to first parameter #263

ericyhwang · 2019-01-16T01:19:50Z

When monitoring for unusual response sizes, it can be useful to record which client request was associated with the response. Unfortunately, ShareDB's current "readSnapshots" middleware, which is how you get info about snapshot responses, doesn't contain information about the associated request.

This PR adds a couple additional fields to the "readSnapshots" middleware's request param, method and parameters.

The "readSnapshots" middleware function now has a first parameter request like this:

function(request, next) {
  let {
    collection,
    snapshots,
    method,  // Backend.prototype.READ_SNAPSHOTS_METHODS: 'fetch', 'query', etc.
    parameters,  // Plain JS object, shape varies depending on method
    snapshotType,  // Backend.prototype.SNAPSHOT_TYPES
    agent
  } = request;
}

…perties The "readSnapshots" middleware function can now look like this: ``` function(request, next) { let { collection, snapshots, requestMethod, // 'fetch', 'query', etc. requestParams, // object whose shape can vary snapshotType, agent } = request; } ```

coveralls · 2019-01-16T01:21:58Z

Coverage increased (+0.02%) to 95.866% when pulling 7e12683 on readSnapshots-context into 7938f7f on master.

ericyhwang · 2019-01-16T01:22:19Z

lib/backend.js

@@ -578,7 +591,16 @@ Backend.prototype._query = function(agent, request, callback) {
  var backend = this;
  request.db.query(request.collection, request.query, request.fields, request.options, function(err, snapshots, extra) {
    if (err) return callback(err);
-    backend._sanitizeSnapshots(agent, request.snapshotProjection, request.collection, snapshots, backend.SNAPSHOT_TYPES.current, function(err) {
+    var requestContext = {
+      requestMethod: 'query',  // TODO: Should this distinguish between queryFetch and querySubscribe?


Question here for reviewers - This currently doesn't distinguish between "queryFetch" and the initial query issued by "querySubscribe". Should it?

The initial "querySubscribe" DB query is for all intents and purposes identical to the query issued by "queryFetch". The big difference is in whether a pub/sub subscription is registered.

yes, let's make the method be the same name as the public method on Backend, so queryFetch or querySubscribe

nateps · 2019-01-16T17:16:42Z

lib/backend.js

@@ -353,20 +350,28 @@ Backend.prototype.getOpsBulk = function(agent, index, fromMap, toMap, callback)
 Backend.prototype.fetch = function(agent, index, id, callback) {
  var start = Date.now();
  var projection = this.projections[index];
-  var collection = (projection) ? projection.target : index;
+  var dbCollection = (projection) ? projection.target : index;


Let's leave this as collection for this PR. we could consider changing it, but we'd want to be consistent everywhere

nateps · 2019-01-16T17:16:55Z

lib/backend.js

    if (err) return callback(err);
    var snapshotProjection = backend._getSnapshotProjection(backend.db, projection);
    var snapshots = [snapshot];
-    backend._sanitizeSnapshots(agent, snapshotProjection, collection, snapshots, backend.SNAPSHOT_TYPES.current, function(err) {
+    var requestContext = {
+      requestMethod: 'fetch',


let's use method

nateps · 2019-01-16T17:17:05Z

lib/backend.js

-    backend._sanitizeSnapshots(agent, snapshotProjection, collection, snapshots, backend.SNAPSHOT_TYPES.current, function(err) {
+    var requestContext = {
+      requestMethod: 'fetch',
+      requestParams: {


let's use parameters

nateps · 2019-01-16T17:23:43Z

lib/backend.js

@@ -578,7 +591,16 @@ Backend.prototype._query = function(agent, request, callback) {
  var backend = this;
  request.db.query(request.collection, request.query, request.fields, request.options, function(err, snapshots, extra) {
    if (err) return callback(err);
-    backend._sanitizeSnapshots(agent, request.snapshotProjection, request.collection, snapshots, backend.SNAPSHOT_TYPES.current, function(err) {
+    var requestContext = {
+      requestMethod: 'query',  // TODO: Should this distinguish between queryFetch and querySubscribe?


yes, let's make the method be the same name as the public method on Backend, so queryFetch or querySubscribe

- requestMethod -> method, requestParams -> parameters - Distinguish methods 'queryFetch' and 'querySubscribe'

alecgibson · 2019-01-30T17:17:41Z

lib/backend.js

-    snapshots: snapshots,
-    snapshotType: snapshotType
-  };
+  requestContext.collection = collection;


It might be a good optimisation to initialise these properties whenever requestContext is initialised. It may be beneficial to declare a class for clarity about what exact information we're expecting in the middleware.

alecgibson · 2019-01-30T17:18:15Z

lib/backend.js

@@ -366,7 +363,15 @@ Backend.prototype.fetch = function(agent, index, id, callback) {
    if (err) return callback(err);
    var snapshotProjection = backend._getSnapshotProjection(backend.db, projection);
    var snapshots = [snapshot];
-    backend._sanitizeSnapshots(agent, snapshotProjection, collection, snapshots, backend.SNAPSHOT_TYPES.current, function(err) {
+    var requestContext = {
+      method: 'fetch',


It would be nice to move these magic strings into an object, similar to the middleware actions. Just to clarify which methods are available, and to make refactoring easy, etc.

ericyhwang · 2019-02-06T17:52:56Z

Thanks for the review, changes made! Give it another look when you've got the chance.

alecgibson · 2019-02-07T09:31:50Z

@ericyhwang changes look good to me. Just greenify the build, and I'll approve.

alecgibson

🚀

ericyhwang · 2019-02-07T17:50:26Z

I hadn't merged in your changes from last week the first time, and your tests caught the fact that I needed to support the new method too - thanks! Tests and CI doing their job. :)

ericyhwang · 2019-02-15T02:45:10Z

#269 reported that this caused a regression in 'readSnapshots' middleware for queryPoll() calls specifically, so I unpublished the new version from NPM.

I'm going to revert this for now so that forks don't pick it up. I'll work on adding a test for the queryPoll+readSnapshots case, fix the issue, and create a new PR.

ericyhwang requested a review from nateps January 16, 2019 01:19

ericyhwang commented Jan 16, 2019

View reviewed changes

nateps reviewed Jan 16, 2019

View reviewed changes

Code review changes for readSnapshots middleware

58e29e6

- requestMethod -> method, requestParams -> parameters - Distinguish methods 'queryFetch' and 'querySubscribe'

alecgibson reviewed Jan 30, 2019

View reviewed changes

Switch readSnapshots middleware context to use named class/constants

57c8598

ericyhwang added 2 commits February 7, 2019 09:28

Merge branch 'master' into readSnapshots-context

15a438f

Support new readSnapshots context info in fetchSnapshotByTimestamp

7e12683

alecgibson approved these changes Feb 7, 2019

View reviewed changes

ericyhwang changed the title ~~For readSnapshots middleware, add requestMethod and requestParams properties~~ For readSnapshots middleware, add method and parameter properties to first parameter Feb 7, 2019

ericyhwang merged commit 66c7e00 into master Feb 14, 2019

ericyhwang deleted the readSnapshots-context branch February 14, 2019 22:41

ericyhwang mentioned this pull request Feb 15, 2019

Revert "For readSnapshots middleware, add method and parameter properties to first parameter" #270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For readSnapshots middleware, add method and parameter properties to first parameter #263

For readSnapshots middleware, add method and parameter properties to first parameter #263

ericyhwang commented Jan 16, 2019 •

edited

Loading

coveralls commented Jan 16, 2019 •

edited

Loading

ericyhwang Jan 16, 2019

nateps Jan 16, 2019

nateps Jan 16, 2019

nateps Jan 16, 2019

nateps Jan 16, 2019

nateps Jan 16, 2019

alecgibson Jan 30, 2019

alecgibson Jan 30, 2019

ericyhwang commented Feb 6, 2019

alecgibson commented Feb 7, 2019

alecgibson left a comment

ericyhwang commented Feb 7, 2019

ericyhwang commented Feb 15, 2019

For readSnapshots middleware, add method and parameter properties to first parameter #263

For readSnapshots middleware, add method and parameter properties to first parameter #263

Conversation

ericyhwang commented Jan 16, 2019 • edited Loading

coveralls commented Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericyhwang commented Feb 6, 2019

alecgibson commented Feb 7, 2019

alecgibson left a comment

Choose a reason for hiding this comment

ericyhwang commented Feb 7, 2019

ericyhwang commented Feb 15, 2019

ericyhwang commented Jan 16, 2019 •

edited

Loading

coveralls commented Jan 16, 2019 •

edited

Loading