From e08234749a5418f07b5116fa3fc4bc7d1121ff01 Mon Sep 17 00:00:00 2001 From: Tanner Barlow Date: Wed, 4 Nov 2020 10:33:23 -0800 Subject: [PATCH] feat: Use relative paths for local file assets --- package-lock.json | 12 +++- package.json | 1 + src/common/mockFactory.ts | 1 + .../providers/storage/localFileSystem.test.ts | 56 ++++++++++++++++++- .../providers/storage/localFileSystem.ts | 12 ++-- .../electronProxyHandler.test.ts | 2 +- .../activeLearning/electronProxyHandler.ts | 4 +- .../activeLearning/objectDetection.ts | 2 +- .../storage/assetProviderFactory.test.ts | 2 +- src/providers/storage/assetProviderFactory.ts | 1 + src/providers/storage/azureBlobStorage.ts | 4 +- .../storage/localFileSystemProxy.test.ts | 37 ++++++++++++ src/providers/storage/localFileSystemProxy.ts | 28 ++++++++-- .../storage/storageProviderFactory.test.ts | 2 +- .../pages/connections/connectionsPage.tsx | 9 +++ src/services/assetService.test.ts | 17 ++++++ src/services/assetService.ts | 29 +++++----- 17 files changed, 186 insertions(+), 33 deletions(-) diff --git a/package-lock.json b/package-lock.json index b4517375..1533a6ba 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11916,6 +11916,12 @@ } } }, + "mock-fs": { + "version": "4.13.0", + "resolved": "https://registry.npmjs.org/mock-fs/-/mock-fs-4.13.0.tgz", + "integrity": "sha512-DD0vOdofJdoaRNtnWcrXe6RQbpHkPPmtqGq14uRX0F8ZKJ5nv89CVTYl/BZdppDxBDaV0hl75htg3abpEWlPZA==", + "dev": true + }, "moo": { "version": "0.4.3", "resolved": "https://registry.npmjs.org/moo/-/moo-0.4.3.tgz", @@ -12097,7 +12103,7 @@ "dependencies": { "semver": { "version": "5.3.0", - "resolved": "http://registry.npmjs.org/semver/-/semver-5.3.0.tgz", + "resolved": "https://registry.npmjs.org/semver/-/semver-5.3.0.tgz", "integrity": "sha1-myzl094C0XxgEq0yaqa00M9U+U8=", "dev": true } @@ -12234,7 +12240,7 @@ }, "chalk": { "version": "1.1.3", - "resolved": "http://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz", + "resolved": "https://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz", "integrity": "sha1-qBFcVeSnAv5NFQq9OHKCKn4J/Jg=", "dev": true, "requires": { @@ -17293,7 +17299,7 @@ "dependencies": { "source-map": { "version": "0.4.4", - "resolved": "http://registry.npmjs.org/source-map/-/source-map-0.4.4.tgz", + "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.4.4.tgz", "integrity": "sha1-66T12pwNyZneaAMti092FzZSA2s=", "dev": true, "requires": { diff --git a/package.json b/package.json index 757e2606..05938203 100644 --- a/package.json +++ b/package.json @@ -119,6 +119,7 @@ "foreman": "^3.0.1", "jest-enzyme": "^7.0.1", "jquery": "^3.3.1", + "mock-fs": "^4.13.0", "node-sass": "^4.14.1", "popper.js": "^1.14.6", "redux-immutable-state-invariant": "^2.1.0", diff --git a/src/common/mockFactory.ts b/src/common/mockFactory.ts index f3902f0b..5630b060 100644 --- a/src/common/mockFactory.ts +++ b/src/common/mockFactory.ts @@ -397,6 +397,7 @@ export default class MockFactory { public static createLocalFileSystemOptions(): ILocalFileSystemProxyOptions { return { folderPath: "C:\\projects\\vott\\project", + relativePath: false, }; } diff --git a/src/electron/providers/storage/localFileSystem.test.ts b/src/electron/providers/storage/localFileSystem.test.ts index 4e3a9015..33c67045 100644 --- a/src/electron/providers/storage/localFileSystem.test.ts +++ b/src/electron/providers/storage/localFileSystem.test.ts @@ -1,7 +1,8 @@ import fs from "fs"; -import path from "path"; +import path, { relative } from "path"; import shortid from "shortid"; import LocalFileSystem from "./localFileSystem"; +import mockFs from "mock-fs"; jest.mock("electron", () => ({ dialog: { @@ -9,14 +10,36 @@ jest.mock("electron", () => ({ }, })); import { dialog } from "electron"; +import { AssetService } from "../../../services/assetService"; describe("LocalFileSystem Storage Provider", () => { let localFileSystem: LocalFileSystem = null; + const sourcePath = path.join("path", "to", "my", "source"); beforeEach(() => { localFileSystem = new LocalFileSystem(null); }); + beforeAll(() => { + mockFs({ + path: { + to: { + my: { + source: { + "file1.jpg": "contents", + "file2.jpg": "contents", + "file3.jpg": "contents", + }, + }, + }, + }, + }); + }); + + afterAll(() => { + mockFs.restore(); + }); + it("writes, reads and deletes a file as text", async () => { const filePath = path.join(process.cwd(), "test-output", `${shortid.generate()}.json`); const contents = { @@ -89,4 +112,35 @@ describe("LocalFileSystem Storage Provider", () => { it("deleting file that doesn't exist resolves successfully", async () => { await expect(localFileSystem.deleteFile("/path/to/fake/file.txt")).resolves.not.toBeNull(); }); + + it("getAssets uses an absolute path when relative not specified", async () => { + AssetService.createAssetFromFilePath = jest.fn(() => []); + await localFileSystem.getAssets(sourcePath); + const calls: any[] = (AssetService.createAssetFromFilePath as any).mock.calls; + expect(calls).toHaveLength(3); + calls.forEach((call, index) => { + const absolutePath = path.join(sourcePath, `file${index + 1}.jpg`); + expect(call).toEqual([ + absolutePath, + undefined, + absolutePath, + ]); + }); + }); + + it("getAssets uses a path relative to the source connection when specified", async () => { + AssetService.createAssetFromFilePath = jest.fn(() => []); + await localFileSystem.getAssets(sourcePath, true); + const calls: any[] = (AssetService.createAssetFromFilePath as any).mock.calls; + expect(calls).toHaveLength(3); + calls.forEach((call, index) => { + const relativePath = `file${index + 1}.jpg`; + const absolutePath = path.join(sourcePath, relativePath); + expect(call).toEqual([ + absolutePath, + undefined, + relativePath, + ]); + }); + }); }); diff --git a/src/electron/providers/storage/localFileSystem.ts b/src/electron/providers/storage/localFileSystem.ts index 0367cc82..8efdc772 100644 --- a/src/electron/providers/storage/localFileSystem.ts +++ b/src/electron/providers/storage/localFileSystem.ts @@ -3,9 +3,10 @@ import fs from "fs"; import path from "path"; import rimraf from "rimraf"; import { IStorageProvider } from "../../../providers/storage/storageProviderFactory"; -import { IAsset, AssetType, StorageType } from "../../../models/applicationState"; +import { IAsset, AssetType, StorageType, IConnection } from "../../../models/applicationState"; import { AssetService } from "../../../services/assetService"; import { strings } from "../../../common/strings"; +import { ILocalFileSystemProxyOptions } from "../../../providers/storage/localFileSystemProxy"; export default class LocalFileSystem implements IStorageProvider { public storageType: StorageType.Local; @@ -136,9 +137,12 @@ export default class LocalFileSystem implements IStorageProvider { }); } - public async getAssets(folderPath?: string): Promise { - return (await this.listFiles(path.normalize(folderPath))) - .map((filePath) => AssetService.createAssetFromFilePath(filePath)) + public async getAssets(sourceConnectionFolderPath?: string, relativePath: boolean = false): Promise { + return (await this.listFiles(path.normalize(sourceConnectionFolderPath))) + .map((filePath) => AssetService.createAssetFromFilePath( + filePath, + undefined, + relativePath ? path.relative(sourceConnectionFolderPath, filePath) : filePath)) .filter((asset) => asset.type !== AssetType.Unknown); } diff --git a/src/providers/activeLearning/electronProxyHandler.test.ts b/src/providers/activeLearning/electronProxyHandler.test.ts index 17eade6e..40c9f943 100644 --- a/src/providers/activeLearning/electronProxyHandler.test.ts +++ b/src/providers/activeLearning/electronProxyHandler.test.ts @@ -18,7 +18,7 @@ describe("Load default model from filesystem with TF io.IOHandler", () => { return Promise.resolve([]); }); - const handler = new ElectronProxyHandler("folder"); + const handler = new ElectronProxyHandler("folder", false); try { const model = await tf.loadGraphModel(handler); } catch (_) { diff --git a/src/providers/activeLearning/electronProxyHandler.ts b/src/providers/activeLearning/electronProxyHandler.ts index cbda70f5..9ed8e9e9 100644 --- a/src/providers/activeLearning/electronProxyHandler.ts +++ b/src/providers/activeLearning/electronProxyHandler.ts @@ -4,8 +4,8 @@ import { LocalFileSystemProxy, ILocalFileSystemProxyOptions } from "../../provid export class ElectronProxyHandler implements tfc.io.IOHandler { protected readonly provider: LocalFileSystemProxy; - constructor(folderPath: string) { - const options: ILocalFileSystemProxyOptions = { folderPath }; + constructor(folderPath: string, relativePath: boolean) { + const options: ILocalFileSystemProxyOptions = { folderPath, relativePath }; this.provider = new LocalFileSystemProxy(options); } diff --git a/src/providers/activeLearning/objectDetection.ts b/src/providers/activeLearning/objectDetection.ts index 196db45a..cc1433f2 100755 --- a/src/providers/activeLearning/objectDetection.ts +++ b/src/providers/activeLearning/objectDetection.ts @@ -53,7 +53,7 @@ export class ObjectDetection { const response = await axios.get(modelFolderPath + "/classes.json"); this.jsonClasses = JSON.parse(JSON.stringify(response.data)); } else { - const handler = new ElectronProxyHandler(modelFolderPath); + const handler = new ElectronProxyHandler(modelFolderPath, false); this.model = await tf.loadGraphModel(handler); this.jsonClasses = await handler.loadClasses(); } diff --git a/src/providers/storage/assetProviderFactory.test.ts b/src/providers/storage/assetProviderFactory.test.ts index 014b17c5..20566247 100644 --- a/src/providers/storage/assetProviderFactory.test.ts +++ b/src/providers/storage/assetProviderFactory.test.ts @@ -25,7 +25,7 @@ class TestAssetProvider implements IAssetProvider { public initialize(): Promise { throw new Error("Method not implemented"); } - public getAssets(containerName?: string): Promise { + public getAssets(): Promise { throw new Error("Method not implemented."); } } diff --git a/src/providers/storage/assetProviderFactory.ts b/src/providers/storage/assetProviderFactory.ts index bfe11192..59418e13 100644 --- a/src/providers/storage/assetProviderFactory.ts +++ b/src/providers/storage/assetProviderFactory.ts @@ -10,6 +10,7 @@ import getHostProcess, { HostProcessType } from "../../common/hostProcess"; export interface IAssetProvider { initialize?(): Promise; getAssets(containerName?: string): Promise; + addDefaultPropsToNewConnection?(connection: IConnection): IConnection; } /** diff --git a/src/providers/storage/azureBlobStorage.ts b/src/providers/storage/azureBlobStorage.ts index a81510c3..2c5f802b 100644 --- a/src/providers/storage/azureBlobStorage.ts +++ b/src/providers/storage/azureBlobStorage.ts @@ -191,8 +191,8 @@ export class AzureBlobStorage implements IStorageProvider { * @param containerName - Container from which to retrieve assets. Defaults to * container specified in Azure Cloud Storage options */ - public async getAssets(containerName?: string): Promise { - containerName = (containerName) ? containerName : this.options.containerName; + public async getAssets(): Promise { + const { containerName } = this.options; const files = await this.listFiles(containerName); const result: IAsset[] = []; for (const file of files) { diff --git a/src/providers/storage/localFileSystemProxy.test.ts b/src/providers/storage/localFileSystemProxy.test.ts index d7c48572..7beaec2c 100644 --- a/src/providers/storage/localFileSystemProxy.test.ts +++ b/src/providers/storage/localFileSystemProxy.test.ts @@ -2,6 +2,7 @@ import { IpcRendererProxy } from "../../common/ipcRendererProxy"; import { LocalFileSystemProxy, ILocalFileSystemProxyOptions } from "./localFileSystemProxy"; import { StorageProviderFactory } from "./storageProviderFactory"; import registerProviders from "../../registerProviders"; +import MockFactory from "../../common/mockFactory"; describe("LocalFileSystem Proxy Storage Provider", () => { it("Provider is registered with the StorageProviderFactory", () => { @@ -19,6 +20,7 @@ describe("LocalFileSystem Proxy Storage Provider", () => { let provider: LocalFileSystemProxy = null; const options: ILocalFileSystemProxyOptions = { folderPath: "/test", + relativePath: false, }; beforeEach(() => { @@ -122,5 +124,40 @@ describe("LocalFileSystem Proxy Storage Provider", () => { expect(IpcRendererProxy.send).toBeCalledWith("LocalFileSystem:listContainers", [expectedContainerPath]); expect(actualFolders).toEqual(expectedFolders); }); + + it("sends relative path argument according to options", async () => { + const sendFunction = jest.fn(); + IpcRendererProxy.send = sendFunction; + await provider.getAssets(); + const { folderPath, relativePath } = options; + expect(IpcRendererProxy.send).toBeCalledWith("LocalFileSystem:getAssets", [folderPath, relativePath]); + sendFunction.mockReset(); + + const newFolderPath = "myFolder"; + const newRelativePath = true; + + const relativeProvider = new LocalFileSystemProxy({ + folderPath: newFolderPath, + relativePath: newRelativePath, + }); + await relativeProvider.getAssets(); + expect(IpcRendererProxy.send).toBeCalledWith("LocalFileSystem:getAssets", [newFolderPath, newRelativePath]); + }); + + it("adds default props to a new connection", () => { + const connection = MockFactory.createTestConnection(); + delete connection.providerOptions["relativePath"]; + expect(connection).not.toHaveProperty("providerOptions.relativePath"); + delete connection.id; + expect(provider.addDefaultPropsToNewConnection(connection)) + .toHaveProperty("providerOptions.relativePath", true); + }); + + it("does not add default props to existing connection", () => { + const connection = MockFactory.createTestConnection(); + delete connection.providerOptions["relativePath"]; + expect(provider.addDefaultPropsToNewConnection(connection)) + .not.toHaveProperty("providerOptions.relativePath"); + }); }); }); diff --git a/src/providers/storage/localFileSystemProxy.ts b/src/providers/storage/localFileSystemProxy.ts index 6247a9e6..be00f3ac 100644 --- a/src/providers/storage/localFileSystemProxy.ts +++ b/src/providers/storage/localFileSystemProxy.ts @@ -1,7 +1,7 @@ import { IpcRendererProxy } from "../../common/ipcRendererProxy"; import { IStorageProvider } from "./storageProviderFactory"; import { IAssetProvider } from "./assetProviderFactory"; -import { IAsset, StorageType } from "../../models/applicationState"; +import { IAsset, IConnection, StorageType } from "../../models/applicationState"; const PROXY_NAME = "LocalFileSystem"; @@ -11,6 +11,7 @@ const PROXY_NAME = "LocalFileSystem"; */ export interface ILocalFileSystemProxyOptions { folderPath: string; + relativePath: boolean; } /** @@ -26,6 +27,7 @@ export class LocalFileSystemProxy implements IStorageProvider, IAssetProvider { if (!this.options) { this.options = { folderPath: null, + relativePath: false, }; } } @@ -125,8 +127,26 @@ export class LocalFileSystemProxy implements IStorageProvider, IAssetProvider { * Retrieve assets from directory * @param folderName - Directory containing assets */ - public getAssets(folderName?: string): Promise { - const folderPath = [this.options.folderPath, folderName].join("/"); - return IpcRendererProxy.send(`${PROXY_NAME}:getAssets`, [folderPath]); + public getAssets(): Promise { + const { folderPath, relativePath } = this.options; + return IpcRendererProxy.send(`${PROXY_NAME}:getAssets`, [folderPath, relativePath]); + } + + /** + * Adds default properties to new connections + * + * Currently adds `relativePath: true` to the providerOptions. Pre-existing connections + * will only use absolute path + * + * @param connection Connection + */ + public addDefaultPropsToNewConnection(connection: IConnection): IConnection { + return connection.id ? connection : { + ...connection, + providerOptions: { + ...connection.providerOptions, + relativePath: true, + } as any, + }; } } diff --git a/src/providers/storage/storageProviderFactory.test.ts b/src/providers/storage/storageProviderFactory.test.ts index 6e8b99ae..de45e224 100644 --- a/src/providers/storage/storageProviderFactory.test.ts +++ b/src/providers/storage/storageProviderFactory.test.ts @@ -51,7 +51,7 @@ class TestStorageProvider implements IStorageProvider { public deleteContainer(folderPath: string): Promise { throw new Error("Method not implemented."); } - public getAssets(containerName?: string): Promise { + public getAssets(): Promise { throw new Error("Method not implemented."); } } diff --git a/src/react/components/pages/connections/connectionsPage.tsx b/src/react/components/pages/connections/connectionsPage.tsx index ebd8da2d..99388905 100644 --- a/src/react/components/pages/connections/connectionsPage.tsx +++ b/src/react/components/pages/connections/connectionsPage.tsx @@ -11,6 +11,7 @@ import ConnectionForm from "./connectionForm"; import ConnectionItem from "./connectionItem"; import "./connectionsPage.scss"; import { toast } from "react-toastify"; +import { AssetProviderFactory } from "../../../../providers/storage/assetProviderFactory"; /** * Properties for Connection Page @@ -134,12 +135,20 @@ export default class ConnectionPage extends React.Component { + connection = this.addDefaultPropsIfNewConnection(connection); await this.props.actions.saveConnection(connection); toast.success(interpolate(strings.connections.messages.saveSuccess, { connection })); this.props.history.goBack(); } + private addDefaultPropsIfNewConnection(connection: IConnection): IConnection { + const assetProvider = AssetProviderFactory.createFromConnection(connection); + return !connection.id && assetProvider.addDefaultPropsToNewConnection + ? assetProvider.addDefaultPropsToNewConnection(connection) + : connection; + } + private onFormCancel() { this.props.history.goBack(); } diff --git a/src/services/assetService.test.ts b/src/services/assetService.test.ts index 58c7e671..b4ebf557 100644 --- a/src/services/assetService.test.ts +++ b/src/services/assetService.test.ts @@ -9,6 +9,7 @@ import HtmlFileReader from "../common/htmlFileReader"; import { encodeFileURI } from "../common/utils"; import _ from "lodash"; import registerMixins from "../registerMixins"; +import MD5 from "md5.js"; describe("Asset Service", () => { describe("Static Methods", () => { @@ -24,6 +25,22 @@ describe("Asset Service", () => { expect(asset.format).toEqual("jpg"); }); + it("creates an asset using file path as identifier", () => { + const path = "c:/dir1/dir2/asset1.jpg"; + const asset = AssetService.createAssetFromFilePath(path); + const expectedIdenfifier = `file:${path}`; + const expectedId = new MD5().update(expectedIdenfifier).digest("hex"); + expect(asset.id).toEqual(expectedId); + }); + + it("creates an asset using passed in identifier", () => { + const path = "C:\\dir1\\dir2\\asset1.jpg"; + const identifier = "asset1.jpg"; + const asset = AssetService.createAssetFromFilePath(path, undefined, identifier); + const expectedId = new MD5().update(identifier).digest("hex"); + expect(asset.id).toEqual(expectedId); + }); + it("creates an asset from an encoded file", () => { const path = "C:\\dir1\\dir2\\asset%201.jpg"; const asset = AssetService.createAssetFromFilePath(path); diff --git a/src/services/assetService.ts b/src/services/assetService.ts index 2ab0ffab..5d2b1ef6 100644 --- a/src/services/assetService.ts +++ b/src/services/assetService.ts @@ -23,13 +23,15 @@ export class AssetService { /** * Create IAsset from filePath - * @param filePath - filepath of asset - * @param fileName - name of asset + * @param assetFilePath - filepath of asset + * @param assetFileName - name of asset */ - public static createAssetFromFilePath(filePath: string, fileName?: string): IAsset { - Guard.empty(filePath); - - const normalizedPath = filePath.toLowerCase(); + public static createAssetFromFilePath( + assetFilePath: string, + assetFileName?: string, + assetIdentifier?: string): IAsset { + Guard.empty(assetFilePath); + const normalizedPath = assetFilePath.toLowerCase(); // If the path is not already prefixed with a protocol // then assume it comes from the local file system @@ -37,17 +39,18 @@ export class AssetService { !normalizedPath.startsWith("https://") && !normalizedPath.startsWith("file:")) { // First replace \ character with / the do the standard url encoding then encode unsupported characters - filePath = encodeFileURI(filePath, true); + assetFilePath = encodeFileURI(assetFilePath, true); } + assetIdentifier = assetIdentifier || assetFilePath; - const md5Hash = new MD5().update(filePath).digest("hex"); - const pathParts = filePath.split(/[\\\/]/); + const md5Hash = new MD5().update(assetIdentifier).digest("hex"); + const pathParts = assetFilePath.split(/[\\\/]/); // Example filename: video.mp4#t=5 // fileNameParts[0] = "video" // fileNameParts[1] = "mp4" // fileNameParts[2] = "t=5" - fileName = fileName || pathParts[pathParts.length - 1]; - const fileNameParts = fileName.split("."); + assetFileName = assetFileName || pathParts[pathParts.length - 1]; + const fileNameParts = assetFileName.split("."); const extensionParts = fileNameParts[fileNameParts.length - 1].split(/[\?#]/); const assetFormat = extensionParts[0]; @@ -58,8 +61,8 @@ export class AssetService { format: assetFormat, state: AssetState.NotVisited, type: assetType, - name: fileName, - path: filePath, + name: assetFileName, + path: assetFilePath, size: null, }; }