Design the router middleware

[whitlockjc]

The swagger-router middleware in swagger-tools was really useful for spinning up a server and prototyping an API. Not only that but this middleware also handled the mocking functionality of swagger-tools' middleware and producing errors in certain situations (missing operations, missing route handlers, ...). But since most Node.js web frameworks have their own router, and some even require owning the routing like restify, the usefulness of swagger-router has been questioned. Not only that but in swagger-tools/issues/221, I proposed we remove the x-swagger-router-controller vendor extension support because it basically was added to support a scenario Swagger doesn't support: non-unique operationId values. I can see both a world with a Swagger-based routing middleware and one without it. I'll let you make the decision but let's look at some information about swagger-router to help the design discussion assuming we keep it.

First, I think the mocking support should be its own middleware. I also think the "missing operations" error handling should be handled in the matcher middleware discussed in #2. I do think this middleware should have a configuration to fail silently (or pass through) or error in the case of a request to a Swagger operation that does not have a route handler associated with it. Now onto the routing discussion.

swagger-router works pretty well because it isn't designed to support everything out of the box but it gives you a way to manually own the routing. That being said, it's not perfect. The main open issue with swagger-router is allowing it to support a nested directory structure of controllers instead of just a single directory. This was an oversight and if we keep the routing middleware, we should support this. We should also consider exactly what we want this middleware to do. Another one that recently came up is swagger-tools/issues/337 that requested swagger-router provide some IoC capabilities or some way to automatically create the routing map but with some way to initialize/prepare route handlers. While I'm not too keen on that last thing, I'm also not oblivious that that automatic route mapping is nice and it sucks to lose it just because your controllers require initialization.

This post is long enough so I'll stop here for now but I would love to hear what you love and hate about swagger-router.

/cc @confuser, @steve-gray

[steve-gray]

This is one of those framework design questions that always messes folks up really: it's very easy to make the 'super-duper-happy-path' easy - but it's then a matter of how steep the gradient gets once you need to do just one little thing differently.

I've worked around this aspect with my own package for generating code that leverages the swagger-router (gulp-swagger-codegen - effectively what I do is generate controller classes with the smarts and then let those be responsible for class startup, so somewhere in the controller I'm pulling in an 'implementation' type (shitty hack: I'm just require()'ing something with the same name in a convention based path).

In those generated controllers, what I have is:

const identitylinkImplementation = require('../implementation/identitylink');

function resolveImplementation(impl) {
  // Validate arguments
  if (!impl) {
    throw new Error('Cannot resolve implementation. require() returned null');
  }

  // Call generator function, if required
  if (typeof impl === 'function') {
    // Determine if we are an ES6 class, if so, generate via new()
    if (/^\s*class\s+/.test(impl.toString())) {
      return new impl();
    }
    return impl();
  }

  // POJSO.
  return impl;
}

// Code can now 'resolveImplementation(identitylinkImplementation)' or similiar.
// Users can return either a map/function-list, a class or a prebuilt object.

I'm going to wire in the IOC support into the code generation templates rather than the underlying framework. Most of my code is really about simplification of the req/res methods and removing the connect/swagger-tools-isms from the specific code people write, the goal is that the code is pretty oblivious to the world it's running in.

Perhaps this whole affair could be simplified by having a function you can pass in that lets people provide their own implementation resolver? Returning null can fall-through to the base implementation, and it provides a single choke-point for people to provide their own IOC-instantiated controller classes.

function customImplementationResolver(actionId) {
  // The 'contract' here is that the return of this function
  // is a function(req, res)?
}

app.use(swayRouter({
  resolver: customImplementationResolver
}))

The challenge then becomes how much fire support we need to give users for that road: likely some simple helper classes, or at least a worked example in the /samples directory showing an IoC being used. Some questions then:

• Do we make these custom resolvers stackable? (i.e. I can just register them in in sequence like connect middleware, and then shortcircuit when we find one)
  • If so, it would seem natural to make the baseline behaviour the bottom of a stack.

I do actually quite like the extended property being there because it provides a grouping key for logical sets of operations. For any 'wide' API, something sensible somewhere would be needed - and swagger itself doesn't have a direct answer.

[whitlockjc]

I agree. What if we allowed you to have options that would work something like this (The example below is a simplified version of the default controller resolver and processor):

app.use(router({
  controllerResolver: function (path, file) {
    var jsRegex = /\.(coffee|js)$/;
    var controllerName;

    if (file.match(jsFileRegex)) {
      controllerName = file.replace(jsFileRegex, '');

      return {
        name: controllerName,
        controller: require(path.resolve(path.join(dir, controllerName)))
      };
    } else {
      return null;
    }
  },
  controllerProcessor: function (controller, path, routeMap) {
    Object.keys(controller).forEach(function (key) {
      var mExport = controller[key];

      if (typeof mExport === 'function') {
        routeMap[path.join('/') + '_' + key] = mExport; 
      }
    });
  }
});

So the router middleware will walk the directory, or directories, recursively and for each file, you can dictate if the file is a controller or not and if it is, you can dictate what route handlers are found from within it. This would allow you to instantiate the controller and/or route handler and it would allow me to do some "super-duper-happy-path" out of the box. Thoughts?

[whitlockjc]

I threw this together pretty quickly but I think if you can dictate what a controller is, and its name, and also dictate what route handlers are, that should handle the non-simple cases but we can still handle the super simple cases out of the box via configuration. I think those two things, coupled with nested controller identification, could be pretty slick.

[whitlockjc]

What about doing away with x-swagger-router-controller and just use operationId for this? So instead of x-swagger-router-controller: Users and operationId: getAllUsers, we could have operationId: Users#getAllUsers? Since operationId must be unique, maybe we don't need x-swagger-router-controller anymore?

[steve-gray]

Yeah, I was basically coming to the same conclusion: we can establish a convention such as # or _ in the name to break it up into a controller/method pair-set. Would be interesting to see how many parsers for Swagger files break with such characters in the operationId names but I suspect any cases like that could easily get a fix/PR done in an hour.

[whitlockjc]

Good to hear. It seems we agree on how the route map gets generated, or provided, but do you have any opinions after that? How a Swagger operation is mapped to a route handler...any opinions? Do we use a vendor extensions for all or part of it? Use the operationId for all or part of it? Do we continue working with the operation method/verb?

[steve-gray]

The vendor extension can seemingly give way to the convention on the operationId for the super-happy-path - but other people who want to use extensions could still go down that route in their mapper - so perhaps the consequence is that we need to ensure the swagger-def is present somehow in that function calls context.

[confuser]

Apologies if I've misunderstood the discussions so far, but based on this, one would have dynamic/meta/if logic to pass in particular dependencies to a controller, presumably via closures/binding, via controllerResolver?

For example, how would one achieve this "controller"?

// controller
module.exports = function (UserModel, SomethingElseHere) {
return { getAllUsers: function (req, res, next) {
        UserModel.find()
    }
}

Would one execute the controller as a function within the controllerProcessor, with the dependencies, and then assign the returned object to the routeMap?

If so, I'm happy with that. I have a few ideas to reduce the amount of bloat for projects with many controllers.

These examples however assume everything is synchronous. I can foresee some use cases where async operations will be desirable.

With regards to operationId, what's the reason for it being unique? I've had a use case in which the same operationId was technically the same code wise, but just had different schemas, in terms of which fields were required. So two different routes, but same middleware.

[whitlockjc]

Yes @confuser, that's the plan. To use options.controllerProcessor to be able to instantiate/prepare/... your controllers and their resulting route handlers.

[Jiropole]

Just a note to thank you for your hard work, and to support this deprecate/refactor effort! Swagger.io and swaggertools have already saved me weeks of work. I hope to see expanded documentation on the new routing and handler system. I wish I could contribute, but as a newb, I am available at your convenience to provide list of newb-oriented FAQs. (ha --I'll get in line)