fix(traversal): fix endless loop in schema traversal

fixes #185

lib/formattingTools.js

@@ -13,6 +13,9 @@ const i18n = require('es2015-i18n-tag').default;
 const s = require('./symbols');
 
 function gentitle(titles, type) {
+  if (!Array.isArray(titles)) {
+    return i18n`Untitled schema`;
+  }
   const [firsttitle] = titles;
   const lasttitle = [...titles].pop();
 

lib/markdownBuilder.js

@@ -306,7 +306,8 @@ function build({
       tableCell(type(definition)),
       tableCell(text(required.indexOf(name) > -1 ? i18n`Required` : i18n`Optional`)),
       tableCell(nullable(definition)),
-      tableCell(link(`${definition[s.slug]}.md`, `${definition[s.id]}#${definition[s.pointer]}`, text(definition[s.titles][0] || i18n`Untitled schema`))),
+      tableCell(link(`${definition[s.slug]}.md`, `${definition[s.id]}#${definition[s.pointer]}`,
+        text(definition[s.titles] && definition[s.titles][0] ? definition[s.titles][0] : i18n`Untitled schema`))),
     ]);
   }
 
@@ -440,7 +441,7 @@ function build({
   function makedefinedinfact(definition) {
     return listItem(paragraph([
       text(i18n`defined in: `),
-      link(`${definition[s.slug]}.md`, `${definition[s.id]}#${definition[s.pointer]}`, text(definition[s.titles][0] || i18n`Untitled schema`)),
+      link(`${definition[s.slug]}.md`, `${definition[s.id]}#${definition[s.pointer]}`, text(definition[s.titles] && definition[s.titles][0] ? definition[s.titles][0] : i18n`Untitled schema`)),
     ]));
   }
 
@@ -702,7 +703,7 @@ function build({
     level = 2) {
     return [
       ...flist(flat(Object.entries(properties || {}).map(([name, definition]) => {
-        const description = definition[s.meta].longdescription || paragraph(text(i18n`no description`));
+        const description = definition[s.meta] && definition[s.meta].longdescription ? definition[s.meta].longdescription : paragraph(text(i18n`no description`));
 
         return [
           heading(level + 1, text(name)),

lib/traverseSchema.js

@@ -9,29 +9,25 @@
  * OF ANY KIND, either express or implied. See the License for the specific language
  * governing permissions and limitations under the License.
  */
-const { list } = require('ferrum');
 
-function sflat(arr) {
-  return arr.reduce((p, v) => {
-    if (Array.isArray(v)) {
-      return [...p, ...v];
-    }
-    return [...p, v];
-  }, []);
-}
+const { parent } = require('./symbols');
 
-/**
- * Traverses a (proxied) JSON schema. This is a less opinionated
- * and tighter traversal.
- * @param {*} schema
- */
-function traverseSchema(schema) {
-  if (Array.isArray(schema)) {
-    return sflat(schema.map(traverseSchema));
+function reducer({ seen, ids }, schema) {
+  if (schema
+    && schema[parent]
+    && (seen.has(schema) || (schema.$id && ids.indexOf(schema.$id) >= 0))) {
+    return { seen, ids };
+  } else if (Array.isArray(schema)) {
+    return schema.reduce(reducer, { seen, ids });
   } else if (schema && typeof schema === 'object') {
-    return sflat([schema, ...Object.values(schema).map(traverseSchema)]);
+    seen.add(schema);
+    if (schema.$id) {
+      ids.push(schema.$id);
+    }
+    return [...Object.values(schema)].reduce(reducer, { seen, ids });
   }
-  return [];
+
+  return { seen, ids };
 }
 
 /**
@@ -40,8 +36,7 @@ function traverseSchema(schema) {
  * @param {Schema} node
  */
 function traverse(schemas) {
-  return sflat(list(schemas).map(traverseSchema));
+  return Array.from(schemas.reduce(reducer, { seen: new Set(), ids: [] }).seen);
 }
 
 module.exports = traverse;
-module.exports.traverseSchema = traverseSchema;

test/cyclicSchemaIntegration.test.js

@@ -11,12 +11,13 @@
  */
 /* eslint-env mocha */
 
-const assert = require('assert');
 const path = require('path');
 const fs = require('fs-extra');
 const { loader } = require('../lib/schemaProxy');
 const { assertMarkdown } = require('./testUtils');
-const build = require('../lib/readmeBuilder');
+const readme = require('../lib/readmeBuilder');
+const markdown = require('../lib/markdownBuilder');
+const traverse = require('../lib/traverseSchema');
 
 describe('Integration Test: Cyclic References', () => {
   let one;
@@ -27,47 +28,28 @@ describe('Integration Test: Cyclic References', () => {
   let proxiedtwo;
   let proxiedthree;
 
-  before('Read Schemas from disk', async () => {
+  let allschemas;
+
+  beforeEach('Read Schemas from disk', async () => {
+    console.log('Reading Schemas');
     one = await fs.readJson(path.resolve(__dirname, 'fixtures', 'cyclic', 'one.schema.json'));
     two = await fs.readJson(path.resolve(__dirname, 'fixtures', 'cyclic', 'two.schema.json'));
     three = await fs.readJson(path.resolve(__dirname, 'fixtures', 'cyclic', 'three.schema.json'));
 
     const myloader = loader();
-    proxiedone = myloader(one, 'one.schema.json');
-    proxiedtwo = myloader(two, 'two.schema.json');
-    proxiedthree = myloader(three, 'three.schema.json');
-  });
-
-  it('Schemas with cyclic references can be loaded', () => {
-    assert.equal(proxiedone.$id, 'http://example.com/schemas/one');
-
-    assert.equal(proxiedone.properties.children.items.anyOf[0].$id,
-      'http://example.com/schemas/three');
-    assert.equal(proxiedone.properties.children.items.anyOf[1].$id,
-      'http://example.com/schemas/two');
 
-    assert.equal(proxiedtwo.$id, 'http://example.com/schemas/two');
-    assert.equal(proxiedtwo.properties.children.items.anyOf[0].$id,
-      'http://example.com/schemas/one');
-    assert.equal(proxiedtwo.properties.children.items.anyOf[1].$id,
-      'http://example.com/schemas/three');
+    console.log('Loading Schemas');
+    proxiedone = myloader(one, path.resolve(__dirname, 'fixtures', 'cyclic', 'one.schema.json'));
+    proxiedtwo = myloader(two, path.resolve(__dirname, 'fixtures', 'cyclic', 'two.schema.json'));
+    proxiedthree = myloader(three, path.resolve(__dirname, 'fixtures', 'cyclic', 'three.schema.json'));
 
-    assert.equal(proxiedthree.$id, 'http://example.com/schemas/three');
-    assert.equal(proxiedthree.properties.children.items.anyOf[0].$id,
-      'http://example.com/schemas/one');
-    assert.equal(proxiedthree.properties.children.items.anyOf[1].$id,
-      'http://example.com/schemas/two');
+    console.log('Traversing Schemas');
 
-    // complete the cycle
-    assert.equal(proxiedone
-      .properties.children.items.anyOf[0]
-      .properties.children.items.anyOf[1]
-      .properties.children.items.anyOf[0].$id,
-    'http://example.com/schemas/one');
+    allschemas = traverse([proxiedone, proxiedtwo, proxiedthree]);
   });
 
   it('Schemas with cyclic references generate README', () => {
-    const builder = build({ readme: true });
+    const builder = readme({ readme: true });
     const result = builder([proxiedone, proxiedtwo, proxiedthree]);
 
     assertMarkdown(result)
@@ -78,6 +60,12 @@ describe('Integration Test: Cyclic References', () => {
   });
 
   it('Schemas with cyclic references generate Markdown', () => {
+    const builder = markdown();
+    const documents = builder(allschemas);
+    assertMarkdown(documents['one-properties-children-items'])
+      .contains('any of');
 
+    assertMarkdown(documents.one)
+      .contains('one-properties-children-items');
   });
 });

test/traverseSchema.test.js

@@ -12,11 +12,13 @@
 /* eslint-env mocha */
 
 const assert = require('assert');
+const fs = require('fs-extra');
+const path = require('path');
 const {
   loader, filename,
 } = require('../lib/schemaProxy');
 
-const { traverseSchema } = require('../lib/traverseSchema');
+const traverse = require('../lib/traverseSchema');
 
 const example = {
   'meta:license': [
@@ -66,9 +68,27 @@ const example = {
 describe('Testing Schema Traversal', () => {
   it('Schema Traversal generates a list', () => {
     const proxied = loader()(example, 'example.schema.json');
-    const schemas = traverseSchema(proxied);
+    const schemas = traverse([proxied]);
 
     assert.equal(schemas.length, 9);
     assert.equal(schemas[8][filename], 'example.schema.json');
   });
+
+  it('Cyclic Schema Traversal generates a list', async () => {
+    const one = await fs.readJson(path.resolve(__dirname, 'fixtures', 'cyclic', 'one.schema.json'));
+    const two = await fs.readJson(path.resolve(__dirname, 'fixtures', 'cyclic', 'two.schema.json'));
+    const three = await fs.readJson(path.resolve(__dirname, 'fixtures', 'cyclic', 'three.schema.json'));
+
+    const myloader = loader();
+    const proxiedone = myloader(one, path.resolve(__dirname, 'fixtures', 'cyclic', 'one.schema.json'));
+    const proxiedtwo = myloader(two, path.resolve(__dirname, 'fixtures', 'cyclic', 'two.schema.json'));
+    const proxiedthree = myloader(three, path.resolve(__dirname, 'fixtures', 'cyclic', 'three.schema.json'));
+
+
+    const schemas = traverse([proxiedone, proxiedtwo, proxiedthree]);
+
+    assert.equal(schemas[0].$id, 'http://example.com/schemas/one');
+    assert.equal(schemas[4].$id, 'http://example.com/schemas/three');
+    assert.equal(schemas[8].$id, 'http://example.com/schemas/two');
+  });
 });