Skip to content
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

Add context propagation middleware #337

Merged
merged 4 commits into from
Nov 5, 2014
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
29 changes: 29 additions & 0 deletions example/context/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
var loopback = require('../../');
var app = loopback();

// Create a LoopBack context for all requests
app.use(loopback.context());

// Store a request property in the context
app.use(function saveHostToContext(req, res, next) {
var ns = loopback.getCurrentContext();
ns.set('host', req.host);
next();
});

app.use(loopback.rest());

var Color = loopback.createModel('color', { 'name': String });
Color.beforeRemote('**', function (ctx, unused, next) {
// Inside LoopBack code, you can read the property from the context
var ns = loopback.getCurrentContext();
console.log('Request to host', ns && ns.get('host'));
next();
});

app.dataSource('db', { connector: 'memory' });
app.model(Color, { dataSource: 'db' });

app.listen(3000, function() {
console.log('A list of colors is available at http://localhost:3000/colors');
});
104 changes: 104 additions & 0 deletions lib/middleware/context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
var loopback = require('../loopback');
var juggler = require('loopback-datasource-juggler');
var remoting = require('strong-remoting');
var cls = require('continuation-local-storage');

module.exports = context;

var name = 'loopback';

function createContext(scope) {
// Make the namespace globally visible via the process.context property
process.context = process.context || {};
var ns = process.context[scope];
if (!ns) {
ns = cls.createNamespace(scope);
process.context[scope] = ns;
// Set up loopback.getCurrentContext()
loopback.getCurrentContext = function() {
return ns;
};

chain(juggler);
chain(remoting);
}
return ns;
}

function context(options) {
options = options || {};
var scope = options.name || name;
var enableHttpContext = options.enableHttpContext || false;
var ns = createContext(scope);
// Return the middleware
return function contextHandler(req, res, next) {
if (req.loopbackContext) {
return next();
}
req.loopbackContext = ns;
// Bind req/res event emitters to the given namespace
ns.bindEmitter(req);
ns.bindEmitter(res);
// Create namespace for the request context
ns.run(function processRequestInContext(context) {
// Run the code in the context of the namespace
if (enableHttpContext) {
ns.set('http', {req: req, res: res}); // Set up the transport context
}
next();
Copy link
Member

Choose a reason for hiding this comment

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

As I said before, I am opposed to exposing the http request & response objects to shared methods. Instead, there should be an easy and well-documented way how to write an express middleware that takes the relevant information from the request and set it on the context.

The new example code should look like this:

CartItem.sum = function(cartId, callback) {
  // ...
  var ns = loopback.getCurrentContext();
  console.log(ns.get('requestUrl'));
};

// elsewhere - add a middleware to fill the context
app.use(function(req, res, next) {
  var ns = loopback.getCurrentContext();
  ns.set('requestUrl', req.url);
  next();
});

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a different view here. Sure, the context should allow set/get specific context attributes. But I think there is nothing wrong by exposing req/res in the context considering a lot of express middlewares use req object as the context holder. I'm fine to introduce an option for the middleware to disable req/res.

Copy link
Member

Choose a reason for hiding this comment

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

But I think there is nothing wrong by exposing req/res in the context considering a lot of express middlewares use req object as the context holder.

We are planning to support websocket based transport soon. What kind of req/resp objects is the websocket adapter going to provide in the context?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are going to be transport specific context for each of the protocols we support. Maybe in theory, the rest middleware should set up req/res instead.

Copy link
Member

Choose a reason for hiding this comment

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

There are going to be transport specific context for each of the protocols we support. Maybe in theory, the rest middleware should set up req/res instead.

The idea is that you should write methods that are unaware of the transport. This is really great for testability since you don't need to setup the transport to test the method... you just call it. It also means your code will work from node and over your desired transport. The goal is a clean separation between transport code and application code as well as being able to serialize the spec required to call the method over the transport.

The context object really helps shorten the list of arguments you have to pass to a method. It also removes the need to create a mapping to an argument (eg. the current user) that would be the same for every method.

Our goal should be that middleware (which ARE transport specific) parse out useful info (that is not transport specific) and provide that in the context.

Copy link
Member

Choose a reason for hiding this comment

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

@ritch you have nailed it! 👍 💯

});
};
}

/**
* Create a chained context
* @param {Object} child The child context
* @param {Object} parent The parent context
* @private
* @constructor
*/
function ChainedContext(child, parent) {
this.child = child;
this.parent = parent;
}

/**
* Get the value by name from the context. If it doesn't exist in the child
* context, try the parent one
* @param {String} name Name of the context property
* @returns {*} Value of the context property
*/
ChainedContext.prototype.get = function(name) {
var val = this.child && this.child.get(name);
if (val === undefined) {
return this.parent && this.parent.get(name);
}
};

ChainedContext.prototype.set = function(name, val) {
if (this.child) {
return this.child.set(name, val);
} else {
return this.parent && this.parent.set(name, val);
}
};

ChainedContext.prototype.reset = function(name, val) {
if (this.child) {
return this.child.reset(name, val);
} else {
return this.parent && this.parent.reset(name, val);
}
};

function chain(child) {
if (typeof child.getCurrentContext === 'function') {
var childContext = new ChainedContext(child.getCurrentContext(),
loopback.getCurrentContext());
child.getCurrentContext = function() {
return childContext;
};
} else {
child.getCurrentContext = loopback.getCurrentContext;
}
}
42 changes: 25 additions & 17 deletions lib/middleware/rest.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/

var loopback = require('../loopback');
var async = require('async');

/*!
* Export the middleware.
Expand All @@ -22,36 +23,43 @@ module.exports = rest;
*/

function rest() {
var tokenParser = null;
return function(req, res, next) {
return function restApiHandler(req, res, next) {
var app = req.app;
var handler = app.handler('rest');
var restHandler = app.handler('rest');

if (req.url === '/routes') {
res.send(handler.adapter.allRoutes());
return res.send(restHandler.adapter.allRoutes());
} else if (req.url === '/models') {
return res.send(app.remotes().toJSON());
} else if (app.isAuthEnabled) {
if (!tokenParser) {
}

var preHandlers;

if (!preHandlers) {
preHandlers = [];
var remotingOptions = app.get('remoting') || {};

var contextOptions = remotingOptions.context;
if (contextOptions !== false) {
if (typeof contextOptions !== 'object')
contextOptions = {};
preHandlers.push(loopback.context(contextOptions));
}

if (app.isAuthEnabled) {
// NOTE(bajtos) It would be better to search app.models for a model
// of type AccessToken instead of searching all loopback models.
// Unfortunately that's not supported now.
// Related discussions:
// https://github.com/strongloop/loopback/pull/167
// https://github.com/strongloop/loopback/commit/f07446a
var AccessToken = loopback.getModelByType(loopback.AccessToken);
tokenParser = loopback.token({ model: AccessToken });
preHandlers.push(loopback.token({ model: AccessToken }));
}

tokenParser(req, res, function(err) {
if (err) {
next(err);
} else {
handler(req, res, next);
}
});
} else {
handler(req, res, next);
}

async.eachSeries(preHandlers.concat(restHandler), function(handler, done) {
handler(req, res, done);
}, next);
};
}
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
"strong-remoting": "^2.4.0",
"uid2": "0.0.3",
"underscore": "~1.7.0",
"underscore.string": "~2.3.3"
"underscore.string": "~2.3.3",
"continuation-local-storage": "~3.1.1"
},
"peerDependencies": {
"loopback-datasource-juggler": "^2.8.0"
Expand Down
101 changes: 101 additions & 0 deletions test/rest.middleware.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,107 @@ describe('loopback.rest', function() {
});
});

describe('context propagation', function() {
var User;

beforeEach(function() {
User = givenUserModelWithAuth();
User.getToken = function(cb) {
var context = loopback.getCurrentContext();
var req = context.get('http').req;
expect(req).to.have.property('accessToken');

var juggler = require('loopback-datasource-juggler');
expect(juggler.getCurrentContext().get('http').req)
.to.have.property('accessToken');

var remoting = require('strong-remoting');
expect(remoting.getCurrentContext().get('http').req)
.to.have.property('accessToken');

cb(null, req && req.accessToken ? req.accessToken.id : null);
};
// Set up the ACL
User.settings.acls.push({principalType: 'ROLE',
principalId: '$authenticated', permission: 'ALLOW',
property: 'getToken'});

loopback.remoteMethod(User.getToken, {
accepts: [],
returns: [
{ type: 'object', name: 'id' }
]
});
});

function invokeGetToken(done) {
givenLoggedInUser(function(err, token) {
if (err) return done(err);
request(app).get('/users/getToken')
.set('Authorization', token.id)
.expect(200)
.end(function(err, res) {
if (err) return done(err);
expect(res.body.id).to.equal(token.id);
done();
});
});
}

it('should enable context using loopback.context', function(done) {
app.use(loopback.context({ enableHttpContext: true }));
app.enableAuth();
app.use(loopback.rest());

invokeGetToken(done);
});

it('should enable context with loopback.rest', function(done) {
app.enableAuth();
app.set('remoting', { context: { enableHttpContext: true } });
app.use(loopback.rest());

invokeGetToken(done);
});

it('should support explicit context', function(done) {
app.enableAuth();
app.use(loopback.context());
app.use(loopback.token(
{ model: loopback.getModelByType(loopback.AccessToken) }));
app.use(function(req, res, next) {
loopback.getCurrentContext().set('accessToken', req.accessToken);
next();
});
app.use(loopback.rest());

User.getToken = function(cb) {
var context = loopback.getCurrentContext();
var accessToken = context.get('accessToken');
expect(context.get('accessToken')).to.have.property('id');

var juggler = require('loopback-datasource-juggler');
context = juggler.getCurrentContext();
expect(context.get('accessToken')).to.have.property('id');

var remoting = require('strong-remoting');
context = remoting.getCurrentContext();
expect(context.get('accessToken')).to.have.property('id');

cb(null, accessToken ? accessToken.id : null);
};

loopback.remoteMethod(User.getToken, {
accepts: [],
returns: [
{ type: 'object', name: 'id' }
]
});

invokeGetToken(done);
});
});

function givenUserModelWithAuth() {
// NOTE(bajtos) It is important to create a custom AccessToken model here,
// in order to overwrite the entry created by previous tests in
Expand Down