feat: Use relative paths for local file assets (#1027)

microsoft · Nov 4, 2020 · d48de94 · d48de94
1 parent 27ce840
commit d48de94
Show file tree

Hide file tree

Showing 17 changed files with 186 additions and 33 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git 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
@@ -398,6 +398,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
@@ -1,22 +1,45 @@
 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: {
         showOpenDialog: jest.fn(),
     },
 }));
 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
@@ -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<IAsset[]> {
-        return (await this.listFiles(path.normalize(folderPath)))
-            .map((filePath) => AssetService.createAssetFromFilePath(filePath))
+    public async getAssets(sourceConnectionFolderPath?: string, relativePath: boolean = false): Promise<IAsset[]> {
+        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
@@ -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
@@ -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
@@ -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
@@ -25,7 +25,7 @@ class TestAssetProvider implements IAssetProvider {
     public initialize(): Promise<void> {
         throw new Error("Method not implemented");
     }
-    public getAssets(containerName?: string): Promise<IAsset[]> {
+    public getAssets(): Promise<IAsset[]> {
         throw new Error("Method not implemented.");
     }
 }
diff --git 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<void>;
     getAssets(containerName?: string): Promise<IAsset[]>;
+    addDefaultPropsToNewConnection?(connection: IConnection): IConnection;
 }
 
 /**

diff --git 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<IAsset[]> {
-        containerName = (containerName) ? containerName : this.options.containerName;
+    public async getAssets(): Promise<IAsset[]> {
+        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
@@ -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
@@ -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<IAsset[]> {
-        const folderPath = [this.options.folderPath, folderName].join("/");
-        return IpcRendererProxy.send(`${PROXY_NAME}:getAssets`, [folderPath]);
+    public getAssets(): Promise<IAsset[]> {
+        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
@@ -51,7 +51,7 @@ class TestStorageProvider implements IStorageProvider {
     public deleteContainer(folderPath: string): Promise<void> {
         throw new Error("Method not implemented.");
     }
-    public getAssets(containerName?: string): Promise<IAsset[]> {
+    public getAssets(): Promise<IAsset[]> {
         throw new Error("Method not implemented.");
     }
 }
diff --git 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<IConnectionPageProps
     }
 
     private onFormSubmit = async (connection: IConnection) => {
+        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();
     }