warn for ambiguous colons in formats (#2335)

Author: davidjgoss

CHANGELOG.md

@@ -10,6 +10,7 @@ Please see [CONTRIBUTING.md](./CONTRIBUTING.md) on how to contribute to Cucumber
 ## [Unreleased]
 ### Fixed
 - Improve handling of formatter paths ([#2315](https://github.com/cucumber/cucumber-js/pull/2315))
+- Warn on ambiguous colons in formatter paths ([#2335](https://github.com/cucumber/cucumber-js/pull/2335))
 
 ## [9.5.1] - 2023-09-06
 ### Fixed

docs/deprecations.md

@@ -54,6 +54,17 @@ The `publishQuiet` option (or `--publish-quiet` on the CLI) was used to hide the
 
 To adapt, remove the option from your configuration files and CLI commands (especially the latter, since the CLI will fail on unrecognised options).
 
+### Ambiguous colons in formats
+
+Deprecated in `9.6.0`. Will be removed in `11.0.0` or later.
+
+User-specified formats where either the formatter name/path or the target path (or both) contains colon(s) are ambiguous because the separator between the two parts is also a colon. Cucumber tries to detect and handle things like Windows drives and `file://` URLs on a best-effort basis, but this logic is being removed in favour of wrapping values in double-quotes.
+
+| Before                                       | After                                            |
+|----------------------------------------------|--------------------------------------------------|
+| `html:file://hostname/formatter/report.html` | `"html":"file://hostname/formatter/report.html"` |
+| `file://C:\custom\formatter`                 | `"file://C:\custom\formatter"`                   |
+
 ## Previous deprecations
 
 For deprecations that have been completed (i.e. the functionality removed), see [UPGRADING.md](../UPGRADING.md).

docs/formatters.md

@@ -7,7 +7,7 @@ cucumber-js provides many built-in Formatters, plus building blocks with which y
 You can specify one or more formats via the `format` configuration option:
 
 - In a configuration file `{ format: ['progress-bar', ['html', 'cucumber-report.html']] }`
-- On the CLI `$ cucumber-js --format progress-bar --format html:cucumber-report.html`
+- On the CLI `$ cucumber-js --format progress-bar --format "html":"cucumber-report.html"`
 
 For each format you specify, you have to provide one or two values. The first (required) is to identify the formatter. It can take a few forms:
 
@@ -18,7 +18,7 @@ For each format you specify, you have to provide one or two values. The first (r
 
 Without a second value, the formatter will print to `stdout`. The second value, if present, is a path to where the formatter output should be written. If the path includes directories that do not yet exist, they will be created.
 
-On the CLI, when specifying both a name and path, you'll need to use `:` as a delimiter. In a configuration file you can do this too, but you can also provide an array with the two values as separate strings, which is recommended. 
+On the CLI, when specifying both a name and path, you'll need to use `:` as a delimiter and wrap each side of it with double quotes. In a configuration file you can do this too, but you can also provide an array with the two values as separate strings, which is recommended. 
 
 Some notes on specifying Formatters:
 

src/api/convert_configuration.ts

@@ -1,12 +1,14 @@
 import {
   IConfiguration,
   isTruthyString,
-  OptionSplitter,
+  splitFormatDescriptor,
 } from '../configuration'
 import { IPublishConfig } from '../formatter'
 import { IRunConfiguration } from './types'
+import { ILogger } from '../logger'
 
 export async function convertConfiguration(
+  logger: ILogger,
   flatConfiguration: IConfiguration,
   env: NodeJS.ProcessEnv
 ): Promise<IRunConfiguration> {
@@ -33,16 +35,17 @@ export async function convertConfiguration(
       strict: flatConfiguration.strict,
       worldParameters: flatConfiguration.worldParameters,
     },
-    formats: convertFormats(flatConfiguration, env),
+    formats: convertFormats(logger, flatConfiguration, env),
   }
 }
 
 function convertFormats(
+  logger: ILogger,
   flatConfiguration: IConfiguration,
   env: NodeJS.ProcessEnv
 ) {
   const splitFormats: string[][] = flatConfiguration.format.map((item) =>
-    Array.isArray(item) ? item : OptionSplitter.split(item)
+    Array.isArray(item) ? item : splitFormatDescriptor(logger, item)
   )
   return {
     stdout:

src/api/convert_configuration_spec.ts

@@ -2,10 +2,15 @@ import { describe, it } from 'mocha'
 import { expect } from 'chai'
 import { convertConfiguration } from './convert_configuration'
 import { DEFAULT_CONFIGURATION } from '../configuration'
+import { FakeLogger } from '../../test/fake_logger'
 
 describe('convertConfiguration', () => {
   it('should convert defaults correctly', async () => {
-    const result = await convertConfiguration(DEFAULT_CONFIGURATION, {})
+    const result = await convertConfiguration(
+      new FakeLogger(),
+      DEFAULT_CONFIGURATION,
+      {}
+    )
 
     expect(result).to.eql({
       formats: {
@@ -41,6 +46,7 @@ describe('convertConfiguration', () => {
 
   it('should map multiple formatters with string notation', async () => {
     const result = await convertConfiguration(
+      new FakeLogger(),
       {
         ...DEFAULT_CONFIGURATION,
         format: [
@@ -66,6 +72,7 @@ describe('convertConfiguration', () => {
 
   it('should map multiple formatters with array notation', async () => {
     const result = await convertConfiguration(
+      new FakeLogger(),
       {
         ...DEFAULT_CONFIGURATION,
         format: [
@@ -91,6 +98,7 @@ describe('convertConfiguration', () => {
 
   it('should map formatters correctly when file:// urls are involved', async () => {
     const result = await convertConfiguration(
+      new FakeLogger(),
       {
         ...DEFAULT_CONFIGURATION,
         format: [

src/api/load_configuration.ts

@@ -44,7 +44,7 @@ export async function loadConfiguration(
   )
   logger.debug('Resolved configuration:', original)
   validateConfiguration(original, logger)
-  const runnable = await convertConfiguration(original, env)
+  const runnable = await convertConfiguration(logger, original, env)
   return {
     useConfiguration: original,
     runConfiguration: runnable,

src/cli/helpers.ts

@@ -3,7 +3,6 @@ import { EventEmitter } from 'events'
 import PickleFilter from '../pickle_filter'
 import { EventDataCollector } from '../formatter/helpers'
 import { doesHaveValue } from '../value_checker'
-import { OptionSplitter } from '../configuration'
 import { Readable } from 'stream'
 import os from 'os'
 import * as messages from '@cucumber/messages'
@@ -71,7 +70,7 @@ export function orderPickles<T = string>(
   order: PickleOrder,
   logger: ILogger
 ): void {
-  const [type, seed] = OptionSplitter.split(order)
+  const [type, seed] = splitOrder(order)
   switch (type) {
     case 'defined':
       break
@@ -91,6 +90,13 @@ export function orderPickles<T = string>(
   }
 }
 
+function splitOrder(order: string) {
+  if (!order.includes(':')) {
+    return [order, '']
+  }
+  return order.split(':')
+}
+
 export async function emitMetaMessage(
   eventBroadcaster: EventEmitter,
   env: NodeJS.ProcessEnv

src/configuration/index.ts

@@ -3,5 +3,5 @@ export * from './default_configuration'
 export * from './from_file'
 export * from './helpers'
 export * from './merge_configurations'
-export * from './option_splitter'
+export * from './split_format_descriptor'
 export * from './types'

src/configuration/option_splitter.ts

@@ -0,0 +1,80 @@
+import { ILogger } from '../logger'
+
+export function splitFormatDescriptor(
+  logger: ILogger,
+  option: string
+): string[] {
+  const doWarning = (result: string[]) => {
+    let expected = `"${result[0]}"`
+    if (result[1]) {
+      expected += `:"${result[1]}"`
+    }
+    logger.warn(
+      `Each part of a user-specified format should be quoted; see https://github.com/cucumber/cucumber-js/blob/main/docs/deprecations.md#ambiguous-colons-in-formats
+Change to ${expected}`
+    )
+  }
+  let result: string[]
+  let match1, match2
+
+  // "foo":"bar" or "foo":bar
+  if ((match1 = option.match(/^"([^"]*)":(.*)$/)) !== null) {
+    // "foo":"bar"
+    if ((match2 = match1[2].match(/^"([^"]*)"$/)) !== null) {
+      result = [match1[1], match2[1]]
+    }
+    // "foo":bar
+    else {
+      result = [match1[1], match1[2]]
+      if (result[1].includes(':')) {
+        doWarning(result)
+      }
+    }
+  }
+  // foo:"bar"
+  else if ((match1 = option.match(/^(.*):"([^"]*)"$/)) !== null) {
+    result = [match1[1], match1[2]]
+    if (result[0].includes(':')) {
+      doWarning(result)
+    }
+  }
+  // "foo"
+  else if ((match1 = option.match(/^"([^"]*)"$/)) !== null) {
+    result = [match1[1], '']
+  }
+  // file://foo or file:///foo or file://C:/foo or file://C:\foo or file:///C:/foo or file:///C:\foo
+  else if (
+    (match1 = option.match(/^(file:\/{2,3}(?:[a-zA-Z]:[/\\])?)(.*)$/)) !== null
+  ) {
+    // file://foo:bar
+    if ((match2 = match1[2].match(/^([^:]*):(.*)$/)) !== null) {
+      result = [match1[1] + match2[1], match2[2]]
+    } else {
+      result = [option, '']
+    }
+    doWarning(result)
+  }
+  // C:\foo or C:/foo
+  else if ((match1 = option.match(/^([a-zA-Z]:[/\\])(.*)$/)) !== null) {
+    // C:\foo:bar or C:/foo:bar
+    if ((match2 = match1[2].match(/^([^:]*):(.*)$/)) !== null) {
+      result = [match1[1] + match2[1], match2[2]]
+    } else {
+      result = [option, '']
+    }
+    doWarning(result)
+  }
+  // foo:bar
+  else if ((match1 = option.match(/^([^:]*):(.*)$/)) !== null) {
+    result = [match1[1], match1[2]]
+    if (option.split(':').length > 2) {
+      doWarning(result)
+    }
+  }
+  // foo
+  else {
+    result = [option, '']
+  }
+
+  return result
+}