diff --git a/packages/cli/e2e/__tests__/test.spec.ts b/packages/cli/e2e/__tests__/test.spec.ts index 4a35fba04..5fc2f9eb2 100644 --- a/packages/cli/e2e/__tests__/test.spec.ts +++ b/packages/cli/e2e/__tests__/test.spec.ts @@ -168,8 +168,8 @@ describe('test', { timeout: 45000 }, () => { await runTest(fixt, []) } catch (err) { if (err instanceof ExecaError) { - expect((err.stderr as unknown as string).replace(/(\n {4})/gm, '')) - .toContain('Error: Resource of type \'check-group\' with logical id \'my-check-group\' already exists.') + expect((err.stdout as unknown as string).replace(/(\n {4})/gm, '')) + .toContain('A check-group with logicalId "my-check-group" already exists.') expect(err.exitCode).toBe(1) } else { throw err diff --git a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts index 0456ad337..176625738 100644 --- a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts +++ b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts @@ -18,19 +18,24 @@ class TestAlertChannel extends AlertChannel { } describe('AlertChannel', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new TestAlertChannel('foo', { - }) - } + new TestAlertChannel('foo', {}) + new TestAlertChannel('foo', {}) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/check-group.spec.ts b/packages/cli/src/constructs/__tests__/check-group.spec.ts index 04cf633b7..6be63d1c3 100644 --- a/packages/cli/src/constructs/__tests__/check-group.spec.ts +++ b/packages/cli/src/constructs/__tests__/check-group.spec.ts @@ -9,20 +9,28 @@ describe('CheckGroup', () => { expect(CheckGroupV1).toBe(CheckGroup) }) - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new CheckGroupV1('foo', { - name: 'Test', - }) - } + new CheckGroupV1('foo', { + name: 'Test', + }) + new CheckGroupV1('foo', { + name: 'Test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { @@ -79,20 +87,28 @@ describe('CheckGroup', () => { }) describe('v2', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new CheckGroupV2('foo', { - name: 'Test', - }) - } + new CheckGroupV2('foo', { + name: 'Test', + }) + new CheckGroupV2('foo', { + name: 'Test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/private-location.spec.ts b/packages/cli/src/constructs/__tests__/private-location.spec.ts index 08fb6ec3c..8080a91d8 100644 --- a/packages/cli/src/constructs/__tests__/private-location.spec.ts +++ b/packages/cli/src/constructs/__tests__/private-location.spec.ts @@ -4,21 +4,30 @@ import { PrivateLocation, Diagnostics } from '../index' import { Project, Session } from '../project' describe('PrivateLocation', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new PrivateLocation('foo', { - name: 'Test', - slugName: 'test', - }) - } + new PrivateLocation('foo', { + name: 'Test', + slugName: 'test', + }) + new PrivateLocation('foo', { + name: 'Test', + slugName: 'test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/project.spec.ts b/packages/cli/src/constructs/__tests__/project.spec.ts index e36f3bf68..af3cbf833 100644 --- a/packages/cli/src/constructs/__tests__/project.spec.ts +++ b/packages/cli/src/constructs/__tests__/project.spec.ts @@ -1,11 +1,11 @@ import { describe, it, expect, beforeEach } from 'vitest' -import { Session } from '../project' +import { Project, Session } from '../project' import { Construct } from '../construct' import { Diagnostics } from '../diagnostics' class TestConstruct extends Construct { - constructor (logicalId: string) { + constructor (logicalId: any) { super('test', logicalId) } @@ -74,4 +74,35 @@ describe('Construct logicalId validation', () => { }), ])) }) + + it('should produce a diagnostic for non-string logicalId instead of throwing', async () => { + const construct = new TestConstruct(123 as any) + expect(construct.logicalId).toBe('123') + const diagnostics = new Diagnostics() + await construct.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('Expected a string but received type "number"'), + }), + ])) + }) + + it('should produce a diagnostic for duplicate logicalId instead of throwing', async () => { + Session.reset() + const project = new Project('test-project', { name: 'Test' }) + Session.project = project + + project.addResource('check', 'duplicate-id', new TestConstruct('duplicate-id')) + project.addResource('check', 'duplicate-id', new TestConstruct('duplicate-id')) + + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('A check with logicalId "duplicate-id" already exists'), + }), + ])) + }) }) diff --git a/packages/cli/src/constructs/__tests__/status-page-service.spec.ts b/packages/cli/src/constructs/__tests__/status-page-service.spec.ts index f913195f8..704a095d7 100644 --- a/packages/cli/src/constructs/__tests__/status-page-service.spec.ts +++ b/packages/cli/src/constructs/__tests__/status-page-service.spec.ts @@ -4,20 +4,28 @@ import { StatusPageService, Diagnostics } from '../index' import { Project, Session } from '../project' describe('StatusPageService', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new StatusPageService('foo', { - name: 'foo', - }) - } + new StatusPageService('foo', { + name: 'foo', + }) + new StatusPageService('foo', { + name: 'foo', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/construct.ts b/packages/cli/src/constructs/construct.ts index 48b1be679..a16ebe7ef 100644 --- a/packages/cli/src/constructs/construct.ts +++ b/packages/cli/src/constructs/construct.ts @@ -56,6 +56,7 @@ export abstract class Construct implements Validate, Bundle { member: boolean /** Absolute path to the check file that created this construct */ checkFileAbsolutePath?: string + private _originalLogicalIdType: string /** * Creates a new construct instance. @@ -66,7 +67,8 @@ export abstract class Construct implements Validate, Bundle { * @param member Whether this construct is a member of the project */ constructor (type: string, logicalId: string, physicalId?: string | number, member?: boolean) { - this.logicalId = logicalId + this._originalLogicalIdType = typeof logicalId + this.logicalId = typeof logicalId === 'string' ? logicalId : String(logicalId) this.type = type this.physicalId = physicalId this.member = member ?? true @@ -127,6 +129,12 @@ export abstract class Construct implements Validate, Bundle { */ // eslint-disable-next-line require-await async validate (diagnostics: Diagnostics): Promise { + if (this._originalLogicalIdType !== 'string') { + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`Expected a string but received type "${this._originalLogicalIdType}".`), + )) + } if (!LOGICAL_ID_PATTERN.test(this.logicalId)) { diagnostics.add(new InvalidPropertyValueDiagnostic( 'logicalId', diff --git a/packages/cli/src/constructs/project.ts b/packages/cli/src/constructs/project.ts index 8cf1f8e34..194693103 100644 --- a/packages/cli/src/constructs/project.ts +++ b/packages/cli/src/constructs/project.ts @@ -4,7 +4,6 @@ import * as api from '../rest/api' import { CheckConfigDefaults } from '../services/checkly-config-loader' import { Parser } from '../services/check-parser/parser' import { Construct } from './construct' -import { ValidationError } from './validator-error' import { Check, AlertChannelSubscription, AlertChannel, CheckGroup, MaintenanceWindow, Dashboard, @@ -62,6 +61,7 @@ export class Project extends Construct { repoUrl?: string logicalId: string testOnlyAllowed = false + duplicateResources: Array<{ type: string, logicalId: string }> = [] data: ProjectData = { 'check': {}, 'check-group': {}, @@ -98,6 +98,13 @@ export class Project extends Construct { async validate (diagnostics: Diagnostics): Promise { await super.validate(diagnostics) + for (const { type, logicalId } of this.duplicateResources) { + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`A ${type} with logicalId "${logicalId}" already exists.`), + )) + } + if (!this.name) { diagnostics.add(new InvalidPropertyValueDiagnostic( 'name', @@ -134,7 +141,8 @@ export class Project extends Construct { return } - throw new Error(`Resource of type '${type}' with logical id '${logicalId}' already exists.`) + this.duplicateResources.push({ type, logicalId }) + return } this.data[type as keyof ProjectData][logicalId] = resource @@ -324,10 +332,6 @@ export class Session { } static validateCreateConstruct (construct: Construct) { - if (typeof construct.logicalId !== 'string') { - throw new ValidationError(`The "logicalId" of a ${construct.type} construct must be a string (logicalId=${construct.logicalId} [${typeof construct.logicalId}])`) - } - if (construct.type === Project.__checklyType) { // Creating the construct is allowed - We're creating the project. } else if (Session.project) {