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

MongoDB - loopback.getCurrentContext() return null #885

Closed
wants to merge 5 commits into from
Closed

MongoDB - loopback.getCurrentContext() return null #885

wants to merge 5 commits into from

Conversation

projectxmaker
Copy link
Contributor

loopback.getCurrentContext() return null after MongoDB call returns.

wrap up @Ichenay's patch lchenay@f075c79 (see this comment for more details: #337 (comment))

Fix #809

loopback.getCurrentContext() return null after MongoDB call returns.

wrap up @Ichenay's patch
lchenay@f075c79
@slnode
Copy link

slnode commented Dec 1, 2014

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos
Copy link
Member

bajtos commented Dec 1, 2014

@slnode ok to test

@bajtos
Copy link
Member

bajtos commented Dec 1, 2014

@projectxmaker thank you for the patch. Could you please add a unit-test verifying the fix? The test should fail without the changes proposed here and it should pass once the patch is applied.

Look at the tests in mongodb driver (mongodb/node-mongodb-native@66f8987) for inspiration. For the loopback test, you should probably create a custom model method that calls domains and then call this method from the test via the REST API.

I put together a sample code to show you what I mean:

var Domain = require('domain');

var Product = loopback.createModel('Product', { base: 'Model' });
Product.check = function(next) {
  Domain.create().run(next);
};
Product.remoteMethod('check', { /*...TODO... */ });

// TODO: add afterRemote hook that will verify whether the context is set

app.model(Product, { dataSource: null });
request(app).get('/products/check')
// etc.

@raymondfeng since you are the author of the context middleware, could you please review this patch too?

@bajtos bajtos self-assigned this Dec 1, 2014
@projectxmaker
Copy link
Contributor Author

@bajtos plz help me to verify this test file below

even when the patch is not applied, and datasource is either mongodb or memory, the result returned from this test is OK.

I don't know why, I missed something ??? O.o

var loopback = require('loopback');
var request = require('supertest');
var Domain = require('domain');
var assert = require('assert');

describe('check loopback.getCurrentContext', function() {
  var app = loopback();

  before(function(){
    var called = false;

    app.use(loopback.rest());
    app.dataSource('db', { connector: 'memory' });
    app.dataSource('mongox', {
        "host": "localhost",
        "port": 27017,
        "database": "api",
        "username": "root",
        "password": "123456",
        "name": "mongodb",
        "connector": "mongodb"
      });

    var TestModel = loopback.createModel({name: 'TestModel', properties: {name: String}, options: { base: 'Model'}});
    app.model(TestModel, {dataSource: "db", public: true});

    // before remote hook
    TestModel.beforeRemote('**', function(remotingCtx, unused, next) {
      //Domain.create().run(function(){
        tmpCtx = loopback.getCurrentContext();
        if (tmpCtx) tmpCtx.set('data', 'test');
        next();
      //});
    });

    // function
    TestModel.test = function(inst, cb) {
      called = true;
      cb();
    };

    // remote method
    TestModel.remoteMethod('test', {
      accepts: {arg: 'inst', type: 'TestModel'},
      returns: {root: true},
      http: {path: '/test', verb: 'get'}
    });

    // after remote hook
    TestModel.afterRemote('**', function(ctxx, inst, next){
      ctxx.result.called = called;
      tmpCtx = loopback.getCurrentContext();
      if (tmpCtx) {
        ctxx.result.data = tmpCtx.get('data');
      }else {
        ctxx.result.data = "";
      }
      next();
    });
  });

  it('should fail without the patch and it should pass once the patch is applied', function(done) {
    request(app)
      .get('/TestModels/test')
      .end(function(err, res) {
        if (err) return done(err);
        assert.equal(res.body.called, true);
        assert.equal(res.body.data, 'test');
        done();
      });
  });
});

@projectxmaker
Copy link
Contributor Author

@bajtos hello, are you still with me on this issue ?

@bajtos
Copy link
Member

bajtos commented Dec 10, 2014

Uff, this was tricky. Here is the relevant part of the code that works for me:

var otherDomain = Domain.create();
otherDomain.uid = 'other';
var emitterInOtherDomain = new EventEmitter();
otherDomain.add(emitterInOtherDomain);
setInterval(function() { emitterInOtherDomain.emit('run'); }, 10);

function runInOtherDomain(fn) {
  emitterInOtherDomain.once('run', fn);
}

TestModel.test = function(inst, cb) {
  tmpCtx = loopback.getCurrentContext();
  if (tmpCtx) tmpCtx.set('data', 'test');
  called = true;
  runInOtherDomain(cb);
};

To use this in your test, you should clear the interval after a test is done.

var runInOtherDomain;
var runnerInterval;
before(function setupRunInOtherDomain() {
  var emitterInOtherDomain = new EventEmitter();
  Domain.create().add(emitterInOtherDomain);

  runInOtherDomain = function(fn) {
    emitterInOtherDomain.once('run', fn);
  }
  runnerInterval = setInterval(function() { emitterInOtherDomain.emit('run'); }, 10);
});

after(function tearDownRunInOtherDomain() {
  clearInterval(runnerInterval);
});

@bajtos
Copy link
Member

bajtos commented Dec 10, 2014

that works for me:

"works for me" means that the test fails against the current loopback version. I did not check whether the test passes with your patch applied.

@bajtos
Copy link
Member

bajtos commented Dec 10, 2014

One more thing: My example above will most likely fail even with the patch in place. Here is a version that is more likely to pass:

TestModel.test = function(inst, cb) {
  tmpCtx = loopback.getCurrentContext();
  if (tmpCtx) tmpCtx.set('data', 'test');
  called = true;
  if (process.domain) cb = proces.domain.bind(cb);  // IMPORTANT
  runInOtherDomain(cb);
};

@projectxmaker
Copy link
Contributor Author

@bajtos thanks, following is final test script that failed without patch and worked with patch.

var loopback = require('loopback');
var request = require('supertest');
var Domain = require('domain');
var assert = require('assert');
var EventEmitter = require("events").EventEmitter;

describe('check loopback.getCurrentContext', function() {
  var app = loopback();
  var runInOtherDomain;
  var runnerInterval;

  before(function(){
    var called = false;
    app.use(loopback.rest());
    app.dataSource('db', { connector: 'memory' });
    app.dataSource('mongox', {
        "host": "localhost",
        "port": 27017,
        "database": "api",
        "username": "root",
        "password": "123456",
        "name": "mongodb",
        "connector": "mongodb"
      });

    var TestModel = loopback.createModel({name: 'TestModel', properties: {name: String}, options: { base: 'Model'}});
    app.model(TestModel, {dataSource: "db", public: true});

    var emitterInOtherDomain = new EventEmitter();
    Domain.create().add(emitterInOtherDomain);

    runInOtherDomain = function(fn) {
      emitterInOtherDomain.once('run', fn);
    }

    runnerInterval = setInterval(function() { emitterInOtherDomain.emit('run'); }, 10);

    // function for remote method
    TestModel.test = function(inst, cb) {
      tmpCtx = loopback.getCurrentContext();
      if (tmpCtx) tmpCtx.set('data', 'test');
      called = true;
      if (process.domain) cb = process.domain.bind(cb);  // IMPORTANT
      runInOtherDomain(cb);
    };

    // remote method
    TestModel.remoteMethod('test', {
      accepts: {arg: 'inst', type: 'TestModel'},
      returns: {root: true},
      http: {path: '/test', verb: 'get'}
    });

    // after remote hook
    TestModel.afterRemote('**', function(ctxx, inst, next){
      ctxx.result.called = called;
      tmpCtx = loopback.getCurrentContext();
      if (tmpCtx) {
        ctxx.result.data = tmpCtx.get('data');
      }else {
        ctxx.result.data = "";
      }
      next();
    });
  });

  after(function tearDownRunInOtherDomain() {
    clearInterval(runnerInterval);
  });

  it('should fail without the patch and it should pass once the patch is applied', function(done) {
    request(app)
      .get('/TestModels/test')
      .end(function(err, res) {
        if (err) return done(err);
        assert.equal(res.body.called, true);
        assert.equal(res.body.data, 'test');
        done();
      });
  });
});

@bajtos
Copy link
Member

bajtos commented Dec 15, 2014

@projectxmaker please include the test in the code of this pull request and remove the definition of unused mongox datasource.

test script for issue:
“MongoDB - loopback.getCurrentContext() return null”

#885
@projectxmaker
Copy link
Contributor Author

@bajtos

  • include the test in the code of this pull: done
  • remove the definition of unused mongox datasource: done


// after remote hook
TestModel.afterRemote('**', function(ctxx, inst, next){
ctxx.result.called = called;
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant. If the method was not called then tmpCtx.get('data') returns undefined.

Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@bajtos
Copy link
Member

bajtos commented Dec 16, 2014

The code is not passing the linter, run npm test and fix all issues reported.

@doublemarked
Copy link

I'm keen on seeing this patch merged. I spent 5 minutes to fix the few minor problems with @projectxmaker's work, and now the tests pass. What's the procedure to move this forward?

https://github.com/doublemarked/loopback/compare/885-loopback.getCurrentContext-return-null

@projectxmaker
Copy link
Contributor Author

@doublemarked 👍 sorry, I was very busy last week, thanks for your fix ^^
@bajtos could you take a look at @doublemarked's work :)

@doublemarked
Copy link

:) No worries, @projectxmaker!

@bajtos
Copy link
Member

bajtos commented Jan 5, 2015

Excuse my late reply, I was on holiday. Please agree which version should be moved forward. If it's the version from @doublemarked, then please open a new pull request.

Also change the name of the test to describe the scenario being tested (e.g. "preserves domain across async calls").

@doublemarked
Copy link

@projectxmaker - let me know how you prefer to proceed

@projectxmaker projectxmaker changed the title MongoDB - loopback.getCurrentContext() return null loopback.getCurrentContext Jan 6, 2015
@projectxmaker projectxmaker changed the title loopback.getCurrentContext MongoDB - loopback.getCurrentContext() return null Jan 6, 2015
@projectxmaker
Copy link
Contributor Author

@doublemarked let keep giving a hand in fixing the issue of the test file if you have time ^^

@bajtos can you give me more details about your following comment, I don't get it O.o

The code setting up runInOtherDomain and cleaning up after the test should go to standalone before/after hooks, to make it clear it is only loosely related to the custom app & model built for the test.

@bajtos
Copy link
Member

bajtos commented Jan 7, 2015

To speed the things up, I have made the final amendments myself and landed the patch via 989abf6:

  • reorganize the test code
  • give the test a descriptive name
  • fix remaining linter issues (e.g. missing EOL at EOF)
  • squash all commits into a single one, make the commit message descriptive

Thank you for the contribution!

@doublemarked @projectxmaker could you please verify that the bug is fixed, before I make a release? Just run npm install strongloop/loopback inside your project and then verify that your app works as expected.

@projectxmaker
Copy link
Contributor Author

cool, I tested and it works properly ;)

@bajtos
Copy link
Member

bajtos commented Jan 7, 2015

Thanks, I have released the fix as loopback@2.8.8.

@doublemarked
Copy link

Hey guys - looks great for me as well. Thank you for the release!

@STRML
Copy link
Member

STRML commented Feb 9, 2015

Please see #1080 - domains are tricky.

@dongmai
Copy link

dongmai commented Jul 20, 2016

Hi @bajtos ,

I'm facing this issue on the nearly latest version. Bellow is my pacakge.json

{
  "name": "test",
  "version": "1.0.0",
  "main": "server/server.js",
  "scripts": {
    "start": "node .",
    "pretest": "eslint .",
    "posttest": "nsp check"
  },
  "dependencies": {
    "async": "^2.0.0",
    "compression": "^1.0.3",
    "cors": "^2.5.2",
    "helmet": "^1.3.0",
    "loopback": "^2.22.0",
    "loopback-boot": "^2.6.5",
    "loopback-component-explorer": "^2.4.0",
    "loopback-connector-mongodb": "^1.15.2",
    "loopback-datasource-juggler": "^2.39.0",
    "loopback-testing": "^1.2.0",
    "serve-favicon": "^2.0.1"
  },
  "devDependencies": {
    "eslint": "^2.5.3",
    "nsp": "^2.1.0"
  },
  "repository": {
    "type": "",
    "url": ""
  },
  "license": "UNLICENSED",
  "description": "test"
}

I've used a middleware to save current logged user data into context but it (getCurrentContext) always returned null when working with MongoDB (memory db works just fine.)

Bellow is my code,

app.use(loopback.token()); 
app.use(loopback.context());

app.use(function (req, res, next) {
    if (!req.accessToken) return next();

    app.models.User.findById(req.accessToken.userId, function(err, user) {
        if (err)   return next(err);
        if (!user) return next(new Error('No user with this access token was found.'));
        res.locals.currentUser = user;

        var loopbackContext = loopback.getCurrentContext();
        console.log('SERVER CTX?' +loopbackContext);
        if (loopbackContext) {
            loopbackContext.set('currentUser', user);
        }else{
            throw new Error('Current context is null.');
        }
        next();
    });
});

Can you please have a look ? Have I done something wrong ?

Thanks.

@hash3r
Copy link

hash3r commented Sep 8, 2016

Have just the same problem with getCurrentContext. It returns null

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.

loopback.getCurrentContext() return null after MongoDB call returns
9 participants