strongloop / loopback-swagger

LoopBack Swagger Spec Integration
Other
44 stars 63 forks source link

Generated spec does not include methods defined after data source connection. #44

Closed ritch closed 7 years ago

ritch commented 8 years ago

Here is an example:

https://github.ibm.com/asim-siddiqui/brazil-POC-Soap/blob/master/veloxCRM/server/boot/createSoapMethods.js#L60

We should look into either a work around for this style of defining remote methods or a change to this module and/or loopback-boot to allow us to generate the swagger for methods defined after the inital boot().

@bajtos @raymondfeng

rblalock commented 8 years ago

FWIW: I ran in to this same thing. Here's another sample: https://github.com/rblalock/lb-soap-example.

bajtos commented 8 years ago

IIUC, you are asking us to include remote methods defined from dataSource.on('connected').

app.datasources.soapService.once('connected', function() {
  MyModel.remoteMethod('foobar', { ... });
});

Recently, we have intentionally modified swagger spec generator in APIC to not connect datasources. This allows APIC to generate swagger spec even when a datasource is misconfigured or the backend service (database, SOAP server, etc.) is not available (temporary downtime, no access to production machines from dev machine, etc.).

I don't see how can we implement support for showing remote methods added once datasource is connected when we are intentionally not connecting datasources.

To be honest, I don't understand what is the purpose of defining remote methods from once('connected'), as opposed to defining them immediately.

Let's take https://github.com/rblalock/lb-soap-example/blob/8f5c4186df6600a870eb0a7d608ff86c779a6882/common/models/weather.js for an example. I would personally rewrite it as follows:

// common/models/weather.js
module.exports = function(Weather) {
  var weatherDS = server.datasources.weather;
  Weather.forecast = function (zip, cb) {
    if (!weather.GetCityForecastByZIP) {
      return cb(new Error('Weather datasource not connected.'));
    }

    Weather.GetCityForecastByZIP(
      { ZIP: zip || '94555'},
      function (err, response) {
        // ...
      });
  });

  Weather.remoteMethod('forecast', { 
    // metadata
  });
});

// server/model-config.json
{
  "Weather": {
    "public": true,
    "dataSource": "weather"
  }
}

Am I missing something?

rblalock commented 8 years ago

@bajtos I'd love not have to use the "connected" event to define the models later. I'm not sure why the soap connector requires us to do that but it does. I'm still getting my head wrapped around loop backs architecture, I can think of several ways in the lifecycle of a connector to ensure this doesn't happen but it's just a matter of tweaking the connector lifecycle on boot.

bajtos commented 8 years ago

I'd love not have to use the "connected" event to define the models later. I'm not sure why the soap connector requires us to do that but it does

This is the clue we needed, thank you! @raymondfeng do you happen to know more about this problem? I can see there is an closed issue https://github.com/strongloop/loopback-connector-soap/issues/17, should we re-open it?

raymondfeng commented 8 years ago

At the moment, most of datasources are instantiated synchronously. In the case of SOAP, we need to load the WSDL asynchronously so that we can discover operations and map them to DataAccessObject. We need to change how datasources are loaded so that models will only attach to a datasource if it's initialized (asynchronously).

The proposed enhancement for loopback-boot at https://github.com/strongloop/loopback-boot/pull/181 allows fully async boot.

rblalock commented 8 years ago

I like the async boot for all. This makes a lot of sense especially if a connector dev wanted to generate models automatically off a schema (think sales force or some extended MySQL connector).

bajtos commented 7 years ago

In the case of SOAP, we need to load the WSDL asynchronously so that we can discover operations and map them to DataAccessObject. We need to change how datasources are loaded so that models will only attach to a datasource if it's initialized (asynchronously).

I think this is a good use case to drive the implementation. I am labelling this issue as "Feature".

stale[bot] commented 7 years ago

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[bot] commented 7 years ago

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.