fix: convert remaining hard errors to soft diagnostics for logicalId validation [red-00]

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', () => {

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', () => {

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', () => {

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'),
+      }),
+    ]))
+  })
 })

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', () => {

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<void> {
+    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',

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<void> {
     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) {