fix: validate logicalId via diagnostics instead of throwing at construction time [red-71]

packages/cli/e2e/__tests__/deploy.spec.ts

@@ -8,7 +8,7 @@ import { DateTime, Duration } from 'luxon'
 import { describe, it, expect, beforeAll, beforeEach, afterAll, afterEach } from 'vitest'
 
 import Projects from '../../src/rest/projects'
-import CheckTypes from '../../src/constants'
+import { CheckTypes } from '../../src/constants'
 import { FixtureSandbox, RunOptions } from '../../src/testing/fixture-sandbox'
 import { ExecaError } from 'execa'
 

packages/cli/src/commands/import/plan.ts

@@ -8,6 +8,7 @@ import chalk from 'chalk'
 import logSymbols from 'log-symbols'
 import { validate as validateUuid } from 'uuid'
 
+import { LOGICAL_ID_PATTERN } from '../../constants'
 import * as api from '../../rest/api'
 import { AuthCommand } from '../authCommand'
 import commonMessages from '../../messages/common-messages'
@@ -526,7 +527,7 @@ ${chalk.cyan('For safety, resources are not deletable until the plan has been co
         message: 'How would you like your project to be identified?',
         initial: suggested,
         validate: input => {
-          if (!/^[A-Za-z0-9_\-/#.]+$/.test(input)) {
+          if (!LOGICAL_ID_PATTERN.test(input)) {
             return `Please only use ASCII letters, numbers, and the `
               + `symbols _, -, /, #, and .`
           }

packages/cli/src/constants.ts

@@ -1,8 +1,8 @@
-const CheckTypes = {
+export const CheckTypes = {
   API: 'API',
   BROWSER: 'BROWSER',
   HEARTBEAT: 'HEARTBEAT',
   MULTI_STEP: 'MULTI_STEP',
 }
 
-export default CheckTypes
+export const LOGICAL_ID_PATTERN = /^[A-Za-z0-9_\-/#.]+$/

packages/cli/src/constructs/__tests__/project.spec.ts

@@ -0,0 +1,77 @@
+import { describe, it, expect, beforeEach } from 'vitest'
+
+import { Session } from '../project'
+import { Construct } from '../construct'
+import { Diagnostics } from '../diagnostics'
+
+class TestConstruct extends Construct {
+  constructor (logicalId: string) {
+    super('test', logicalId)
+  }
+
+  describe (): string {
+    return `Test:${this.logicalId}`
+  }
+
+  synthesize () {
+    return null
+  }
+}
+
+describe('Construct logicalId validation', () => {
+  beforeEach(() => {
+    Session.reset()
+    Session.project = { addResource: () => {} } as any
+  })
+
+  it('should pass validation for valid logicalIds', async () => {
+    const validIds = ['my-check', 'my_check', 'myCheck123', 'my/check', 'my#check', 'my.check']
+    for (const id of validIds) {
+      const construct = new TestConstruct(id)
+      const diagnostics = new Diagnostics()
+      await construct.validate(diagnostics)
+      expect(diagnostics.isFatal(), `Expected "${id}" to be valid`).toBe(false)
+    }
+  })
+
+  it('should store the original logicalId without modification', () => {
+    const construct = new TestConstruct('my check')
+    expect(construct.logicalId).toBe('my check')
+  })
+
+  it('should produce an error diagnostic for logicalIds with spaces', async () => {
+    const construct = new TestConstruct('my check')
+    const diagnostics = new Diagnostics()
+    await construct.validate(diagnostics)
+    expect(diagnostics.isFatal()).toBe(true)
+    expect(diagnostics.observations).toEqual(expect.arrayContaining([
+      expect.objectContaining({
+        message: expect.stringContaining('contains invalid characters'),
+      }),
+    ]))
+  })
+
+  it('should produce an error diagnostic for logicalIds with special characters', async () => {
+    const construct = new TestConstruct('my@check!')
+    const diagnostics = new Diagnostics()
+    await construct.validate(diagnostics)
+    expect(diagnostics.isFatal()).toBe(true)
+    expect(diagnostics.observations).toEqual(expect.arrayContaining([
+      expect.objectContaining({
+        message: expect.stringContaining('contains invalid characters'),
+      }),
+    ]))
+  })
+
+  it('should produce an error diagnostic for empty logicalIds', async () => {
+    const construct = new TestConstruct('')
+    const diagnostics = new Diagnostics()
+    await construct.validate(diagnostics)
+    expect(diagnostics.isFatal()).toBe(true)
+    expect(diagnostics.observations).toEqual(expect.arrayContaining([
+      expect.objectContaining({
+        message: expect.stringContaining('contains invalid characters'),
+      }),
+    ]))
+  })
+})

packages/cli/src/constructs/check-group-v1.ts

@@ -15,7 +15,7 @@ import {
 import { AlertEscalation } from './alert-escalation-policy'
 import { Diagnostics } from './diagnostics'
 import { DeprecatedConstructDiagnostic, DeprecatedPropertyDiagnostic, InvalidPropertyValueDiagnostic } from './construct-diagnostics'
-import CheckTypes from '../constants'
+import { CheckTypes } from '../constants'
 import { CheckConfigDefaults } from '../services/checkly-config-loader'
 import { AlertChannelSubscription } from './alert-channel-subscription'
 import { BrowserCheck } from './browser-check'

packages/cli/src/constructs/construct.ts

@@ -1,5 +1,7 @@
 import path from 'node:path'
 
+import { LOGICAL_ID_PATTERN } from '../constants'
+import { InvalidPropertyValueDiagnostic } from './construct-diagnostics'
 import { Diagnostics } from './diagnostics'
 import { Session } from './project'
 import { Ref } from './ref'
@@ -123,9 +125,14 @@ export abstract class Construct implements Validate, Bundle {
    * @param diagnostics The Diagnostics instance that any issues should be added to.
    * @returns A Promise that resolves when validation is complete.
    */
-  // eslint-disable-next-line @typescript-eslint/no-unused-vars, require-await
+  // eslint-disable-next-line require-await
   async validate (diagnostics: Diagnostics): Promise<void> {
-    return
+    if (!LOGICAL_ID_PATTERN.test(this.logicalId)) {
+      diagnostics.add(new InvalidPropertyValueDiagnostic(
+        'logicalId',
+        new Error(`"${this.logicalId}" contains invalid characters. Only A-Z, a-z, 0-9, _, -, /, #, and . are allowed.`),
+      ))
+    }
   }
 
   /**

packages/cli/src/constructs/heartbeat-monitor.ts

@@ -1,7 +1,7 @@
 import { Monitor, MonitorProps } from './monitor'
 import { Session } from './project'
 import { DateTime } from 'luxon'
-import CheckTypes from '../constants'
+import { CheckTypes } from '../constants'
 
 type TimeUnits = 'seconds' | 'minutes' | 'hours' | 'days'
 

packages/cli/src/constructs/multi-step-check.ts

@@ -3,7 +3,7 @@ import fs from 'node:fs/promises'
 import { CheckProps, RuntimeCheck, RuntimeCheckProps } from './check'
 import { Session, SharedFileRef } from './project'
 import { Content, Entrypoint, isContent, isEntrypoint } from './construct'
-import CheckTypes from '../constants'
+import { CheckTypes } from '../constants'
 import { PlaywrightConfig } from './playwright-config'
 import { Diagnostics } from './diagnostics'
 import { InvalidPropertyValueDiagnostic, UnsupportedRuntimeFeatureDiagnostic } from './construct-diagnostics'

packages/cli/src/constructs/project.ts

@@ -328,10 +328,6 @@ export class Session {
       throw new ValidationError(`The "logicalId" of a ${construct.type} construct must be a string (logicalId=${construct.logicalId} [${typeof construct.logicalId}])`)
     }
 
-    if (!/^[A-Za-z0-9_\-/#.]+$/.test(construct.logicalId)) {
-      throw new ValidationError(`The "logicalId" can only include the following characters: [A-Za-z0-9_-/#.]. (logicalId='${construct.logicalId}')`)
-    }
-
     if (construct.type === Project.__checklyType) {
       // Creating the construct is allowed - We're creating the project.
     } else if (Session.project) {

packages/cli/src/services/project-parser.ts

@@ -149,6 +149,7 @@ export async function parseProject (opts: ProjectParseOpts): Promise<Project> {
     currentCommand,
     enableWorkspaces = true,
   } = opts
+
   const project = new Project(projectLogicalId, {
     name: projectName,
     repoUrl,