Repository: Redocly/redocly-cli
Pull request #1057 (2023-04-21 09:57:19)

fix: improve error handling
## What/Why/How?

- Do not throw known errors
- Show error if api path refer to folder

## Reference
Closes: https://github.com/Redocly/redocly-cli/issues/844
## Testing

## Screenshots (optional)
![Знімок екрана 2023-04-24 о 10 29 51](https://user-images.githubusercontent.com/106662428/233928399-907abaf6-35f5-4e22-8904-8b6a098837a3.png)


## Check yourself

- [x] Code is linted
- [x] Tested with redoc/reference-docs/workflows
- [x] All new/updated code is covered with tests

## Security

- [x] Security impact of change has been considered
- [x] Code follows company security practices and guidelines


github-actions[bot]: <!-- jest coverage report action for options with hash 3339ae53a38c44382c0fb4491bdb282d -->

## Coverage report 



| <div title="Status of coverage:&#10; 🟢 - ok&#10; 🟡 - slightly more than threshold&#10; 🔴 - under the threshold">St.<sup>:grey_question:</sup></div> | Category   | Percentage | Covered / Total |
| :----------------------------------------------------------------------------------------------------------------------------------------------------: | :--------- | :--------- | :-------------: |
|                                                                           🟡                                                                           | Statements | 74.01%     |    3468/4686    |
|                                                                           🟡                                                                           | Branches   | 64.91%     |    1972/3038    |
|                                                                           🟡                                                                           | Functions  | 66.21%     |     582/879     |
|                                                                           🟡                                                                           | Lines      | 74.04%     |    3226/4357    |


<details><summary>Show files with reduced coverage 🔻</summary>
<br/>

| <div title="Status of coverage:&#10; 🟢 - ok&#10; 🟡 - slightly more than threshold&#10; 🔴 - under the threshold">St.<sup>:grey_question:</sup></div> | File                | Statements | Branches                                                       | Functions | Lines  |
| :----------------------------------------------------------------------------------------------------------------------------------------------------: | :------------------ | :--------- | :------------------------------------------------------------- | :-------- | :----- |
|                                                                           🟢                                                                           | core/src/resolve.ts | 94.86%     | <div title="Base coverage is: 91.95%">90.91% (-1.04% 🔻)</div> | 100%      | 94.61% |

</details>

## Test suite run success
577 tests passing in 85 suites.

<p align="right">Report generated by <a href="https://github.com/ArtiomTr/jest-coverage-report-action">🧪jest coverage report action</a> from 9c0848889d56ac4467f25e1db5e63c7b74ded617</p>


Patch:
diff --git a/packages/cli/src/__mocks__/@redocly/openapi-core.ts b/packages/cli/src/__mocks__/@redocly/openapi-core.ts
index f3fcb52c..834d33b0 100644
--- a/packages/cli/src/__mocks__/@redocly/openapi-core.ts
+++ b/packages/cli/src/__mocks__/@redocly/openapi-core.ts
@@ -1,5 +1,5 @@
 import { ConfigFixture } from './../../__tests__/fixtures/config';
-import { Document, ResolveError } from '@redocly/openapi-core';
+import { Document } from '@redocly/openapi-core';
 import { firstDocument, secondDocument } from '../documents';
 
 export const __redoclyClient = {
@@ -48,6 +48,20 @@ export class BaseResolver {
     );
 }
 
+export class ResolveError extends Error {
+  constructor(public originalError: Error) {
+    super(originalError.message);
+    Object.setPrototypeOf(this, ResolveError.prototype);
+  }
+}
+
+export class YamlParseError extends Error {
+  constructor(public originalError: Error) {
+    super(originalError.message);
+    Object.setPrototypeOf(this, YamlParseError.prototype);
+  }
+}
+
 export enum OasVersion {
   Version2 = 'oas2',
   Version3_0 = 'oas3_0',
diff --git a/packages/cli/src/utils.ts b/packages/cli/src/utils.ts
index 3de6d24f..1106b29c 100644
--- a/packages/cli/src/utils.ts
+++ b/packages/cli/src/utils.ts
@@ -225,23 +225,25 @@ export function pluralize(label: string, num: number) {
 }
 
 export function handleError(e: Error, ref: string) {
-  if (e instanceof ResolveError) {
-    process.stderr.write(`Failed to resolve api definition at ${ref}:\n\n  - ${e.message}.\n\n`);
-  } else if (e instanceof YamlParseError) {
-    process.stderr.write(`Failed to parse api definition at ${ref}:\n\n  - ${e.message}.\n\n`);
+  switch (e.constructor) {
+    case ResolveError:
+      return exitWithError(`Failed to resolve api definition at ${ref}:\n\n  - ${e.message}.`);
+    case YamlParseError:
+      return exitWithError(`Failed to parse api definition at ${ref}:\n\n  - ${e.message}.`);
     // TODO: codeframe
-  } else {
-    if (e instanceof CircularJSONNotSupportedError) {
+    case CircularJSONNotSupportedError: {
       process.stderr.write(
         red(`Detected circular reference which can't be converted to JSON.\n`) +
           `Try to use ${blue('yaml')} output or remove ${blue('--dereferenced')}.\n\n`
       );
-    } else {
+      return process.exit(1);
+    }
+    default: {
       process.stderr.write(`Something went wrong when processing ${ref}:\n\n  - ${e.message}.\n\n`);
+      process.exitCode = 1;
+      throw e;
     }
   }
-  process.exitCode = 1;
-  throw e;
 }
 
 export function printLintTotals(totals: Totals, definitionsCount: number) {
diff --git a/packages/core/src/resolve.ts b/packages/core/src/resolve.ts
index 540b402d..ce9979a6 100644
--- a/packages/core/src/resolve.ts
+++ b/packages/core/src/resolve.ts
@@ -115,11 +115,15 @@ export class BaseResolver {
         const { body, mimeType } = await readFileFromUrl(absoluteRef, this.config.http);
         return new Source(absoluteRef, body, mimeType);
       } else {
+        if (fs.lstatSync(absoluteRef).isDirectory()) {
+          throw new Error(`Expected a file but received a folder at ${absoluteRef}`);
+        }
         const content = await fs.promises.readFile(absoluteRef, 'utf-8');
         // In some cases file have \r\n line delimeters like on windows, we should skip it.
         return new Source(absoluteRef, content.replace(/\r\n/g, '\n'));
       }
     } catch (error) {
+      error.message = error.message.replace(', lstat', '');
       throw new ResolveError(error);
     }
   }
@@ -150,6 +154,7 @@ export class BaseResolver {
     isRoot: boolean = false
   ): Promise<Document | ResolveError | YamlParseError> {
     const absoluteRef = this.resolveExternalRef(base, ref);
+
     const cachedDocument = this.cache.get(absoluteRef);
     if (cachedDocument) {
       return cachedDocument;


Test patch:
diff --git a/packages/cli/src/__tests__/utils.test.ts b/packages/cli/src/__tests__/utils.test.ts
index c87a077a..b5a424e4 100644
--- a/packages/cli/src/__tests__/utils.test.ts
+++ b/packages/cli/src/__tests__/utils.test.ts
@@ -4,12 +4,22 @@ import {
   pathToFilename,
   printConfigLintTotals,
   langToExt,
+  handleError,
+  CircularJSONNotSupportedError,
   sortTopLevelKeysForOas,
 } from '../utils';
-import { ResolvedApi, Totals, isAbsoluteUrl, Oas3Definition } from '@redocly/openapi-core';
-import { red, yellow } from 'colorette';
+import {
+  ResolvedApi,
+  Totals,
+  isAbsoluteUrl,
+  ResolveError,
+  YamlParseError,
+  Oas3Definition,
+} from '@redocly/openapi-core';
+import { blue, red, yellow } from 'colorette';
 import { existsSync } from 'fs';
 import * as path from 'path';
+import * as process from 'process';
 
 jest.mock('os');
 jest.mock('colorette');
@@ -340,3 +350,61 @@ describe('sorTopLevelKeysForOas', () => {
     });
   });
 });
+
+describe('handleErrors', () => {
+  const ref = 'openapi/test.yaml';
+
+  const redColoretteMocks = red as jest.Mock<any, any>;
+
+  beforeEach(() => {
+    jest.spyOn(process.stderr, 'write').mockImplementation(() => true);
+    jest.spyOn(process, 'exit').mockImplementation((code) => code as never);
+    redColoretteMocks.mockImplementation((text) => text);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('should handle ResolveError', () => {
+    const resolveError = new ResolveError(new Error('File not found'));
+    handleError(resolveError, ref);
+    expect(redColoretteMocks).toHaveBeenCalledTimes(1);
+    expect(process.exit).toHaveBeenCalledWith(1);
+    expect(process.stderr.write).toHaveBeenCalledWith(
+      `Failed to resolve api definition at openapi/test.yaml:\n\n  - File not found.\n\n`
+    );
+  });
+
+  it('should handle YamlParseError', () => {
+    const yamlParseError = new YamlParseError(new Error('Invalid yaml'), {} as any);
+    handleError(yamlParseError, ref);
+    expect(redColoretteMocks).toHaveBeenCalledTimes(1);
+    expect(process.exit).toHaveBeenCalledWith(1);
+    expect(process.stderr.write).toHaveBeenCalledWith(
+      `Failed to parse api definition at openapi/test.yaml:\n\n  - Invalid yaml.\n\n`
+    );
+  });
+
+  it('should handle CircularJSONNotSupportedError', () => {
+    const circularError = new CircularJSONNotSupportedError(new Error('Circular json'));
+    handleError(circularError, ref);
+    expect(process.exit).toHaveBeenCalledWith(1);
+    expect(process.stderr.write).toHaveBeenCalledWith(
+      `Detected circular reference which can't be converted to JSON.\n` +
+        `Try to use ${blue('yaml')} output or remove ${blue('--dereferenced')}.\n\n`
+    );
+  });
+
+  it('should throw unknown error', () => {
+    const testError = new Error('Test error');
+    try {
+      handleError(testError, ref);
+    } catch (e) {
+      expect(e).toEqual(testError);
+    }
+    expect(process.stderr.write).toHaveBeenCalledWith(
+      `Something went wrong when processing openapi/test.yaml:\n\n  - Test error.\n\n`
+    );
+  });
+});
diff --git a/packages/core/src/__tests__/resolve.test.ts b/packages/core/src/__tests__/resolve.test.ts
index 5aa99e9e..a9ad83f9 100644
--- a/packages/core/src/__tests__/resolve.test.ts
+++ b/packages/core/src/__tests__/resolve.test.ts
@@ -5,6 +5,7 @@ import { resolveDocument, BaseResolver, Document } from '../resolve';
 import { parseYamlToDocument } from '../../__tests__/utils';
 import { Oas3Types } from '../types/oas3';
 import { normalizeTypes } from '../types';
+import * as fs from 'fs';
 
 describe('collect refs', () => {
   it('should resolve local refs', async () => {
@@ -405,4 +406,25 @@ describe('collect refs', () => {
 
     expect(Array.from(resolvedRefs.values()).pop()!.node).toEqual({ type: 'string' });
   });
+
+  it('should throw error if ref is folder', async () => {
+    const cwd = path.join(__dirname, 'fixtures/resolve');
+    const rootDocument = parseYamlToDocument(
+      outdent`
+        openapi: 3.0.0
+        components:
+          $ref: "./transitive/components.yaml#/components/schemas/a"
+      `,
+      path.join(cwd, 'foobar')
+    );
+    jest.spyOn(fs, 'lstatSync').mockImplementation((_) => ({ isDirectory: () => true } as any));
+
+    const resolvedRefs = await resolveDocument({
+      rootDocument,
+      externalRefResolver: new BaseResolver(),
+      rootType: normalizeTypes(Oas3Types).Root,
+    });
+
+    expect(Array.from(resolvedRefs.values()).pop()!.error).toBeInstanceOf(Error);
+  });
 });
diff --git a/packages/core/src/rules/__tests__/no-unresolved-refs.test.ts b/packages/core/src/rules/__tests__/no-unresolved-refs.test.ts
index 225964cd..8474a74f 100644
--- a/packages/core/src/rules/__tests__/no-unresolved-refs.test.ts
+++ b/packages/core/src/rules/__tests__/no-unresolved-refs.test.ts
@@ -36,7 +36,7 @@ describe('oas3 boolean-parameter-prefixes', () => {
               "source": "foobar.yaml",
             },
           ],
-          "message": "Can't resolve $ref: ENOENT: no such file or directory, open 'invalid.yaml'",
+          "message": "Can't resolve $ref: ENOENT: no such file or directory 'invalid.yaml'",
           "ruleId": "no-unresolved-refs",
           "severity": "error",
           "suggest": Array [],