refactor: Centralized input validation (#142)

* Initial implementaion of service account json, user fraction and status validation

* Move validation to input-validation
Also created some more validators

* Rearrange whatsnew tests to clear out test folder

* Return error messages for input validation

* Create tests for some validation code

* Separate user fraction and status validation

* Write more tests and fix some bugs

* Refactor to use promises

* Fix release file loading from glob

__tests__/input-validation.test.ts

@@ -0,0 +1,74 @@
+import { validateInAppUpdatePriority, validateReleaseFiles, validateStatus, validateUserFraction } from "../src/input-validation"
+
+test("invalid in-app update priority fails validation", async () => {
+    const testValues = [-1, 6, -1000, 1000]
+    for (const value of testValues) {
+        await expect(validateInAppUpdatePriority(value)).rejects.toThrowError()
+    }
+})
+
+test("valid in-app update priority passes validation", async () => {
+    const testValues = [0, 1, 2, 3, 4, 5, undefined]
+    for (const value of testValues) {
+        await validateInAppUpdatePriority(value)
+    }
+})
+
+test("invalid releaseFiles glob fails validation", async () => {
+    const testValues = ['./__tests__/releasefiles/*.null', `./__tests__/releasefiles/nonexistent-release.aab`]
+    await expect(validateReleaseFiles(testValues)).rejects.toThrowError()
+})
+
+test("valid releaseFiles glob passes validation", async () => {
+    const testValues = ['./__tests__/releasefiles/*.aab', `./__tests__/releasefiles/release.aab`]
+    await validateReleaseFiles(testValues)
+})
+
+test("fractionless status without fraction passes validation", async () => {
+    const testValues = ['completed', 'draft']
+    for (const value of testValues) {
+        await validateStatus(value, false)
+    }
+})
+
+test("fractionless status with fraction fails validation", async () => {
+    const testValues = ['completed', 'draft']
+    for (const value of testValues) {
+        await expect(validateStatus(value, true)).rejects.toThrowError()
+    }
+})
+
+test("fractioned status without fraction fails validation", async () => {
+    const testValues = ['inProgress', 'halted']
+    for (const value of testValues) {
+        await expect(validateStatus(value, false)).rejects.toThrowError()
+    }
+})
+
+test("fractioned status with fraction passes validation", async () => {
+    const testValues = ['inProgress', 'halted']
+    for (const value of testValues) {
+        await validateStatus(value, true)
+    }
+})
+
+test("invalid status fails validation", async () => {
+    const testValues = ['statusGoBrrr', undefined]
+    for (const value of testValues) {
+        await expect(validateStatus(value, true)).rejects.toThrowError()
+    }
+})
+
+test("invalid user fraction fails validation", async () => {
+    const testValues = [0, 1, -1, 2, NaN]
+    for (const value of testValues) {
+        await expect(validateUserFraction(value)).rejects.toThrowError()
+    }
+})
+
+test("valid user fraction passes validation", async () => {
+    const testValues = [0.1, 0.9, 0.5, undefined]
+    for (const value of testValues) {
+        await validateUserFraction(value)
+    }
+})

__tests__/whatsnew.test.ts

@@ -1,12 +1,7 @@
-import * as process from 'process'
-import * as cp from 'child_process'
-import * as path from 'path'
 import {readLocalizedReleaseNotes} from "../src/whatsnew";
-import {androidpublisher_v3} from "@googleapis/androidpublisher";
-import LocalizedText = androidpublisher_v3.Schema$LocalizedText;
 
 test("read localized whatsnew files", async () => {
-    let texts = await readLocalizedReleaseNotes("./__tests__");
+    let texts = await readLocalizedReleaseNotes("./__tests__/whatsnew");
     expect(texts).toHaveLength(6);
     expect(texts).toContainEqual({
         language: "en-US",
@@ -32,4 +27,4 @@ test("read localized whatsnew files", async () => {
         language: "et",
         text: "test_changelog_file_estonian"
     });
-});
+});

action.yml

@@ -33,8 +33,7 @@ inputs:
     required: false
   userFraction:
     description: "Portion of users who should get the staged version of the app. Accepts values between 0.0 and 1.0 (exclusive-exclusive)."
-    required: true
-    default: 1.0
+    required: false
   status:
     description: "Release status. This can be set to 'draft' to complete the release at some other time."
     required: false

src/input-validation.ts

@@ -0,0 +1,56 @@
+import fg from "fast-glob";
+
+export async function validateUserFraction(userFraction: number | undefined): Promise<void> {
+    if (userFraction != undefined) {
+        // If userFraction was set, perform basic validation
+        if (isNaN(userFraction)) {
+            return Promise.reject(new Error(`'userFraction' must be a number! Got ${userFraction}`))
+        }
+        if (userFraction >= 1 || userFraction <= 0) {
+            return Promise.reject(new Error(`'userFraction' must be between 0 and 1! Got ${userFraction}`))
+        }
+    }
+}
+
+export async function validateStatus(status: string | undefined, hasUserFraction: boolean): Promise<void> {
+    // If status was set, perform basic validation
+    if (status != 'completed' && status != 'inProgress' && status != 'halted' && status != 'draft') {
+        return Promise.reject(new Error(`Invalid status provided! Must be one of 'completed', 'inProgress', 'halted', 'draft'. Got ${status ?? "undefined"}`))
+    }
+
+    // Validate userFraction is correct for the given status
+    switch (status) {
+        case 'completed':
+        case 'draft':
+            if (hasUserFraction) {
+                return Promise.reject(new Error(`Status 'completed' does not support 'userFraction'`))
+            }
+            break
+        case 'halted':
+        case 'inProgress':
+            if (!hasUserFraction) {
+                return Promise.reject(new Error(`Status '${status}' requires a 'userFraction' to be set`))
+            }
+            break
+    }
+}
+
+export async function validateInAppUpdatePriority(inAppUpdatePriority: number | undefined): Promise<void> {
+    if (inAppUpdatePriority) {
+        if (inAppUpdatePriority < 0 || inAppUpdatePriority > 5) {
+            return Promise.reject(new Error('inAppUpdatePriority must be between 0 and 5, inclusive-inclusive'))
+        }
+    }
+}
+
+export async function validateReleaseFiles(releaseFiles: string[] | undefined): Promise<string[]> {
+    if (!releaseFiles) {
+        return Promise.reject(new Error(`You must provide 'releaseFiles' in your configuration`))
+    } else {
+        const files = await fg(releaseFiles)
+        if (!files.length) {
+            return Promise.reject(new Error(`Unable to find any release file matching ${releaseFiles.join(',')}`))
+        }
+        return files
+    }
+}

src/main.ts

@@ -1,9 +1,10 @@
 import * as core from '@actions/core';
 import * as fs from "fs";
-import fg from "fast-glob";
 import {uploadToPlayStore} from "./edits";
+import { validateInAppUpdatePriority, validateReleaseFiles, validateStatus, validateUserFraction } from "./input-validation"
 import * as google from '@googleapis/androidpublisher';
-import { unlink } from 'fs/promises';
+import { unlink, writeFile } from 'fs/promises';
+import { exit } from 'process';
 
 const auth = new google.auth.GoogleAuth({
     scopes: ['https://www.googleapis.com/auth/androidpublisher']
@@ -21,84 +22,32 @@ async function run() {
         const releaseName = core.getInput('releaseName', { required: false });
         const track = core.getInput('track', { required: true });
         const inAppUpdatePriority = core.getInput('inAppUpdatePriority', { required: false });
-        const userFraction = parseFloat(core.getInput('userFraction', { required: true }));
-        let status = core.getInput('status', { required: false });
+        const userFraction = core.getInput('userFraction', { required: false })
+        const status = core.getInput('status', { required: false });
         const whatsNewDir = core.getInput('whatsNewDirectory', { required: false });
         const mappingFile = core.getInput('mappingFile', { required: false });
         const debugSymbols = core.getInput('debugSymbols', { required: false });
         const changesNotSentForReview = core.getInput('changesNotSentForReview', { required: false }) == 'true';
         const existingEditId = core.getInput('existingEditId');
 
-        // Validate that we have a service account json in some format
-        if (!serviceAccountJson && !serviceAccountJsonRaw) {
-            console.log("No service account json key provided!");
-            core.setFailed("You must provide one of 'serviceAccountJson' or 'serviceAccountJsonPlainText' to use this action");
-            return;
-        }
+        await validateServiceAccountJson(serviceAccountJsonRaw, serviceAccountJson)
 
-        // If the user has provided the raw plain text via secrets, or w/e, then write to file and
-        // set appropriate env variable for the auth
-        if (serviceAccountJsonRaw) {
-            const serviceAccountFile = "./serviceAccountJson.json";
-            fs.writeFileSync(serviceAccountFile, serviceAccountJsonRaw, {
-                encoding: 'utf8'
-            });
+        // Validate user fraction
+        const userFractionFloat = parseFloat(userFraction)
+        await validateUserFraction(userFractionFloat)
 
-            // Insure that the api can find our service account credentials
-            core.exportVariable("GOOGLE_APPLICATION_CREDENTIALS", serviceAccountFile);
-        }
-
-        if (serviceAccountJson) {
-            // Insure that the api can find our service account credentials
-            core.exportVariable("GOOGLE_APPLICATION_CREDENTIALS", serviceAccountJson);
-        }
-
-        // Validate user fraction as a number, and within [0.0, 1.0]
-        if (userFraction < 0.0 || userFraction > 1.0) {
-            core.setFailed('A provided userFraction must be between 0.0 and 1.0, inclusive-inclusive');
-            return;
-        }
+        // Validate release status
+        await validateStatus(status, userFraction != undefined)
 
         // Validate the inAppUpdatePriority to be a valid number in within [0, 5]
-        let inAppUpdatePriorityInt: number | undefined = parseInt(inAppUpdatePriority);
-        if (!isNaN(inAppUpdatePriorityInt)) {
-            if (inAppUpdatePriorityInt < 0 || inAppUpdatePriorityInt > 5) {
-                core.setFailed('inAppUpdatePriority must be between 0 and 5, inclusive-inclusive');
-                return;
-            }
-        } else {
-            inAppUpdatePriorityInt = undefined;
-        }
+        const inAppUpdatePriorityInt: number | undefined = parseInt(inAppUpdatePriority);
+        await validateInAppUpdatePriority(inAppUpdatePriorityInt)
 
         // Check release files while maintaining backward compatibility
-        let validatedReleaseFiles: string[] = [];
-        if (releaseFiles.length == 0 && !releaseFile) {
-            core.setFailed(`You must provide either 'releaseFile' or 'releaseFiles' in your configuration.`);
-            return;
-        } else if (releaseFiles.length == 0 && releaseFile) {
-            core.warning(`WARNING!! 'releaseFile' is deprecated and will be removed in a future release. Please migrate to 'releaseFiles'.`);
-            core.debug(`Validating ${releaseFile} exists`)
-            if (!fs.existsSync(releaseFile)) {
-                core.setFailed(`Unable to find release file @ ${releaseFile}`);
-                return;
-            } else {
-                validatedReleaseFiles = [releaseFile];
-            }
-        } else if (releaseFiles.length > 0) {
-            core.debug(`Finding files ${releaseFiles.join(',')}`)
-            const files = await fg(releaseFiles);
-            if (!files.length) {
-                core.setFailed(`Unable to find any release file @ ${releaseFiles.join(',')}`);
-                return;
-            }
-            validatedReleaseFiles = files;
-        }
-
-        if (status != undefined) {
-            if (!validateStatus(status, userFraction)) return
-        } else {
-            status = inferStatus(userFraction)
+        if (releaseFile) {
+            core.warning(`WARNING!! 'releaseFile' is deprecated and will be removed in a future release. Please migrate to 'releaseFiles'`)
         }
+        const validatedReleaseFiles: string[] = await validateReleaseFiles(releaseFiles ?? [releaseFile])
 
         if (whatsNewDir != undefined && whatsNewDir.length > 0 && !fs.existsSync(whatsNewDir)) {
             core.setFailed(`Unable to find 'whatsnew' directory @ ${whatsNewDir}`);
@@ -122,7 +71,7 @@ async function run() {
             applicationId: packageName,
             track: track,
             inAppUpdatePriority: inAppUpdatePriorityInt || 0,
-            userFraction: userFraction,
+            userFraction: userFractionFloat,
             whatsNewDir: whatsNewDir,
             mappingFile: mappingFile,
             debugSymbols: debugSymbols,
@@ -150,35 +99,27 @@ async function run() {
     }
 }
 
-function inferStatus(userFraction: number): string {
-    let status: string
-    if (userFraction >= 1.0) {
-        status = 'completed'
-    } else if (userFraction > 0) {
-        status = 'inProgress'
-    } else {
-        status = 'halted'
+async function validateServiceAccountJson(serviceAccountJsonRaw: string | undefined, serviceAccountJson: string | undefined) {
+    if (serviceAccountJson && serviceAccountJsonRaw) {
+        // If the user provided both, print a warning one will be ignored
+        core.warning('Both \'serviceAccountJsonPlainText\' and \'serviceAccountJson\' were provided! \'serviceAccountJson\' will be ignored.')
     }
-    core.info("Inferred status to be " + status)
-    return status
-}
 
-function validateStatus(status: string, userFraction: number): boolean {
-    switch (status) {
-        case 'completed':
-            if (userFraction < 1.0) {
-                core.setFailed("Status 'completed' requires 'userFraction' 1.0")
-                return false
-            }
-            break
-        case 'inProgress':
-            if (userFraction >= 1.0 && userFraction <= 0) {
-                core.setFailed("Status 'inProgress' requires 'userFraction' between 0.0 and 1.0")
-                return false
-            }
-            break
+    if (serviceAccountJsonRaw) {
+        // If the user has provided the raw plain text, then write to file and set appropriate env variable
+        const serviceAccountFile = "./serviceAccountJson.json";
+        await writeFile(serviceAccountFile, serviceAccountJsonRaw, {
+            encoding: 'utf8'
+        });
+        core.exportVariable("GOOGLE_APPLICATION_CREDENTIALS", serviceAccountFile)
+    } else if (serviceAccountJson) {
+        // If the user has provided the json path, then set appropriate env variable
+        core.exportVariable("GOOGLE_APPLICATION_CREDENTIALS", serviceAccountJson)
+    } else {
+        // If the user provided neither, fail and exit
+        core.setFailed("You must provide one of 'serviceAccountJsonPlainText' or 'serviceAccountJson' to use this action")
+        exit()
     }
-    return true
 }
 
 void run();