From 877d09f23e3666b66fdcc05021abbb26125bae27 Mon Sep 17 00:00:00 2001 From: August Cayzer Date: Sun, 7 Jun 2026 17:39:25 +0000 Subject: [PATCH] Ignore trailing positional arguments in the global flag parser The flag parser in common/flags/index.ts runs in a static initialiser at module load and parses process.argv. Once it had seen a --flag, any later token that was neither another flag nor a flag value caused it to throw "Arg neither flag name nor flag value", crashing the entire CLI before yargs ran. yargs accepts positional arguments after flags (for example `dataform run --some-flag value my_project`), so this lightweight parser was rejecting valid argument orderings. Ignore any token that is neither a flag nor a flag value instead of throwing, exactly as already happened for positional arguments before the first flag; yargs remains responsible for parsing positionals. Extract the parsing loop into an exported parseArgv function so it can be unit-tested in isolation, and add common/flags/index_test.ts covering the regression and related orderings. Fixes #2198. --- common/flags/BUILD | 22 ++++++++++++ common/flags/index.ts | 71 +++++++++++++++++++------------------- common/flags/index_test.ts | 57 ++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 35 deletions(-) create mode 100644 common/flags/index_test.ts diff --git a/common/flags/BUILD b/common/flags/BUILD index 3d6514c38..778c0acc3 100644 --- a/common/flags/BUILD +++ b/common/flags/BUILD @@ -1,16 +1,38 @@ package(default_visibility = ["//visibility:public"]) load("//tools:ts_library.bzl", "ts_library") +load("@df//testing:index.bzl", "ts_test_suite") ts_library( name = "flags", srcs = glob( ["*.ts"], + exclude = ["*_test.ts", "*.spec.ts"], ), deps = [ "//:modules-fix", "@npm//@types/long", "@npm//@types/node", + "@npm//long", + ], +) + +ts_test_suite( + name = "tests", + srcs = glob(["*_test.ts"]), + data = [ + "@npm//source-map-support", + ], + templated_args = [ + "--node_options=--require=source-map-support/register", + "--bazel_patch_module_resolver", + ], + deps = [ + ":flags", + "@df//testing", + "@npm//@types/chai", + "@npm//@types/node", + "@npm//chai", ], ) diff --git a/common/flags/index.ts b/common/flags/index.ts index 6173da668..d9c72f3e0 100644 --- a/common/flags/index.ts +++ b/common/flags/index.ts @@ -39,47 +39,48 @@ export class Flags { Flags.parsedArgv[flagName] = value; } - private static readonly parsedArgv = (() => { - const parsedArgv: { [flagName: string]: string } = {}; - - const splitArgv = []; - for (let arg of process.argv) { - // TODO: This is a temporary hack to be backwards-compatible with yargs behaviour, where - // to switch off a boolean flag, it requires a 'no-' prefix in front of the flag name. - if (arg.startsWith("--no-")) { - arg = `--${arg.slice(5)}=false`; - } - if (arg.startsWith("--") && arg.includes("=")) { - splitArgv.push(arg.slice(0, arg.indexOf("="))); - splitArgv.push(arg.slice(arg.indexOf("=") + 1)); - } else { - splitArgv.push(arg); - } - } - - let flagsStarted = false; - let currentFlagName = ""; - for (const splitArg of splitArgv) { - if (splitArg.startsWith("--")) { - flagsStarted = true; - currentFlagName = splitArg.slice(2); - parsedArgv[currentFlagName] = ""; - } else if (currentFlagName) { - parsedArgv[currentFlagName] = splitArg; - currentFlagName = ""; - } else if (flagsStarted) { - throw new Error(`Arg neither flag name nor flag value: ${splitArg}`); - } - } - - return parsedArgv; - })(); + private static readonly parsedArgv = parseArgv(process.argv); private static invalidFlagValueError(flagName: string) { return new Error(`Invalid flag value: ${Flags.getRawFlagValue(flagName)} [${flagName}]`); } } +// Parses an argv array into a map of flag name to flag value. Any token that is neither a flag +// nor a flag value (for example the command and its positional arguments) is ignored, as yargs +// is responsible for parsing those. +export function parseArgv(argv: string[]): { [flagName: string]: string } { + const parsedArgv: { [flagName: string]: string } = {}; + + const splitArgv = []; + for (let arg of argv) { + // TODO: This is a temporary hack to be backwards-compatible with yargs behaviour, where + // to switch off a boolean flag, it requires a 'no-' prefix in front of the flag name. + if (arg.startsWith("--no-")) { + arg = `--${arg.slice(5)}=false`; + } + if (arg.startsWith("--") && arg.includes("=")) { + splitArgv.push(arg.slice(0, arg.indexOf("="))); + splitArgv.push(arg.slice(arg.indexOf("=") + 1)); + } else { + splitArgv.push(arg); + } + } + + let currentFlagName = ""; + for (const splitArg of splitArgv) { + if (splitArg.startsWith("--")) { + currentFlagName = splitArg.slice(2); + parsedArgv[currentFlagName] = ""; + } else if (currentFlagName) { + parsedArgv[currentFlagName] = splitArg; + currentFlagName = ""; + } + } + + return parsedArgv; +} + export interface IFlag { get(): T; } diff --git a/common/flags/index_test.ts b/common/flags/index_test.ts new file mode 100644 index 000000000..2da49e50b --- /dev/null +++ b/common/flags/index_test.ts @@ -0,0 +1,57 @@ +import { expect } from "chai"; + +import { parseArgv } from "df/common/flags"; +import { suite, test } from "df/testing"; + +suite("flags", () => { + suite("parseArgv", () => { + test("parses a flag followed by its value", () => { + expect(parseArgv(["node", "script", "--foo", "bar"])).deep.equals({ foo: "bar" }); + }); + + test("parses a --flag=value pair", () => { + expect(parseArgv(["node", "script", "--foo=bar"])).deep.equals({ foo: "bar" }); + }); + + test("parses multiple flags", () => { + expect(parseArgv(["node", "script", "--foo", "bar", "--baz", "qux"])).deep.equals({ + foo: "bar", + baz: "qux" + }); + }); + + test("ignores positional arguments before flags", () => { + expect(parseArgv(["node", "script", "run", "my_project", "--foo", "bar"])).deep.equals({ + foo: "bar" + }); + }); + + test("ignores positional arguments after a flag and its value", () => { + // Regression test for https://github.com/dataform-co/dataform/issues/2198: a positional + // argument after a flag used to throw "Arg neither flag name nor flag value", which + // crashed the CLI. + expect(parseArgv(["node", "script", "run", "--foo", "bar", "my_project"])).deep.equals({ + foo: "bar" + }); + }); + + test("ignores positional arguments interleaved with flags", () => { + expect(parseArgv(["node", "script", "--foo", "bar", "pos", "--baz", "qux"])).deep.equals({ + foo: "bar", + baz: "qux" + }); + }); + + test("returns an empty object when no flags are present", () => { + expect(parseArgv(["node", "script", "run", "my_project"])).deep.equals({}); + }); + + test("treats a --no- prefix as setting the flag to false", () => { + expect(parseArgv(["node", "script", "--no-foo"])).deep.equals({ foo: "false" }); + }); + + test("records a value-less trailing flag as an empty string", () => { + expect(parseArgv(["node", "script", "--foo"])).deep.equals({ foo: "" }); + }); + }); +});