fix(@angular-devkit/core): name the unknown option in a schema validation error

An unknown property in `angular.json` or in a builder's options reported
`Data path "" must NOT have additional properties(allowedCommonJsDependencies).`,
which names neither the object the property was found in nor what is valid
there. It now reads `Unknown option "allowedCommonJsDependencies" at the top
level. Valid options are: assets, browser, ...`, and `at "/cli"` where the
instance path is not empty. Every other error reads `Option at "/outputPath"
must be string.` in place of the `Data path` prefix, keeping the list of
allowed values an enum error already appended.

Listing the valid options needs ajv's `verbose` option, which is what puts
`parentSchema` on an error. That is the schema the property was rejected by, so
a `$ref` and a `oneOf` branch each list their own options; a schema that
declares no `properties` at all stops the sentence after the location rather
than offering an empty list.

Author: thekhegay

packages/angular_devkit/core/src/json/schema/registry.ts

@@ -61,19 +61,26 @@ export class SchemaValidationException extends BaseException {
     }
 
     const messages = errors.map((err) => {
-      let message = `Data path ${JSON.stringify(err.instancePath)} ${err.message}`;
-      if (err.params) {
-        switch (err.keyword) {
-          case 'additionalProperties':
-            message += `(${err.params.additionalProperty})`;
-            break;
-
-          case 'enum':
-            message += `. Allowed values are: ${(err.params.allowedValues as string[] | undefined)
-              ?.map((v) => `"${v}"`)
-              .join(', ')}`;
-            break;
-        }
+      const location = err.instancePath ? `"${err.instancePath}"` : 'the top level';
+
+      if (err.keyword === 'additionalProperties') {
+        const unknown = err.params?.additionalProperty;
+        // `parentSchema` is the schema that rejected the property, which ajv only attaches when
+        // the validator was created with `verbose: true`. A schema that declares no `properties`
+        // of its own, such as one using only `patternProperties`, has no options to offer.
+        const known = Object.keys(err.parentSchema?.properties ?? {});
+
+        return (
+          `Unknown option "${unknown}" at ${location}.` +
+          (known.length ? ` Valid options are: ${known.join(', ')}.` : '')
+        );
+      }
+
+      let message = `Option at ${location} ${err.message}`;
+      if (err.keyword === 'enum' && err.params) {
+        message += `. Allowed values are: ${(err.params.allowedValues as string[] | undefined)
+          ?.map((v) => `"${v}"`)
+          .join(', ')}`;
       }
 
       return message + '.';
@@ -106,6 +113,8 @@ export class CoreSchemaRegistry implements SchemaRegistry {
       strict: false,
       loadSchema: (uri: string) => this._fetch(uri),
       passContext: true,
+      // Needed to list the valid options of the object an unknown option was found in.
+      verbose: true,
     });
 
     ajvAddFormats(this._ajv);

packages/angular_devkit/core/src/json/schema/registry_spec.ts

@@ -7,8 +7,10 @@
  */
 
 /* eslint-disable @typescript-eslint/no-explicit-any */
+import { JsonValue } from '../utils';
 import { SchemaFormat } from './interface';
 import { CoreSchemaRegistry, SchemaValidationException } from './registry';
+import { JsonSchema } from './schema';
 import { addUndefinedDefaults } from './transforms';
 
 describe('CoreSchemaRegistry', () => {
@@ -126,7 +128,7 @@ describe('CoreSchemaRegistry', () => {
     const result = await validator(data);
     expect(result.success).toBe(false);
     expect(new SchemaValidationException(result.errors).message).toContain(
-      `Data path "/packageManager" must be equal to one of the allowed values. Allowed values are: "npm", "yarn", "pnpm".`,
+      `Option at "/packageManager" must be equal to one of the allowed values. Allowed values are: "npm", "yarn", "pnpm".`,
     );
   });
 
@@ -340,4 +342,96 @@ describe('CoreSchemaRegistry', () => {
     expect(deprecatedMessages[1]).toBe('Option "bar" is deprecated.');
     expect(result.success).toBe(true, result.errors);
   });
+
+  describe('error messages', () => {
+    async function messagesFor(schema: JsonSchema, data: JsonValue): Promise<string[]> {
+      const registry = new CoreSchemaRegistry();
+      const validator = await registry.compile(schema);
+      const result = await validator(data);
+      expect(result.success).toBe(false);
+
+      return SchemaValidationException.createMessages(result.errors);
+    }
+
+    it('names an unknown option and the options that are valid there', async () => {
+      const messages = await messagesFor(
+        {
+          properties: { version: { type: 'number' }, projects: { type: 'object' } },
+          additionalProperties: false,
+        },
+        { allowedCommonJsDependencies: [] },
+      );
+
+      expect(messages).toEqual([
+        'Unknown option "allowedCommonJsDependencies" at the top level. ' +
+          'Valid options are: version, projects.',
+      ]);
+    });
+
+    it('points at the object an unknown option was found in', async () => {
+      const messages = await messagesFor(
+        {
+          properties: {
+            cli: {
+              type: 'object',
+              properties: { cache: { type: 'object' }, packageManager: { type: 'string' } },
+              additionalProperties: false,
+            },
+          },
+        },
+        { cli: { completion: true } },
+      );
+
+      expect(messages).toEqual([
+        'Unknown option "completion" at "/cli". Valid options are: cache, packageManager.',
+      ]);
+    });
+
+    it('looks through a $ref for the valid options', async () => {
+      const messages = await messagesFor(
+        {
+          $ref: '#/definitions/global',
+          definitions: {
+            global: {
+              type: 'object',
+              properties: { cli: { type: 'object' }, schematics: { type: 'object' } },
+              additionalProperties: false,
+            },
+          },
+        },
+        { version: 1 },
+      );
+
+      expect(messages).toEqual([
+        'Unknown option "version" at the top level. Valid options are: cli, schematics.',
+      ]);
+    });
+
+    it('omits the valid options when the schema does not list any', async () => {
+      const messages = await messagesFor(
+        {
+          $ref: '#/definitions/cli',
+          definitions: {
+            cli: {
+              type: 'object',
+              patternProperties: { '^x-': { type: 'string' } },
+              additionalProperties: false,
+            },
+          },
+        },
+        { completion: true },
+      );
+
+      expect(messages).toEqual(['Unknown option "completion" at the top level.']);
+    });
+
+    it('reports a value of the wrong type', async () => {
+      const messages = await messagesFor(
+        { properties: { outputPath: { type: 'string' } } },
+        { outputPath: 42 },
+      );
+
+      expect(messages).toEqual(['Option at "/outputPath" must be string.']);
+    });
+  });
 });

tests/e2e/tests/commands/config/config-global-validation.ts

@@ -9,21 +9,15 @@ export default async function () {
   let ngError: Error;
 
   ngError = await expectToFail(() => silentNg('config', 'cli.completion.prompted', 'true'));
-  assert.match(
-    ngError.message,
-    /Data path "\/cli" must NOT have additional properties\(completion\)\./,
-  );
+  assert.match(ngError.message, /Unknown option "completion" at "\/cli"\./);
 
   ngError = await expectToFail(() =>
     silentNg('config', '--global', 'cli.completion.invalid', 'true'),
   );
-  assert.match(
-    ngError.message,
-    /Data path "\/cli\/completion" must NOT have additional properties\(invalid\)\./,
-  );
+  assert.match(ngError.message, /Unknown option "invalid" at "\/cli\/completion"\./);
 
   ngError = await expectToFail(() => silentNg('config', '--global', 'cli.cache.enabled', 'true'));
-  assert.match(ngError.message, /Data path "\/cli" must NOT have additional properties\(cache\)\./);
+  assert.match(ngError.message, /Unknown option "cache" at "\/cli"\./);
 
   ngError = await expectToFail(() => silentNg('config', 'cli.completion.prompted'));
   assert.match(ngError.message, /Value cannot be found\./);

tests/e2e/tests/commands/config/config-set.ts

@@ -6,10 +6,7 @@ export default async function () {
   let ngError: Error;
 
   ngError = await expectToFail(() => silentNg('config', 'cli.warnings.zzzz', 'true'));
-  assert.match(
-    ngError.message,
-    /Data path "\/cli\/warnings" must NOT have additional properties\(zzzz\)\./,
-  );
+  assert.match(ngError.message, /Unknown option "zzzz" at "\/cli\/warnings"\./);
 
   ngError = await expectToFail(() => silentNg('config', 'cli.warnings.zzzz'));
   assert.match(ngError.message, /Value cannot be found\./);