From afb67603b390368dc8a8db4e24f45aa663f2ed9e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 5 Oct 2021 18:58:23 +0200 Subject: [PATCH 1/7] errors: added new assertion APIs Signed-off-by: Patrik Oldsberg --- packages/errors/api-report.md | 19 +++++ packages/errors/src/errors/assertion.test.ts | 79 ++++++++++++++++++++ packages/errors/src/errors/assertion.ts | 48 ++++++++++++ packages/errors/src/errors/index.ts | 2 + 4 files changed, 148 insertions(+) create mode 100644 packages/errors/src/errors/assertion.test.ts create mode 100644 packages/errors/src/errors/assertion.ts diff --git a/packages/errors/api-report.md b/packages/errors/api-report.md index 429807d9db..4290ea0db1 100644 --- a/packages/errors/api-report.md +++ b/packages/errors/api-report.md @@ -5,6 +5,11 @@ ```ts import { JsonObject } from '@backstage/config'; +// Warning: (ae-missing-release-tag) "assertError" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export function assertError(val: unknown): asserts val is ErrorLike; + // @public export class AuthenticationError extends CustomErrorBase {} @@ -23,6 +28,15 @@ export function deserializeError( data: SerializedError, ): T; +// Warning: (ae-missing-release-tag) "ErrorLike" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export type ErrorLike = { + name: string; + message: string; + stack?: string; +}; + // @public export type ErrorResponse = { error: SerializedError; @@ -38,6 +52,11 @@ export type ErrorResponse = { // @public export class InputError extends CustomErrorBase {} +// Warning: (ae-missing-release-tag) "isError" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export function isError(val: unknown): val is ErrorLike; + // @public export class NotAllowedError extends CustomErrorBase {} diff --git a/packages/errors/src/errors/assertion.test.ts b/packages/errors/src/errors/assertion.test.ts new file mode 100644 index 0000000000..28367c2a68 --- /dev/null +++ b/packages/errors/src/errors/assertion.test.ts @@ -0,0 +1,79 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { assertError, isError } from './assertion'; +import { NotFoundError } from './common'; +import { CustomErrorBase } from './CustomErrorBase'; + +const areErrors = [ + { name: 'e', message: '' }, + new Error(), + new NotFoundError(), + Object.create({ name: 'e', message: '' }), + Object.assign(Object.create({ name: 'e' }), { + get message() { + return ''; + }, + }), + new (class extends class { + message = ''; + } { + name = 'e'; + })(), + new (class SubclassError extends CustomErrorBase {})(), + new (class SubclassError extends NotFoundError {})(), +]; + +const notErrors = [ + null, + 0, + 'loller', + Symbol(), + [], + BigInt(0), + false, + true, + { name: 'e' }, + { message: '' }, + { name: '', message: 'oh no' }, + new (class {})(), +]; + +describe('assertError', () => { + it.each(areErrors)('should assert that things are errors %#', error => { + expect(assertError(error)).toBeUndefined(); + }); + + it.each(notErrors)( + 'should assert that things are not errors %#', + notError => { + expect(() => assertError(notError)).toThrow(); + }, + ); +}); + +describe('isError', () => { + it.each(areErrors)('should assert that things are errors %#', error => { + expect(isError(error)).toBe(true); + }); + + it.each(notErrors)( + 'should assert that things are not errors %#', + notError => { + expect(isError(notError)).toBe(false); + }, + ); +}); diff --git a/packages/errors/src/errors/assertion.ts b/packages/errors/src/errors/assertion.ts new file mode 100644 index 0000000000..a0b2d59c11 --- /dev/null +++ b/packages/errors/src/errors/assertion.ts @@ -0,0 +1,48 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export type ErrorLike = { + name: string; + message: string; + stack?: string; +}; + +export function isError(val: unknown): val is ErrorLike { + if (typeof val !== 'object' || val === null || Array.isArray(val)) { + return false; + } + const maybe = val as Partial; + if (typeof maybe.name !== 'string' || maybe.name === '') { + return false; + } + if (typeof maybe.message !== 'string') { + return false; + } + return true; +} + +export function assertError(val: unknown): asserts val is ErrorLike { + if (typeof val !== 'object' || val === null || Array.isArray(val)) { + throw new Error(`Encountered invalid error, not an object, got '${val}'`); + } + const maybe = val as Partial; + if (typeof maybe.name !== 'string' || maybe.name === '') { + throw new Error(`Encountered error object without a name, got '${val}'`); + } + if (typeof maybe.message !== 'string') { + throw new Error(`Encountered error object without a message, got '${val}'`); + } +} diff --git a/packages/errors/src/errors/index.ts b/packages/errors/src/errors/index.ts index cc9d15b1ff..02670f4de9 100644 --- a/packages/errors/src/errors/index.ts +++ b/packages/errors/src/errors/index.ts @@ -14,6 +14,8 @@ * limitations under the License. */ +export { assertError, isError } from './assertion'; +export type { ErrorLike } from './assertion'; export { AuthenticationError, ConflictError, From 93c537f40a6b238ef8776310ac4d8688989131e6 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 5 Oct 2021 22:39:16 +0200 Subject: [PATCH 2/7] catalog-backend: use assertError Signed-off-by: Patrik Oldsberg --- .../src/ingestion/processors/UrlReaderProcessor.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/catalog-backend/src/ingestion/processors/UrlReaderProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/UrlReaderProcessor.ts index cf889eb577..6a85cc07c7 100644 --- a/plugins/catalog-backend/src/ingestion/processors/UrlReaderProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/UrlReaderProcessor.ts @@ -16,6 +16,7 @@ import { UrlReader } from '@backstage/backend-common'; import { Entity, LocationSpec } from '@backstage/catalog-model'; +import { assertError } from '@backstage/errors'; import parseGitUrl from 'git-url-parse'; import limiterFactory from 'p-limit'; import { Logger } from 'winston'; @@ -91,6 +92,7 @@ export class UrlReaderProcessor implements CatalogProcessor { }); } } catch (error) { + assertError(error); const message = `Unable to read ${location.type}, ${error}`; if (error.name === 'NotModifiedError' && cacheItem) { for (const parseResult of cacheItem.value) { From 6077d61e735e7dbfda2919be090fb30e28034c76 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 7 Oct 2021 12:17:33 +0200 Subject: [PATCH 3/7] errors: added changeset for new assertion helpers Signed-off-by: Patrik Oldsberg --- .changeset/real-mice-beg.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/real-mice-beg.md diff --git a/.changeset/real-mice-beg.md b/.changeset/real-mice-beg.md new file mode 100644 index 0000000000..bcb4f0ca9a --- /dev/null +++ b/.changeset/real-mice-beg.md @@ -0,0 +1,5 @@ +--- +'@backstage/errors': patch +--- + +Two new helpers have been added that make it easier to migrate to considering thrown errors to be of the type `unknown` in TypeScript. The helpers are `assertError` and `isError`, and can be called to make sure that an unknown value conforms to the shape of an `ErrorLike` object. The `assertError` function is a type-guard that throws in the case of a mismatch, while `isError` returns false. From 9d18bc8ba46c7ed5583544f8d0ca587d37c79eb9 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 7 Oct 2021 23:54:24 +0200 Subject: [PATCH 4/7] errors: added ForwardedError, unknown cause, and unknown fields in ErrorLike Signed-off-by: Patrik Oldsberg --- .changeset/real-mice-beg.md | 2 ++ packages/errors/src/errors/CustomErrorBase.ts | 23 ++++++++++++++----- packages/errors/src/errors/assertion.ts | 1 + packages/errors/src/errors/common.test.ts | 3 ++- packages/errors/src/errors/common.ts | 17 ++++++++++++++ packages/errors/src/errors/index.ts | 1 + 6 files changed, 40 insertions(+), 7 deletions(-) diff --git a/.changeset/real-mice-beg.md b/.changeset/real-mice-beg.md index bcb4f0ca9a..7dde97f6c2 100644 --- a/.changeset/real-mice-beg.md +++ b/.changeset/real-mice-beg.md @@ -3,3 +3,5 @@ --- Two new helpers have been added that make it easier to migrate to considering thrown errors to be of the type `unknown` in TypeScript. The helpers are `assertError` and `isError`, and can be called to make sure that an unknown value conforms to the shape of an `ErrorLike` object. The `assertError` function is a type-guard that throws in the case of a mismatch, while `isError` returns false. + +A new error constructor has also been added, `ForwardedError`, which can be used to add context to a forwarded error. It requires both a message and a cause, and inherits the `name` property from the `cause`. diff --git a/packages/errors/src/errors/CustomErrorBase.ts b/packages/errors/src/errors/CustomErrorBase.ts index a392135c13..4e8674b4dc 100644 --- a/packages/errors/src/errors/CustomErrorBase.ts +++ b/packages/errors/src/errors/CustomErrorBase.ts @@ -14,17 +14,28 @@ * limitations under the License. */ +import { isError } from './assertion'; + /** @public */ export class CustomErrorBase extends Error { readonly cause?: Error; - constructor(message?: string, cause?: Error) { + constructor(message?: string, cause?: Error | unknown) { + let assignedCause: Error | undefined = undefined; + let fullMessage = message; - if (cause) { - if (fullMessage) { - fullMessage += `; caused by ${cause}`; + if (cause !== undefined) { + let causeStr; + if (isError(cause)) { + assignedCause = cause; + causeStr = String(cause); } else { - fullMessage = `caused by ${cause}`; + causeStr = `unknown error '${cause}'`; + } + if (fullMessage) { + fullMessage += `; caused by ${causeStr}`; + } else { + fullMessage = `caused by ${causeStr}`; } } @@ -33,6 +44,6 @@ export class CustomErrorBase extends Error { Error.captureStackTrace?.(this, this.constructor); this.name = this.constructor.name; - this.cause = cause; + this.cause = assignedCause; } } diff --git a/packages/errors/src/errors/assertion.ts b/packages/errors/src/errors/assertion.ts index a0b2d59c11..4e8d939461 100644 --- a/packages/errors/src/errors/assertion.ts +++ b/packages/errors/src/errors/assertion.ts @@ -18,6 +18,7 @@ export type ErrorLike = { name: string; message: string; stack?: string; + [unknownKeys: string]: unknown; }; export function isError(val: unknown): val is ErrorLike { diff --git a/packages/errors/src/errors/common.test.ts b/packages/errors/src/errors/common.test.ts index 232c3e78ef..1b57be2540 100644 --- a/packages/errors/src/errors/common.test.ts +++ b/packages/errors/src/errors/common.test.ts @@ -18,7 +18,8 @@ import * as errors from './common'; describe('common', () => { it('extends Error properly', () => { - for (const [name, E] of Object.entries(errors)) { + const { ForwardedError: _, ...optionalCauseErrors } = { ...errors }; + for (const [name, E] of Object.entries(optionalCauseErrors)) { const error = new E('abcdef'); expect(error.name).toBe(name); expect(error.message).toBe('abcdef'); diff --git a/packages/errors/src/errors/common.ts b/packages/errors/src/errors/common.ts index dad236d4b5..f9300b9371 100644 --- a/packages/errors/src/errors/common.ts +++ b/packages/errors/src/errors/common.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { isError } from './assertion'; import { CustomErrorBase } from './CustomErrorBase'; /* @@ -72,3 +73,19 @@ export class ConflictError extends CustomErrorBase {} * @public */ export class NotModifiedError extends CustomErrorBase {} + +/** + * An error that forwards an underlying cause with additional context in the message. + * + * The `name` property of the error will be inherited from the `cause` if + * possible, and will otherwise be set to `'Error'`. + * + * @public + */ +export class ForwardedError extends CustomErrorBase { + constructor(message: string, cause: Error | unknown) { + super(message, cause); + + this.name = isError(cause) ? this.constructor.name : 'Error'; + } +} diff --git a/packages/errors/src/errors/index.ts b/packages/errors/src/errors/index.ts index 02670f4de9..bc2fb2aa4e 100644 --- a/packages/errors/src/errors/index.ts +++ b/packages/errors/src/errors/index.ts @@ -19,6 +19,7 @@ export type { ErrorLike } from './assertion'; export { AuthenticationError, ConflictError, + ForwardedError, InputError, NotAllowedError, NotFoundError, From 36e67d2f24027f795f277a72e710870e7308360e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 8 Oct 2021 17:41:29 +0200 Subject: [PATCH 5/7] more strict error type checking in most packages and backend plugins Signed-off-by: Patrik Oldsberg --- .changeset/tiny-panthers-jump.md | 22 ++++++++++++++ .../src/database/connectors/postgres.ts | 4 +-- .../src/reading/AwsS3UrlReader.test.ts | 4 +-- .../src/reading/AwsS3UrlReader.ts | 5 ++-- .../src/reading/integration.test.ts | 6 +++- .../src/util/DockerContainerRunner.ts | 6 ++-- packages/cli/package.json | 1 + .../commands/create-plugin/createPlugin.ts | 3 ++ packages/cli/src/commands/index.ts | 2 ++ .../commands/remove-plugin/removePlugin.ts | 8 +++++ packages/cli/src/commands/versions/bump.ts | 5 ++-- packages/cli/src/lib/run.ts | 11 +++++-- packages/config-loader/package.json | 1 + packages/config-loader/src/lib/env.ts | 2 ++ .../config-loader/src/lib/schema/collect.ts | 2 ++ .../config-loader/src/lib/transform/apply.ts | 2 ++ packages/config-loader/src/loader.ts | 5 ++-- .../DiscoveryApi/UrlPatternDiscovery.test.ts | 10 +++---- .../DiscoveryApi/UrlPatternDiscovery.ts | 29 ++++++++++--------- .../src/routing/RoutingProvider.test.tsx | 6 ++-- .../LoginRequestListItem.tsx | 13 +++------ .../src/layout/SignInPage/auth0Provider.tsx | 3 +- .../src/layout/SignInPage/commonProvider.tsx | 3 +- .../src/extensions/extensions.tsx | 18 ++++++++---- packages/create-app/src/createApp.ts | 6 ++-- packages/e2e-test/package.json | 1 + packages/e2e-test/src/lib/helpers.ts | 7 +++-- .../src/stages/generate/helpers.ts | 10 +++++-- .../src/stages/generate/techdocs.ts | 6 ++-- .../techdocs-common/src/stages/prepare/url.ts | 5 ++-- .../src/stages/publish/awsS3.test.ts | 2 +- .../src/stages/publish/awsS3.ts | 10 +++++-- .../stages/publish/azureBlobStorage.test.ts | 4 +-- .../src/stages/publish/azureBlobStorage.ts | 7 ++++- .../src/stages/publish/googleStorage.ts | 3 ++ .../src/stages/publish/local.ts | 2 ++ .../publish/migrations/GoogleMigration.ts | 2 ++ .../src/stages/publish/openStackSwift.ts | 7 ++++- .../src/lib/oauth/OAuthAdapter.ts | 12 ++++---- .../src/providers/saml/provider.ts | 9 +++--- plugins/auth-backend/src/service/router.ts | 3 +- .../catalog-backend-module-ldap/package.json | 1 + .../src/ldap/client.ts | 5 ++-- .../src/database/DefaultProcessingDatabase.ts | 7 +++-- .../processors/AwsS3DiscoveryProcessor.ts | 3 +- .../src/legacy/ingestion/LocationReaders.ts | 2 +- .../DefaultCatalogProcessingEngine.ts | 3 +- .../DefaultCatalogProcessingOrchestrator.ts | 8 ++++- .../processing/ProcessorOutputCollector.ts | 2 ++ plugins/catalog-import/package.json | 1 + .../StepInitAnalyzeUrl/StepInitAnalyzeUrl.tsx | 4 +-- .../StepPrepareCreatePullRequest.tsx | 2 ++ .../StepReviewLocation/StepReviewLocation.tsx | 2 ++ plugins/catalog-react/package.json | 1 + .../UnregisterEntityDialog.tsx | 3 ++ .../DeleteEntityDialog.tsx | 2 ++ .../src/service/CoverageUtils.test.ts | 4 +-- plugins/kubernetes-backend/package.json | 1 + .../cluster-locator/GkeClusterLocator.test.ts | 2 +- .../src/cluster-locator/GkeClusterLocator.ts | 6 ++-- .../actions/builtin/github/githubWebhook.ts | 2 ++ .../src/scaffolder/actions/builtin/helpers.ts | 2 ++ .../actions/builtin/publish/github.ts | 4 +++ .../src/scaffolder/tasks/StorageTaskBroker.ts | 2 ++ .../src/scaffolder/tasks/TaskWorker.ts | 3 ++ .../scaffolder-backend/src/service/helpers.ts | 2 ++ .../src/DocsBuilder/builder.ts | 8 +++-- .../src/service/DocsSynchronizer.ts | 3 +- 68 files changed, 248 insertions(+), 104 deletions(-) create mode 100644 .changeset/tiny-panthers-jump.md diff --git a/.changeset/tiny-panthers-jump.md b/.changeset/tiny-panthers-jump.md new file mode 100644 index 0000000000..d059b826da --- /dev/null +++ b/.changeset/tiny-panthers-jump.md @@ -0,0 +1,22 @@ +--- +'@backstage/backend-common': patch +'@backstage/cli': patch +'@backstage/config-loader': patch +'@backstage/core-app-api': patch +'@backstage/core-components': patch +'@backstage/core-plugin-api': patch +'@backstage/create-app': patch +'@backstage/techdocs-common': patch +'@backstage/plugin-auth-backend': patch +'@backstage/plugin-catalog': patch +'@backstage/plugin-catalog-backend': patch +'@backstage/plugin-catalog-backend-module-ldap': patch +'@backstage/plugin-catalog-import': patch +'@backstage/plugin-catalog-react': patch +'@backstage/plugin-code-coverage-backend': patch +'@backstage/plugin-kubernetes-backend': patch +'@backstage/plugin-scaffolder-backend': patch +'@backstage/plugin-techdocs-backend': patch +--- + +Internal updates to apply more strict checks to throw errors. diff --git a/packages/backend-common/src/database/connectors/postgres.ts b/packages/backend-common/src/database/connectors/postgres.ts index f6e42d1945..62de2e001e 100644 --- a/packages/backend-common/src/database/connectors/postgres.ts +++ b/packages/backend-common/src/database/connectors/postgres.ts @@ -17,6 +17,7 @@ import knexFactory, { Knex } from 'knex'; import { Config } from '@backstage/config'; +import { ForwardedError } from '@backstage/errors'; import { mergeDatabaseConfig } from '../config'; import { DatabaseConnector } from '../types'; import defaultNameOverride from './defaultNameOverride'; @@ -94,8 +95,7 @@ function requirePgConnectionString() { try { return require('pg-connection-string').parse; } catch (e) { - const message = `Postgres: Install 'pg-connection-string'`; - throw new Error(`${message}\n${e.message}`); + throw new ForwardedError("Postgres: Install 'pg-connection-string'", e); } } diff --git a/packages/backend-common/src/reading/AwsS3UrlReader.test.ts b/packages/backend-common/src/reading/AwsS3UrlReader.test.ts index 0bc0e4f97f..6959edcc8b 100644 --- a/packages/backend-common/src/reading/AwsS3UrlReader.test.ts +++ b/packages/backend-common/src/reading/AwsS3UrlReader.test.ts @@ -167,7 +167,7 @@ describe('AwsS3UrlReader', () => { ), ).rejects.toThrow( Error( - `Could not retrieve file from S3: not a valid AWS S3 URL: https://test-bucket.s3.us-east-2.NOTamazonaws.com/file.yaml`, + `Could not retrieve file from S3; caused by Error: not a valid AWS S3 URL: https://test-bucket.s3.us-east-2.NOTamazonaws.com/file.yaml`, ), ); }); @@ -216,7 +216,7 @@ describe('AwsS3UrlReader', () => { ), ).rejects.toThrow( Error( - `Could not retrieve file from S3: not a valid AWS S3 URL: https://test-bucket.s3.us-east-2.NOTamazonaws.com/file.yaml`, + `Could not retrieve file from S3; caused by Error: not a valid AWS S3 URL: https://test-bucket.s3.us-east-2.NOTamazonaws.com/file.yaml`, ), ); }); diff --git a/packages/backend-common/src/reading/AwsS3UrlReader.ts b/packages/backend-common/src/reading/AwsS3UrlReader.ts index cb03ec4f71..7fd0f6ba19 100644 --- a/packages/backend-common/src/reading/AwsS3UrlReader.ts +++ b/packages/backend-common/src/reading/AwsS3UrlReader.ts @@ -27,6 +27,7 @@ import { } from './types'; import getRawBody from 'raw-body'; import { AwsS3Integration, ScmIntegrations } from '@backstage/integration'; +import { ForwardedError } from '@backstage/errors'; import { ListObjectsV2Output, ObjectList } from 'aws-sdk/clients/s3'; const parseURL = ( @@ -162,7 +163,7 @@ export class AwsS3UrlReader implements UrlReader { etag: etag, }; } catch (e) { - throw new Error(`Could not retrieve file from S3: ${e.message}`); + throw new ForwardedError('Could not retrieve file from S3', e); } } @@ -203,7 +204,7 @@ export class AwsS3UrlReader implements UrlReader { return await this.deps.treeResponseFactory.fromReadableArray(responses); } catch (e) { - throw new Error(`Could not retrieve file tree from S3: ${e.message}`); + throw new ForwardedError('Could not retrieve file tree from S3', e); } } diff --git a/packages/backend-common/src/reading/integration.test.ts b/packages/backend-common/src/reading/integration.test.ts index ae557be583..951df29605 100644 --- a/packages/backend-common/src/reading/integration.test.ts +++ b/packages/backend-common/src/reading/integration.test.ts @@ -15,6 +15,7 @@ */ import { ConfigReader } from '@backstage/config'; +import { isError } from '@backstage/errors'; import { getVoidLogger } from '../logging'; import { UrlReaders } from './UrlReaders'; @@ -71,7 +72,10 @@ function withRetries(count: number, fn: () => Promise) { error = err; } } - if (!error.message.match(/rate limit|Too Many Requests/)) { + if ( + isError(error) && + !error.message.match(/rate limit|Too Many Requests/) + ) { throw error; } else { console.warn('Request was rate limited', error); diff --git a/packages/backend-common/src/util/DockerContainerRunner.ts b/packages/backend-common/src/util/DockerContainerRunner.ts index da3ce66ec2..b81bd995ca 100644 --- a/packages/backend-common/src/util/DockerContainerRunner.ts +++ b/packages/backend-common/src/util/DockerContainerRunner.ts @@ -16,6 +16,7 @@ import Docker from 'dockerode'; import fs from 'fs-extra'; +import { ForwardedError } from '@backstage/errors'; import { PassThrough } from 'stream'; import { ContainerRunner, RunContainerOptions } from './ContainerRunner'; @@ -45,8 +46,9 @@ export class DockerContainerRunner implements ContainerRunner { try { await this.dockerClient.ping(); } catch (e) { - throw new Error( - `This operation requires Docker. Docker does not appear to be available. Docker.ping() failed with: ${e.message}`, + throw new ForwardedError( + 'This operation requires Docker. Docker does not appear to be available. Docker.ping() failed with', + e, ); } diff --git a/packages/cli/package.json b/packages/cli/package.json index ecc3d3b4e1..ebd8f0169c 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -31,6 +31,7 @@ "@backstage/cli-common": "^0.1.4", "@backstage/config": "^0.1.10", "@backstage/config-loader": "^0.6.10", + "@backstage/errors": "^0.1.2", "@hot-loader/react-dom": "^16.13.0", "@lerna/package-graph": "^4.0.0", "@lerna/project": "^4.0.0", diff --git a/packages/cli/src/commands/create-plugin/createPlugin.ts b/packages/cli/src/commands/create-plugin/createPlugin.ts index 6ddd1f7160..a5ea565cae 100644 --- a/packages/cli/src/commands/create-plugin/createPlugin.ts +++ b/packages/cli/src/commands/create-plugin/createPlugin.ts @@ -24,6 +24,7 @@ import camelCase from 'lodash/camelCase'; import upperFirst from 'lodash/upperFirst'; import os from 'os'; import { Command } from 'commander'; +import { assertError } from '@backstage/errors'; import { parseOwnerIds, addCodeownersEntry, @@ -173,6 +174,7 @@ async function buildPlugin(pluginFolder: string) { ); }); } catch (error) { + assertError(error); Task.error(error.message); break; } @@ -329,6 +331,7 @@ export default async (cmd: Command) => { Task.log(); Task.exit(); } catch (error) { + assertError(error); Task.error(error.message); Task.log('It seems that something went wrong when creating the plugin 🤔'); diff --git a/packages/cli/src/commands/index.ts b/packages/cli/src/commands/index.ts index d6686f2655..302f76cc5d 100644 --- a/packages/cli/src/commands/index.ts +++ b/packages/cli/src/commands/index.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { assertError } from '@backstage/errors'; import { CommanderStatic } from 'commander'; import { exitWithError } from '../lib/errors'; @@ -252,6 +253,7 @@ function lazy( process.exit(0); } catch (error) { + assertError(error); exitWithError(error); } }; diff --git a/packages/cli/src/commands/remove-plugin/removePlugin.ts b/packages/cli/src/commands/remove-plugin/removePlugin.ts index 9e1e18bb7b..11fdc8bb11 100644 --- a/packages/cli/src/commands/remove-plugin/removePlugin.ts +++ b/packages/cli/src/commands/remove-plugin/removePlugin.ts @@ -20,6 +20,7 @@ import inquirer, { Answers, Question } from 'inquirer'; import { getCodeownersFilePath } from '../../lib/codeowners'; import { paths } from '../../lib/paths'; import { Task } from '../../lib/tasks'; +import { assertError } from '@backstage/errors'; const BACKSTAGE = '@backstage'; @@ -35,6 +36,7 @@ export const checkExists = async (rootDir: string, pluginName: string) => { ); } } catch (e) { + assertError(e); throw new Error( chalk.red( ` There was an error removing plugin ${chalk.cyan(pluginName)}: ${ @@ -51,6 +53,7 @@ export const removePluginDirectory = async (destination: string) => { try { await fse.remove(destination); } catch (e) { + assertError(e); throw Error( chalk.red( ` There was a problem removing the plugin directory: ${e.message}`, @@ -67,6 +70,7 @@ export const removeSymLink = async (destination: string) => { try { await fse.remove(destination); } catch (e) { + assertError(e); throw Error( chalk.red( ` Could not remove symbolic link\t${chalk.cyan(destination)}: ${ @@ -106,6 +110,7 @@ export const removeReferencesFromPluginsFile = async ( try { await removeAllStatementsContainingID(pluginsFile, pluginNameCapitalized); } catch (e) { + assertError(e); throw new Error( chalk.red( ` There was an error removing export statement for plugin ${chalk.cyan( @@ -125,6 +130,7 @@ export const removePluginFromCodeOwners = async ( try { await removeAllStatementsContainingID(codeOwnersFile, pluginName); } catch (e) { + assertError(e); throw new Error( chalk.red( ` There was an error removing code owners statement for plugin ${chalk.cyan( @@ -165,6 +171,7 @@ export const removeReferencesFromAppPackage = async ( 'utf-8', ); } catch (e) { + assertError(e); throw new Error( chalk.red( ` Failed to remove plugin as dependency in app: ${chalk.cyan( @@ -245,6 +252,7 @@ export default async () => { ); Task.log(); } catch (error) { + assertError(error); Task.error(error.message); Task.log('It seems that something went wrong when removing the plugin 🤔'); } diff --git a/packages/cli/src/commands/versions/bump.ts b/packages/cli/src/commands/versions/bump.ts index 257a83e2be..7b967839b2 100644 --- a/packages/cli/src/commands/versions/bump.ts +++ b/packages/cli/src/commands/versions/bump.ts @@ -17,6 +17,7 @@ import fs from 'fs-extra'; import chalk from 'chalk'; import semver from 'semver'; +import { isError } from '@backstage/errors'; import { resolve as resolvePath } from 'path'; import { run } from '../../lib/run'; import { paths } from '../../lib/paths'; @@ -59,7 +60,7 @@ export default async () => { try { target = await findTargetVersion(name); } catch (error) { - if (error.name === 'NotFoundError') { + if (isError(error) && error.name === 'NotFoundError') { console.log(`Package info not found, ignoring package ${name}`); return; } @@ -97,7 +98,7 @@ export default async () => { try { target = await findTargetVersion(name); } catch (error) { - if (error.name === 'NotFoundError') { + if (isError(error) && error.name === 'NotFoundError') { console.log(`Package info not found, ignoring package ${name}`); return; } diff --git a/packages/cli/src/lib/run.ts b/packages/cli/src/lib/run.ts index 7cf22a6df9..4efb791c30 100644 --- a/packages/cli/src/lib/run.ts +++ b/packages/cli/src/lib/run.ts @@ -23,6 +23,7 @@ import { import { ExitCodeError } from './errors'; import { promisify } from 'util'; import { LogFunc } from './logging'; +import { assertError, ForwardedError } from '@backstage/errors'; const execFile = promisify(execFileCb); @@ -75,10 +76,14 @@ export async function runPlain(cmd: string, ...args: string[]) { const { stdout } = await execFile(cmd, args, { shell: true }); return stdout.trim(); } catch (error) { - if (error.stderr) { - process.stderr.write(error.stderr); + assertError(error); + if ('stderr' in error) { + process.stderr.write(error.stderr as Buffer); } - throw new ExitCodeError(error.code, [cmd, ...args].join(' ')); + if (typeof error.code === 'number') { + throw new ExitCodeError(error.code, [cmd, ...args].join(' ')); + } + throw new ForwardedError('Unknown execution error', error); } } diff --git a/packages/config-loader/package.json b/packages/config-loader/package.json index a719c6389b..ebca3ffdf3 100644 --- a/packages/config-loader/package.json +++ b/packages/config-loader/package.json @@ -32,6 +32,7 @@ "dependencies": { "@backstage/cli-common": "^0.1.4", "@backstage/config": "^0.1.9", + "@backstage/errors": "^0.1.2", "@types/json-schema": "^7.0.6", "ajv": "^7.0.3", "chokidar": "^3.5.2", diff --git a/packages/config-loader/src/lib/env.ts b/packages/config-loader/src/lib/env.ts index b244c06c64..57cfc077a7 100644 --- a/packages/config-loader/src/lib/env.ts +++ b/packages/config-loader/src/lib/env.ts @@ -15,6 +15,7 @@ */ import { AppConfig, JsonObject } from '@backstage/config'; +import { assertError } from '@backstage/errors'; const ENV_PREFIX = 'APP_CONFIG_'; @@ -96,6 +97,7 @@ function safeJsonParse(str: string): [Error | null, any] { try { return [null, JSON.parse(str)]; } catch (err) { + assertError(err); return [err, str]; } } diff --git a/packages/config-loader/src/lib/schema/collect.ts b/packages/config-loader/src/lib/schema/collect.ts index f8b134119b..de6aff376f 100644 --- a/packages/config-loader/src/lib/schema/collect.ts +++ b/packages/config-loader/src/lib/schema/collect.ts @@ -24,6 +24,7 @@ import { import { ConfigSchemaPackageEntry } from './types'; import { getProgramFromFiles, generateSchema } from 'typescript-json-schema'; import { JsonObject } from '@backstage/config'; +import { assertError } from '@backstage/errors'; type Item = { name?: string; @@ -189,6 +190,7 @@ function compileTsSchemas(paths: string[]) { [path.split(sep).join('/')], // Unix paths are expected for all OSes here ) as JsonObject | null; } catch (error) { + assertError(error); if (error.message !== 'type Config not found') { throw error; } diff --git a/packages/config-loader/src/lib/transform/apply.ts b/packages/config-loader/src/lib/transform/apply.ts index 690d280266..091f9c23c5 100644 --- a/packages/config-loader/src/lib/transform/apply.ts +++ b/packages/config-loader/src/lib/transform/apply.ts @@ -15,6 +15,7 @@ */ import { JsonObject, JsonValue } from '@backstage/config'; +import { assertError } from '@backstage/errors'; import { TransformFunc } from './types'; import { isObject } from './utils'; @@ -46,6 +47,7 @@ export async function applyConfigTransforms( break; } } catch (error) { + assertError(error); throw new Error(`error at ${path}, ${error.message}`); } } diff --git a/packages/config-loader/src/loader.ts b/packages/config-loader/src/loader.ts index 315b44c4ee..4abcc2e314 100644 --- a/packages/config-loader/src/loader.ts +++ b/packages/config-loader/src/loader.ts @@ -19,6 +19,7 @@ import yaml from 'yaml'; import chokidar from 'chokidar'; import { resolve as resolvePath, dirname, isAbsolute, basename } from 'path'; import { AppConfig } from '@backstage/config'; +import { ForwardedError } from '@backstage/errors'; import { applyConfigTransforms, readEnvConfig, @@ -114,9 +115,7 @@ export async function loadConfig( try { fileConfigs = await loadConfigFiles(); } catch (error) { - throw new Error( - `Failed to read static configuration file, ${error.message}`, - ); + throw new ForwardedError('Failed to read static configuration file', error); } const envConfigs = await readEnvConfig(process.env); diff --git a/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.test.ts b/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.test.ts index 9cd666e8a3..7fcbde97df 100644 --- a/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.test.ts +++ b/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.test.ts @@ -45,25 +45,25 @@ describe('UrlPatternDiscovery', () => { it('should validate that the pattern is a valid URL', () => { expect(() => { UrlPatternDiscovery.compile('example.com'); - }).toThrow('Invalid discovery URL pattern, Invalid URL: example.com'); + }).toThrow("Invalid discovery URL pattern, URL 'example.com' is invalid"); expect(() => { UrlPatternDiscovery.compile('http://'); - }).toThrow('Invalid discovery URL pattern, Invalid URL: http://'); + }).toThrow("Invalid discovery URL pattern, URL 'http://' is invalid"); expect(() => { UrlPatternDiscovery.compile('abc123'); - }).toThrow('Invalid discovery URL pattern, Invalid URL: abc123'); + }).toThrow("Invalid discovery URL pattern, URL 'abc123' is invalid"); expect(() => { UrlPatternDiscovery.compile('http://example.com:{{pluginId}}'); }).toThrow( - 'Invalid discovery URL pattern, Invalid URL: http://example.com:pluginId', + "Invalid discovery URL pattern, URL 'http://example.com:pluginId' is invalid", ); expect(() => { UrlPatternDiscovery.compile('/{{pluginId}}'); - }).toThrow('Invalid discovery URL pattern, Invalid URL: /pluginId'); + }).toThrow("Invalid discovery URL pattern, URL '/pluginId' is invalid"); expect(() => { UrlPatternDiscovery.compile('http://localhost/{{pluginId}}?forbidden'); diff --git a/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.ts b/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.ts index 81cc5a26b1..58e14c9f04 100644 --- a/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.ts +++ b/packages/core-app-api/src/apis/implementations/DiscoveryApi/UrlPatternDiscovery.ts @@ -16,6 +16,8 @@ import { DiscoveryApi } from '@backstage/core-plugin-api'; +const ERROR_PREFIX = 'Invalid discovery URL pattern,'; + /** * UrlPatternDiscovery is a lightweight DiscoveryApi implementation. * It uses a single template string to construct URLs for each plugin. @@ -30,21 +32,22 @@ export class UrlPatternDiscovery implements DiscoveryApi { */ static compile(pattern: string): UrlPatternDiscovery { const parts = pattern.split(/\{\{\s*pluginId\s*\}\}/); + const urlStr = parts.join('pluginId'); + let url; try { - const urlStr = parts.join('pluginId'); - const url = new URL(urlStr); - if (url.hash) { - throw new Error('URL must not have a hash'); - } - if (url.search) { - throw new Error('URL must not have a query'); - } - if (urlStr.endsWith('/')) { - throw new Error('URL must not end with a slash'); - } - } catch (error) { - throw new Error(`Invalid discovery URL pattern, ${error.message}`); + url = new URL(urlStr); + } catch { + throw new Error(`${ERROR_PREFIX} URL '${urlStr}' is invalid`); + } + if (url.hash) { + throw new Error(`${ERROR_PREFIX} URL must not have a hash`); + } + if (url.search) { + throw new Error(`${ERROR_PREFIX} URL must not have a query`); + } + if (urlStr.endsWith('/')) { + throw new Error(`${ERROR_PREFIX} URL must not end with a slash`); } return new UrlPatternDiscovery(parts); diff --git a/packages/core-app-api/src/routing/RoutingProvider.test.tsx b/packages/core-app-api/src/routing/RoutingProvider.test.tsx index 12d9e9ee33..fbdd34ae68 100644 --- a/packages/core-app-api/src/routing/RoutingProvider.test.tsx +++ b/packages/core-app-api/src/routing/RoutingProvider.test.tsx @@ -83,7 +83,7 @@ const MockRouteSource = (props: { } catch (ex) { return (
- Error at {props.name}: {ex.message} + Error at {props.name}, {String(ex)}
); } @@ -293,12 +293,12 @@ describe('discovery', () => { expect( rendered.getByText( - `Error at outsideWithParams: Cannot route to ${ref3} with parent ${ref5} as it has parameters`, + `Error at outsideWithParams, Error: Cannot route to ${ref3} with parent ${ref5} as it has parameters`, ), ).toBeInTheDocument(); expect( rendered.getByText( - `Error at outsideNoParams: Cannot route to ${ref3} with parent ${ref5} as it has parameters`, + `Error at outsideNoParams, Error: Cannot route to ${ref3} with parent ${ref5} as it has parameters`, ), ).toBeInTheDocument(); }); diff --git a/packages/core-components/src/components/OAuthRequestDialog/LoginRequestListItem.tsx b/packages/core-components/src/components/OAuthRequestDialog/LoginRequestListItem.tsx index b911a9a1d9..ef0d2373e4 100644 --- a/packages/core-components/src/components/OAuthRequestDialog/LoginRequestListItem.tsx +++ b/packages/core-components/src/components/OAuthRequestDialog/LoginRequestListItem.tsx @@ -21,6 +21,7 @@ import ListItemText from '@material-ui/core/ListItemText'; import Typography from '@material-ui/core/Typography'; import Button from '@material-ui/core/Button'; import React, { useState } from 'react'; +import { isError } from '@backstage/errors'; import { PendingAuthRequest } from '@backstage/core-plugin-api'; export type LoginRequestListItemClassKey = 'root'; @@ -42,14 +43,14 @@ type RowProps = { const LoginRequestListItem = ({ request, busy, setBusy }: RowProps) => { const classes = useItemStyles(); - const [error, setError] = useState(); + const [error, setError] = useState(); const handleContinue = async () => { setBusy(true); try { await request.trigger(); } catch (e) { - setError(e); + setError(isError(e) ? e.message : 'An unspecified error occurred'); } finally { setBusy(false); } @@ -64,13 +65,7 @@ const LoginRequestListItem = ({ request, busy, setBusy }: RowProps) => { - {error.message || 'An unspecified error occurred'} - - ) - } + secondary={error && {error}} />