Skip to content

Clear handler cache #193

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.

Sign up for GitHub

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

Merged
ritch merged 1 commit into from
Feb 18, 2014
Merged

Clear handler cache #193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions lib/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ app.model = function (Model, config) {
var remotingClassName = compat.getClassNameForRemoting(Model);
this.remotes().exports[remotingClassName] = Model;
this.models().push(Model);
clearHandlerCache(this);
Model.shared = true;
Model.app = this;
Model.emit('attached', this);
Expand Down Expand Up @@ -639,6 +640,10 @@ function tryReadConfig(cwd, fileName) {
}
}

function clearHandlerCache(app) {
app._handlers = undefined;
}

/**
* Install all express middleware required by LoopBack.
*
Expand Down
16 changes: 12 additions & 4 deletions test/app.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ describe('app', function() {
expect(app.remotes().exports).to.eql({ color: Color });
});

it('clears handler cache', function() {
var originalRestHandler = app.handler('rest');
var Color = db.createModel('color', {name: String});
app.model(Color);
var newRestHandler = app.handler('rest');
expect(originalRestHandler).to.not.equal(newRestHandler);
});

@bajtos bajtos Feb 19, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you rewrite this test to describe the behaviour as observed by a LB user - i.e. Color is available via REST - instead of describing the implementation details?

Something along these lines:

app.use(loopback.rest());
request(app).get('/colors').expect(404, function(err, res) {
  if (err) return done(err);
  var Color = db.createModel('color', {name: String});
  app.model(Color);
  request(app).get('/colors').expect(200, done);
});

@bajtos bajtos Feb 19, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I have not realised the patch had already landed. Never mind, this is not critical, the test can stay as it is.

@gcirne gcirne Feb 19, 2014

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could rewrite the test, no problem. Then maybe you guys could merge it
again.

What do you think?

On Wednesday, February 19, 2014, Miroslav Bajtoš notifications@github.com
wrote:

In test/app.test.js:

@@ -29,6 +29,14 @@ describe('app', function() {
expect(app.remotes().exports).to.eql({ color: Color });
});

  • it('clears handler cache', function() {
  •  var originalRestHandler = app.handler('rest');

  •  var Color = db.createModel('color', {name: String});

  •  app.model(Color);

  •  var newRestHandler = app.handler('rest');

  •  expect(originalRestHandler).to.not.equal(newRestHandler);
  • });

Hmm, I have not realised the patch had already landed. Never mind, this is
not critical, the test can stay as it is.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/193/files#r9861989
.

Guilherme Machado Cirne
gcirne@gmail.com

@bajtos bajtos Feb 19, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@gcirne Don't worry about that.

@ritch ritch Feb 19, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bajtos has a good point. We are specifying (via the test) that calling app.model() results in a different handler. Ideally we would be able to rewrite the handler mechanism and this test should still pass.

@gcirne - feel free to submit a PR that rewrites the test. Not a priority IMO though.


describe('in compat mode', function() {
before(function() {
loopback.compat.usePluralNamesForRemoting = true;
Expand Down Expand Up @@ -89,7 +97,7 @@ describe('app', function() {
beforeEach(function () {
app.boot({
app: {
port: 3000,
port: 3000,
host: '127.0.0.1',
restApiRoot: '/rest-api',
foo: {bar: 'bat'},
Expand All @@ -106,7 +114,7 @@ describe('app', function() {
dataSources: {
'the-db': {
connector: 'memory'
}
}
}
});
});
Expand Down Expand Up @@ -153,14 +161,14 @@ describe('app', function() {
var app = loopback();
app.boot({
app: {
port: undefined,
port: undefined,
host: undefined
}
});
return app;
}
});

it('should be honored', function() {
var assertHonored = function (portKey, hostKey) {
process.env[hostKey] = randomPort();
Expand Down