Improve script/category name validation

- Use better error messages with more context. - Unify their validation logic and share tests. - Validate also type of the name. - Refactor node (Script/Category) parser tests for easier future changes and cleaner test code (using `TestBuilder` to do dirty work in unified way). - Add more tests. Custom `Error` properties are compared manually due to `chai` not supporting deep equality checks (chaijs/chai#1065, chaijs/chai#1405).
undergroundwires · Mar 11, 2022 · b210aad · b210aad
1 parent 65902e5
commit b210aad
Show file tree

Hide file tree

Showing 17 changed files with 853 additions and 192 deletions.
diff --git a/src/application/Parser/CategoryParser.ts b/src/application/Parser/CategoryParser.ts
@@ -3,6 +3,8 @@ import type {
 } from '@/application/collections/';
 import { Script } from '@/domain/Script';
 import { Category } from '@/domain/Category';
+import { NodeValidator } from '@/application/Parser/NodeValidation/NodeValidator';
+import { NodeType } from '@/application/Parser/NodeValidation/NodeType';
 import { parseDocUrls } from './DocumentationParser';
 import { ICategoryCollectionParseContext } from './Script/ICategoryCollectionParseContext';
 import { parseScript } from './Script/ScriptParser';
@@ -12,58 +14,97 @@ let categoryIdCounter = 0;
 export function parseCategory(
   category: CategoryData,
   context: ICategoryCollectionParseContext,
+  factory: CategoryFactoryType = CategoryFactory,
 ): Category {
   if (!context) { throw new Error('missing context'); }
-  ensureValid(category);
+  return parseCategoryRecursively({
+    categoryData: category,
+    context,
+    factory,
+  });
+}
+
+interface ICategoryParseContext {
+  readonly categoryData: CategoryData,
+  readonly context: ICategoryCollectionParseContext,
+  readonly factory: CategoryFactoryType,
+  readonly parentCategory?: CategoryData,
+}
+// eslint-disable-next-line consistent-return
+function parseCategoryRecursively(context: ICategoryParseContext): Category {
+  ensureValidCategory(context.categoryData, context.parentCategory);
   const children: ICategoryChildren = {
     subCategories: new Array<Category>(),
     subScripts: new Array<Script>(),
   };
-  for (const data of category.children) {
-    parseCategoryChild(data, children, category, context);
+  for (const data of context.categoryData.children) {
+    parseNode({
+      nodeData: data,
+      children,
+      parent: context.categoryData,
+      factory: context.factory,
+      context: context.context,
+    });
+  }
+  try {
+    return context.factory(
+      /* id: */ categoryIdCounter++,
+      /* name: */ context.categoryData.category,
+      /* docs: */ parseDocUrls(context.categoryData),
+      /* categories: */ children.subCategories,
+      /* scripts: */ children.subScripts,
+    );
+  } catch (err) {
+    new NodeValidator({
+      type: NodeType.Category,
+      selfNode: context.categoryData,
+      parentNode: context.parentCategory,
+    }).throw(err.message);
   }
-  return new Category(
-    /* id: */ categoryIdCounter++,
-    /* name: */ category.category,
-    /* docs: */ parseDocUrls(category),
-    /* categories: */ children.subCategories,
-    /* scripts: */ children.subScripts,
-  );
 }
 
-function ensureValid(category: CategoryData) {
-  if (!category) {
-    throw Error('missing category');
-  }
-  if (!category.children || category.children.length === 0) {
-    throw Error(`category has no children: "${category.category}"`);
-  }
-  if (!category.category || category.category.length === 0) {
-    throw Error('category has no name');
-  }
+function ensureValidCategory(category: CategoryData, parentCategory?: CategoryData) {
+  new NodeValidator({
+    type: NodeType.Category,
+    selfNode: category,
+    parentNode: parentCategory,
+  })
+    .assertDefined(category)
+    .assertValidName(category.category)
+    .assert(
+      () => category.children && category.children.length > 0,
+      `"${category.category}" has no children.`,
+    );
 }
 
 interface ICategoryChildren {
   subCategories: Category[];
   subScripts: Script[];
 }
 
-function parseCategoryChild(
-  data: CategoryOrScriptData,
-  children: ICategoryChildren,
-  parent: CategoryData,
-  context: ICategoryCollectionParseContext,
-) {
-  if (isCategory(data)) {
-    const subCategory = parseCategory(data as CategoryData, context);
-    children.subCategories.push(subCategory);
-  } else if (isScript(data)) {
-    const scriptData = data as ScriptData;
-    const script = parseScript(scriptData, context);
-    children.subScripts.push(script);
+interface INodeParseContext {
+  readonly nodeData: CategoryOrScriptData;
+  readonly children: ICategoryChildren;
+  readonly parent: CategoryData;
+  readonly factory: CategoryFactoryType;
+  readonly context: ICategoryCollectionParseContext;
+}
+function parseNode(context: INodeParseContext) {
+  const validator = new NodeValidator({ selfNode: context.nodeData, parentNode: context.parent });
+  validator.assertDefined(context.nodeData);
+  if (isCategory(context.nodeData)) {
+    const subCategory = parseCategoryRecursively({
+      categoryData: context.nodeData as CategoryData,
+      context: context.context,
+      factory: context.factory,
+      parentCategory: context.parent,
+    });
+    context.children.subCategories.push(subCategory);
+  } else if (isScript(context.nodeData)) {
+    const script = parseScript(context.nodeData as ScriptData, context.context);
+    context.children.subScripts.push(script);
   } else {
-    throw new Error(`Child element is neither a category or a script.
-                Parent: ${parent.category}, element: ${JSON.stringify(data)}`);
+    validator.throw('Node is neither a category or a script.');
   }
 }
 
@@ -73,14 +114,22 @@ function isScript(data: CategoryOrScriptData): data is ScriptData {
 }
 
 function isCategory(data: CategoryOrScriptData): data is CategoryData {
-  const { category } = data as CategoryData;
-  return category && category.length > 0;
+  return hasProperty(data, 'category');
+}
+
+function hasCode(data: InstructionHolder): boolean {
+  return hasProperty(data, 'code');
 }
 
-function hasCode(holder: InstructionHolder): boolean {
-  return holder.code && holder.code.length > 0;
+function hasCall(data: InstructionHolder) {
+  return hasProperty(data, 'call');
 }
 
-function hasCall(holder: InstructionHolder) {
-  return holder.call !== undefined;
+function hasProperty(object: unknown, propertyName: string) {
+  return Object.prototype.hasOwnProperty.call(object, propertyName);
 }
+
+export type CategoryFactoryType = (
+  ...parameters: ConstructorParameters<typeof Category>) => Category;
+
+const CategoryFactory: CategoryFactoryType = (...parameters) => new Category(...parameters);
diff --git a/src/application/Parser/NodeValidation/NodeData.ts b/src/application/Parser/NodeValidation/NodeData.ts
@@ -0,0 +1,3 @@
+import type { ScriptData, CategoryData } from '@/application/collections/';
+
+export type NodeData = CategoryData | ScriptData;
diff --git a/src/application/Parser/NodeValidation/NodeDataError.ts b/src/application/Parser/NodeValidation/NodeDataError.ts
@@ -0,0 +1,35 @@
+import { NodeType } from './NodeType';
+import { NodeData } from './NodeData';
+
+export class NodeDataError extends Error {
+  constructor(message: string, public readonly context: INodeDataErrorContext) {
+    super(createMessage(message, context));
+    Object.setPrototypeOf(this, new.target.prototype); // https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-2.html#support-for-newtarget
+    this.name = new.target.name;
+  }
+}
+
+export interface INodeDataErrorContext {
+  readonly type?: NodeType;
+  readonly selfNode: NodeData;
+  readonly parentNode?: NodeData;
+}
+
+function createMessage(errorMessage: string, context: INodeDataErrorContext) {
+  let message = '';
+  if (context.type !== undefined) {
+    message += `${NodeType[context.type]}: `;
+  }
+  message += errorMessage;
+  message += `\n${dump(context)}`;
+  return message;
+}
+
+function dump(context: INodeDataErrorContext): string {
+  const printJson = (obj: unknown) => JSON.stringify(obj, undefined, 2);
+  let output = `Self: ${printJson(context.selfNode)}`;
+  if (context.parentNode) {
+    output += `\nParent: ${printJson(context.parentNode)}`;
+  }
+  return output;
+}
diff --git a/src/application/Parser/NodeValidation/NodeType.ts b/src/application/Parser/NodeValidation/NodeType.ts
@@ -0,0 +1,4 @@
+export enum NodeType {
+  Script,
+  Category,
+}
diff --git a/src/application/Parser/NodeValidation/NodeValidator.ts b/src/application/Parser/NodeValidation/NodeValidator.ts
@@ -0,0 +1,38 @@
+import { INodeDataErrorContext, NodeDataError } from './NodeDataError';
+import { NodeData } from './NodeData';
+
+export class NodeValidator {
+  constructor(private readonly context: INodeDataErrorContext) {
+
+  }
+
+  public assertValidName(nameValue: string) {
+    return this
+      .assert(
+        () => Boolean(nameValue),
+        'missing name',
+      )
+      .assert(
+        () => typeof nameValue === 'string',
+        `Name (${JSON.stringify(nameValue)}) is not a string but ${typeof nameValue}.`,
+      );
+  }
+
+  public assertDefined(node: NodeData) {
+    return this.assert(
+      () => node !== undefined && node !== null && Object.keys(node).length > 0,
+      'missing node data',
+    );
+  }
+
+  public assert(validationPredicate: () => boolean, errorMessage: string) {
+    if (!validationPredicate()) {
+      this.throw(errorMessage);
+    }
+    return this;
+  }
+
+  public throw(errorMessage: string) {
+    throw new NodeDataError(errorMessage, this.context);
+  }
+}
diff --git a/src/application/Parser/Script/ScriptParser.ts b/src/application/Parser/Script/ScriptParser.ts
@@ -5,22 +5,31 @@ import { IScriptCode } from '@/domain/IScriptCode';
 import { ScriptCode } from '@/domain/ScriptCode';
 import { parseDocUrls } from '../DocumentationParser';
 import { createEnumParser, IEnumParser } from '../../Common/Enum';
+import { NodeType } from '../NodeValidation/NodeType';
+import { NodeValidator } from '../NodeValidation/NodeValidator';
 import { ICategoryCollectionParseContext } from './ICategoryCollectionParseContext';
 
+// eslint-disable-next-line consistent-return
 export function parseScript(
   data: ScriptData,
   context: ICategoryCollectionParseContext,
   levelParser = createEnumParser(RecommendationLevel),
+  scriptFactory: ScriptFactoryType = ScriptFactory,
 ): Script {
-  validateScript(data);
+  const validator = new NodeValidator({ type: NodeType.Script, selfNode: data });
+  validateScript(data, validator);
   if (!context) { throw new Error('missing context'); }
-  const script = new Script(
-    /* name: */ data.name,
-    /* code: */ parseCode(data, context),
-    /* docs: */ parseDocUrls(data),
-    /* level: */ parseLevel(data.recommend, levelParser),
-  );
-  return script;
+  try {
+    const script = scriptFactory(
+      /* name: */ data.name,
+      /* code: */ parseCode(data, context),
+      /* docs: */ parseDocUrls(data),
+      /* level: */ parseLevel(data.recommend, levelParser),
+    );
+    return script;
+  } catch (err) {
+    validator.throw(err.message);
+  }
 }
 
 function parseLevel(
@@ -40,21 +49,24 @@ function parseCode(script: ScriptData, context: ICategoryCollectionParseContext)
   return new ScriptCode(script.code, script.revertCode, context.syntax);
 }
 
-function ensureNotBothCallAndCode(script: ScriptData) {
-  if (script.code && script.call) {
-    throw new Error('cannot define both "call" and "code"');
-  }
-  if (script.revertCode && script.call) {
-    throw new Error('cannot define "revertCode" if "call" is defined');
-  }
+function validateScript(script: ScriptData, validator: NodeValidator) {
+  validator
+    .assertDefined(script)
+    .assertValidName(script.name)
+    .assert(
+      () => Boolean(script.code || script.call),
+      'Must define either "call" or "code".',
+    )
+    .assert(
+      () => !(script.code && script.call),
+      'Cannot define both "call" and "code".',
+    )
+    .assert(
+      () => !(script.revertCode && script.call),
+      'Cannot define "revertCode" if "call" is defined.',
+    );
 }
 
-function validateScript(script: ScriptData) {
-  if (!script) {
-    throw new Error('missing script');
-  }
-  if (!script.code && !script.call) {
-    throw new Error('must define either "call" or "code"');
-  }
-  ensureNotBothCallAndCode(script);
-}
+export type ScriptFactoryType = (...parameters: ConstructorParameters<typeof Script>) => Script;
+
+const ScriptFactory: ScriptFactoryType = (...parameters) => new Script(...parameters);
diff --git a/src/domain/Script.ts b/src/domain/Script.ts
@@ -12,7 +12,7 @@ export class Script extends BaseEntity<string> implements IScript {
   ) {
     super(name);
     if (!code) {
-      throw new Error(`missing code (script: ${name})`);
+      throw new Error('missing code');
     }
     validateLevel(level);
   }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		import type { ScriptData, CategoryData } from '@/application/collections/';

		export type NodeData = CategoryData \| ScriptData;