diff --git a/.changeset/tough-buckets-listen.md b/.changeset/tough-buckets-listen.md new file mode 100644 index 000000000..e0cec0d26 --- /dev/null +++ b/.changeset/tough-buckets-listen.md @@ -0,0 +1,5 @@ +--- +"@logto/app-insights": patch +--- + +fix JSON stringify circular structure issue diff --git a/packages/app-insights/jest.config.js b/packages/app-insights/jest.config.js new file mode 100644 index 000000000..5656d3347 --- /dev/null +++ b/packages/app-insights/jest.config.js @@ -0,0 +1,13 @@ +/** @type {import('jest').Config} */ +const config = { + transform: {}, + coveragePathIgnorePatterns: ['/node_modules/', '/src/__mocks__/'], + coverageReporters: ['text-summary', 'lcov'], + coverageProvider: 'v8', + roots: ['./lib'], + moduleNameMapper: { + '^(chalk|inquirer)$': '/../shared/lib/esm/module-proxy.js', + }, +}; + +export default config; diff --git a/packages/app-insights/package.json b/packages/app-insights/package.json index 7064ae0b2..af3ff3024 100644 --- a/packages/app-insights/package.json +++ b/packages/app-insights/package.json @@ -24,10 +24,12 @@ "scripts": { "precommit": "lint-staged", "build": "rm -rf lib/ && tsc -p tsconfig.build.json", - "build:test": "pnpm build", + "build:test": "rm -rf lib/ && tsc -p tsconfig.test.json --sourcemap", "dev": "tsc -p tsconfig.build.json --watch --preserveWatchOutput --incremental", "lint": "eslint --ext .ts src", "lint:report": "pnpm lint --format json --output-file report.json", + "test:only": "NODE_OPTIONS=--experimental-vm-modules jest", + "test": "pnpm build:test && pnpm test:only", "prepack": "pnpm build" }, "devDependencies": { @@ -35,10 +37,12 @@ "@silverhand/eslint-config-react": "3.0.1", "@silverhand/ts-config": "3.0.0", "@silverhand/ts-config-react": "3.0.0", + "@types/jest": "^29.4.0", "@types/node": "^18.11.18", "@types/react": "^18.0.31", "eslint": "^8.34.0", "history": "^5.3.0", + "jest": "^29.5.0", "lint-staged": "^13.0.0", "prettier": "^2.8.2", "react": "^18.0.0", diff --git a/packages/app-insights/src/node.ts b/packages/app-insights/src/node.ts index cc010f76e..5ea63fb74 100644 --- a/packages/app-insights/src/node.ts +++ b/packages/app-insights/src/node.ts @@ -1,33 +1,7 @@ import { trySafe } from '@silverhand/essentials'; import type { TelemetryClient } from 'applicationinsights'; -export const normalizeError = (error: unknown) => { - const errorObject = error instanceof Error ? error : new Error(String(error)); - - /** - * - Ensure the message if not empty otherwise Application Insights will respond 400 - * and the error will not be recorded. - * - We stringify error object here since other error properties won't show on the - * ApplicationInsights details page. - */ - const message = JSON.stringify( - errorObject, - // ApplicationInsights shows call stack, no need to stringify - Object.getOwnPropertyNames(errorObject).filter((value) => value !== 'stack') - ); - - // Ensure we don't mutate the original error - const normalized = new Error(message); - - // Manually clone key fields of the error for AppInsights display - /* eslint-disable @silverhand/fp/no-mutation */ - normalized.name = errorObject.name; - normalized.stack = errorObject.stack; - normalized.cause = errorObject.cause; - /* eslint-enable @silverhand/fp/no-mutation */ - - return normalized; -}; +import { normalizeError } from './normalize-error.js'; class AppInsights { client?: TelemetryClient; @@ -55,7 +29,8 @@ class AppInsights { return true; } - trackException(error: unknown) { + /** The function is async to avoid blocking the main script and force the use of `await` or `void`. */ + async trackException(error: unknown) { this.client?.trackException({ exception: normalizeError(error) }); } } diff --git a/packages/app-insights/src/normalize-error.test.ts b/packages/app-insights/src/normalize-error.test.ts new file mode 100644 index 000000000..798d7cac6 --- /dev/null +++ b/packages/app-insights/src/normalize-error.test.ts @@ -0,0 +1,47 @@ +import { normalizeError } from './normalize-error.js'; + +describe('normalizeError()', () => { + it('should create a new `Error` object with the stringified error as the message', () => { + class CustomError extends Error { + name = 'CustomError'; + foo = 'bar'; + } + + const error = new CustomError('test'); + const normalized = normalizeError(error); + expect(normalized).not.toBe(error); + expect(normalized.message).toBe('{"message":"test","name":"CustomError","foo":"bar"}'); + }); + + it('should use the string constructor if the error is not an `Error` instance', () => { + const error = 'test'; + const normalized = normalizeError(error); + expect(normalized).not.toBe(error); + expect(normalized.message).toBe('"test"'); + }); + + it('should be able to handle circular references and stringify the "stack" key for non-error objects', () => { + const circularObject: { message: string; circular?: unknown; stack: string } = { + message: 'test', + stack: 'foo', + }; + // eslint-disable-next-line @silverhand/fp/no-mutation + circularObject.circular = circularObject; + + const normalized = normalizeError(circularObject); + expect(normalized).not.toBe(circularObject); + expect(normalized.message).toBe('{"message":"test","stack":"foo","circular":"[Circular ~]"}'); + }); + + it('should be able to handle circular error classes', () => { + class CircularError extends Error { + name = 'CircularError'; + circular = this; + } + + const circularError = new CircularError(); + const normalized = normalizeError(circularError); + expect(normalized).not.toBe(circularError); + expect(normalized.message).toBe('{"name":"CircularError","circular":"[Circular ~]"}'); + }); +}); diff --git a/packages/app-insights/src/normalize-error.ts b/packages/app-insights/src/normalize-error.ts new file mode 100644 index 000000000..36799b9ed --- /dev/null +++ b/packages/app-insights/src/normalize-error.ts @@ -0,0 +1,97 @@ +import { trySafe } from '@silverhand/essentials'; + +const transformedKey = Symbol('Indicates an object is transformed from an `Error` instance'); + +// eslint-disable-next-line @typescript-eslint/ban-types +const isObject = (value: unknown): value is object => typeof value === 'object' && value !== null; + +// Edited from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#circular_references +function getCircularReplacer() { + const ancestors: unknown[] = []; + const transformedMap = new WeakMap(); + + const transformErrorValue = (value: unknown) => { + // Special handling for `Error` instances since they have non-enumerable properties + if (value instanceof Error) { + if (!transformedMap.has(value)) { + const transformed: Record = {}; + + for (const key of Object.getOwnPropertyNames(value)) { + // @ts-expect-error getOwnPropertyNames() returns the valid keys + // eslint-disable-next-line @silverhand/fp/no-mutation + transformed[key] = value[key]; + } + + // eslint-disable-next-line @silverhand/fp/no-mutation + transformed[transformedKey] = true; + + transformedMap.set(value, transformed); + } + + return transformedMap.get(value); + } + + return value; + }; + + return function (this: unknown, key: string, value: unknown) { + // Ignore `stack` property since ApplicationInsights will show it + if ( + isObject(this) && + Object.prototype.hasOwnProperty.call(this, transformedKey) && + key === 'stack' + ) { + return; + } + + if (!isObject(value)) { + return value; + } + + // `this` is the object that value is contained in, + // i.e., its direct parent. + while (ancestors.length > 0 && ancestors.at(-1) !== this) { + // eslint-disable-next-line @silverhand/fp/no-mutating-methods + ancestors.pop(); + } + + const transformed = transformErrorValue(value); + + if (ancestors.includes(transformed)) { + return '[Circular ~]'; + } + + // eslint-disable-next-line @silverhand/fp/no-mutating-methods + ancestors.push(transformed); + return transformed; + }; +} + +/** + * Clone and stringify error object for ApplicationInsights. The stringified result + * will be used as the error message. This is necessary because ApplicationInsights + * only shows limited error properties. + */ +export const normalizeError = (error: unknown) => { + /** + * - Ensure the message if not empty otherwise Application Insights will respond 400 + * and the error will not be recorded. + * - We stringify error object here since other error properties won't show on the + * ApplicationInsights details page. + */ + const message = trySafe(() => JSON.stringify(error, getCircularReplacer())) ?? 'Unknown error'; + + // Ensure we don't mutate the original error + const normalized = new Error(message); + + if (error instanceof Error) { + // Manually clone key fields of the error for AppInsights display + /* eslint-disable @silverhand/fp/no-mutation */ + normalized.name = error.name; + normalized.stack = error.stack; + normalized.cause = error.cause; + /* eslint-enable @silverhand/fp/no-mutation */ + } + + return normalized; +}; diff --git a/packages/app-insights/tsconfig.json b/packages/app-insights/tsconfig.json index dbfcba652..83eac9c0a 100644 --- a/packages/app-insights/tsconfig.json +++ b/packages/app-insights/tsconfig.json @@ -2,7 +2,7 @@ "extends": "@silverhand/ts-config-react/tsconfig.base", "compilerOptions": { "outDir": "lib", - "types": ["node"] + "types": ["node", "jest"], }, "include": [ "src" diff --git a/packages/app-insights/tsconfig.test.json b/packages/app-insights/tsconfig.test.json new file mode 100644 index 000000000..f30817b04 --- /dev/null +++ b/packages/app-insights/tsconfig.test.json @@ -0,0 +1,8 @@ +{ + "extends": "./tsconfig.build", + "compilerOptions": { + "isolatedModules": false, + "allowJs": true, + }, + "include": ["src"] +} diff --git a/packages/cloud/src/middleware/with-error-report.ts b/packages/cloud/src/middleware/with-error-report.ts index 0972ac636..f2e6ced3f 100644 --- a/packages/cloud/src/middleware/with-error-report.ts +++ b/packages/cloud/src/middleware/with-error-report.ts @@ -8,7 +8,7 @@ import type { BaseContext, NextFunction } from '@withtyped/server'; export default function withErrorReport() { return async (context: InputContext, next: NextFunction) => { await tryThat(next(context), (error) => { - appInsights.trackException(error); + void appInsights.trackException(error); throw error; }); }; diff --git a/packages/core/src/app/init.ts b/packages/core/src/app/init.ts index 51dac8475..5dcdb9823 100644 --- a/packages/core/src/app/init.ts +++ b/packages/core/src/app/init.ts @@ -39,7 +39,7 @@ export default async function initApp(app: Koa): Promise { const tenant = await trySafe(tenantPool.get(tenantId), (error) => { ctx.status = error instanceof TenantNotFoundError ? 404 : 500; - appInsights.trackException(error); + void appInsights.trackException(error); }); if (!tenant) { @@ -52,7 +52,7 @@ export default async function initApp(app: Koa): Promise { tenant.requestEnd(); } catch (error: unknown) { tenant.requestEnd(); - appInsights.trackException(error); + void appInsights.trackException(error); throw error; } diff --git a/packages/core/src/caches/index.ts b/packages/core/src/caches/index.ts index 448557eba..4b8612017 100644 --- a/packages/core/src/caches/index.ts +++ b/packages/core/src/caches/index.ts @@ -18,7 +18,7 @@ export class RedisCache implements CacheStore { url: conditional(!yes(redisUrl) && redisUrl), }); this.client.on('error', (error) => { - appInsights.trackException(error); + void appInsights.trackException(error); }); } } diff --git a/packages/core/src/middleware/koa-error-handler.ts b/packages/core/src/middleware/koa-error-handler.ts index d904e8f97..4081a1f9f 100644 --- a/packages/core/src/middleware/koa-error-handler.ts +++ b/packages/core/src/middleware/koa-error-handler.ts @@ -21,7 +21,7 @@ export default function koaErrorHandler(): Middleware< } // Report all exceptions to ApplicationInsights - appInsights.trackException(error); + void appInsights.trackException(error); if (error instanceof RequestError) { ctx.status = error.status; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 063d457a5..f7cfa0137 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -67,6 +67,9 @@ importers: '@silverhand/ts-config-react': specifier: 3.0.0 version: 3.0.0(typescript@5.0.2) + '@types/jest': + specifier: ^29.4.0 + version: 29.4.0 '@types/node': specifier: ^18.11.18 version: 18.11.18 @@ -79,6 +82,9 @@ importers: history: specifier: ^5.3.0 version: 5.3.0 + jest: + specifier: ^29.5.0 + version: 29.5.0(@types/node@18.11.18) lint-staged: specifier: ^13.0.0 version: 13.0.0 @@ -9318,7 +9324,7 @@ packages: /@types/is-ci@3.0.0: resolution: {integrity: sha512-Q0Op0hdWbYd1iahB+IFNQcWXFq4O0Q5MwQP7uN0souuQ4rPg1vEYcnIOfr1gY+M+6rc8FGoRaBO1mOOvL29sEQ==} dependencies: - ci-info: 3.5.0 + ci-info: 3.8.0 dev: true /@types/istanbul-lib-coverage@2.0.4: @@ -10632,10 +10638,6 @@ packages: mitt: 3.0.0 dev: true - /ci-info@3.5.0: - resolution: {integrity: sha512-yH4RezKOGlOhxkmhbeNuC4eYZKAUsEaGtBuBzDDP1eFUKiccDWzBABxBfOx31IDwDIXMTxWuwAxUGModvkbuVw==} - dev: true - /ci-info@3.8.0: resolution: {integrity: sha512-eXTggHWSooYhq49F2opQhuHWgzucfF2YgODK4e1566GQs5BIfP30B0oenwBJHfWxAs2fyPB1s7Mg949zLf61Yw==} engines: {node: '>=8'} @@ -11289,6 +11291,17 @@ packages: /dayjs@1.11.6: resolution: {integrity: sha512-zZbY5giJAinCG+7AGaw0wIhNZ6J8AhWuSXKvuc1KAyMiRsvGQWqh4L+MomvhdAYjN+lqvVCMq1I41e3YHvXkyQ==} + /debug@3.2.7: + resolution: {integrity: sha512-CFjzYYAi4ThfiQvizrFQevTTXHtnCqWfe7x1AhgEscTz6ZbLbfoLRLPugTQyBth6f8ZERVUSyWHFD/7Wu4t1XQ==} + peerDependencies: + supports-color: '*' + peerDependenciesMeta: + supports-color: + optional: true + dependencies: + ms: 2.1.3 + dev: true + /debug@3.2.7(supports-color@5.5.0): resolution: {integrity: sha512-CFjzYYAi4ThfiQvizrFQevTTXHtnCqWfe7x1AhgEscTz6ZbLbfoLRLPugTQyBth6f8ZERVUSyWHFD/7Wu4t1XQ==} peerDependencies: @@ -11830,7 +11843,7 @@ packages: /eslint-import-resolver-node@0.3.7: resolution: {integrity: sha512-gozW2blMLJCeFpBwugLTGyvVjNoeo1knonXAcatC6bjPBZitotxdWf7Gimr25N4c0AAOo4eOUfaG82IJPDpqCA==} dependencies: - debug: 3.2.7(supports-color@5.5.0) + debug: 3.2.7 is-core-module: 2.11.0 resolve: 1.22.1 transitivePeerDependencies: @@ -11879,7 +11892,7 @@ packages: optional: true dependencies: '@typescript-eslint/parser': 5.57.0(eslint@8.34.0)(typescript@5.0.2) - debug: 3.2.7(supports-color@5.5.0) + debug: 3.2.7 eslint: 8.34.0 eslint-import-resolver-node: 0.3.7 eslint-import-resolver-typescript: 3.5.4(eslint-plugin-import@2.27.5)(eslint@8.34.0) @@ -11931,7 +11944,7 @@ packages: array-includes: 3.1.6 array.prototype.flat: 1.3.1 array.prototype.flatmap: 1.3.1 - debug: 3.2.7(supports-color@5.5.0) + debug: 3.2.7 doctrine: 2.1.0 eslint: 8.34.0 eslint-import-resolver-node: 0.3.7 @@ -12579,12 +12592,12 @@ packages: resolution: {integrity: sha512-dm9s5Pw7Jc0GvMYbshN6zchCA9RgQlzzEZX3vylR9IqFfS8XciblUXOKfW6SiuJ0e13eDYZoZV5wdrev7P3Nwg==} engines: {node: ^10.12.0 || >=12.0.0} dependencies: - flatted: 3.2.5 + flatted: 3.2.7 rimraf: 3.0.2 dev: true - /flatted@3.2.5: - resolution: {integrity: sha512-WIWGi2L3DyTUvUrwRKgGi9TwxQMUEqPOPQBVi71R96jZXJdFskXEmf54BoZaS1kknGODoIGASGEzBUYdyMCBJg==} + /flatted@3.2.7: + resolution: {integrity: sha512-5nqDSxl8nn5BSNxyR3n4I6eDmbolI6WT+QqR547RwxQapgjQBmtktdP+HTBb/a/zLsbzERTONyUB5pefh5TtjQ==} dev: true /follow-redirects@1.15.1: @@ -13595,7 +13608,7 @@ packages: resolution: {integrity: sha512-ZYvCgrefwqoQ6yTyYUbQu64HsITZ3NfKX1lzaEYdkTDcfKzzCI/wthRRYKkdjHKFVgNiXKAKm65Zo1pk2as/QQ==} hasBin: true dependencies: - ci-info: 3.5.0 + ci-info: 3.8.0 dev: true /is-core-module@2.11.0: