diff --git a/src/actions/portal/toc/new-toc.ts b/src/actions/portal/toc/new-toc.ts index 5fe5b1db..63f8b601 100644 --- a/src/actions/portal/toc/new-toc.ts +++ b/src/actions/portal/toc/new-toc.ts @@ -13,6 +13,8 @@ import { import { withDirPath } from '../../../infrastructure/tmp-extensions.js'; import { TempContext } from '../../../types/temp-context.js'; import { PortalService } from '../../../infrastructure/services/portal-service.js'; +import { err, ok, Result } from 'neverthrow'; +import { ServiceError } from '../../../infrastructure/service-error.js'; export class ContentContext { private readonly fileService = new FileService(); @@ -52,6 +54,17 @@ export class PortalNewTocAction { this.prompts.invalidBuildDirectory(buildDirectory); return ActionResult.failed(); } + + const specDirectory = buildDirectory.join('spec'); + if (!(await this.fileService.directoryExists(specDirectory))) { + this.prompts.specDirectoryNotFound(buildDirectory); + return ActionResult.failed(); + } + if (!(await buildContext.getSpecContext().validate())) { + this.prompts.specDirectoryEmpty(buildDirectory); + return ActionResult.failed(); + } + const buildConfig = await buildContext.getBuildFileContents(); const contentDirectory = buildDirectory.join(buildConfig.contentFolder()); @@ -63,18 +76,11 @@ export class PortalNewTocAction { return ActionResult.cancelled(); } - const tocComponents: TocComponents = await (async () => { - const specDirectory = buildDirectory.join('spec'); - - if (!(await this.fileService.directoryExists(specDirectory))) { - this.prompts.fallingBackToDefault(); - return TocComponents.empty(); - } - - return await withDirPath(async (tempDirectory) => { - const tempContext = new TempContext(tempDirectory); - const specZipPath = await tempContext.zip(specDirectory); - const specFileStream = await this.fileService.getStream(specZipPath); + const tocComponentsResult: Result = await withDirPath(async (tempDirectory) => { + const tempContext = new TempContext(tempDirectory); + const specZipPath = await tempContext.zip(specDirectory); + const specFileStream = await this.fileService.getStream(specZipPath); + try { const result = await this.prompts.extractTocData( this.portalService.generateTocData(specFileStream, this.configDirectory, this.commandMetadata), expandEndpoints, @@ -82,15 +88,22 @@ export class PortalNewTocAction { expandWebhooks, expandCallbacks ); - specFileStream.close(); if (result.isErr()) { - this.prompts.fallingBackToDefault(); - return TocComponents.empty(); + return err(result.error); } - return TocComponents.fromTocData(result.value); - }); - })(); + return ok(TocComponents.fromTocData(result.value)); + } finally { + specFileStream.close(); + } + }); + + if (tocComponentsResult.isErr()) { + this.prompts.tocExtractionFailed(tocComponentsResult.error.errorMessage); + return ActionResult.failed(); + } + const tocComponents = tocComponentsResult.value; + const contentContext = new ContentContext(contentDirectory); const contentExists = await contentContext.exists(); diff --git a/src/infrastructure/service-error.ts b/src/infrastructure/service-error.ts index 840b31cd..d6af9802 100644 --- a/src/infrastructure/service-error.ts +++ b/src/infrastructure/service-error.ts @@ -1,6 +1,7 @@ import axios from "axios"; import { ApiError, ProblemDetailsError } from "@apimatic/sdk"; import { format as f } from "../prompts/format.js"; +import { discardStreamBody } from "../utils/utils.js"; export enum ServiceErrorCode { NotFound = "NOT_FOUND", @@ -97,7 +98,15 @@ function mapApiError(error: ApiError): ServiceError { } export function handleServiceError(error: unknown): ServiceError { - if (error instanceof ApiError) return mapApiError(error); + if (error instanceof ApiError) { + // A `callAsStream` error body would otherwise hang the CLI. Order against + // `mapApiError` is free: `error.result` is only populated when the SDK + // drained the body itself, so a live stream and a readable `result` never + // coexist. + const serviceError = mapApiError(error); + discardStreamBody(error.body); + return serviceError; + } if (axios.isAxiosError(error)) { const status = error.response?.status; diff --git a/src/infrastructure/services/transformation-service.ts b/src/infrastructure/services/transformation-service.ts index 119d4268..b6a77d3a 100644 --- a/src/infrastructure/services/transformation-service.ts +++ b/src/infrastructure/services/transformation-service.ts @@ -18,6 +18,7 @@ import { FilePath } from "../../types/file/filePath.js"; import { CommandMetadata } from "../../types/common/command-metadata.js"; import { err, ok, Result} from "neverthrow"; import { ServiceError } from "../service-error.js"; +import { discardStreamBody } from "../../utils/utils.js"; export interface TransformViaFileParams { file: FilePath; @@ -65,7 +66,7 @@ export class TransformationService { apiValidationSummary }); } catch (error) { - return err(await this.handleTransformationErrors(error)); + return err(this.handleTransformationErrors(error)); } } @@ -80,18 +81,33 @@ export class TransformationService { }; }; - private readonly handleTransformationErrors = async (error: unknown): Promise => { - if (error instanceof ApiError) { - const apiError = error as ApiError; - if (apiError.statusCode === 400) { - return "Your API Definition is invalid. Please use the APIMatic VS Code Extension to fix the errors and try again."; - } else if (apiError.statusCode === 401) { - const message = JSON.parse(apiError.body as string).message; - return ServiceError.unauthorizedWithHint(message).errorMessage; - } - return `Error ${apiError.statusCode}: An error occurred during the transformation. Please try again or contact support@apimatic.io for assistance.`; - } else { + private parseApiMessage(body: unknown): string | null { + if (typeof body !== "string") return null; + try { + return (JSON.parse(body) as { message?: string })?.message ?? null; + } catch { + return null; + } + } + + private readonly handleTransformationErrors = (error: unknown): string => { + if (!(error instanceof ApiError)) { return "An unexpected error occurred while validating your API Definition. Please try again later. If the problem persists, please reach out to our team at support@apimatic.io"; } + + // `downloadTransformedFile` is a stream endpoint, so its error body is an + // undrained response that would keep the event loop alive. Discarding it + // first is safe: a string body (from `transformViaFile`) has no `destroy` + // and is left intact for `parseApiMessage` below. + discardStreamBody(error.body); + + switch (error.statusCode) { + case 400: + return "Your API Definition is invalid. Please use the APIMatic VS Code Extension to fix the errors and try again."; + case 401: + return ServiceError.unauthorizedWithHint(this.parseApiMessage(error.body)).errorMessage; + default: + return `Error ${error.statusCode}: An error occurred during the transformation. Please try again or contact support@apimatic.io for assistance.`; + } }; } diff --git a/src/prompts/portal/toc/new-toc.ts b/src/prompts/portal/toc/new-toc.ts index e4e8ea93..0b3aa182 100644 --- a/src/prompts/portal/toc/new-toc.ts +++ b/src/prompts/portal/toc/new-toc.ts @@ -21,15 +21,22 @@ export class PortalNewTocPrompts { return overwrite; } - public fallingBackToDefault() { - log.warn(`Falling back to the default TOC structure.`); + public specDirectoryNotFound(directory: DirectoryPath) { + log.error( + `The ${f.var("spec")} directory was not found in ${f.path(directory)}.\n` + + `Add your API specification there and try again.` + ); + } + + public specDirectoryEmpty(directory: DirectoryPath) { + log.error(`The ${f.var("spec")} directory in ${f.path(directory)} is either empty or invalid.`); } public tocFileAlreadyExists() { log.error(`Please enter a different destination path or delete the existing toc.yml file and try again.`); } - public logError(message: string) { + public tocExtractionFailed(message: string) { log.error(message); } diff --git a/src/types/toc/toc-components.ts b/src/types/toc/toc-components.ts index 3b40ed5d..ff91010f 100644 --- a/src/types/toc/toc-components.ts +++ b/src/types/toc/toc-components.ts @@ -60,10 +60,6 @@ export class TocComponents { this.callbackGroups = callbackGroups; } - static empty(): TocComponents { - return new TocComponents(new Map(), [], [], [], [], [], new Map(), new Map()); - } - static fromTocData(tocData: TocData): TocComponents { return new TocComponents( TocComponents.toEndpointGroups(tocData.endpoints), diff --git a/src/utils/utils.ts b/src/utils/utils.ts index 43b605f8..b4ae494b 100644 --- a/src/utils/utils.ts +++ b/src/utils/utils.ts @@ -19,6 +19,30 @@ export async function parseStreamBodyToJson(body: NodeJS.ReadableStream): Promis return JSON.parse(text); } +// `destroy()` is not on `NodeJS.ReadableStream` — it lives on `stream.Readable` +// and `IncomingMessage` — so narrowing an `ApiError.body` union still won't let +// us call it. Probe for the method instead. +function isDestroyable(value: unknown): value is { destroy: () => void } { + return typeof (value as { destroy?: unknown })?.destroy === "function"; +} + +// Counterpart to `parseStreamBodyToJson`: releases a response body we are never +// going to read. An `ApiError.body` from a `callAsStream` endpoint is an +// undrained `IncomingMessage` whose socket keeps the Node event loop alive, so +// the CLI hangs after printing its outro — `outro()` only sets `process.exitCode` +// and we never call `process.exit()`. +// +// The axios adapter yields a stream on Node and a Blob elsewhere, so the probe +// above is a real platform branch. `destroy()` is idempotent, making this safe +// on bodies the SDK has already drained via `loadResult`. Callers that need the +// body must read it *before* calling this (see `PortalService.generatePortal`, +// which returns the 422 body as the error report). +export function discardStreamBody(body: unknown): void { + if (isDestroyable(body)) { + body.destroy(); + } +} + export const toPascalCase = (str: string): string => { return str .split(" ") diff --git a/test/actions/portal/toc/new-toc.test.ts b/test/actions/portal/toc/new-toc.test.ts new file mode 100644 index 00000000..f1dbf932 --- /dev/null +++ b/test/actions/portal/toc/new-toc.test.ts @@ -0,0 +1,69 @@ +import * as path from "path"; +import fsExtra from "fs-extra"; +import { expect } from "chai"; +import { dir as tmpDir, DirectoryResult } from "tmp-promise"; +import { PortalNewTocAction } from "../../../../src/actions/portal/toc/new-toc.js"; +import { DirectoryPath } from "../../../../src/types/file/directoryPath.js"; +import { CommandMetadata } from "../../../../src/types/common/command-metadata.js"; + +const COMMAND_METADATA: CommandMetadata = { commandName: "portal toc new", shell: "test" }; + +// `portal toc new` used to fall back to the default TOC when it had no spec to +// extract from — writing a toc.yml with no Endpoints, Events or Models sections +// over a possibly valid file, and exiting 0. The spec is now required. These +// cases return before any request is made, so no service stubbing is needed. +describe("PortalNewTocAction spec directory requirement", () => { + let tmpDirResult: DirectoryResult; + let buildDirectory: string; + let action: PortalNewTocAction; + + const execute = () => action.execute(new DirectoryPath(buildDirectory)); + + beforeEach(async () => { + tmpDirResult = await tmpDir({ unsafeCleanup: true }); + buildDirectory = path.join(tmpDirResult.path, "build"); + await fsExtra.ensureDir(buildDirectory); + // `BuildContext.validate()` only checks that the build file exists — the + // spec checks run before its contents are ever read. + await fsExtra.writeJson(path.join(buildDirectory, "APIMATIC-BUILD.json"), {}); + + action = new PortalNewTocAction(new DirectoryPath(tmpDirResult.path), COMMAND_METADATA); + }); + + afterEach(async () => { + await tmpDirResult.cleanup(); + }); + + it("fails when the spec directory is missing", async () => { + const result = await execute(); + + expect(result.isFailed()).to.be.true; + }); + + it("fails when the spec directory exists but is empty", async () => { + await fsExtra.ensureDir(path.join(buildDirectory, "spec")); + + const result = await execute(); + + expect(result.isFailed()).to.be.true; + }); + + it("fails when the spec directory holds only dotfiles", async () => { + // `FileService.directoryEmpty` filters dot-prefixed entries, so a spec + // directory kept alive by a .gitkeep still has nothing to extract. + const specDirectory = path.join(buildDirectory, "spec"); + await fsExtra.ensureDir(specDirectory); + await fsExtra.writeFile(path.join(specDirectory, ".gitkeep"), ""); + + const result = await execute(); + + expect(result.isFailed()).to.be.true; + }); + + it("writes no toc.yml when the spec is missing", async () => { + await execute(); + + const written = await fsExtra.readdir(buildDirectory); + expect(written).to.deep.equal(["APIMATIC-BUILD.json"]); + }); +}); diff --git a/test/utils/utils.test.ts b/test/utils/utils.test.ts new file mode 100644 index 00000000..56c15325 --- /dev/null +++ b/test/utils/utils.test.ts @@ -0,0 +1,51 @@ +import { Readable } from "stream"; +import { expect } from "chai"; +import { discardStreamBody } from "../../src/utils/utils.js"; + +// `ApiError.body` is `string | Blob | NodeJS.ReadableStream`, and which one it +// is depends on the endpoint: `callAsStream` yields a stream on Node, a Blob +// elsewhere, and `callAsJson` yields a string. `discardStreamBody` has to +// release the first without disturbing the others, so the probe is the contract. +describe("discardStreamBody", () => { + it("destroys a readable stream body", () => { + const stream = Readable.from(["chunk"]); + expect(stream.destroyed).to.be.false; + + discardStreamBody(stream); + + expect(stream.destroyed).to.be.true; + }); + + it("is idempotent on an already destroyed stream", () => { + // The SDK drains error bodies itself via `loadResult` on the `throwOn` path, + // so a body reaching here may already be finished. + const stream = Readable.from(["chunk"]); + stream.destroy(); + + expect(() => discardStreamBody(stream)).to.not.throw(); + expect(stream.destroyed).to.be.true; + }); + + it("leaves a string body intact for callers that parse it afterwards", () => { + // `transformation-service` discards before reading the 401 message; that is + // only safe because a `callAsJson` string body has no `destroy`. + const body = JSON.stringify({ message: "Authorization has been denied" }); + + discardStreamBody(body); + + expect((JSON.parse(body) as { message: string }).message).to.equal("Authorization has been denied"); + }); + + it("ignores a Blob-like body with no destroy method", () => { + expect(() => discardStreamBody({ size: 12, type: "application/zip" })).to.not.throw(); + }); + + it("ignores undefined and null bodies", () => { + expect(() => discardStreamBody(undefined)).to.not.throw(); + expect(() => discardStreamBody(null)).to.not.throw(); + }); + + it("ignores a destroy property that is not callable", () => { + expect(() => discardStreamBody({ destroy: "not a function" })).to.not.throw(); + }); +});