From 6a4f3018a14c97723f379d2ea34a24a48dde5cad Mon Sep 17 00:00:00 2001
From: Lyza Danger Gardner <lyza@lyza.com>
Date: Tue, 12 Apr 2016 15:33:30 -0700
Subject: [PATCH] Add namespacing checking on patterns and collections

---
 README.md                                     |   9 +-
 src/parse/patterns.js                         |  23 ++-
 .../badPatterns/anotherBadCollection/bar.html |   0
 .../anotherBadCollection/collection.yml       |   5 +
 .../badPatterns/badCollection/collection.yml  |   4 +
 .../badPatterns/badCollection/foo.html        |   0
 test/fixtures/badPatterns/localId.html        |   6 +
 test/parse/patterns.js                        | 136 ++++++++++++------
 8 files changed, 137 insertions(+), 46 deletions(-)
 create mode 100644 test/fixtures/badPatterns/anotherBadCollection/bar.html
 create mode 100644 test/fixtures/badPatterns/anotherBadCollection/collection.yml
 create mode 100644 test/fixtures/badPatterns/badCollection/collection.yml
 create mode 100644 test/fixtures/badPatterns/badCollection/foo.html
 create mode 100644 test/fixtures/badPatterns/localId.html

diff --git a/README.md b/README.md
index 266e9bb..dc03ed7 100644
--- a/README.md
+++ b/README.md
@@ -70,7 +70,7 @@ Each file that matches the `pages` glob (`options.src.pages.glob`) will generate
 
 Collections are "meta" resources. Each directory within the glob match for `options.src.patterns.glob` that contains at least one matching pattern file is considered a "collection." By default, collections are named based on their directory name. One output HTML page is generated per collection.
 
-##### Collection Metadata
+##### Collection Metadata (Special Properties)
 
 Creating a file named `collection.yaml`, `collection.yml` or `collection.json` in a pattern directory allows you to override data about that collection. Accepted properties are:
 
@@ -80,6 +80,13 @@ Creating a file named `collection.yaml`, `collection.yml` or `collection.json` i
 
 `hidden` and `order` values can also be defined in individual patterns' front matter. Local pattern data will override data in `collection` metadata files.
 
+Unlike other resources, properties in `collection` metadata files that are _not_ one the properties listed here will be ignored.
+
+##### Reserved Properties
+
+* `items`: Used by Drizzle to store _all_ of the patterns in this collection (even hidden ones)
+* `patterns`: Used by Drizzle to store all of the _visible_, _ordered_ patterns in this collection.
+
 #### Data
 
 Files that match `options.src.data.globs` will be parsed and made available to templates. See documentation about global scope in the templates section.
diff --git a/src/parse/patterns.js b/src/parse/patterns.js
index 060ddbe..82eafdd 100644
--- a/src/parse/patterns.js
+++ b/src/parse/patterns.js
@@ -12,6 +12,19 @@ const isPattern = obj => obj.hasOwnProperty('path');
 const collectionPath = itms => path.dirname(itms[Object.keys(itms).pop()].path);
 const collectionKey = itms => collectionPath(itms).split(path.sep).pop();
 
+function checkNamespaceCollision (key, obj, id, options = {}) {
+  if (Array.isArray(key)) {
+    return key.map(oneKey =>
+      checkNamespaceCollision(oneKey, obj, id, options)
+    );
+  }
+  if (obj && obj.hasOwnProperty(key)) {
+    DrizzleError.error(new DrizzleError(`'${id}' has local '${key}'
+property set. This is a Drizzle reserved property and will be
+overwritten. See docs.`, DrizzleError.LEVELS.WARN), options.debug);
+  }
+}
+
 /**
  * Should this pattern be hidden per collection or pattern metadata?
  * @param {Object} collection   Collection obj
@@ -65,8 +78,12 @@ const hasPatternOrdering = itms => {
  */
 function buildPattern (patternObj, options) {
   const patternFile = { path: patternObj.path };
+  const patternId = resourceId(patternFile,
+    options.src.patterns.basedir, 'patterns');
+  checkNamespaceCollision('id', patternObj.data,
+    `Pattern ${patternId}`, options);
   return Object.assign(patternObj, {
-    id: resourceId(patternFile, options.src.patterns.basedir, 'patterns'),
+    id: patternId,
     name: (patternObj.data && patternObj.data.name) ||
       titleCase(resourceKey(patternFile))
   });
@@ -148,9 +165,11 @@ function buildCollection (collectionObj, options) {
   return readFiles(collectionGlob(items), options).then(collData => {
     const collectionMeta = (collData.length) ? collData[0].contents : {};
     collectionObj.collection = Object.assign ({
-      items: items,
       name: titleCase(collectionKey(items))
     }, collectionMeta);
+    checkNamespaceCollision(['items', 'patterns'], collectionObj.collection,
+      `Collection ${collectionObj.collection.name}`, options);
+    collectionObj.collection.items = items;
     collectionObj.collection.patterns = buildOrderedPatterns(
       collectionObj.collection);
     return collectionObj;
diff --git a/test/fixtures/badPatterns/anotherBadCollection/bar.html b/test/fixtures/badPatterns/anotherBadCollection/bar.html
new file mode 100644
index 0000000..e69de29
diff --git a/test/fixtures/badPatterns/anotherBadCollection/collection.yml b/test/fixtures/badPatterns/anotherBadCollection/collection.yml
new file mode 100644
index 0000000..ac5bd8c
--- /dev/null
+++ b/test/fixtures/badPatterns/anotherBadCollection/collection.yml
@@ -0,0 +1,5 @@
+patterns:
+  - this
+  - is
+  - not
+  - OK
diff --git a/test/fixtures/badPatterns/badCollection/collection.yml b/test/fixtures/badPatterns/badCollection/collection.yml
new file mode 100644
index 0000000..d391aae
--- /dev/null
+++ b/test/fixtures/badPatterns/badCollection/collection.yml
@@ -0,0 +1,4 @@
+items:
+  - no
+  - no nope
+  - not here
diff --git a/test/fixtures/badPatterns/badCollection/foo.html b/test/fixtures/badPatterns/badCollection/foo.html
new file mode 100644
index 0000000..e69de29
diff --git a/test/fixtures/badPatterns/localId.html b/test/fixtures/badPatterns/localId.html
new file mode 100644
index 0000000..43c1966
--- /dev/null
+++ b/test/fixtures/badPatterns/localId.html
@@ -0,0 +1,6 @@
+---
+id: "This is a no-no"
+name: "Overridden Name Is Fine"
+---
+
+This pattern tries to define a local ID.
diff --git a/test/parse/patterns.js b/test/parse/patterns.js
index 3044e6c..362ac8b 100644
--- a/test/parse/patterns.js
+++ b/test/parse/patterns.js
@@ -3,36 +3,44 @@ var config = require('../config');
 var expect = chai.expect;
 var parsePatterns = require('../../dist/parse/patterns');
 var utils = require('../../dist/utils/object');
+var DrizzleError = require('../../dist/utils/error');
 
 describe ('parse/patterns', () => {
-  var opts, patternData;
-  before (() => {
-    opts = config.init(config.fixtureOpts);
-    return opts.then(parsePatterns).then(pData => {
-      patternData = pData;
+  describe('building pattern data', () => {
+    var opts, patternData;
+    before (() => {
+      opts = config.init(config.fixtureOpts);
+      return opts.then(parsePatterns).then(pData => {
+        patternData = pData;
+      });
     });
-  });
-  it ('should correctly build data object from patterns', () => {
-    expect(patternData).to.be.an('object');
-    expect(patternData).to.have.keys(
-      'collection', 'fingers', 'components', 'typography');
-    expect(patternData.collection.items).to.have.keys('pink');
-  });
-  describe('basic collection data on patterns', () => {
-    it ('should add basic collection data', () => {
-      expect(patternData.collection).to.contain.keys(
-        'name', 'items', 'patterns');
+    it ('should correctly build data object from patterns', () => {
+      expect(patternData).to.be.an('object');
+      expect(patternData).to.have.keys(
+        'collection', 'fingers', 'components', 'typography');
+      expect(patternData.collection.items).to.have.keys('pink');
+    });
+    describe('basic collection data on patterns', () => {
+      it ('should add basic collection data', () => {
+        expect(patternData.collection).to.contain.keys(
+          'name', 'items', 'patterns');
+      });
     });
   });
-  describe('`name` prop override', () => {
+  describe ('overriding data with properties', () => {
+    var opts, patternData;
+    before (() => {
+      opts = config.init(config.fixtureOpts);
+      return opts.then(parsePatterns).then(pData => {
+        patternData = pData;
+      });
+    });
     it ('should allow override of name property', () => {
       expect(patternData.components.button.collection.items.aardvark)
         .to.contain.key('name');
       expect(patternData.components.button.collection.items.aardvark.name)
         .to.equal('Something Else');
     });
-  });
-  describe ('data field parsing', () => {
     it ('should run data fields through parsers', () => {
       var ideal = patternData.fingers.collection.items.ideal;
       expect(ideal).to.contain.keys('data');
@@ -40,16 +48,12 @@ describe ('parse/patterns', () => {
       expect(ideal.data.ancillary).to.be.a('string');
       expect(ideal.data.ancillary).to.contain('<ul>');
     });
-  });
-  describe ('pattern object structure', () => {
     it ('should define the appropriate properties for each pattern', () => {
       var aPattern = patternData.components.button.collection.items.base;
       expect(aPattern).to.have.keys('id', 'name', 'data', 'path', 'contents');
       expect(aPattern).not.to.have.keys('notes', 'links');
       expect(aPattern.data).to.contain.keys('notes', 'links');
     });
-  });
-  describe('pattern IDs', () => {
     it ('should derive IDs for patterns', () => {
       expect(patternData.collection.items.pink).to.include.key('id');
       expect(patternData.collection.items.pink.id).to.equal('patterns.pink');
@@ -64,35 +68,81 @@ describe ('parse/patterns', () => {
       expect(utils.deepPattern(longId, patternData)).to.be.an('object')
         .and.to.contain.keys('name', 'data', 'path', 'id', 'contents');
     });
+    it ('should add relevant properties to individual pattern objects', () => {
+      expect(patternData.collection.items.pink).to.be.an('object');
+      expect(patternData.collection.items.pink).to.have.keys(
+        'name', 'id', 'contents', 'path', 'data');
+    });
   });
-  it ('should add relevant properties to individual pattern objects', () => {
-    expect(patternData.collection.items.pink).to.be.an('object');
-    expect(patternData.collection.items.pink).to.have.keys(
-      'name', 'id', 'contents', 'path', 'data');
-  });
-  describe ('parse/collections', () => {
+  describe('parsing collections', () => {
+    var opts, patternData;
+    before (() => {
+      opts = config.init(config.fixtureOpts);
+      return opts.then(parsePatterns).then(pData => {
+        patternData = pData;
+      });
+    });
     it ('should create basic stub objects for collections', () => {
       var collection = patternData.components.button.collection;
       expect(collection.name).not.to.be;
       expect(collection).to.contain.keys('items', 'patterns');
     });
-    describe ('hidden patterns', () => {
-      it ('should hide patterns that have front matter to that effect', () => {
-        var collection = patternData.components.button.collection;
-        expect(collection.patterns).not.to.contain(collection.items.hello);
-        expect(collection.patterns).to.contain(collection.items.base);
+    it ('should hide patterns that have front matter to that effect', () => {
+      var collection = patternData.components.button.collection;
+      expect(collection.patterns).not.to.contain(collection.items.hello);
+      expect(collection.patterns).to.contain(collection.items.base);
+    });
+    it ('should order patterns per front matter', () => {
+      var collection = patternData.components.button.collection;
+      expect(collection.patterns[0]).to.equal(collection.items.disabled);
+      expect(collection.patterns[1]).to
+        .equal(collection.items['color-variation']);
+      expect(collection.patterns[2]).to.equal(collection.items.aardvark);
+      expect(collection.patterns[3]).to.equal(collection.items.base);
+    });
+  });
+  describe ('pattern error situations', () => {
+    var opts;
+    beforeEach (() => {
+      return config.init(config.fixtureOpts).then(options => opts = options);
+    });
+    it ('should raise an error if pattern has `id` set locally', () => {
+      opts.src.patterns = {
+        glob: config.fixturePath('badPatterns/localId.html'),
+        basedir: config.fixturePath('badPatterns')
+      };
+      return parsePatterns(opts).catch(error => {
+        expect(error).to.be.instanceof(DrizzleError);
+        expect(error.message).to.contain('Drizzle reserved property');
+      });
+    });
+  });
+  describe ('collection error situations', () => {
+    var opts;
+    beforeEach (() => {
+      return config.init(config.fixtureOpts).then(options => opts = options);
+    });
+    it ('should raise an error if collection has `items` set locally', () => {
+      opts.src.patterns = {
+        glob: config.fixturePath('badPatterns/badCollection/*.html'),
+        basedir: config.fixturePath('badPatterns')
+      };
+      return parsePatterns(opts).catch(error => {
+        expect(error).to.be.instanceof(DrizzleError);
+        expect(error.message).to.contain('Drizzle reserved property');
+        expect(error.message).to.contain('items');
       });
     });
-    describe ('pattern ordering', () => {
-      it ('should order patterns per front matter', () => {
-        var collection = patternData.components.button.collection;
-        expect(collection.patterns[0]).to.equal(collection.items.disabled);
-        expect(collection.patterns[1]).to
-          .equal(collection.items['color-variation']);
-        expect(collection.patterns[2]).to.equal(collection.items.aardvark);
-        expect(collection.patterns[3]).to.equal(collection.items.base);
+    it ('should raise an error if collection has `patterns` set', () => {
+      opts.src.patterns = {
+        glob: config.fixturePath('badPatterns/anotherBadCollection/*.html'),
+        basedir: config.fixturePath('badPatterns')
+      };
+      return parsePatterns(opts).catch(error => {
+        expect(error).to.be.instanceof(DrizzleError);
+        expect(error.message).to.contain('Drizzle reserved property');
+        expect(error.message).to.contain('patterns');
       });
     });
   });
-
 });