[Breaking]: ResourceTypeRegistry rewrite & description merging changes

- Remove all ResourceTypeRegistry setters and instead require all
resource descriptions to be provided on construction. These setters
just added extra, unnecessary opportunities for mutation that could
lead to bugs. And they required us to duplicate merge code, as in the
old behaviors setter—a problem which would only have gotten worse now
that we’re more rigorously merging in data from parent resource
descriptions (see below). Having all data upfront also means that the
merges that we do use less memory (have more shared nodes) than they
would have otherwise, thanks to Immutable (see below).

- Parent types’ Transforms are now inherited, so if a sub type doesn’t
define a `beforeSave` or `beforeRender` transform, its parent type’s
will be used. If a subtype _does_ define its own transform, however, it
still has access to the parent transform via the `superFn` argument, as
before.

- Also, all other data from parent descriptions (info, dbAdapters,
defaultIncludes, etc) is inherited by child types too. The only
exception is `labelMappers`, for which only the child’s value is used
(without merging). At the moment, though, merging on defaultIncludes is
a bit rough (the merge happens based on array indices), so take that
into account.

- Rename types() getter to `typeNames()` since it returns the type
names, not all the resource descriptions.

- Use Immutable.js under the hood. This saves us needing to copy values
defensively before we hand them to a consumer—if we’re comfortable
handing off raw Immutable objects. If we’re not, we can still get a
plain object more easily and robustly than before with `toJS()`—which
is good, because before we were doing such defensive copies quite
inconsistently (e.g. in the urlTemplates getter but not the info one)
and not always recursively.

package.json

@@ -19,6 +19,7 @@
     "content-type": "1.x.x",
     "dasherize": "2.0.x",
     "flat": "^1.2.1",
+    "immutable": "^3.7.5",
     "jade": "1.11.x",
     "lodash": "^3.10.0",
     "negotiator": "ethanresnick/negotiator#full-parse-access",

src/ResourceTypeRegistry.js

@@ -1,124 +1,132 @@
 import merge from "lodash/object/merge";
+import Immutable from "immutable";
+import {pseudoTopSort} from "./util/misc";
 
 /**
  * A private array of properties that will be used by the class below to
- * automatically generate simple getter setters for each property, all
- * following same format. Those getters/setters will take the resource type
- * whose property is being retrieved/set, and the value to set it to, if any.
+ * automatically generate simple getters for each property, all following the
+ * same format. Those getters will take the name of the resource type whose
+ * property is being retrieved.
  */
-const autoGetterSetterProps = ["dbAdapter", "beforeSave", "beforeRender",
+const autoGetterProps = ["dbAdapter", "beforeSave", "beforeRender", "behaviors",
   "labelMappers", "defaultIncludes", "info", "parentType"];
 
 /**
- * Global defaults for resource descriptions, to be merged into defaults
- * provided to the ResourceTypeRegistry, which are in turn merged into defaults
- * provided in each resource type descriptions.
+ * Global defaults for all resource descriptions, to be merged into the
+ * defaults provided to the ResourceTypeRegistry, which are in turn merged
+ * into the values provided in each resource type description.
  */
-const globalResourceDefaults = {
+const globalResourceDefaults = Immutable.fromJS({
   behaviors: {
     dasherizeOutput: { enabled: true }
   }
-};
+});
+
+const typesKey = Symbol();
 
 /**
  * To fulfill a JSON API request, you often need to know about all the resources
- * in the system--not just the primary resource associated with the type being
- * requested. For example, if the request is for a User, you might need to
- * include related Projects, so the code handling the users request needs access
- * to the Project resource's beforeSave and beforeRender methods. Similarly, it
- * would need access to url templates that point at relationships on the Project
- * resources. Etc. So we handle this by introducing a ResourceTypeRegistry that
- * the Controller can have access to. Each resource type is registered by its
- * JSON API type and has a number of properties defining it.
+ * types in the system--not just the type that is the primary target of the
+ * request. For example, if the request is for a User (or Users), you might need
+ * to include related Projects, so the code handling the users request needs
+ * access to the Project resource's beforeSave and beforeRender methods; its
+ * url templates; etc. So we handle this by introducing a ResourceTypeRegistry
+ * that the Controller can have access to. Each resource type is registered by
+ * its JSON API type and has a number of properties defining it.
  */
 export default class ResourceTypeRegistry {
-  constructor(typeDescriptions = [], descriptionDefaults = {}) {
-    this._resourceTypes = {};
-    this._descriptionDefaults = merge({}, globalResourceDefaults, descriptionDefaults);
-    typeDescriptions.forEach((it) => { this.type(it); });
-  }
-
-  type(type, description) {
-    // create a one-argument version that takes the
-    // type as a key on the description object.
-    if(typeof type === "object" && typeof description === "undefined") {
-      description = type;
-      type = type.type;
-      delete description.type;
+  constructor(typeDescriptions = {}, descriptionDefaults = {}) {
+    this[typesKey] = {};
+    descriptionDefaults = globalResourceDefaults.mergeDeep(descriptionDefaults);
+
+    // Sort the types so we can register them in an order that respects their
+    // parentType. First, we pre-process the typeDescriptions to create edges
+    // pointing to each node's children (rather than the links we have by
+    // default, which point to the parent). Then we do an abridged topological
+    // sort that works in this case. Below, nodes is a list of type names.
+    // Roots are nodes with no parents. Edges is a map, with each key being the
+    // name of a starting node A, and the value being a set of node names for
+    // which there is an edge from A to that node.
+    const nodes = [], roots = [], edges = {};
+
+    for(const typeName in typeDescriptions) {
+      const nodeParentType = typeDescriptions[typeName].parentType;
+      nodes.push(typeName);
+
+      if(nodeParentType) {
+        edges[nodeParentType] = edges[nodeParentType] || {};
+        edges[nodeParentType][typeName] = true;
+      }
+      else {
+        roots.push(typeName);
+      }
     }
 
-    if(description) {
-      this._resourceTypes[type] = {};
-
-      // Merge description defaults into provided description
-      description = merge({}, this._descriptionDefaults, description);
+    const typeRegistrationOrder = pseudoTopSort(nodes, edges, roots);
+
+    // register the types, in order
+    typeRegistrationOrder.forEach((typeName) => {
+      const parentType = typeDescriptions[typeName].parentType;
+
+      // defaultIncludes need to be made into an object if they came as an array.
+      // TODO: Remove support for array format before v3. It's inconsistent.
+      const thisDescriptionRaw = Immutable.fromJS(typeDescriptions[typeName]);
+      const thisDescriptionMerged = descriptionDefaults.mergeDeep(thisDescriptionRaw);
+
+      this[typesKey][typeName] = (parentType) ?
+        // If we have a parentType, we merge in all the parent's fields,
+        // BUT we then overwrite labelMappers with just the ones directly
+        // from this description. We don't inherit labelMappers because a
+        // labelMapper is a kind of filter, and the results of a filter
+        // on the parent type may not be instances of the subtype.
+        this[typesKey][parentType].mergeDeep(thisDescriptionRaw)
+          .set("labelMappers", thisDescriptionRaw.get("labelMappers")) :
+
+        // If we don't have a parentType, just register
+        // the description merged with the universal defaults
+        thisDescriptionMerged;
+    });
+  }
 
-      // Set all the properties for the type that the description provides.
-      autoGetterSetterProps.concat(["urlTemplates", "behaviors"]).forEach((k) => {
-        if(Object.prototype.hasOwnProperty.call(description, k)) {
-          this[k](type, description[k]);
-        }
-      });
-    }
-    else if(this._resourceTypes[type]) {
-      return Object.assign({}, this._resourceTypes[type]);
-    }
+  type(typeName) {
+    return this.hasType(typeName) ? this[typesKey][typeName].toJS() : undefined;
   }
 
-  types() {
-    return Object.keys(this._resourceTypes);
+  hasType(typeName) {
+    return typeName in this[typesKey];
   }
 
-  //calling the arg "templatesToSet" to avoid conflict with templates var below
-  urlTemplates(type, templatesToSet) {
-    this._resourceTypes[type] = this._resourceTypes[type] || {};
-
-    switch(arguments.length) {
-      case 1:
-        return this._resourceTypes[type].urlTemplates
-          ? Object.assign({}, this._resourceTypes[type].urlTemplates)
-          : this._resourceTypes[type].urlTemplates;
-
-      case 0:
-        let templates = {};
-        for(let currType in this._resourceTypes) {
-          templates[currType] = this.urlTemplates(currType);
-        }
-        return templates;
-
-      default:
-        this._resourceTypes[type].urlTemplates = templatesToSet;
-    }
+  typeNames() {
+    return Object.keys(this[typesKey]);
   }
 
-  behaviors(type, behaviorsToSet) {
-    this._resourceTypes[type] = this._resourceTypes[type] || {};
-    if (behaviorsToSet) {
-      this._resourceTypes[type].behaviors =
-        merge({}, this._descriptionDefaults.behaviors, behaviorsToSet);
+  urlTemplates(type) {
+    if(type) {
+      const maybeDesc = this[typesKey][type];
+      const maybeTemplates = maybeDesc ? maybeDesc.get('urlTemplates') : maybeDesc;
+      return maybeTemplates ? maybeTemplates.toJS() : maybeTemplates;
     }
 
-    else {
-      return this._resourceTypes[type].behaviors;
-    }
+    return Object.keys(this[typesKey]).reduce((prev, typeName) => {
+      prev[typeName] = this.urlTemplates(typeName);
+      return prev;
+    }, {});
   }
 }
 
-autoGetterSetterProps.forEach((propName) => {
-  ResourceTypeRegistry.prototype[propName] = makeGetterSetter(propName);
+autoGetterProps.forEach((propName) => {
+  ResourceTypeRegistry.prototype[propName] = makeGetter(propName);
 });
 
+function makeGetter(attrName) {
+  return function(type) {
+    const maybeDesc = this[typesKey][type];
+    const maybeVal = maybeDesc ? maybeDesc.get(attrName) : maybeDesc;
 
-function makeGetterSetter(attrName) {
-  return function(type, optValue) {
-    this._resourceTypes[type] = this._resourceTypes[type] || {};
-
-    if(optValue) {
-      this._resourceTypes[type][attrName] = optValue;
+    if(maybeVal instanceof Immutable.Map || maybeVal instanceof Immutable.List) {
+      return maybeVal.toJS();
     }
 
-    else {
-      return this._resourceTypes[type][attrName];
-    }
+    return maybeVal;
   };
 }

src/controllers/API.js

@@ -72,7 +72,7 @@ class APIController {
         response.headers.vary = "Accept";
 
         // If the type requested in the endpoint hasn't been registered, we 404.
-        if(!registry.type(request.type)) {
+        if(!registry.hasType(request.type)) {
           throw new APIError(404, undefined, `${request.type} is not a valid type.`);
         }
 

src/controllers/Documentation.js

@@ -27,8 +27,8 @@ export default class DocumentationController {
 
     // Store in the resourcesMap the info object about each type,
     // as returned by @getTypeInfo.
-    this.registry.types().forEach((type) => {
-      data.resourcesMap[type] = this.getTypeInfo(type);
+    this.registry.typeNames().forEach((typeName) => {
+      data.resourcesMap[typeName] = this.getTypeInfo(typeName);
     });
 
     this.templateData = data;

src/util/misc.js

@@ -38,3 +38,53 @@ export function isSubsetOf(setArr, potentialSubsetArr) {
 export function isPlainObject(obj) {
   return typeof obj === "object" && !(Array.isArray(obj) || obj === null);
 }
+
+/**
+ * Perform a pseudo-topological sort on the provided graph. Pseudo because it
+ * assumes that each node only has 0 or 1 incoming edges, as is the case with
+ * graphs for parent-child inheritance hierarchies (w/o multiple inheritance).
+ * Uses https://en.wikipedia.org/wiki/Topological_sorting#Kahn.27s_algorithm
+ *
+ * @param  {string[]} nodes A list of nodes, where each node is just a string.
+ *
+ * @param {string[]} roots The subset of nodes that have no incoming edges.
+ *
+ * @param  {object} edges The edges, expressed such that each key is a starting
+ * node A, and the value is a set of nodes (as an object literal like
+ * {nodeName: true}) for each of which there is an edge from A to that node.
+ *
+ * @return {string[]} The nodes, sorted.
+ */
+export function pseudoTopSort(nodes, edges, roots) {
+  // Do some defensive copying, in case the caller didn't.
+  roots = roots.slice();
+  nodes = nodes.slice();
+  edges = Object.assign({}, edges);
+  for(const key in edges) { edges[key] = Object.assign({}, edges[key]); }
+
+  // "L = Empty list that will contain the sorted elements"
+  const sortResult = [];
+
+  // "while S is non-empty do"
+  while(roots.length) {
+    // "remove a node n from S"
+    const thisRoot = roots.pop();
+    const thisRootChildren = edges[thisRoot] || {};
+
+    // "add n to tail of L"
+    sortResult.push(thisRoot);
+
+    // "for each node m with an edge e from n to m do"
+    for(const child in thisRootChildren) {
+      // "remove edge e from the graph"
+      delete thisRootChildren[child];
+
+      // SKIP: "if m has no other incoming edges..."
+      // we don't need this check because we assumed max 1 incoming edge.
+      // But: "then insert m into S".
+      roots.push(child);
+    }
+  }
+
+  return sortResult;
+}

test/app/database/models/organization.js

@@ -13,7 +13,8 @@ function OrganizationSchema() {
     description: {
       type: String
     },
-    liaisons: [{ref: "Person", type: ObjectId}]
+    liaisons: [{ref: "Person", type: ObjectId}],
+    modified: Date
   });
 }
 

test/app/src/index.js

@@ -6,19 +6,19 @@ import database from "../database/index";
  * Export a promise for the app.
  */
 export default database.then(function(dbModule) {
-  const adapter = new API.dbAdapters.Mongoose(dbModule.models())
-    , registry = new API.ResourceTypeRegistry()
-    , Controller = new API.controllers.API(registry);
-
-  ["people", "organizations", "schools"].forEach(function(resourceType) {
-    const description = require("./resource-descriptions/" + resourceType);
-    description.dbAdapter = adapter;
-    registry.type(resourceType, description);
+  const adapter = new API.dbAdapters.Mongoose(dbModule.models());
+  const registry = new API.ResourceTypeRegistry({
+    "people": require("./resource-descriptions/people"),
+    "organizations": require("./resource-descriptions/organizations"),
+    "schools": require("./resource-descriptions/schools")
+  }, {
+    dbAdapter: adapter
   });
 
   // Initialize the automatic documentation.
   // Note: don't do this til after you've registered all your resources.)
   const Docs = new API.controllers.Documentation(registry, {name: "Example API"});
+  const Controller = new API.controllers.API(registry);
 
   // Initialize the express app + front controller.
   const app = express();

test/app/src/resource-descriptions/organizations.js

@@ -3,6 +3,12 @@ module.exports = {
     "self": "http://127.0.0.1:3000/organizations/{id}",
     "relationship": "http://127.0.0.1:3000/organizations/{ownerId}/links/{path}"
   },
+
+  beforeRender: function(resource) {
+    resource.attrs.addedBeforeRender = true;
+    return resource;
+  },
+
   beforeSave: function(resource) {
     resource.attrs.description = "Added a description in beforeSave";
     return resource;

test/app/src/resource-descriptions/schools.js

@@ -23,10 +23,10 @@ module.exports = {
     }
   },
 
-  beforeSave: function(resource) {
+  beforeSave: function(resource, req, res, superFn) {
     return new Promise((resolve, reject) => {
-      resource.attrs.description = "Modified in a Promise";
+      resource.attrs.modified = new Date("2015-10-27T05:16:57.257Z");
       resolve(resource);
-    });
+    }).then((transformed) => superFn(transformed, req, res));
   }
 };

test/integration/create-resource/index.js

@@ -59,15 +59,17 @@ describe("Creating Resources", () => {
       describe("beforeSave", () => {
         it("should execute beforeSave hook", () => {
           expect(createdResource.attributes.description).to.equal("Added a description in beforeSave");
+          expect(createdResource.attributes.modified).to.equal("2015-01-01T00:00:00.000Z");
         });
 
-        it("should allow beforeSave to return a Promise", (done) => {
+        it("should allow beforeSave to return a Promise and support super()", (done) => {
           Agent.request("POST", "/schools")
             .type("application/vnd.api+json")
             .send({"data": VALID_SCHOOL_RESOURCE_NO_ID})
             .promise()
             .then((response) => {
-              expect(response.body.data.attributes.description).to.equal("Modified in a Promise");
+              expect(response.body.data.attributes.description).to.equal("Added a description in beforeSave");
+              expect(response.body.data.attributes.modified).to.equal("2015-10-27T05:16:57.257Z");
               done();
             }, done).catch(done);
         });