Ensure kv parsing returns undefined for empty cases (#115)

google-github-actions · Jun 3, 2024 · d5b91a3 · d5b91a3
1 parent 9e902e2
commit d5b91a3
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 33 deletions.
diff --git a/src/kv.ts b/src/kv.ts
@@ -91,10 +91,10 @@ export function joinKVStringForGCloud(
  *
  * @param input String with key/value pairs to parse.
  */
-export function parseKVString(input: string): KVPair {
+export function parseKVString(input: string): KVPair | undefined {
   input = (input || '').trim();
   if (!input) {
-    return {};
+    return undefined;
   }
 
   const result: KVPair = {};
@@ -164,11 +164,11 @@ export function parseKVString(input: string): KVPair {
  *
  * @param filePath Path to the file on disk to parse.
  */
-export function parseKVFile(filePath: string): KVPair {
+export function parseKVFile(filePath: string): KVPair | undefined {
   try {
     const content = presence(readFileSync(filePath, 'utf8'));
     if (!content || content.length < 1) {
-      return {};
+      return undefined;
     }
 
     if (content[0] === '{' || content[0] === '[') {
@@ -199,9 +199,13 @@ export function parseKVFile(filePath: string): KVPair {
  *
  * @return List of key=value pairs.
  */
-export function parseKVJSON(str: string): KVPair {
+export function parseKVJSON(str: string): KVPair | undefined {
   str = (str || '').trim();
   if (!str) {
+    return undefined;
+  }
+
+  if (str === '{}') {
     return {};
   }
 
@@ -243,11 +247,18 @@ export function parseKVJSON(str: string): KVPair {
  *
  * @param str YAML content to parse as K=V pairs.
  */
-export function parseKVYAML(str: string): KVPair {
-  if (!str || str.trim().length === 0) {
+export function parseKVYAML(str: string): KVPair | undefined {
+  const trimmed = (str || '').trim();
+  if (!trimmed) {
+    return undefined;
+  }
+
+  if (trimmed === '{}') {
     return {};
   }
 
+  // Parse the original string here, since trimming could have changed
+  // indentation.
   const yamlContent = YAML.parse(str) as KVPair;
 
   const result: KVPair = {};
@@ -270,21 +281,15 @@ export function parseKVYAML(str: string): KVPair {
  * @param kvString String of KEY=VALUE pairs.
  * @param kvFilePath Path on disk to a YAML file of KEY: VALUE pairs.
  */
-export function parseKVStringAndFile(kvString?: string, kvFilePath?: string): KVPair {
+export function parseKVStringAndFile(kvString?: string, kvFilePath?: string): KVPair | undefined {
   kvString = (kvString || '').trim();
   kvFilePath = (kvFilePath || '').trim();
 
-  let result: Record<string, string> = {};
+  const fromFile = kvFilePath ? parseKVFile(kvFilePath) : undefined;
+  const fromString = kvString ? parseKVString(kvString) : undefined;
 
-  if (kvFilePath) {
-    const parsed = parseKVFile(kvFilePath);
-    result = { ...result, ...parsed };
+  if (fromFile === undefined && fromString === undefined) {
+    return undefined;
   }
-
-  if (kvString) {
-    const parsed = parseKVString(kvString);
-    result = { ...result, ...parsed };
-  }
-
-  return result;
+  return Object.assign({}, fromFile, fromString);
 }
diff --git a/tests/kv.test.ts b/tests/kv.test.ts
@@ -106,6 +106,11 @@ describe('kv', { concurrency: true }, async () => {
       {
         name: 'empty string',
         input: '',
+        expected: undefined,
+      },
+      {
+        name: 'braces',
+        input: '{}',
         expected: {},
       },
       {
@@ -257,6 +262,11 @@ ftyRyC/83GkAjs88l5eGxNE=
       {
         name: 'empty string',
         input: '',
+        expected: undefined,
+      },
+      {
+        name: 'braces',
+        input: '{}',
         expected: {},
       },
       {
@@ -300,6 +310,11 @@ ftyRyC/83GkAjs88l5eGxNE=
       {
         name: 'empty string',
         input: '',
+        expected: undefined,
+      },
+      {
+        name: 'braces',
+        input: '{}',
         expected: {},
       },
       {
@@ -382,21 +397,68 @@ ftyRyC/83GkAjs88l5eGxNE=
   });
 
   test('#parseKVStringAndFile', async (suite) => {
-    await suite.test('handles null kvString and kvFilePath', async () => {
-      const kvString = '';
-      const kvFile = '';
-
-      const result = parseKVStringAndFile(kvString, kvFile);
-      assert.deepStrictEqual(result, {});
-    });
+    const cases = [
+      {
+        name: 'undefined kvstring and kvfile',
+        kvString: undefined,
+        kvFileContents: undefined,
+        expected: undefined,
+      },
+      {
+        name: 'empty kvstring and kvfile',
+        kvString: '',
+        kvFileContents: '',
+        expected: undefined,
+      },
+      {
+        name: 'braces kvstring and undefined kvfile',
+        kvString: '{}',
+        kvFileContents: undefined,
+        expected: {},
+      },
+      {
+        name: 'undefined kvstring and braces kvfile',
+        kvString: undefined,
+        kvFileContents: '{}',
+        expected: {},
+      },
+      {
+        name: 'braces kvstring and braces kvfile',
+        kvString: '{}',
+        kvFileContents: '{}',
+        expected: {},
+      },
+      {
+        name: 'partial kvstring and braces kvfile',
+        kvString: 'FOO=bar',
+        kvFileContents: '{}',
+        expected: { FOO: 'bar' },
+      },
+      {
+        name: 'braces kvstring and partial kvfile',
+        kvString: '{}',
+        kvFileContents: 'FOO=bar',
+        expected: { FOO: 'bar' },
+      },
+      {
+        name: 'merges with kvstring taking precedence',
+        kvString: 'FOO=bar',
+        kvFileContents: 'FOO=zap,ZIP=zap',
+        expected: { FOO: 'bar', ZIP: 'zap' },
+      },
+    ];
 
-    await suite.test('merges kvString and kvFile', async () => {
-      const kvString = `FOO=other foo`;
-      const kvFile = randomFilepath();
-      await fs.writeFile(kvFile, `FOO: 'bar'\nZIP: 'zap'`);
+    for await (const tc of cases) {
+      await suite.test(tc.name, async () => {
+        let kvFile = '';
+        if (tc.kvFileContents !== undefined) {
+          kvFile = randomFilepath();
+          await fs.writeFile(kvFile, tc.kvFileContents);
+        }
 
-      const result = parseKVStringAndFile(kvString, kvFile);
-      assert.deepStrictEqual(result, { FOO: 'other foo', ZIP: 'zap' });
-    });
+        const result = parseKVStringAndFile(tc.kvString, kvFile);
+        assert.deepStrictEqual(result, tc.expected);
+      });
+    }
   });
 });