Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ensure hashed directory for extension #7188

Merged
merged 10 commits into from
Feb 21, 2023
27 changes: 27 additions & 0 deletions packages/core/src/common/utils/find-key-contains.injectable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Copyright (c) OpenLens Authors. All rights reserved.
* Licensed under MIT License. See LICENSE in root directory for more information.
*/
import { getInjectable } from "@ogre-tools/injectable";

export type FindKeyContains = <T>(map: Map<string, T>, text: string) => T | undefined;

const findKeyContainsInjectable = getInjectable({
id: "find-key-contains",

instantiate: (): FindKeyContains => {
return <T>(map: Map<string, T>, text: string) => {
const entries = map.entries();

for (const [key, value] of entries) {
if (key.includes(text)) {
jweak marked this conversation as resolved.
Show resolved Hide resolved
return value;
}
}

return undefined;
};
},
});
jweak marked this conversation as resolved.
Show resolved Hide resolved

export default findKeyContainsInjectable;
50 changes: 50 additions & 0 deletions packages/core/src/common/utils/find-key-contains.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright (c) OpenLens Authors. All rights reserved.
* Licensed under MIT License. See LICENSE in root directory for more information.
*/
import { getDiForUnitTesting } from "../../renderer/getDiForUnitTesting";
import type { FindKeyContains } from "./find-key-contains.injectable";
import findKeyContainsInjectable from "./find-key-contains.injectable";

describe("find-key-contains", () => {
let map: Map<string, string>;

let findKeyContains: FindKeyContains;

beforeEach(() => {
const di = getDiForUnitTesting({ doGeneralOverrides: true });

findKeyContains = di.inject(findKeyContainsInjectable);
});

describe("map with entries", () => {
beforeEach(() => {
map = new Map();
map.set("some-key", "some-value");
});

it("given key starts with key, returns value", () => {
expect(findKeyContains(map, "some")).toEqual("some-value");
});

it("given key ends with key, returns value", () => {
expect(findKeyContains(map, "key")).toEqual("some-value");
});

it("given key is in middle of key, returns value", () => {
map.set("some-long-key", "some-value-for-long-key");

expect(findKeyContains(map, "long")).toEqual("some-value-for-long-key");
});

it("given key does not include text, returns undefined", () => {
expect(findKeyContains(map, "not")).toEqual(undefined);
});
});

describe("map with no entries", () => {
it("it returns undefined", () => {
expect(findKeyContains(map, "some-text")).toEqual(undefined);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import joinPathsInjectable from "../../../common/path/join-paths.injectable";
import directoryForExtensionDataInjectable from "./directory-for-extension-data.injectable";
import ensureDirInjectable from "../../../common/fs/ensure-dir.injectable";
import getHashInjectable from "./get-hash.injectable";
import findKeyContainsInjectable from "../../../common/utils/find-key-contains.injectable";

export type EnsureHashedDirectoryForExtension = (extensionName: string, registeredExtensions: ObservableMap<string, string>) => Promise<string>;

Expand All @@ -24,8 +25,15 @@ const ensureHashedDirectoryForExtensionInjectable = getInjectable({
const directoryForExtensionData = di.inject(directoryForExtensionDataInjectable);
const ensureDirectory = di.inject(ensureDirInjectable);
const getHash = di.inject(getHashInjectable);
const findKeyContains = di.inject(findKeyContainsInjectable)<string>;

return async (extensionName, registeredExtensions) => {
jweak marked this conversation as resolved.
Show resolved Hide resolved
const legacyDirPath = findKeyContains(registeredExtensions, extensionName);

if (legacyDirPath) {
return legacyDirPath;
jweak marked this conversation as resolved.
Show resolved Hide resolved
}

const dirPath = await getOrInsertWithAsync(registeredExtensions, extensionName, async () => {
const salt = (await randomBytes(32)).toString("hex");
const hashedName = getHash(`${extensionName}/${salt}`);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* Copyright (c) OpenLens Authors. All rights reserved.
* Licensed under MIT License. See LICENSE in root directory for more information.
*/
import type { ObservableMap } from "mobx";
import { observable, runInAction } from "mobx";
import { getDiForUnitTesting } from "../../../main/getDiForUnitTesting";
import type { EnsureHashedDirectoryForExtension } from "./ensure-hashed-directory-for-extension.injectable";
import ensureHashedDirectoryForExtensionInjectable from "./ensure-hashed-directory-for-extension.injectable";
import ensureDirInjectable from "../../../common/fs/ensure-dir.injectable";
import directoryForExtensionDataInjectable from "./directory-for-extension-data.injectable";

describe("ensure-hashed-directory-for-extension", () => {
Copy link
Contributor

@Iku-turso Iku-turso Feb 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking if case of NPM-packages with a namespaced name (eg. "name": "@some-namespace/some-package-name") will require their own tests and maybe implementations. WDYT?

let ensureHashedDirectoryForExtension: EnsureHashedDirectoryForExtension;
let ensureDirMock: jest.Mock;
let registeredExtensions: ObservableMap<string, string>;

beforeEach(() => {
const di = getDiForUnitTesting({ doGeneralOverrides: true });

ensureDirMock = jest.fn();

di.override(ensureDirInjectable, () => ensureDirMock);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Another way to test ensureDir and its effects would be to test it using those file system fakes. Generally it leads to bit more credible, but also sometimes easier to write tests.

di.override(directoryForExtensionDataInjectable, () => "some-directory-for-extension-data");

ensureHashedDirectoryForExtension = di.inject(
ensureHashedDirectoryForExtensionInjectable,
);

registeredExtensions = observable.map();
});

it("given registered extension exists, returns existing directory", async () => {
runInAction(() => {
registeredExtensions.set("some-extension-name", "some-directory");
});

const actual = await ensureHashedDirectoryForExtension(
"some-extension-name",
registeredExtensions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I wonder if registeredExtensions could be promoted to be a dependency instead of function parameter for a bit less complexity. There's only one set of those, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there only one? I'm not sure. That is a part of Store, does it have some kind main / renderer thing going on with syncing stuff. Can we move registeredExtensions from the Store to a dependency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There indeed is only one, because fileSystemProvisionerStoreInjectable is a singleton (that being the default lifecycle of an injectable).

This means we could extract eg. registeredExtensionsInjectable, and make it a dependency for both fileSystemProvisionerStoreInjectable and ensureHashedDirectoryForExtensionInjectable.

Being diligent about separating dependencies and eg. function arguments will lead to a cleaner kitchen, as it creates more easily reusable functions, and hides irrelevant details out of sight.

);

expect(actual).toBe("some-directory");
});

it("given registered extension does not exist, returns random directory", async () => {
const actual = await ensureHashedDirectoryForExtension(
"some-extension-name",
registeredExtensions,
);

expect(actual).not.toBe("some-directory");
Copy link
Contributor

@Iku-turso Iku-turso Feb 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test eg. expect(actual).toBe("some-random-directory"); would be more credible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, fixed

});

it("given extension directory was saved based on extension's package.json path, returns existing directory", async () => {
runInAction(() => {
registeredExtensions.set("/Users/some-user/Library/Application Support/Lens/node_modules/some-extension-name/package.json", "some-directory");
});
const actual = await ensureHashedDirectoryForExtension(
"some-extension-name",
registeredExtensions,
);

expect(actual).toBe("some-directory");
});

});