Closed ritch closed 7 years ago
@ritch - the initial chunk of code sits in server/server.js
correct?
A boot script works fine... The goal is to add this to core
The mechanism of passing the context object via the options
argument looks ok, it's consistent with what we already do for transactions.
However, I dislike the fact that we want to expose the full remoting context to all model methods.
IMO, model methods should be as much transport (remoting-context) agnostic as possible. In my mind, here is the basic promise of LoopBack and strong-remoting: write your model methods once, then invoke them using whatever transport is most appropriate (REST RPC, JSON-RPC, WebSocket, MQTT, etc.). Once users start coupling their method implementation with remoting context details, it will be difficult to switch to a different transport. For example, I believe the websocket transport does not have the concept of req
and res
objects as provided by express.
We already had a similar discussion when implementing loopback.context()
middleware, see https://github.com/strongloop/loopback/pull/337#issuecomment-59007194 and the following comments. The agreement was to implement a flag (disabled by default) which turns on this new behavior.
I am proposing to do the same here, for example:
// usage - code-first
app.enableRemotingContextInjection();
// usage via loopback-boot
// server/config.json
{
port: 3000,
enableRemotingContextInjection: true,
// etc.
}
// implementation - provided by LoopBack
app.enableRemotingContextInjection = function() {
app.remotes().before('*.*', inject);
app.remotes().before('*.prototype.*', function(ctx, instance, next) {
inject(ctx, next);
});
// etc.
};
Thoughts?
I think we should leave users the choice, either this or use the cls
approach. Sometimes you need access to the context, possibly even outside of the methods covered above.
@ritch how would one set the accessToken or a user object?
eg. When I create a middleware after loopback.token() to set things up, how would I get access to ctx.options.remoteCtx
to set things?
nvm, I understand that ctx.options.remoteCtx
is an object with req
and res
etc.
I'm finding that using the above sample code does not inject the remoteCtx
when calling a models methods directly.
Eg. I have a contractor model that hasMany contracts. The Contract.before('access', function () { }) hook is breaking when I try to access a contractor (ctx.options is undefined) but works fine when accessing the contract directly
Example: In contractor.js
Contractor.afterRemote('**', function (ctx, next) {
var Contract = Contractor.app.models.Contract
Contract.find(function (contracts) {
console.log(contracts)
})
})
Blows up with a 500 error when accessing contractor via explorer with the message:
TypeError: Cannot read property 'req' of undefined
which comes from the Contract.before('access') when trying to access ctx.options.remoteCtx.req which works fine when accessing Contract directly
Just to summarize the error I'm seeing...
Anytime I access a related model, whether it's using includes in the url or just running a MyRelatedModel.find, the before access hooks in the myRelatedModel instance context.options does not contain remoteCtx.
Anytime I access a related model, whether it's using includes in the url or just running a MyRelatedModel.find, the before access hooks in the myRelatedModel instance context.options does not contain remoteCtx.
This is a know issue of remoting hooks, see https://github.com/strongloop/loopback/issues/737. If you are accessing your Contract model via Contractor model, then you need to configure context-injecting hook for Contractor too.
You need to pass the options.remoteCtx
yourself in the example you mentioned above.
Contractor.afterRemote('**', function (ctx, next) {
var Contract = Contractor.app.models.Contract
var filter = {};
var options = {remoteCtx: ctx};
// the example above was also missing the `err` argument
Contract.find(filter, options, function (err, contracts) {
console.log(contracts);
});
});
This highlights a downside of this approach: you must pass the remoteCtx
around yourself. The advantage is you get to control when the remoteCtx
is set. It is also much less magic (and error prone) than using the cls
approach.
This approach breaks User.login
for us. The 3rd argument here: https://github.com/strongloop/loopback/blob/master/common/models/user.js#L169 is now the options arg (containing the remoteCtx object), which results in an error here: https://github.com/strongloop/loopback/blob/master/common/models/user.js#L228
Thanks @ritch most helpful info. I think i'm almost there. Small problem though:
Via explorer: GET /contracts
Contract.observe('access', function (ctx, next) {
//ctx has no access to remoting context
next();
});
I believe this is just the same issue you describe a fix for above except that I'm not sure (since I'm not directly calling Contract.find
) how I can inject the remote context via the options object you describe above. My understanding is that if I could directly call:
Contract.find(filter, { remoteCtx: ctx }, function (err, contracts) {
})
then in:
Contract.observe('access', function (ctx, next) {
//ctx.options.remoteCtx will be set
});
@digitalsadhu you need to inject the remoting context into options argument via a remoting hook. See the code in the issue description.
function inject(ctx, next) {
var options = hasOptions(ctx.method.accepts) && (ctx.args.options || {});
if(options) {
options.remoteCtx = ctx;
ctx.args.options = options;
}
next();
}
app.remotes().before('*.*', inject);
app.remotes().before('*.prototype.*', function(ctx, instance, next) {
inject(ctx, next);
});
This code will inject remoting context to all models and all methods. If you prefer to inject the context only for Contract methods, then you can change the hook spec:
app.remotes().before('Contract.*', inject);
app.remotes().before('Contract.prototype.*', function(ctx, instance, next) {
inject(ctx, next);
});
Thanks @bajtos I am doing that in a context.js boot script. Thats working fine in most cases. There are a couple places where its not working:
Contract.observe('access') doesn't get ctx.options.remoteCtx where as Contract.observe('after save') does.
I should also say, I've logged out the injection and it does happen before the access hook is called and all looks well, it just never makes it onto the access hook ctx object
This works:
Contract.observe('after save', function createHistory(ctx, next) {
var History = Contract.app.models.History;
var contract = ctx.instance;
History.create({
contractId: contract.id,
createdBy: ctx.options.remoteCtx.req.user.name,
snapshot: JSON.stringify(contract)
}, next);
});
This doesn't work:
Contract.observe('access', function limitContractStaff(ctx, next) {
var user = ctx.options.remoteCtx.req.user;
//ctx.options === {}
if (user.group === roles.MANAGER)
ctx.query.where = merge(ctx.query.where || {}, { createdBy: user.name });
next();
});
NVM, pretty sure its just an overly complicated model with side effects I wasn't aware of. Sorry for the spam ;)
@digitalsadhu OK :) OTOH, it is possible that your model method doesn't have options
argument and therefore the remoting hook does not inject remoteCtx
.
Nope. I've tracked down the last of my issues and now have a working solution. Every case was just me not explicitly passing {remoteCtx:ctx} to model methods such as find or findById etc. once I had tracked down all of those everything worked.
@bajtos @ritch
FWIW The reason I've jumped through hoops to get rid of getCurrentContext with this approach is that I've repeatedly run into issues with getCurrentContext returning null with various connectors or platforms. Eg. windows with mssql connector
The latest I've discovered (and should file a bug) is that if you use the memory connector and give a file to add persistence. The use of async.queue to save changes to the json file seems to be the culprit (I did some investigation and removed the use of async.queue and the issue went away).
I have a feeling you guys are already pretty aware of these types of issues and I suspect its mostly related to cls
right?
Anyway, my comment being, I think it be really great to see a simpler approach such as the one provided in this issue used.
Thanks for all the help!
The use of async.queue to save changes to the json file seems to be the culprit (I did some investigation and removed the use of async.queue and the issue went away).
Thanks for digging in this far...
I have a feeling you guys are already pretty aware of these types of issues and I suspect its mostly related to cls right?
Yes - cls
is not a dependency I am comfortable with. The only reason we ever considered it was the simplicity in using getCurrentContext()
... Your experience with it not reliably returning the context is what I have feared for a while. I would recommend moving away from getCurrentContext()
and using my prescribed method. You have to be diligent about passing the remoteCtx
around, which as you've seen is a bit tricky... but this is much easier than trying to figure out if getCurrentContext
is broken or not every time you see it returning null
.
Tip: For put requests to update a model eg. PUT model/1 I found I needed to modify my injection script like so:
app.remotes().before('*.prototype.*', function(ctx, instance, next) {
if (typeof instance === 'function') {
next = instance
}
inject(ctx, next);
});
As for some reason instance is the callback function and next is some other context like object. (Perhaps instance and next are reversed?)
A bit odd but seems to do the trick.
@ritch @bajtos I've found some significant performance issues with this approach.
The problem: Significantly increased wait times (experiencing 20 second TTFB) with multiple simultaneous requests. You don't really notice anything if you just perform 1 request after another (with eg. curl.)
This function is the culprit:
app.remotes().methods().forEach(function(method) {
if(!hasOptions(method.accepts)) {
method.accepts.push({
arg: 'options',
type: 'object',
injectCtx: true
});
}
});
As soon as that code is commented out, problem goes away.
Any ideas why this might be? Is there anyway I can work around this? (Other than reducing simultaneous queries to the server)
@ritch @bajtos Ok figured it out. You can't just inject the entire httpcontext. It's just not performant. I am now injecting my user object directly and all is well again.
function inject(ctx, next) {
var options = hasOptions(ctx.method.accepts) && (ctx.args.options || {});
if(options) {
options.user = ctx.req.user;
ctx.args.options = options;
}
next();
}
@ritch One further thing to note is that the use of the hasOptions
method in your implementation above results in some remote methods getting 2 options args because if the remote method already had an options object arg that did not have the injectCtx
property on it then it is treated as having no options object at all. A new one then gets added and so accepts
ends up like:
accepts: [
{arg: 'options', type: 'object'},
{arg: 'options', type: 'object', injectCtx: true}
]
Not a huge deal as in practice this only happens for the change-stream
methods
I have some ideas for solving the performance issues and keeping a clean separation between remote hooks and impls.
Will link the PR when I have something concrete. Thanks for all the feedback.
@ritch N.P. Awesome will look forward to PR.
I am experiencing the same issue but the current solution doesn't work for me. I have the logged user in the options as suggested in a remote method , but my remote method is calling updateAttribute
and this calls before save
and in before save
i don't have the logged user in the options object
Also looks like it is breaking the login method, this is the error I am getting after adding the suggested code:
2015-10-11T08:05:47.393Z - error: uncaughtException: fn is not a function
{ date: 'Sun Oct 11 2015 11:05:47 GMT+0300 (Jerusalem Daylight Time)',
process:
{ pid: 9604,
uid: null,
gid: null,
cwd: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager',
execPath: 'C:\\Program Files\\nodejs\\node.exe',
version: 'v4.1.2',
argv:
[ 'C:\\Program Files\\nodejs\\node.exe',
'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\server\\server.js' ],
memoryUsage: { rss: 122175488, heapTotal: 93701632, heapUsed: 63284664 } },
os: { loadavg: [ 0, 0, 0 ], uptime: 264889.8970633 },
trace:
[ { column: 9,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback\\common\\models\\user.js',
function: 'tokenHandler',
line: 228,
method: null,
native: false },
{ column: 13,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\relation-definition.js',
function: null,
line: 1814,
method: null,
native: false },
{ column: 19,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\dao.js',
function: null,
line: 320,
method: null,
native: false },
{ column: 49,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js',
function: 'doNotify',
line: 93,
method: null,
native: false },
{ column: 49,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js',
function: 'doNotify',
line: 93,
method: null,
native: false },
{ column: 49,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js',
function: 'doNotify',
line: 93,
method: null,
native: false },
{ column: 49,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js',
function: 'doNotify',
line: 93,
method: null,
native: false },
{ column: 5,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js',
function: 'Function.ObserverMixin._notifyBaseObservers',
line: 116,
method: 'ObserverMixin._notifyBaseObservers',
native: false },
{ column: 8,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js',
function: 'Function.ObserverMixin.notifyObserversOf',
line: 91,
method: 'ObserverMixin.notifyObserversOf',
native: false },
{ column: 15,
file: 'c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js',
function: 'Function.ObserverMixin._notifyBaseObservers',
line: 114,
method: 'ObserverMixin._notifyBaseObservers',
native: false } ],
stack:
[ 'TypeError: fn is not a function',
' at tokenHandler (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback\\common\\models\\user.js:228:9)',
' at c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\relation-definition.js:1814:13',
' at c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\dao.js:320:19',
' at doNotify (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js:93:49)',
' at doNotify (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js:93:49)',
' at doNotify (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js:93:49)',
' at doNotify (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js:93:49)',
' at Function.ObserverMixin._notifyBaseObservers (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js:116:5)',
' at Function.ObserverMixin.notifyObserversOf (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js:91:8)',
' at Function.ObserverMixin._notifyBaseObservers (c:\\Users\\rezra3\\Documents\\GitHub\\shutdownManager\\node_modules\\loopback-datasource-juggler\\lib\\observer.js:114:15)' ] }
I am using In-Memory connector
@ezraroi are you passing your options through to updateAttributes? See Ritchs comment above about needing to pass the options object along yourself.
Hello, I refactored my code as you suggested with remote ctx, but then I can 't login to the system because of:
TypeError: fn is not a function on node_modules\loopback\common\models\user.js:261:9
Seems that code modifies all remote methods including standard ones.
Yeah, what do we have to do to not break existing code with this hack? Does it end with injecting it into login? And why is it so hard to keep the ctx?
any updates on a more performant and solid/recommended way from LB?
@ritch
*.prototype.updateAttritubes
, for example, PUT rest api calls to /Model/{id}, because the next in this case is the second parameter and not the third!ctx.options.remoteCtx.req.accessToken.userId
instead of ctx.options.remoteCtx.accessToken.user
Is this issue affecting dynamic role resolvers? We are using resolvers for our ACL and wonder if the context passed to a resolver is "save" to use, or is it actually suffering the same problem as getCurrentContext()
described here ?
To illustrate an example of a resolver, we do something like this:
// /server/boot/script.js
module.exports = function(app) {
var Role = app.models.Role;
Role.registerResolver('teamMember', function(role, context, cb) {
var userId = context.accessToken.userId; // could this be affected by this issue?
if (userId === 1) {
cb(null, true);
} else {
cb(null, false);
}
}
}
Note that the resolver function is called before any hooks registered with app.remotes().before('*.*', inject)
fire, so the injected context will not be present.
Another issue with using the proposed work-around: there doesn't seem to be a way to have access to the remoteCtx
within a custom validation function.
User.validate('name', customValidator, {message: 'Bad name'});
function customValidator(err) {
// the validation function does not get the options passed in, so how do we get the injected context??
var userId = loopback.getCurrentContext().get('accessToken').userId; // how do we replace this?
if (userId === 1) err();
});
be careful using this workaround. if you have multiple concurrent requests hitting a remote method that is using this, you will likely get cyclical loop in the SharedMethod.convertArg
method.
the issue is the traverse(raw).forEach(...)
which seems to get caught up on some async flow with the concurrent http reqs.
wasn't fun to track down.
you can work around it in two ways, both of which will skip this traverse, which is not needed on the HttpContext
you're setting here.
1 . method.accepts.push
can setup the config object to use http.source === 'context'
and be sure to directly set the argument you created with the actual HttpContext. something like this (I renamed mine)
server.remotes().methods().forEach(method => {
if (!hasOptions(method.accepts)) {
method.accepts.push({
arg: 'httpCtx',
description: '**Do not implement in clients**.',
type: Object,
injectCtx: true,
source: 'context'
});
}
});
function inject(ctx, next) {
const httpCtx = hasOptions(ctx.method.accepts) && ctx;
if (httpCtx) ctx.args.httpCtx = httpCtx;
next();
}
2 . use a custom object to inject as the options.
function RemoteCtx(ctx) {
Object.assign(this, ctx);
}
then when you are setting the ctx
in inject
, be sure to create a new instance of this object.
httpCtx = new RemoteCtx(ctx);
options.remoteCtx = httpCtx;
hope this is helpful!
Any progress on this?
I think the proposed workaround is pretty dirty (because related model methods have a different signature, methods like login/logout have to be excluded, ... )
@ritch this doesn't seem to work with methods with relationships, for example model1.prototype.__create__model2
- I get an error of next is not a function
in the server.js
file.
Any thoughts there? Any other alternatives?
The user.login
would crash the app for the injected new argument:
prj\node_modules\loopback\common\models\user.js:262
fn(defaultError);
^
TypeError: fn is not a function
at prj\node_modules\loopback\common\models\user.js:262:13
at prj\node_modules\loopback\common\models\user.js:313:9
Why not attach the remoteCtx
as session to the context of the operator hooks directly?
Model.observe('access', function(context, next){
var remoteCtx = context.session;
if (remoteCtx) {...}
next()
})
One of the method where options is not available is isValid method of model
Can I lobby to have the issues with getCurrentContext() and this suggestion/issue page etc referenced in the loopback docs (e.g., https://docs.strongloop.com/display/public/LB/Using+current+context)?
Could save others some serious headaches.
Note: After implementing this all of my api calls -- both Rest and Angular bindings -- now require options param (neither work and the app hangs when options is not included!)
These both work:
//Angular Binding
MainUser.find({
options:{},
filter:{
limit: params.count(),
skip: params.page() * params.count()
}
}, function(data){
debugger;
})
//Rest URL
"/api/MainUsers?filter=%7B"limit":10,"skip":10%7D&options=%7B%7D"
This won't work: //Rest URL "/api/MainUsers?filter=%7B"limit":10,"skip":10%7D
Will look into it... I did change the options param to required:false... to no avail.
Thoughts??
@souluniversal It should be Model.find(filter, options)
.
I create a component with black and white list supports: https://github.com/snowyu/loopback-component-remote-ctx.js
btw: I create a component with black and white list supports. https://github.com/snowyu/loopback-component-remote-ctx.js
Sitting late at night and deploying our newly developed solution on customers staging server for tomorrow's presentation when suddenly this getCurrentContext()==null issue occurs. It's been working on our dev machines and our test server without hassle.
Feeling literally nauseous about this whole thing at the moment, with no time to rewrite the codebase, especially when recommended workaround breaks vital functionalities as login/logout etc.
For the moment I'm too exhausted to bring more constructive ideas to the table which leaves me more or less just whining, my apologies. But I really think this is an issue we need to address in a more comprehensive manner and bring a reliable - yet easy to use - solution.
We also experienced a sudden entrance of the getCurrentContext()
is null. The suggested solution does not sound very appealing. Attaching a custom object to every remote method is not something we like to do.
However, there is the option of passing the request and response to a custom method. You just add them to your accepted arguments. From there you will be able to get the user token etc.
For example:
AppUser.remoteMethod('upload', {
http: {path: '/files', verb: 'post'},
accepts: [
{arg: 'req', type: 'object', http: {source: 'req'}},
{arg: 'res', type: 'object', http: {source: 'res'}},
],
})
Then you can define your custom method as
AppUser.upload = ( req, res, next) => { ..}
I don't know how loopback attaches the arguments to the remote method. But it seems to be consistent. One of the many magical features loopback offers.
A potential fix that I've found (if it's working fine on your dev machine) is to run npm shrinkwrap
and install/deploy again with the npm-shrinkwrap.json
present in the deployed bundle.
It seems that various subdependencies/relationships cause this issue, though I have no idea which combination does it.
We also experienced this all of a sudden in the last few days. Nothing changed directly on our end. My guess is that a dependency of a dependency somewhere updated their version of the async module (which I know can cause this issue). I spent the best part of 2 days trying to rework things following the recommendation here. I was using the https://github.com/snowyu/loopback-component-remote-ctx.js component from @snowyu and whilst I can get that working, I can not bring myself to update our entire codebase of 40+ models and thousands of methods and operation hooks to try to manually pass the context through to every single function. Manually passing the context around like that brings a spaghetti nightmare and an unmaintainable (or readable) codebase.
We too have resorted to using npm shrinkwrap to lock things down to a known working state. Without doing this there is no way to guarantee that what works on one build will not fail on the next as the issue can be brought on simply by the order in which npm installs the dependency tree.
The proposed solution from this thread is not a viable alternative to cls IMHO.
@mrfelton +1 for this solution not being viable for a larger codebase. Glad that the npm-shrinkwrap
approach worked for you.
UPDATE 2016-12-22 We ended up implementing a different solution as described in https://github.com/strongloop/loopback/pull/3023.
Tasks
master
branch - see https://github.com/strongloop/loopback/pull/3023 (released inloopback@3.2.0
)2.x
- see https://github.com/strongloop/loopback/issues/3048 (released inloopback@2.37.0
)Original description for posterity
The example below allows you to inject the remote context object (from strong remoting) into all methods that accept an
options
argument.Why? So you can use the remote context in remote methods, operation hooks, connector implementation, etc.
This approach is specifically designed to allow you to do what is possible with
loopback.getCurrentContext()
but without the dependency oncls
. The injection approach is much simpler and should be quite a bit faster sincecls
adds some significant overhead.@bajtos @fabien @raymondfeng