why isPlainObject?

[Santinell]

Why you use isPlainObject in handlerCacheFromDir?

I want use complex Objects for controllers. In ideal - ES6 classes.
for ES5 Object methods can be used _.each
for ES6 classes is needed workaround.

[whitlockjc]

I use isPlainObject because the API expects a map of key/value pairs. I can relax things a bit to let you pass in a class instance but in 1.5 years, this is the first request so I'm not sure how useful it would be other than to you.

[Santinell]

Hmm... i think i can generate map and use it in options.controllers

[whitlockjc]

I'm closing this for now but if you get something in mind for class-based stuff, let me know. Also, PRs are appreciated. :)

[pixelshaded]

+1 to this. We use typescript to generate our controllers and were exporting instances of a class which is not compatible with isPlainObject. Means we can't create controllers that inherit from other controllers. isPlainObject is very limiting.

[whitlockjc]

I don't mind changing isPlainObject as it really doesn't need to be that specific, if possible.

[pixelshaded]

yes but isObject allows arrays, functions, regexes, new Number(0), and new String('') which I don't think you accept.

[pixelshaded]

the work around for us may be to use _.toPlainObject on the instance before exporting, but I dont know how that might affect the 'this' inside method.

[whitlockjc]

Well, that's why I did this in the first place. It use to be isObject. I'll figure something out.

[pixelshaded]

I tried _.toPlainObject and it destroyed the references to 'this'. This project was actually passing in a map object with instances before. I was refactoring to the use the folder convention instead and thats when stuff stopped working.

[pixelshaded]

could always brute force it:

if (_.isObject(controller) && !_.isArray(controller) && !_.isFunction(controller) && !_.isRegExp(controller) && !_.isString(controller) && !_.isNumber(controller))

[whitlockjc]

That's ugly but if that's what I have to do because there is no other option, I don't mind.

[pixelshaded]

I've been looking around for some solutions. This may work and be the cleanest:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/toString#Using_toString()_to_detect_object_class

Object.prototype.toString.call(controller) === '[object Object]'

Interally this comes from an inaccessible property [[Class]] and possible native values are:
http://stackoverflow.com/questions/3250379/what-is-the-call-function-doing-in-this-javascript-statement/3250415#3250415

"Object"
"Array"
"Function"
"Date"
"RegExp"
"String"
"Number"
"Boolean"
"Error" for error objects such as instances of ReferenceError, TypeError, SyntaxError, Error, etc
"Math" for the global Math object
"JSON" for the global JSON object defined on the ECMAScript 5th Ed. spec.
"Arguments" for the arguments object (also introduced on the ES5 spec.)
"null" (introduced just a couple of days ago in the ES5 errata)
"undefined"

[pixelshaded]

Actually I was slightly wrong on it working before. Before we had our own middleware imlementation: we would get the operation and controller info from the meta data middleware and use that to check for the existence of said method on controller and then execute it. It seems that swagger-router looks at the object and scans every property to put it in a handler cache. This means that every property on the controller must be a function. Since our controllers are classes, they may have some members that aren't functions. This is causing throws on start up. I may just need to revert back to our custom router. We were only using swagger-router for stubs.

[whitlockjc]

Are you getting errors or warnings? The reason I ask is if I load your modules, I will skip non-function exports gracefully and give you debug output: https://github.com/apigee-127/swagger-tools/blob/master/middleware/swagger-router.js#L96 If you're providing me with the controller map, the handler itself must be a function and I will error in that case: https://github.com/apigee-127/swagger-tools/blob/master/middleware/swagger-router.js#L369

[pixelshaded]

Providing the controller map doesn't work for me because the _.each is going over every property on the controller, throwing if they aren't functions rather than ignoring them. I would argue that non function properties should be ignored since they obviously couldn't be handlers.

Since we are using complex classes, the controller may have a property on it that holds a logger for instance (that is used in other methods). The existence of this logger will cause swagger-router to throw.

Here is our implementation of a swagger router which depends on the metadata middleware:

import * as requireDir from 'require-dir';
import * as express from 'express';
import * as swaggerTools from 'swagger-tools';
import * as _ from 'lodash';
import loggerProvider from '../init/LoggerProvider';

export default (controllersPath: string): express.RequestHandler => {

  const controllerMap: { [controllerName: string]: any } = requireDir(controllersPath);

  const requestHandler: express.RequestHandler = (req: swaggerTools.IRequest, res: express.Response, next: express.NextFunction) => {
    if (!req.swagger ||
      !req.swagger.path ||
      !req.swagger.path['x-swagger-router-controller'] ||
      !req.swagger.operation ||
      !req.swagger.operation.operationId) {
      next();
      return;
    }

    const controllerName: string = req.swagger.path['x-swagger-router-controller'];
    const methodName: string = req.swagger.operation.operationId;

    if (!controllerMap[controllerName]) {
      loggerProvider.logger.warn(`${controllerName} did not exist in ${controllersPath}.`);
      next();
      return;
    }

    if (!controllerMap[controllerName][methodName]) {
      loggerProvider.logger.warn(`${methodName} operation did not exist on swagger controller ${controllerName}.`);
      next();
      return;
    } else if (!_.isFunction(controllerMap[controllerName][methodName])) {
      loggerProvider.logger.warn(`${methodName} operation on swagger controller ${controllerName} was not a function.`);
      next();
      return;
    }

    controllerMap[controllerName][methodName](req, res, next);
  };

  return requestHandler;
};

[whitlockjc]

I think I better understand the issue now and I've come to the conclusion that it's working as intended. options.controllers is documented such that when it is an object, the key is the routing key ([{ControllerName}_]{HandlerFunctionName}) and the value is the function that handles the requests, not a class or a controller or a module. The problem is that require-dir will not give you a mapping that adheres to the contract. It shouldn't take much effort to take the output from require-dir to create a valid controller map.

The whole purpose behind allowing options.controllers to be an object provided by the user was to allow the user to manually generate the controller map for cases where we couldn't do it for them. This also allows the user to use ES6/TypeScript/... without swagger-tools having to explicitly support these technologies. It also meant that swagger-tools didn't have to implement IoC or anything else but it does mean that the server author has to generate a valid options.controllers.

Here is an example: https://github.com/apigee-127/swagger-tools/blob/master/test/2.0/test-middleware-swagger-router.js#L121

[pixelshaded]

Looks like I had made the assumption that it was controllerName as the key and the value was the instance with all the methods (operation ids) as properties. I figured this existed in case the instance couldn't construct itself.

Does passing in a folder iterate over every property on the module and validate it is a function as well? Because I would prefer removing our custom implementation (i.e. fix isPlainObject).

[whitlockjc]

If you pass in a folder, it will attempt to require each module in that folder and iterate over each export. But...if the exported value is not a function, it is logged and skipped.

[pixelshaded]

Cool. What were your thoughts on using Object.prototype.toString instead of lodash's isPlainObject?

[whitlockjc]

I don't mind doing that.

[pixelshaded]

make it so :) if you need/want a PR let me know

[whitlockjc]

PRs are always welcome. I am currently working on a json-refs release and plan on updating swagger-tools and sway to include the new version. So I'll likely wait until that's done.

[pixelshaded]

#402