Fix ensure hashed directory for extension #7188

jweak · 2023-02-17T14:47:29Z

Earlier open lens versions stored extension_data directory based on path of extension's package.json. This causes problems because extensions have moved to new location. This ensures backward compatibility that extension will get the same directory than before the change and migrates the key to a new implementation that does not depend on the installed directory.

Fixes #7052

Earlier open lens versions stored extension_data directory based on path of extension's package.json. This causes problems because extensions have moved to new location. This ensures backward compatibility that extension will get the same directory than before the change. Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Nokel81

I don't understand why this is needed.

packages/core/src/common/utils/find-key-contains.injectable.ts

...ion-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.injectable.ts

packages/core/src/common/utils/find-key-contains.injectable.ts

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts

Iku-turso · 2023-02-20T07:28:22Z

...extension-loader/file-system-provisioner-store/get-path-to-legacy-package-json.injectable.ts

+
+export type GetPathToLegacyPackageJson = (extensionName: string) => string;
+
+const getPathToLegacyPackageJson = getInjectable({


This could be unit tested as part of ensure-hashed-directory-for-extension.test.ts, instead of coupling tests too tightly with implementation details.

sure we can just get rid off the specific tests for this dependency

Iku-turso · 2023-02-20T07:30:52Z

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts

+
+    const actual = await ensureHashedDirectoryForExtension(
+      "some-extension-name",
+      registeredExtensions,


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?

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?

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.

Iku-turso · 2023-02-20T07:34:36Z

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts

+
+    ensureDirMock = jest.fn();
+
+    di.override(ensureDirInjectable, () => ensureDirMock);


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.

Iku-turso · 2023-02-20T07:35:42Z

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts

+    expect(actual).toBe("some-directory");
+  });
+
+  it("given extension directory was saved based on extension's package.json path, ensure dir is called with right parameter", async () => {


Instead of "right parameter", one could document what makes the parameter right :)

good call. done

...sions/extension-loader/file-system-provisioner-store/get-path-to-legacy-package-json.test.ts

...extension-loader/file-system-provisioner-store/get-path-to-legacy-package-json.injectable.ts

Iku-turso · 2023-02-20T07:40:03Z

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts

+      registeredExtensions,
+    );
+
+    expect(actual).not.toBe("some-directory");


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

good catch, fixed

Iku-turso · 2023-02-20T07:44:19Z

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts

+import directoryForExtensionDataInjectable from "./directory-for-extension-data.injectable";
+import directoryForUserDataInjectable from "../../../common/app-paths/directory-for-user-data/directory-for-user-data.injectable";
+
+describe("ensure-hashed-directory-for-extension", () => {


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?

...ion-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.injectable.ts

...sions/extension-loader/file-system-provisioner-store/get-path-to-legacy-package-json.test.ts

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Iku-turso · 2023-02-21T12:26:02Z

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts

+      expect(actual).toBe("some-directory");
+    });
+
+    it("ensure dir is called with some directory", async () => {


"ensure dir" is a technical phenomenon, and in general, unit tests benefit more from describing motivations of the real world. That would mean eg.: it("ensures that the said extension directory exists", ...).

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

* Fix ensure hashed directory for extension Earlier open lens versions stored extension_data directory based on path of extension's package.json. This causes problems because extensions have moved to new location. This ensures backward compatibility that extension will get the same directory than before the change. Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Reconstruct the key for old hashed directories Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Remove unnecessary return Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Use sync version of random bytes Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Add migration to new type of key Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Fix global override for random bytes to not return a promise Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Make registeredExtensions a dependency Signed-off-by: Juho Heikka <juho.heikka@gmail.com> --------- Signed-off-by: Juho Heikka <juho.heikka@gmail.com> (cherry picked from commit d8acff6)

* Fix ensure hashed directory for extension Earlier open lens versions stored extension_data directory based on path of extension's package.json. This causes problems because extensions have moved to new location. This ensures backward compatibility that extension will get the same directory than before the change. Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Reconstruct the key for old hashed directories Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Remove unnecessary return Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Use sync version of random bytes Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Add migration to new type of key Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Fix global override for random bytes to not return a promise Signed-off-by: Juho Heikka <juho.heikka@gmail.com> * Make registeredExtensions a dependency Signed-off-by: Juho Heikka <juho.heikka@gmail.com> --------- Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jweak added the bug Something isn't working label Feb 17, 2023

jweak added this to the 6.4.0 milestone Feb 17, 2023

jweak requested review from jansav, Iku-turso, apialaviiva and samitiilikainen February 17, 2023 14:47

jweak requested a review from a team as a code owner February 17, 2023 14:47

Merge branch 'master' into ensure-hashed-directory-fix

b2a1ee6

Nokel81 reviewed Feb 17, 2023

View reviewed changes

packages/core/src/common/utils/find-key-contains.injectable.ts Outdated Show resolved Hide resolved

...ion-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.injectable.ts Outdated Show resolved Hide resolved

jweak commented Feb 17, 2023

View reviewed changes

...ion-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.injectable.ts Outdated Show resolved Hide resolved

jweak commented Feb 17, 2023

View reviewed changes

packages/core/src/common/utils/find-key-contains.injectable.ts Outdated Show resolved Hide resolved

Reconstruct the key for old hashed directories

9405899

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Iku-turso reviewed Feb 20, 2023

View reviewed changes

...extension-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.test.ts Outdated Show resolved Hide resolved

Iku-turso reviewed Feb 20, 2023

View reviewed changes

...sions/extension-loader/file-system-provisioner-store/get-path-to-legacy-package-json.test.ts Outdated Show resolved Hide resolved

Iku-turso reviewed Feb 20, 2023

View reviewed changes

...extension-loader/file-system-provisioner-store/get-path-to-legacy-package-json.injectable.ts Outdated Show resolved Hide resolved

Iku-turso reviewed Feb 20, 2023

View reviewed changes

...ion-loader/file-system-provisioner-store/ensure-hashed-directory-for-extension.injectable.ts Outdated Show resolved Hide resolved

Iku-turso reviewed Feb 20, 2023

View reviewed changes

...sions/extension-loader/file-system-provisioner-store/get-path-to-legacy-package-json.test.ts Outdated Show resolved Hide resolved

jweak added 3 commits February 21, 2023 09:31

Remove unnecessary return

4b6bfc1

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Use sync version of random bytes

169bb1b

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add migration to new type of key

8d9d710

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jweak force-pushed the ensure-hashed-directory-fix branch from 1c9e881 to 8d9d710 Compare February 21, 2023 07:31

jweak added 2 commits February 21, 2023 10:08

Merge branch 'master' into ensure-hashed-directory-fix

b729962

Fix global override for random bytes to not return a promise

795084d

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Iku-turso reviewed Feb 21, 2023

View reviewed changes

Iku-turso previously approved these changes Feb 21, 2023

View reviewed changes

Make registeredExtensions a dependency

845c727

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jweak dismissed Iku-turso’s stale review via 845c727 February 21, 2023 13:27

Merge branch 'master' into ensure-hashed-directory-fix

080ae95

Iku-turso approved these changes Feb 21, 2023

View reviewed changes

jweak merged commit d8acff6 into master Feb 21, 2023

jweak deleted the ensure-hashed-directory-fix branch February 21, 2023 14:09

This was referenced Feb 22, 2023

Release 6.4.0-beta.14 #7213

Merged

Release 6.4.0-beta.15 #7218

Merged

Release 6.4.0-beta.16 #7230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ensure hashed directory for extension #7188

Fix ensure hashed directory for extension #7188

jweak commented Feb 17, 2023 •

edited

Nokel81 left a comment

Iku-turso Feb 20, 2023 •

edited

jweak Feb 20, 2023

Iku-turso Feb 20, 2023

jweak Feb 20, 2023

Iku-turso Feb 21, 2023

Iku-turso Feb 20, 2023

Iku-turso Feb 20, 2023

jweak Feb 21, 2023

Iku-turso Feb 20, 2023 •

edited

jweak Feb 21, 2023

Iku-turso Feb 20, 2023 •

edited

Iku-turso Feb 21, 2023


		export type GetPathToLegacyPackageJson = (extensionName: string) => string;

		const getPathToLegacyPackageJson = getInjectable({


		ensureDirMock = jest.fn();

		di.override(ensureDirInjectable, () => ensureDirMock);

Fix ensure hashed directory for extension #7188

Fix ensure hashed directory for extension #7188

Conversation

jweak commented Feb 17, 2023 • edited

Nokel81 left a comment

Choose a reason for hiding this comment

Iku-turso Feb 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iku-turso Feb 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iku-turso Feb 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jweak commented Feb 17, 2023 •

edited

Iku-turso Feb 20, 2023 •

edited

Iku-turso Feb 20, 2023 •

edited

Iku-turso Feb 20, 2023 •

edited