From 2fc57718a687e57dbc2e8cda0d9ff6b5fe6ee64f Mon Sep 17 00:00:00 2001 From: Krzysztof Wolski Date: Fri, 2 Sep 2022 15:39:15 +0200 Subject: [PATCH] Code review changes --- src/APL/fileAPL.test.ts | 114 +++++++++++++------------------------- src/APL/fileAPL.ts | 91 +++++++++++++++--------------- src/APL/vercelAPL.test.ts | 81 +++++++++++++-------------- src/APL/vercelAPL.ts | 70 ++++++++++++----------- 4 files changed, 155 insertions(+), 201 deletions(-) diff --git a/src/APL/fileAPL.test.ts b/src/APL/fileAPL.test.ts index 280107b..84c4d73 100644 --- a/src/APL/fileAPL.test.ts +++ b/src/APL/fileAPL.test.ts @@ -1,7 +1,7 @@ import { promises as fsPromises } from "fs"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { FileAPL, loadDataFromFile, saveDataToFile } from "./fileAPL"; +import { FileAPL } from "./fileAPL"; const stubAuthData = { domain: "example.com", @@ -9,10 +9,20 @@ const stubAuthData = { }; describe("APL", () => { + afterEach(() => { + vi.clearAllMocks(); + }); + describe("FileAPL", () => { describe("get", () => { - afterEach(() => { - vi.clearAllMocks(); + it("Should throw error when JSON parse fails", async () => { + vi.spyOn(fsPromises, "access").mockResolvedValue(); + vi.spyOn(fsPromises, "readFile").mockResolvedValue("Not a valid JSON"); + + const apl = new FileAPL(); + await expect(apl.get(stubAuthData.domain)).rejects.toThrow( + "File APL could not read auth data from the .saleor-app-auth.json file" + ); }); it("Returns auth data for existing domain", async () => { @@ -34,11 +44,32 @@ describe("APL", () => { }); }); - describe("delete", () => { - afterEach(() => { - vi.clearAllMocks(); - }); + describe("set", () => { + it("Handle write file errors", async () => { + const spyWriteFile = vi.spyOn(fsPromises, "writeFile").mockImplementation(() => { + throw Error("Write error"); + }); + const apl = new FileAPL(); + + await expect(apl.set(stubAuthData)).rejects.toThrow( + "File APL was unable to save auth data" + ); + expect(spyWriteFile).toBeCalledWith(".saleor-app-auth.json", JSON.stringify(stubAuthData)); + }); + }); + + it("Successfully save to file", async () => { + const spyWriteFile = vi.spyOn(fsPromises, "writeFile").mockResolvedValue(); + + const apl = new FileAPL(); + + await expect(apl.set(stubAuthData)); + + expect(spyWriteFile).toBeCalledWith(".saleor-app-auth.json", JSON.stringify(stubAuthData)); + }); + + describe("delete", () => { it("Should override file when called with known domain", async () => { vi.spyOn(fsPromises, "access").mockResolvedValue(); vi.spyOn(fsPromises, "readFile").mockResolvedValue(JSON.stringify(stubAuthData)); @@ -66,10 +97,6 @@ describe("APL", () => { }); describe("getAll", () => { - afterEach(() => { - vi.clearAllMocks(); - }); - it("Should return list with one item when auth data are existing", async () => { vi.spyOn(fsPromises, "access").mockResolvedValue(); vi.spyOn(fsPromises, "readFile").mockResolvedValue(JSON.stringify(stubAuthData)); @@ -89,69 +116,4 @@ describe("APL", () => { }); }); }); - - describe("FileAPL utils", () => { - describe("saveDataToFile", () => { - afterEach(() => { - vi.clearAllMocks(); - }); - - it("Save existing auth data to file", async () => { - const spyWriteFile = vi.spyOn(fsPromises, "writeFile").mockResolvedValue(); - await saveDataToFile("test.json", stubAuthData); - - expect(spyWriteFile).toBeCalledWith("test.json", JSON.stringify(stubAuthData)); - }); - - it("Save empty file when no auth data provided", async () => { - const spyWriteFile = vi.spyOn(fsPromises, "writeFile").mockResolvedValue(); - await saveDataToFile("test.json"); - expect(spyWriteFile).toBeCalledWith("test.json", "{}"); - }); - - it("Handle write file errors", async () => { - const spyWriteFile = vi.spyOn(fsPromises, "writeFile").mockImplementation(() => { - throw Error("Write error"); - }); - await saveDataToFile("test.json"); - expect(spyWriteFile).toBeCalledWith("test.json", "{}"); - }); - }); - - describe("loadDataFromFile", () => { - afterEach(() => { - vi.clearAllMocks(); - }); - - it("Load existing auth data", async () => { - vi.spyOn(fsPromises, "access").mockResolvedValue(); - vi.spyOn(fsPromises, "readFile").mockResolvedValue(JSON.stringify(stubAuthData)); - expect(await loadDataFromFile("test.json")).toStrictEqual(stubAuthData); - }); - - it("Should return undefined when JSON parse fails", async () => { - vi.spyOn(fsPromises, "access").mockResolvedValue(); - vi.spyOn(fsPromises, "readFile").mockResolvedValue("Not a valid JSON"); - expect(await loadDataFromFile("test.json")).toBe(undefined); - }); - - it("Should return undefined when auth token is missing", async () => { - vi.spyOn(fsPromises, "access").mockResolvedValue(); - const stubInvalidAuthData = { - domain: "example.com", - }; - vi.spyOn(fsPromises, "readFile").mockResolvedValue(JSON.stringify(stubInvalidAuthData)); - expect(await loadDataFromFile("test.json")).toBe(undefined); - }); - - it("Should return undefined when domain is missing", async () => { - vi.spyOn(fsPromises, "access").mockResolvedValue(); - const stubInvalidAuthData = { - token: "token", - }; - vi.spyOn(fsPromises, "readFile").mockResolvedValue(JSON.stringify(stubInvalidAuthData)); - expect(await loadDataFromFile("test.json")).toBe(undefined); - }); - }); - }); }); diff --git a/src/APL/fileAPL.ts b/src/APL/fileAPL.ts index 5699238..a1653d7 100644 --- a/src/APL/fileAPL.ts +++ b/src/APL/fileAPL.ts @@ -5,46 +5,6 @@ import { APL, AuthData } from "./apl"; const debug = debugPkg.debug("app-sdk:FileAPL"); -/** - * Load auth data from a file and return it as AuthData format. - * In case of incomplete or invalid data, return `undefined`. - * - * @param {string} fileName - */ -export const loadDataFromFile = async (fileName: string): Promise => { - debug(`Load auth data from the ${fileName} file`); - let parsedData: Record = {}; - try { - await fsPromises.access(fileName); - parsedData = JSON.parse(await fsPromises.readFile(fileName, "utf-8")); - } catch (err) { - debug(`Could not read auth data from the ${fileName} file`, err); - return undefined; - } - const { token, domain } = parsedData; - if (token && domain) { - return { token, domain }; - } - return undefined; -}; - -/** - * Save auth data to file. - * When `authData` argument is empty, will overwrite file with empty values. - * - * @param {string} fileName - * @param {AuthData} [authData] - */ -export const saveDataToFile = async (fileName: string, authData?: AuthData) => { - debug(`Save auth data to the ${fileName} file`); - const newData = authData ? JSON.stringify(authData) : "{}"; - try { - await fsPromises.writeFile(fileName, newData); - } catch (err) { - debug(`Could not save auth data to the ${fileName} file`, err); - } -}; - export type FileAPLConfig = { fileName?: string; }; @@ -68,8 +28,49 @@ export class FileAPL implements APL { this.fileName = config?.fileName || ".saleor-app-auth.json"; } + /** + * Load auth data from a file and return it as AuthData format. + * In case of incomplete or invalid data, return `undefined`. + * + * @param {string} fileName + */ + private async loadDataFromFile(): Promise { + debug(`Load auth data from the ${this.fileName} file`); + let parsedData: Record = {}; + try { + await fsPromises.access(this.fileName); + parsedData = JSON.parse(await fsPromises.readFile(this.fileName, "utf-8")); + } catch (err) { + debug(`Could not read auth data from the ${this.fileName} file`, err); + throw new Error(`File APL could not read auth data from the ${this.fileName} file`); + } + const { token, domain } = parsedData; + if (token && domain) { + return { token, domain }; + } + return undefined; + } + + /** + * Save auth data to file. + * When `authData` argument is empty, will overwrite file with empty values. + * + * @param {string} fileName + * @param {AuthData} [authData] + */ + private async saveDataToFile(authData?: AuthData) { + debug(`Save auth data to the ${this.fileName} file`); + const newData = authData ? JSON.stringify(authData) : "{}"; + try { + await fsPromises.writeFile(this.fileName, newData); + } catch (err) { + debug(`Could not save auth data to the ${this.fileName} file`, err); + throw new Error("File APL was unable to save auth data"); + } + } + async get(domain: string) { - const authData = await loadDataFromFile(this.fileName); + const authData = await this.loadDataFromFile(); if (domain === authData?.domain) { return authData; } @@ -77,19 +78,19 @@ export class FileAPL implements APL { } async set(authData: AuthData) { - await saveDataToFile(this.fileName, authData); + await this.saveDataToFile(authData); } async delete(domain: string) { - const authData = await loadDataFromFile(this.fileName); + const authData = await this.loadDataFromFile(); if (domain === authData?.domain) { - await saveDataToFile(this.fileName); + await this.saveDataToFile(); } } async getAll() { - const authData = await loadDataFromFile(this.fileName); + const authData = await this.loadDataFromFile(); if (!authData) { return []; diff --git a/src/APL/vercelAPL.test.ts b/src/APL/vercelAPL.test.ts index 320e15d..af55b4e 100644 --- a/src/APL/vercelAPL.test.ts +++ b/src/APL/vercelAPL.test.ts @@ -1,12 +1,6 @@ import { afterEach, describe, expect, it } from "vitest"; -import { - DOMAIN_VARIABLE_NAME, - SALEOR_DEPLOYMENT_TOKEN, - SALEOR_REGISTER_APP_URL, - TOKEN_VARIABLE_NAME, - VercelAPL, -} from "./vercelAPL"; +import { VercelAPL, VercelAPLVariables } from "./vercelAPL"; const aplConfig = { deploymentToken: "token", @@ -19,58 +13,63 @@ const stubAuthData = { }; describe("APL", () => { + const initialEnv = { ...process.env }; + + afterEach(() => { + process.env = { ...initialEnv }; + }); + describe("VercelAPL", () => { describe("constructor", () => { - const initialEnv = { ...process.env }; - - afterEach(() => { - process.env = { ...initialEnv }; - }); - it("Raise an error when configuration is missing", async () => { - delete process.env[SALEOR_REGISTER_APP_URL]; - process.env[SALEOR_DEPLOYMENT_TOKEN] = "token"; + delete process.env[VercelAPLVariables.SALEOR_REGISTER_APP_URL]; + process.env[VercelAPLVariables.SALEOR_DEPLOYMENT_TOKEN] = "token"; expect(() => new VercelAPL()).toThrow(); - process.env[SALEOR_REGISTER_APP_URL] = "http://example.com"; - delete process.env[SALEOR_DEPLOYMENT_TOKEN]; + process.env[VercelAPLVariables.SALEOR_REGISTER_APP_URL] = "http://example.com"; + delete process.env[VercelAPLVariables.SALEOR_DEPLOYMENT_TOKEN]; expect(() => new VercelAPL()).toThrow(); }); }); - it("Constructor with config values", async () => { + it("Constructs VercelAPL instance when deploymentToken and registerAppURL provided", async () => { expect(() => new VercelAPL(aplConfig)).not.toThrow(); }); - it("Constructor with config values from environment variables", async () => { - process.env[SALEOR_REGISTER_APP_URL] = aplConfig.registerAppURL; - process.env[SALEOR_DEPLOYMENT_TOKEN] = aplConfig.deploymentToken; + it("Constructs VercelAPL instance with config values from environment variables", async () => { + process.env[VercelAPLVariables.SALEOR_REGISTER_APP_URL] = aplConfig.registerAppURL; + process.env[VercelAPLVariables.SALEOR_DEPLOYMENT_TOKEN] = aplConfig.deploymentToken; expect(() => new VercelAPL()).not.toThrow(); }); + it("Test if constructor use options over environment variables", async () => { + process.env[VercelAPLVariables.SALEOR_REGISTER_APP_URL] = "environment"; + process.env[VercelAPLVariables.SALEOR_DEPLOYMENT_TOKEN] = "environment"; + + const apl = await new VercelAPL({ deploymentToken: "option", registerAppURL: "option" }); + // eslint-disable-next-line dot-notation + expect(apl["deploymentToken"]).toBe("option"); + // eslint-disable-next-line dot-notation + expect(apl["registerAppURL"]).toBe("option"); + }); + describe("get", () => { describe("Read existing auth data from env", () => { - const initialEnv = { ...process.env }; - - afterEach(() => { - process.env = { ...initialEnv }; - }); - it("Read existing auth data", async () => { - process.env[TOKEN_VARIABLE_NAME] = stubAuthData.token; - process.env[DOMAIN_VARIABLE_NAME] = stubAuthData.domain; + process.env[VercelAPLVariables.TOKEN_VARIABLE_NAME] = stubAuthData.token; + process.env[VercelAPLVariables.DOMAIN_VARIABLE_NAME] = stubAuthData.domain; - const apl = new VercelAPL(); + const apl = new VercelAPL(aplConfig); expect(await apl.get(stubAuthData.domain)).toStrictEqual(stubAuthData); }); it("Return undefined when unknown domain requested", async () => { - process.env[TOKEN_VARIABLE_NAME] = stubAuthData.token; - process.env[DOMAIN_VARIABLE_NAME] = stubAuthData.domain; + process.env[VercelAPLVariables.TOKEN_VARIABLE_NAME] = stubAuthData.token; + process.env[VercelAPLVariables.DOMAIN_VARIABLE_NAME] = stubAuthData.domain; const apl = new VercelAPL(aplConfig); @@ -78,8 +77,8 @@ describe("APL", () => { }); it("Return undefined when no data is defined", async () => { - delete process.env[TOKEN_VARIABLE_NAME]; - delete process.env[DOMAIN_VARIABLE_NAME]; + delete process.env[VercelAPLVariables.TOKEN_VARIABLE_NAME]; + delete process.env[VercelAPLVariables.DOMAIN_VARIABLE_NAME]; const apl = new VercelAPL(aplConfig); @@ -90,15 +89,9 @@ describe("APL", () => { describe("getAll", () => { describe("Read existing auth data from env", () => { - const initialEnv = { ...process.env }; - - afterEach(() => { - process.env = { ...initialEnv }; - }); - it("Read existing auth data", async () => { - process.env[TOKEN_VARIABLE_NAME] = stubAuthData.token; - process.env[DOMAIN_VARIABLE_NAME] = stubAuthData.domain; + process.env[VercelAPLVariables.TOKEN_VARIABLE_NAME] = stubAuthData.token; + process.env[VercelAPLVariables.DOMAIN_VARIABLE_NAME] = stubAuthData.domain; const apl = new VercelAPL(aplConfig); @@ -106,8 +99,8 @@ describe("APL", () => { }); it("Return empty list when no auth data are existing", async () => { - delete process.env[TOKEN_VARIABLE_NAME]; - delete process.env[DOMAIN_VARIABLE_NAME]; + delete process.env[VercelAPLVariables.TOKEN_VARIABLE_NAME]; + delete process.env[VercelAPLVariables.DOMAIN_VARIABLE_NAME]; const apl = new VercelAPL(aplConfig); diff --git a/src/APL/vercelAPL.ts b/src/APL/vercelAPL.ts index 1d425c9..6595182 100644 --- a/src/APL/vercelAPL.ts +++ b/src/APL/vercelAPL.ts @@ -6,14 +6,16 @@ import { APL, AuthData } from "./apl"; const debug = debugPkg.debug("app-sdk:VercelAPL"); -export const TOKEN_VARIABLE_NAME = "SALEOR_AUTH_TOKEN"; -export const DOMAIN_VARIABLE_NAME = "SALEOR_DOMAIN"; -export const SALEOR_REGISTER_APP_URL = "SALEOR_REGISTER_APP_URL"; -export const SALEOR_DEPLOYMENT_TOKEN = "SALEOR_DEPLOYMENT_TOKEN"; +export const VercelAPLVariables = { + TOKEN_VARIABLE_NAME: "SALEOR_AUTH_TOKEN", + DOMAIN_VARIABLE_NAME: "SALEOR_DOMAIN", + SALEOR_REGISTER_APP_URL: "SALEOR_REGISTER_APP_URL", + SALEOR_DEPLOYMENT_TOKEN: "SALEOR_DEPLOYMENT_TOKEN", +}; const getEnvAuth = (): AuthData | undefined => { - const token = process.env[TOKEN_VARIABLE_NAME]; - const domain = process.env[DOMAIN_VARIABLE_NAME]; + const token = process.env[VercelAPLVariables.TOKEN_VARIABLE_NAME]; + const domain = process.env[VercelAPLVariables.DOMAIN_VARIABLE_NAME]; if (!token || !domain) { return undefined; } @@ -23,30 +25,6 @@ const getEnvAuth = (): AuthData | undefined => { }; }; -const saveDataToVercel = async ( - registerAppURL: string, - deploymentToken: string, - authData?: AuthData -) => { - debug("Saving data to Vercel"); - - try { - await fetch(registerAppURL, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ - token: deploymentToken, - envs: { - [TOKEN_VARIABLE_NAME]: authData?.token || "", - [DOMAIN_VARIABLE_NAME]: authData?.domain || "", - }, - }), - }); - } catch (error) { - debug("Error during saving the data:", error); - } -}; - export type VercelAPLConfig = { registerAppURL?: string; deploymentToken?: string; @@ -71,14 +49,14 @@ export class VercelAPL implements APL { private deploymentToken: string; constructor(config?: VercelAPLConfig) { - const registerAppURL = config?.registerAppURL || process.env.SALEOR_REGISTER_APP_URL; + const registerAppURL = + config?.registerAppURL || process.env[VercelAPLVariables.SALEOR_REGISTER_APP_URL]; if (!registerAppURL) { - debug("Missing registerAppURL"); throw new Error("Misconfiguration: please provide registerAppUrl"); } - const deploymentToken = config?.deploymentToken || process.env.SALEOR_DEPLOYMENT_TOKEN; + const deploymentToken = + config?.deploymentToken || process.env[VercelAPLVariables.SALEOR_DEPLOYMENT_TOKEN]; if (!deploymentToken) { - debug("Missing deploymentToken"); throw new Error("Misconfiguration: please provide deploymentToken"); } @@ -86,6 +64,26 @@ export class VercelAPL implements APL { this.deploymentToken = deploymentToken; } + private async saveDataToVercel(authData?: AuthData) { + debug(`saveDataToVercel with: ${authData}`); + try { + await fetch(this.registerAppURL, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + token: this.deploymentToken, + envs: { + [VercelAPLVariables.TOKEN_VARIABLE_NAME]: authData?.token || "", + [VercelAPLVariables.DOMAIN_VARIABLE_NAME]: authData?.domain || "", + }, + }), + }); + } catch (error) { + debug("Error during saving the data:", error); + throw new Error(`VercelAPL was not able to save auth data${error}`); + } + } + async get(domain: string) { const authData = getEnvAuth(); @@ -96,13 +94,13 @@ export class VercelAPL implements APL { } async set(authData: AuthData) { - await saveDataToVercel(this.registerAppURL, this.deploymentToken, authData); + await this.saveDataToVercel(authData); } async delete(domain: string) { if (domain === getEnvAuth()?.domain) { // Override existing data with the empty values - await saveDataToVercel(this.registerAppURL, this.deploymentToken); + await this.saveDataToVercel(); } }