Fix relation race condition in model glob #3565

zbarbuto · 2017-08-15T01:37:39Z

Description

#3504 added support for glob-style patterns when disabling remote methods. However, if a related model is attached after the initial model is configured, the globs do not work. I have updated to reconfigure shared method properties after each new model is attached to remoting.

This was not caught in testing of #3504 as the root model was attached after the related model. Have updated the test to fail without the fix.

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-08-15T01:37:40Z

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

slnode · 2017-08-15T01:37:46Z

Can one of the admins verify this patch?

slnode · 2017-08-15T01:37:46Z

Can one of the admins verify this patch?

slnode · 2017-08-15T01:37:46Z

Can one of the admins verify this patch?

slnode · 2017-08-15T01:37:46Z

Can one of the admins verify this patch?

bajtos

Hi @zbarbuto, thank you for catching the bug and sending a pull request to fix it! Please take a look at my comments below 👇

bajtos · 2017-08-15T13:09:37Z

lib/application.js

@@ -124,6 +124,9 @@ app.model = function(Model, config) {
  if (arguments.length > 1) {
    config = config || {};
    configureModel(Model, config, this);
+    this.on('modelRemoted', function() {
+      setSharedMethodSharedProperties(Model, this, config);


This will add one listener for each application model, which may trigger maxListener warning.

Is there a way how to install a single listener which will walk through application's models to apply "sharedMethods" configuration?

So the problem was that setSharedMethodSharedProperties wasn't exported anywhere. This made it difficult to call it from somewhere useful and setup only one watcher. I've moved it to its own file so it can be called from loopback.js and application.js. Let me know if you can think of a better solution.

The other issue is that the model config only lives for the duration of app.model run. I stuck it on the Model.config property so I could get to it from loopback.js but this is probably not the best way to do it (and is causing another test to fail). Is there a better way to make the config available on the model itself at a later time?

Hey, sorry for the delay.

So the problem was that setSharedMethodSharedProperties wasn't exported anywhere. This made it difficult to call it from somewhere useful and setup only one watcher. I've moved it to its own file so it can be called from loopback.js and application.js. Let me know if you can think of a better solution.

I like that 👍

The other issue is that the model config only lives for the duration of app.model run. I stuck it on the Model.config property so I could get to it from loopback.js but this is probably not the best way to do it (and is causing another test to fail). Is there a better way to make the config available on the model itself at a later time?

I am proposing to include the config in the arguments of modelRemoted event, see

loopback/lib/application.js

Line 149 in 6ba35c2

this.emit('modelRemoted', Model.sharedClass);

// my proposal this.emit('modelRemoted', Model.sharedClass, config);

I am proposing to include the config in the arguments of modelRemoted event, see
this.emit('modelRemoted', Model.sharedClass, config);

The problem I see here is that that only emits the config for the model being remoted. We need to re-configure every model in the app and - therefore - get at the config for every model somehow. This is why I was storing it on the models individually. Maybe it should be a sub-property of sharedClass ie Model.sharedClass.config?

You are right, I did not realize that all classes need to be reconfigured. Let me take a closer look to better understand why some tests are failing with your current approach.

bajtos · 2017-08-15T13:10:25Z

test/loopback.test.js

@@ -853,6 +852,7 @@ describe('loopback', function() {
          },
        },
      });
+      app.model(RelatedModel, {dataSource: 'test'});


I think we should have two tests: one checking the execution path where the related model has been already configured, the second checking the case when the related model is configured later.

bajtos · 2017-08-15T13:11:10Z

@slnode ok to test

bajtos

Looks mostly good, please rework the way how model config is passed to the event listeners per my comment above.

bajtos · 2017-08-30T12:36:00Z

lib/application.js

@@ -17,6 +17,7 @@ var extend = require('util')._extend;
 var RemoteObjects = require('strong-remoting');
 var classify = require('underscore.string/classify');
 var camelize = require('underscore.string/camelize');
+var setSharedMethodSharedProperties = require('./set-shared-method-properties');


Nitpick: Perhaps "configure-shared-methods" would be a better name?

bajtos · 2017-08-30T12:39:01Z

test/loopback.test.js

-    it('hides methods for related models using globs', function() {
+    it('hides methods for related models using globs (model configured first)', function() {
+      var TestModel = app.registry.createModel(uniqueModelName);
+      var RelatedModel = app.registry.createModel(uniqueModelName);


Both TestModel and RelatedModel are using the same model name, that's looks suspicious to me. Since we are using local per-app registry now, there is no need to use unique model names anymore. You can simply do the following:

const TestModel = app.registry.createModel('TestModel'); const RelatedModel = app.registry.createModel('RelatedModel'); // ...

(Also note that we are using ES6 syntax now, see http://loopback.io/doc/en/contrib/style-guide.html#variable-declarations)

Ah, I see, wasn't aware this was the case. Would like like me to refactor the whole PR or just these is fine?

I think fixing it just here (as you have already done) is fine 👍

slnode · 2017-08-31T02:15:32Z

Can one of the admins verify this patch?

slnode · 2017-08-31T02:15:32Z

Can one of the admins verify this patch?

slnode · 2017-08-31T02:15:32Z

Can one of the admins verify this patch?

slnode · 2017-08-31T02:15:33Z

Can one of the admins verify this patch?

bajtos · 2017-08-31T09:08:14Z

@zbarbuto I fixed the test failure in dc12f54 and also cleaned the code a bit. PTAL and let me know if you are ok with the result.

zbarbuto · 2017-08-31T10:09:35Z

@bajtos variable name updated and rebased. Your help has been much appreciated.

bajtos

One more detail to improve, the rest LGTM. Could you please squash all commits into a single one and provide a descriptive commit message following our 50/72 style?

bajtos · 2017-08-31T10:17:52Z

lib/application.js

@@ -17,6 +17,7 @@ var extend = require('util')._extend;
 var RemoteObjects = require('strong-remoting');
 var classify = require('underscore.string/classify');
 var camelize = require('underscore.string/camelize');
+var configureSharedMethods = require('./configure-shared-methods');


This require() is no longer needed, let's remove it. (I should have removed it myself, sorry!)

Globs working depended on the order that models were imported. Remote sharing is now re-calculated whenever a new model is remoted.

bajtos · 2017-09-01T07:44:21Z

Landed, thank you for the contribution! 🎉

calebfroese · 2017-09-22T02:16:10Z

@bajtos Is there a minor release coming soon with this fix?

bajtos · 2017-09-25T14:04:02Z

@calebfroese thank you for the poke, here is the new release: loopback@3.12.0

zbarbuto requested review from bajtos, clark0x, ebarault and fabien as code owners August 15, 2017 01:37

bajtos suggested changes Aug 15, 2017

View reviewed changes

bajtos self-assigned this Aug 15, 2017

bajtos added the bug label Aug 15, 2017

bajtos suggested changes Aug 30, 2017

View reviewed changes

zbarbuto force-pushed the fix/shared-glob branch from dc12f54 to 0fa0768 Compare August 31, 2017 10:08

zbarbuto requested a review from lehni as a code owner August 31, 2017 10:08

bajtos approved these changes Aug 31, 2017

View reviewed changes

Fix relation race condition in model glob

d405432

Globs working depended on the order that models were imported. Remote sharing is now re-calculated whenever a new model is remoted.

zbarbuto force-pushed the fix/shared-glob branch from 0fa0768 to d405432 Compare August 31, 2017 23:48

bajtos merged commit 0f40ca8 into strongloop:master Sep 1, 2017

bajtos mentioned this pull request Sep 13, 2017

disableRemoteMethod does not recognize method aliases #1936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix relation race condition in model glob #3565

Fix relation race condition in model glob #3565

zbarbuto commented Aug 15, 2017

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

bajtos left a comment

bajtos Aug 15, 2017

zbarbuto Aug 17, 2017

zbarbuto Aug 17, 2017

bajtos Aug 30, 2017

zbarbuto Aug 31, 2017

bajtos Aug 31, 2017

bajtos Aug 15, 2017

zbarbuto Aug 17, 2017

bajtos commented Aug 15, 2017

bajtos left a comment

bajtos Aug 30, 2017

bajtos Aug 30, 2017

zbarbuto Aug 31, 2017

bajtos Aug 31, 2017

slnode commented Aug 31, 2017

slnode commented Aug 31, 2017

slnode commented Aug 31, 2017

slnode commented Aug 31, 2017

bajtos commented Aug 31, 2017

zbarbuto commented Aug 31, 2017 •

edited

Loading

bajtos left a comment

bajtos Aug 31, 2017

zbarbuto Aug 31, 2017

bajtos commented Sep 1, 2017

calebfroese commented Sep 22, 2017

bajtos commented Sep 25, 2017

Fix relation race condition in model glob #3565

Fix relation race condition in model glob #3565

Conversation

zbarbuto commented Aug 15, 2017

Description

Checklist

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

slnode commented Aug 15, 2017

bajtos left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Aug 15, 2017

bajtos left a 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

slnode commented Aug 31, 2017

slnode commented Aug 31, 2017

slnode commented Aug 31, 2017

slnode commented Aug 31, 2017

bajtos commented Aug 31, 2017

zbarbuto commented Aug 31, 2017 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 1, 2017

calebfroese commented Sep 22, 2017

bajtos commented Sep 25, 2017

zbarbuto commented Aug 31, 2017 •

edited

Loading