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

PersistedModel's updateAll method crashes the server when invoked via the remote connector #3717

Closed
angfal opened this issue Dec 5, 2017 · 16 comments · Fixed by strongloop/strong-remoting#472
Assignees
Labels

Comments

@angfal
Copy link
Contributor

@angfal angfal commented Dec 5, 2017

Description

I have a model with a remote datasource. When I tries to execute updateAll method my server crashes.

The problem is loopback registers updateAll remote method with an object type, which is not supported by strong-remoting type registry.

setRemoting(PersistedModel, 'updateAll', {
   ...
   returns: {
     arg: 'info',
     description: 'Information related to the outcome of the operation',
     type: { // THE PROBLEM PLACE
       count: {
         type: 'number',
         description: 'The number of instances updated',
       },
     },
     root: true,
   },
   http: {verb: 'post', path: '/update'},
 });

Reproduction

To reproduce the problem, just run this test in any loopback application. It fails on updateAll shared method.

describe('Type Registry Tests', () => {
  let Model;

  before('init a memory datasource', () => {
    if (!app.dataSources.db) {
      app.dataSource('db', {connector: 'memory'});
    }
  });

  beforeEach('create a model and attach it to the app', () => {
    Model = app.registry.createModel({
      name: specs.modelName()
    });
    app.model(Model, {dataSource: 'db'});
  });

  it('should be compatible with get converter method', () => {
    _.forEach(app.models[Model.modelName].sharedClass.methods(), (method) => {
      should(() => {
        _.forEach(method.returns, ({type}) => {
          
          // The type is {count: {type: 'number', description: '...'} 
          // for updateAll method, but it supports only string and array
          app.remotes()._typeRegistry.getConverter(type);
        });
      }).not.throw();
    });
  });
});

Additional information

"loopback": "3.17.0",
"strong-remoting": "3.6.0",

@angfal angfal mentioned this issue Dec 5, 2017
2 of 2 tasks complete
@jannyHou

This comment has been minimized.

Copy link
Contributor

@jannyHou jannyHou commented Dec 11, 2017

Hi @angfal I am trying to reproduce your error, are you running any endpoint from the Explorer(usually http://localhost:3000/explorer/)? If so can you post the endpoint name here? Thanks.

I have a model with a remote datasource. When I tries to execute updateAll method my server crashes.

I am trying to figure out how do you execute the updateAll method.

@jannyHou jannyHou self-assigned this Dec 11, 2017
@jannyHou jannyHou added the triaging label Dec 11, 2017
@angfal

This comment has been minimized.

Copy link
Contributor Author

@angfal angfal commented Dec 11, 2017

@jannyHou I have two loopback servers A and B. The server A contains a model with a remote connector to the server B. When a function from the server A executes 'Model.updateAll' request to the server B, I receive success result, but the server A crashes when it tries to process the response.

I can provide a sandbox example if it doesn't help

@angfal

This comment has been minimized.

Copy link
Contributor Author

@angfal angfal commented Dec 11, 2017

The links to the sandbox:

Just run the both servers and execute Message.updateAll service from the explorer of serverA (3000 port). The server crashes

@jannyHou

This comment has been minimized.

Copy link
Contributor

@jannyHou jannyHou commented Dec 27, 2017

@angfal Thank you now I can see the error, the count's type is number, so its type is string, which should pass the following assertion:

assert(typeof type === 'string' || Array.isArray(type),
    'type must be either an array or a string.');

Probably for some reason the type for count is detected incorrectly...looking into the bug. Will update here when have the fix PR.

@jannyHou jannyHou added bug and removed triaging labels Dec 27, 2017
@jannyHou

This comment has been minimized.

Copy link
Contributor

@jannyHou jannyHou commented Dec 27, 2017

FYI, I created a PR for it, but not sure is it a proper fix, would like to hear opinions from strong-remoting experts before review.
#3741

@angfal

This comment has been minimized.

Copy link
Contributor Author

@angfal angfal commented Dec 27, 2017

@jannyHou Thank you very much! But I made the same pull above. It was rejected

@jannyHou

This comment has been minimized.

Copy link
Contributor

@jannyHou jannyHou commented Dec 28, 2017

I see...closed #3741, I find the rejected PR, following the chat there.

@stale

This comment has been minimized.

Copy link

@stale stale bot commented Feb 26, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 26, 2018
@angfal

This comment has been minimized.

Copy link
Contributor Author

@angfal angfal commented Feb 27, 2018

@jannyHou Any news here?

@stale stale bot removed the stale label Feb 27, 2018
@stale

This comment has been minimized.

Copy link

@stale stale bot commented Apr 28, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 28, 2018
@dhmlau dhmlau removed the stale label May 10, 2018
@ewrayjohnson

This comment has been minimized.

Copy link

@ewrayjohnson ewrayjohnson commented Jul 17, 2019

Wanting to keep this open as I see no fix yet.

@stale

This comment has been minimized.

Copy link

@stale stale bot commented Sep 17, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 17, 2019
@bajtos

This comment has been minimized.

Copy link
Member

@bajtos bajtos commented Sep 24, 2019

Cross-posting #3718 (comment):

Hello, thank you for the pull request. I am afraid we cannot revert #2842 because it's required for client code-generators to work, see the discussion in #2104.

We need to fix strong-remoting and/or loopback-connector-remote to correctly support anonymously defined types in remote invocations.

Let me add major label to this issue to prevent stalebot from trying to automatically close it. (And also because LB should not crash on unhandled errors!)

@stale stale bot removed the stale label Sep 24, 2019
@bajtos bajtos added major and removed in-progress labels Sep 24, 2019
@bajtos bajtos changed the title Loopback persisted-model.js isn't compatible with strong-remoting type-registry.js PersisteModel's updateAll method crashes the server when invoked via the remote connector Sep 24, 2019
@bajtos bajtos changed the title PersisteModel's updateAll method crashes the server when invoked via the remote connector PersistedModel's updateAll method crashes the server when invoked via the remote connector Sep 24, 2019
@agnes512

This comment has been minimized.

Copy link
Contributor

@agnes512 agnes512 commented Sep 24, 2019

Discussion from the estimation meeting:
possible solution: deserialize the object into JSON

@bajtos

This comment has been minimized.

Copy link
Member

@bajtos bajtos commented Oct 4, 2019

@bajtos

This comment has been minimized.

Copy link
Member

@bajtos bajtos commented Oct 7, 2019

Fixed in strong-remoting@3.15.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.