Refactor route-helper & add tests.

- Uses model-helper to parse types for swagger.
- Separated returns & accepts hacks.
- Documentation fixes
- TODO add param regex

lib/route-helper.js

@@ -7,43 +7,28 @@
 var debug = require('debug')('loopback-explorer:routeHelpers');
 var _cloneDeep = require('lodash.clonedeep');
 var translateKeys = require('./translate-keys');
+var modelHelper = require('./model-helper');
 
 /**
  * Export the routeHelper singleton.
  */
 var routeHelper = module.exports = {
   /**
+   * Given a route, generate an API description and add it to the doc.
+   * If a route shares a path with another route (same path, different verb),
+   * add it as a new operation under that API description.
+   * 
    * Routes can be translated to API declaration 'operations',
    * but they need a little massaging first. The `accepts` and
    * `returns` declarations need some basic conversions to be compatible. 
    *
    * This method will convert the route and add it to the doc.
-   * @param  {Class} class All remoting classes used by the API.
    * @param  {Route} route    Strong Remoting Route object.
    * @param  {Class} classDef Strong Remoting class.
    * @param  {Object} doc     The class's backing API declaration doc.
    */
   addRouteToAPIDeclaration: function (route, classDef, doc) {
-    
+    var api = routeHelper.routeToAPIDoc(route, classDef);
-    // Some fixes to returns/accepts
-    var processedRoute = doRouteParameterHacks(route, classDef);
-
-    // Add the api to the spec. If the path already exists, add as another operation
-    // under the api; otherwise add a new api.
-    addRouteToDoc(processedRoute, doc);
-  }
-};
-
-/**
- * Given a route, generate an API description and add it to the doc.
- * If a route shares a path with another route (same path, different verb),
- * add it as a new operation under that API description.
- * 
- * @param {Route} route Route.
- * @param {Object} doc  Current document.
- */
-function addRouteToDoc(route, doc) {
-  var api = routeToAPI(route);
     var matchingAPIs = doc.apis.filter(function(existingAPI) {
       return existingAPI.path === api.path;
     });
@@ -52,25 +37,22 @@ function addRouteToDoc(route, doc) {
     } else {
       doc.apis.push(api);
     }
-}
+  }, 
 
   /**
- * Process a route.
+   * Massage route.accepts.
- * Contains some hacks to fix some incompatibilities in the accepts and returns
+   * @param  {Object} route    Strong Remoting Route object.
- * descriptions.
+   * @param  {Class}  classDef Strong Remoting class.
- * @param  {Route} route     A Route.
+   * @return {Object}          Modified Route object.
- * @param  {Class} classDef  The backing strong remoting class.
    */
-function doRouteParameterHacks(route, classDef) {
+  hackAcceptsDefinition: function hackAcceptsDefinition(route, classDef) {
-  // Don't modify the existing route as some pieces (such as `returns`) may be shared between routes.
-  route = _cloneDeep(route);
-
     var split = route.method.split('.');
-  if (classDef && classDef.sharedCtor && classDef.sharedCtor.accepts && split.length > 2 /* HACK */) {
+    if (classDef && classDef.sharedCtor && 
+        classDef.sharedCtor.accepts && split.length > 2 /* HACK */) {
       route.accepts = (route.accepts || []).concat(classDef.sharedCtor.accepts);
     }
 
-  // Filter out parameters that are generated from the incoming request or body,
+    // Filter out parameters that are generated from the incoming request,
     // or generated by functions that use those resources.
     route.accepts = (route.accepts || []).filter(function(arg){
       if (!arg.http) return true;
@@ -82,6 +64,19 @@ function doRouteParameterHacks(route, classDef) {
       return true;
     });
 
+    // Translate LDL keys to Swagger keys.
+    route.accepts = (route.accepts || []).map(translateKeys);
+
+    return route;
+  },
+
+  /**
+   * Massage route.returns.
+   * @param  {Object} route    Strong Remoting Route object.
+   * @param  {Class}  classDef Strong Remoting class.
+   * @return {Object}          Modified Route object.
+   */
+  hackReturnsDefinition: function hackReturnsDefinition(route, classDef) {
     // HACK: makes autogenerated REST routes return the correct model name.
     var returns = route.returns && route.returns[0];
     if (returns && returns.arg === 'data') {
@@ -96,48 +91,70 @@ function doRouteParameterHacks(route, classDef) {
     }
 
     // Translate LDL keys to Swagger keys.
-  route.accepts = (route.accepts || []).map(translateKeys);
     route.returns = (route.returns || []).map(translateKeys);
 
-  debug('route %j', route);
+    // Convert `returns` into a single object for later conversion into an 
+    // operation object.
+    if (route.returns && route.returns.length > 1) {
+      // TODO ad-hoc model definition in the case of multiple return values.
+      route.returns = {model: 'object'}; 
+    } else {
+      route.returns = route.returns[0] || {};
+    }
 
     return route;
-}
+  },
 
   /**
    * Converts from an sl-remoting-formatted "Route" description to a
    * Swagger-formatted "API" description.
    * See https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#523-operation-object
    */
+  routeToAPIDoc: function routeToAPIDoc(route, classDef) {
+    var returnDesc;
 
-function routeToAPI(route) {
+    // Don't modify the existing route as some pieces (such as `returns`) 
-  var returnDesc = route.returns && route.returns[0];
+    // may be shared between routes.
+    route = _cloneDeep(route);
 
-  return {
+    // Some parameters need to be altered; eventually most of this should 
-    path: convertPathFragments(route.path),
+    // be removed.
+    route = routeHelper.hackAcceptsDefinition(route, classDef);
+    route = routeHelper.hackReturnsDefinition(route, classDef);
+
+    debug('route %j', route);
+
+    var apiDoc = {
+      path: routeHelper.convertPathFragments(route.path),
       operations: [{
-      method: convertVerb(route.verb),
+        method: routeHelper.convertVerb(route.verb),
-      nickname: route.method.replace(/\./g, '_'), // [rfeng] Swagger UI doesn't escape '.' for jQuery selector
+        // [rfeng] Swagger UI doesn't escape '.' for jQuery selector
-      type: returnDesc ? returnDesc.model || prepareDataType(returnDesc.type) : 'void',
+        nickname: route.method.replace(/\./g, '_'), 
-      items: returnDesc ? returnDesc.items : '',
+        // Per the spec:
-      parameters: route.accepts ? route.accepts.map(acceptToParameter(route)) : [],
+        // https://github.com/wordnik/swagger-spec/blob/master/versions/1.2.md#523-operation-object
-      responseMessages: [], // TODO(schoon) - We don't have descriptions for this yet.
+        // This is the only object that may have a type of 'void'.
+        type: route.returns.model || route.returns.type || 'void',
+        parameters: route.accepts.map(routeHelper.acceptToParameter(route)),
+        // TODO(schoon) - We don't have descriptions for this yet.
+        responseMessages: [], 
         summary: route.description, // TODO(schoon) - Excerpt?
         notes: '' // TODO(schoon) - `description` metadata?
       }]
     };
-}
+    // Convert types and return.
+    return routeHelper.extendWithType(apiDoc);
+  },
 
-function convertPathFragments(path) {
+  convertPathFragments: function convertPathFragments(path) {
     return path.split('/').map(function (fragment) {
       if (fragment.charAt(0) === ':') {
         return '{' + fragment.slice(1) + '}';
       }
       return fragment;
     }).join('/');
-}
+  },
 
-function convertVerb(verb) {
+  convertVerb: function convertVerb(verb) {
     if (verb.toLowerCase() === 'all') {
       return 'POST';
     }
@@ -147,14 +164,13 @@ function convertVerb(verb) {
     }
 
     return verb.toUpperCase();
-}
+  },
 
   /**
- * A generator to convert from an sl-remoting-formatted "Accepts" description to
+   * A generator to convert from an sl-remoting-formatted "Accepts" description 
- * a Swagger-formatted "Parameter" description.
+   * to a Swagger-formatted "Parameter" description.
    */
-
+  acceptToParameter: function acceptToParameter(route) {
-function acceptToParameter(route) {
     var type = 'form';
 
     if (route.verb.toLowerCase() === 'get') {
@@ -179,7 +195,7 @@ function acceptToParameter(route) {
         paramType: paramType || type,
         name: name,
         description: accepts.description,
-      type: accepts.model || prepareDataType(accepts.type),
+        type: accepts.type,
         required: !!accepts.required,
         defaultValue: accepts.defaultValue,
         minimum: accepts.minimum,
@@ -187,43 +203,37 @@ function acceptToParameter(route) {
         allowMultiple: false
       };
 
+      out = routeHelper.extendWithType(out);
+
       // HACK: Derive the type from model
       if(out.name === 'data' && out.type === 'object') {
         out.type = route.method.split('.')[0];
       }
 
-    if (out.type === 'array') {
-      out.items = {
-        type: prepareDataType(accepts.type[0])
-      };
-    }
-
       return out;
     };
-}
+  },
 
   /**
- * Converts from an sl-remoting data type to a Swagger dataType.
+   * Extends an Operation Object or Parameter object with 
+   * a proper Swagger type and optional `format` and `items` fields.
+   * Does not modify original object.
+   * @param  {Object} obj Object to extend.
+   * @return {Object}     Extended object.
    */
+  extendWithType: function extendWithType(obj) {
+    obj = _cloneDeep(obj);
 
-function prepareDataType(type) {
+    // Format the `type` property using our LDL converter.
-  if (!type) {
+    var typeDesc = modelHelper
-    return 'void';
+      .LDLPropToSwaggerDataType({type: obj.model || obj.type});
+    // The `typeDesc` may have additional attributes, such as
+    // `format` for non-primitive types.
+    Object.keys(typeDesc).forEach(function(key){
+      obj[key] = typeDesc[key];
+    });
+    return obj;
   }
+};
 
-  if(Array.isArray(type)) {
-    return 'array';
-  }
 
-  // TODO(schoon) - Add support for complex dataTypes, "models", etc.
-  switch (type) {
-    case 'buffer':
-      return 'byte';
-    case 'date':
-      return 'Date';
-    case 'number':
-      return 'double';
-  }
-
-  return type;
-}

test/route-helper.test.js

@@ -0,0 +1,66 @@
+'use strict';
+
+var routeHelper = require('../lib/route-helper');
+var expect = require('chai').expect;
+var _defaults = require('lodash.defaults');
+
+describe('route-helper', function() {
+  // TODO
+  it('returns "object" when a route has multiple return values', function() {
+    var doc = createAPIDoc({
+      returns: [
+        { arg: 'max', type: 'number' },
+        { arg: 'min', type: 'number' },
+        { arg: 'avg', type: 'number' }
+      ]
+    });
+    expect(doc.operations[0].type).to.equal('object');
+  });
+
+  it('converts path params when they exist in the route name', function() {
+    var doc = createAPIDoc({
+      accepts: [
+        {arg: 'id', type: 'string'}
+      ],
+      path: '/test/:id'
+    });
+    var paramDoc = doc.operations[0].parameters[0];
+    expect(paramDoc.paramType).to.equal('path');
+    expect(paramDoc.name).to.equal('id');
+    expect(paramDoc.required).to.equal(false);
+  });
+
+  // FIXME need regex in routeHelper.acceptToParameter
+  xit('won\'t convert path params when they don\'t exist in the route name', function() {
+    var doc = createAPIDoc({
+      accepts: [
+        {arg: 'id', type: 'string'}
+      ],
+      path: '/test/:identifier'
+    });
+    var paramDoc = doc.operations[0].parameters[0];
+    expect(paramDoc.paramType).to.equal('query');
+  });
+
+  it('correctly coerces param types', function() {
+    var doc = createAPIDoc({
+      accepts: [
+        {arg: 'binaryData', type: 'buffer'}
+      ]
+    });
+    var paramDoc = doc.operations[0].parameters[0];
+    expect(paramDoc.paramType).to.equal('query');
+    expect(paramDoc.type).to.equal('string');
+    expect(paramDoc.format).to.equal('byte');
+  });
+
+});
+
+// Easy wrapper around createRoute
+function createAPIDoc(def) {
+  return routeHelper.routeToAPIDoc(_defaults(def, {
+    path: '/test',
+    verb: 'GET',
+    method: 'test.get'
+  }));
+}