From 64e37d98ad7a9225bcefd2f8b1859f44a4a1b76e Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sat, 29 Jun 2024 08:34:23 +0200 Subject: [PATCH] feat: built-in package resolution (#364) * initial implementation No unit-tests yet * rename service * add js doc * use node-fetch Easier to mock * add tests for check-url * add jsdoc * add tests for unity package check * use existing error types * add tests for CheckIsBuiltInPackage * add jsdoc * drop old internal-package logic * add deps e2e tests * fix incorrect built-in version resolution Used the wrong property * add add e2e test --- src/cli/index.ts | 12 ++- src/domain/domain-name.ts | 15 ---- src/io/check-url.ts | 28 +++++++ src/services/built-in-package-check.ts | 71 ++++++++++++++++ src/services/dependency-resolving.ts | 21 +++-- src/services/unity-package-check.ts | 31 +++++++ test/domain/domain-name.test.ts | 22 +---- test/e2e/add.e2e.ts | 14 ++++ test/e2e/deps.e2e.ts | 32 ++++++++ test/io/check-url.test.ts | 61 ++++++++++++++ test/services/built-in-package-check.test.ts | 86 ++++++++++++++++++++ test/services/unity-package-check.test.ts | 45 ++++++++++ 12 files changed, 396 insertions(+), 42 deletions(-) create mode 100644 src/io/check-url.ts create mode 100644 src/services/built-in-package-check.ts create mode 100644 src/services/unity-package-check.ts create mode 100644 test/e2e/deps.e2e.ts create mode 100644 test/io/check-url.test.ts create mode 100644 test/services/built-in-package-check.test.ts create mode 100644 test/services/unity-package-check.test.ts diff --git a/src/cli/index.ts b/src/cli/index.ts index 4baeafb0..f58fd642 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -50,6 +50,9 @@ import { DebugLog } from "../logging"; import { makeDetermineEditorVersion } from "../services/determine-editor-version"; import { makeRemovePackages } from "../services/remove-packages"; import { makeRunChildProcess } from "../io/child-process"; +import { makeCheckIsBuiltInPackage } from "../services/built-in-package-check"; +import { makeCheckIsUnityPackage } from "../services/unity-package-check"; +import { makeCheckUrlExists } from "../io/check-url"; // Composition root @@ -83,6 +86,7 @@ const removePackages = makeRemovePackages( loadProjectManifest, writeProjectManifest ); +const checkUrlExists = makeCheckUrlExists(); const parseEnv = makeParseEnv(log, getUpmConfigPath, loadUpmConfig, getCwd); const determineEditorVersion = makeDetermineEditorVersion(loadProjectVersion); @@ -91,9 +95,15 @@ const npmLogin = makeNpmLogin(regClient, debugLog); const resolveRemovePackumentVersion = makeResolveRemotePackumentVersion(fetchPackument); const resolveLatestVersion = makeResolveLatestVersion(fetchPackument); +const checkIsUnityPackage = makeCheckIsUnityPackage(checkUrlExists); +const checkIsBuiltInPackage = makeCheckIsBuiltInPackage( + checkIsUnityPackage, + fetchPackument +); const resolveDependencies = makeResolveDependency( resolveRemovePackumentVersion, - resolveLatestVersion + resolveLatestVersion, + checkIsBuiltInPackage ); const saveAuthToUpmConfig = makeSaveAuthToUpmConfig( loadUpmConfig, diff --git a/src/domain/domain-name.ts b/src/domain/domain-name.ts index f8d0a0d5..56abff32 100644 --- a/src/domain/domain-name.ts +++ b/src/domain/domain-name.ts @@ -25,21 +25,6 @@ export function isDomainName(s: string): s is DomainName { return segments.every((segment) => segmentRegex.test(segment)); } -/** - * Detect if the given package name is an internal package. - * @param name The name of the package. - */ -export const isInternalPackage = (name: DomainName): boolean => { - const internals = [ - "com.unity.ugui", - "com.unity.2d.sprite", - "com.unity.2d.tilemap", - "com.unity.package-manager-ui", - "com.unity.ugui", - ]; - return /com.unity.modules/i.test(name) || internals.includes(name); -}; - /** * Constructs a domain-name from a string. * @param s The string. diff --git a/src/io/check-url.ts b/src/io/check-url.ts new file mode 100644 index 00000000..570cb1e1 --- /dev/null +++ b/src/io/check-url.ts @@ -0,0 +1,28 @@ +import { AsyncResult, Err, Ok, Result } from "ts-results-es"; +import { GenericNetworkError } from "./common-errors"; +import fetch from "node-fetch"; + +/** + * Function for checking if an url exists. + * @param url The url to check. + */ +export type CheckUrlExists = ( + url: string +) => AsyncResult; + +/** + * Makes a {@link CheckUrlExists} function. + */ +export function makeCheckUrlExists(): CheckUrlExists { + return (url) => { + return new AsyncResult( + Result.wrapAsync(() => fetch(url, { method: "HEAD" })) + ) + .mapErr(() => new GenericNetworkError()) + .andThen((reponse) => { + if (reponse.status === 200) return Ok(true); + else if (reponse.status === 404) return Ok(false); + else return Err(new GenericNetworkError()); + }); + }; +} diff --git a/src/services/built-in-package-check.ts b/src/services/built-in-package-check.ts new file mode 100644 index 00000000..7bb8f59d --- /dev/null +++ b/src/services/built-in-package-check.ts @@ -0,0 +1,71 @@ +import { DomainName } from "../domain/domain-name"; +import { SemanticVersion } from "../domain/semantic-version"; +import { AsyncResult } from "ts-results-es"; +import { + CheckIsUnityPackage, + CheckIsUnityPackageError, +} from "./unity-package-check"; +import { FetchPackument } from "../io/packument-io"; +import { unityRegistryUrl } from "../domain/registry-url"; +import { recordKeys } from "../utils/record-utils"; +import { AsyncOk } from "../utils/result-utils"; +import { + GenericNetworkError, + RegistryAuthenticationError, +} from "../io/common-errors"; +import { FetchAllPackumentsError } from "../io/all-packuments-io"; + +/** + * Error which may occur when checking whether a package is built-in. + */ +export type CheckIsBuiltInPackageError = + | CheckIsUnityPackageError + | FetchAllPackumentsError; + +/** + * Function for checking whether a specific package version is built-in. + * @param packageName The packages name. + * @param version The specific version to check. + * @returns A boolean indicating whether the package version is built-in. + */ +export type CheckIsBuiltInPackage = ( + packageName: DomainName, + version: SemanticVersion +) => AsyncResult; + +/** + * Makes a {@link CheckIsBuiltInPackage} function. + */ +export function makeCheckIsBuiltInPackage( + checkIsUnityPackage: CheckIsUnityPackage, + fetchPackument: FetchPackument +): CheckIsBuiltInPackage { + function checkExistsOnUnityRegistry( + packageName: DomainName, + version: SemanticVersion + ): AsyncResult { + return fetchPackument({ url: unityRegistryUrl, auth: null }, packageName) + .map((maybePackument) => { + if (maybePackument === null) return false; + const versions = recordKeys(maybePackument.versions); + return versions.includes(version); + }) + .mapErr((error) => { + if (error instanceof RegistryAuthenticationError) + throw new Error( + "Authentication with Unity registry failed, even though it does not require authentication." + ); + + return error; + }); + } + + return (packageName, version) => { + return checkIsUnityPackage(packageName).andThen((isUnityPackage) => { + if (!isUnityPackage) return AsyncOk(false); + return checkExistsOnUnityRegistry(packageName, version).map( + (existsOnUnityRegistry) => !existsOnUnityRegistry + ); + }); + }; +} diff --git a/src/services/dependency-resolving.ts b/src/services/dependency-resolving.ts index 2cb35bb5..14f484c7 100644 --- a/src/services/dependency-resolving.ts +++ b/src/services/dependency-resolving.ts @@ -1,4 +1,4 @@ -import { DomainName, isInternalPackage } from "../domain/domain-name"; +import { DomainName } from "../domain/domain-name"; import { SemanticVersion } from "../domain/semantic-version"; import { addToCache, emptyPackumentCache } from "../packument-cache"; import { @@ -27,6 +27,10 @@ import { GenericNetworkError, RegistryAuthenticationError, } from "../io/common-errors"; +import { + CheckIsBuiltInPackage, + CheckIsBuiltInPackageError, +} from "./built-in-package-check"; export type DependencyBase = { /** @@ -69,7 +73,8 @@ type NameVersionPair = Readonly<[DomainName, SemanticVersion]>; */ export type DependencyResolveError = | ResolveLatestVersionError - | FetchPackumentError; + | FetchPackumentError + | CheckIsBuiltInPackageError; /** * Function for resolving all dependencies for a package. @@ -92,7 +97,8 @@ export type ResolveDependencies = ( */ export function makeResolveDependency( resolveRemovePackumentVersion: ResolveRemotePackumentVersion, - resolveLatestVersion: ResolveLatestVersion + resolveLatestVersion: ResolveLatestVersion, + checkIsBuiltInPackage: CheckIsBuiltInPackage ): ResolveDependencies { // TODO: Add tests for this service @@ -150,13 +156,18 @@ export function makeResolveDependency( if (!isProcessed) { // add entry to processed list processedList.push(entry); - const isInternal = isInternalPackage(entryName); + const isInternalResult = await checkIsBuiltInPackage( + entryName, + entryVersion + ).promise; + if (isInternalResult.isErr()) return isInternalResult; + const isInternal = isInternalResult.value; const isSelf = entryName === name; if (isInternal) { depsValid.push({ name: entryName, - version: latestVersion, + version: entryVersion, source: "built-in", self: isSelf, }); diff --git a/src/services/unity-package-check.ts b/src/services/unity-package-check.ts new file mode 100644 index 00000000..ec69996b --- /dev/null +++ b/src/services/unity-package-check.ts @@ -0,0 +1,31 @@ +import { DomainName } from "../domain/domain-name"; +import { AsyncResult } from "ts-results-es"; +import { CheckUrlExists } from "../io/check-url"; +import { GenericNetworkError } from "../io/common-errors"; + +/** + * Error which may occur when checking whether a package is a Unity package. + */ +export type CheckIsUnityPackageError = GenericNetworkError; + +/** + * Function for checking whether a package is an official Unity package. + * @param packageName The name of the package. + * @returns A boolean indicating whether the package is a Unity package. + */ +export type CheckIsUnityPackage = ( + packageName: DomainName +) => AsyncResult; + +/** + * Makes a {@link CheckIsUnityPackage} function. + */ +export function makeCheckIsUnityPackage( + checkUrlExists: CheckUrlExists +): CheckIsUnityPackage { + return (packageName) => { + // A package is an official Unity package if it has a documentation page + const url = `https://docs.unity3d.com/Manual/${packageName}.html`; + return checkUrlExists(url); + }; +} diff --git a/test/domain/domain-name.test.ts b/test/domain/domain-name.test.ts index 49844e53..979d9ab5 100644 --- a/test/domain/domain-name.test.ts +++ b/test/domain/domain-name.test.ts @@ -1,8 +1,4 @@ -import { - isDomainName, - isInternalPackage, - makeDomainName, -} from "../../src/domain/domain-name"; +import { isDomainName } from "../../src/domain/domain-name"; describe("domain-name", () => { describe("validation", () => { @@ -31,20 +27,4 @@ describe("domain-name", () => { expect(isDomainName(s)).toBeFalsy(); }); }); - - describe("internal package", () => { - it("test com.otherorg.software", () => { - expect( - isInternalPackage(makeDomainName("com.otherorg.software")) - ).not.toBeTruthy(); - }); - it("test com.unity.ugui", () => { - expect(isInternalPackage(makeDomainName("com.unity.ugui"))).toBeTruthy(); - }); - it("test com.unity.modules.tilemap", () => { - expect( - isInternalPackage(makeDomainName("com.unity.modules.tilemap")) - ).toBeTruthy(); - }); - }); }); diff --git a/test/e2e/add.e2e.ts b/test/e2e/add.e2e.ts index f8062a71..9c2fddad 100644 --- a/test/e2e/add.e2e.ts +++ b/test/e2e/add.e2e.ts @@ -252,4 +252,18 @@ describe("add packages", () => { ]) ); }); + + it("should add package with built-in dependencies", async () => { + // See https://github.com/openupm/openupm-cli/issues/155 + await testSuccessfulAdd( + [ + { + packageName: "org.khronos.unitygltf", + expectedVersion: "2.12.0", + addVersion: "2.12.0", + }, + ], + ["org.khronos.unitygltf"] + ); + }); }); diff --git a/test/e2e/deps.e2e.ts b/test/e2e/deps.e2e.ts new file mode 100644 index 00000000..a85b2b0b --- /dev/null +++ b/test/e2e/deps.e2e.ts @@ -0,0 +1,32 @@ +import { prepareHomeDirectory } from "./setup/directories"; +import { runOpenupm } from "./run"; +import { ResultCodes } from "../../src/cli/result-codes"; + +describe("print package dependencies", () => { + it("should print built-in dependencies", async () => { + // See https://github.com/openupm/openupm-cli/issues/133 + const homeDir = await prepareHomeDirectory(); + + const output = await runOpenupm(homeDir, [ + "deps", + "com.unity.polyspatial@0.7.1", + ]); + + expect(output.code).toEqual(ResultCodes.Ok); + expect(output.stdErr).toEqual( + expect.arrayContaining([ + // Taken from https://packages.unity.com/com.unity.polyspatial + expect.stringContaining("com.unity.nuget.newtonsoft-json@3.0.2"), + expect.stringContaining("com.unity.render-pipelines.universal@14.0.1"), + expect.stringContaining("com.unity.collections@2.1.4"), + expect.stringContaining("com.unity.textmeshpro@3.0.6"), + expect.stringContaining("com.unity.xr.core-utils@2.4.0-exp.3"), + expect.stringContaining("com.unity.ext.flatsharp@0.10.1"), + expect.stringContaining("com.unity.modules.particlesystem@1.0.0"), + expect.stringContaining("com.unity.inputsystem@1.4.4"), + expect.stringContaining("com.unity.modules.video@1.0.0"), + expect.stringContaining("com.unity.ugui@1.0.0"), + ]) + ); + }); +}); diff --git a/test/io/check-url.test.ts b/test/io/check-url.test.ts new file mode 100644 index 00000000..ab99c726 --- /dev/null +++ b/test/io/check-url.test.ts @@ -0,0 +1,61 @@ +import { makeCheckUrlExists } from "../../src/io/check-url"; +import fetch, { Response } from "node-fetch"; +import { GenericNetworkError } from "../../src/io/common-errors"; + +jest.mock("node-fetch"); + +describe("check url exists", () => { + function makeDependencies() { + const checkUrlExists = makeCheckUrlExists(); + return { checkUrlExists } as const; + } + + it("should be true if url responds with 200", async () => { + jest.mocked(fetch).mockResolvedValue({ + status: 200, + } as Response); + const { checkUrlExists } = makeDependencies(); + + const result = await checkUrlExists("https://some.url.com").promise; + + expect(result).toBeOk((actual) => expect(actual).toBeTruthy()); + }); + + it("should be false if url responds with 404", async () => { + jest.mocked(fetch).mockResolvedValue({ + status: 404, + } as Response); + const { checkUrlExists } = makeDependencies(); + + const result = await checkUrlExists("https://some.url.com").promise; + + expect(result).toBeOk((actual) => expect(actual).toBeFalsy()); + }); + + it.each([100, 201, 301, 401, 500])( + "should be fail for other status codes (%d)", + async (statusCode) => { + jest.mocked(fetch).mockResolvedValue({ + status: statusCode, + } as Response); + const { checkUrlExists } = makeDependencies(); + + const result = await checkUrlExists("https://some.url.com").promise; + + expect(result).toBeError((actual) => + expect(actual).toBeInstanceOf(GenericNetworkError) + ); + } + ); + + it("should be fail if request fails", async () => { + jest.mocked(fetch).mockRejectedValueOnce(new Error("Network bad")); + const { checkUrlExists } = makeDependencies(); + + const result = await checkUrlExists("https://some.url.com").promise; + + expect(result).toBeError((actual) => + expect(actual).toBeInstanceOf(GenericNetworkError) + ); + }); +}); diff --git a/test/services/built-in-package-check.test.ts b/test/services/built-in-package-check.test.ts new file mode 100644 index 00000000..a2aab1e0 --- /dev/null +++ b/test/services/built-in-package-check.test.ts @@ -0,0 +1,86 @@ +import { CheckIsUnityPackage } from "../../src/services/unity-package-check"; +import { makeCheckIsBuiltInPackage } from "../../src/services/built-in-package-check"; +import { mockService } from "./service.mock"; +import { FetchPackument } from "../../src/io/packument-io"; +import { AsyncErr, AsyncOk } from "../../src/utils/result-utils"; +import { makeDomainName } from "../../src/domain/domain-name"; +import { makeSemanticVersion } from "../../src/domain/semantic-version"; +import { UnityPackument } from "../../src/domain/packument"; +import { GenericNetworkError } from "../../src/io/common-errors"; + +describe("is built-in package", () => { + const somePackage = makeDomainName("com.some.package"); + const someVersion = makeSemanticVersion("1.0.0"); + + function makeDependencies() { + const checkIsUnityPackage = mockService(); + + const fetchPackument = mockService(); + + const checkIsBuiltInPackage = makeCheckIsBuiltInPackage( + checkIsUnityPackage, + fetchPackument + ); + return { checkIsBuiltInPackage, checkIsUnityPackage, fetchPackument }; + } + + it("should be false if package is not a Unity package", async () => { + const { checkIsBuiltInPackage, checkIsUnityPackage } = makeDependencies(); + checkIsUnityPackage.mockReturnValue(AsyncOk(false)); + + const result = await checkIsBuiltInPackage(somePackage, someVersion) + .promise; + + expect(result).toBeOk((actual) => expect(actual).toBeFalsy()); + }); + + it("should be false if package is Unity package and exists on Unity registry", async () => { + const { checkIsBuiltInPackage, checkIsUnityPackage, fetchPackument } = + makeDependencies(); + checkIsUnityPackage.mockReturnValue(AsyncOk(true)); + fetchPackument.mockReturnValue( + AsyncOk({ versions: { [someVersion]: {} } } as UnityPackument) + ); + + const result = await checkIsBuiltInPackage(somePackage, someVersion) + .promise; + + expect(result).toBeOk((actual) => expect(actual).toBeFalsy()); + }); + + it("should be true if package is Unity package, but does not exist on Unity registry", async () => { + const { checkIsBuiltInPackage, checkIsUnityPackage, fetchPackument } = + makeDependencies(); + checkIsUnityPackage.mockReturnValue(AsyncOk(true)); + fetchPackument.mockReturnValue(AsyncOk(null)); + + const result = await checkIsBuiltInPackage(somePackage, someVersion) + .promise; + + expect(result).toBeOk((actual) => expect(actual).toBeTruthy()); + }); + + it("should fail if Unity package check failed", async () => { + const expected = new GenericNetworkError(); + const { checkIsBuiltInPackage, checkIsUnityPackage } = makeDependencies(); + checkIsUnityPackage.mockReturnValue(AsyncErr(expected)); + + const result = await checkIsBuiltInPackage(somePackage, someVersion) + .promise; + + expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + }); + + it("should fail if Unity registry check failed", async () => { + const expected = new GenericNetworkError(); + const { checkIsBuiltInPackage, checkIsUnityPackage, fetchPackument } = + makeDependencies(); + checkIsUnityPackage.mockReturnValue(AsyncOk(true)); + fetchPackument.mockReturnValue(AsyncErr(expected)); + + const result = await checkIsBuiltInPackage(somePackage, someVersion) + .promise; + + expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + }); +}); diff --git a/test/services/unity-package-check.test.ts b/test/services/unity-package-check.test.ts new file mode 100644 index 00000000..e902b1fb --- /dev/null +++ b/test/services/unity-package-check.test.ts @@ -0,0 +1,45 @@ +import { makeCheckIsUnityPackage } from "../../src/services/unity-package-check"; +import { mockService } from "./service.mock"; +import { CheckUrlExists } from "../../src/io/check-url"; +import { AsyncErr, AsyncOk } from "../../src/utils/result-utils"; +import { makeDomainName } from "../../src/domain/domain-name"; +import { GenericNetworkError } from "../../src/io/common-errors"; + +describe("is unity package", () => { + const somePackage = makeDomainName("com.some.package"); + + function makeDependencies() { + const checkUrlExists = mockService(); + + const checkIsUnityPackage = makeCheckIsUnityPackage(checkUrlExists); + return { checkIsUnityPackage, checkUrlExists } as const; + } + + it("should be true if manual page exists", async () => { + const { checkIsUnityPackage, checkUrlExists } = makeDependencies(); + checkUrlExists.mockReturnValue(AsyncOk(true)); + + const result = await checkIsUnityPackage(somePackage).promise; + + expect(result).toBeOk((actual) => expect(actual).toBeTruthy()); + }); + + it("should be false if manual page does not exist", async () => { + const { checkIsUnityPackage, checkUrlExists } = makeDependencies(); + checkUrlExists.mockReturnValue(AsyncOk(false)); + + const result = await checkIsUnityPackage(somePackage).promise; + + expect(result).toBeOk((actual) => expect(actual).toBeFalsy()); + }); + + it("should fail if page status could not be verified", async () => { + const expected = new GenericNetworkError(); + const { checkIsUnityPackage, checkUrlExists } = makeDependencies(); + checkUrlExists.mockReturnValue(AsyncErr(expected)); + + const result = await checkIsUnityPackage(somePackage).promise; + + expect(result).toBeError((actual) => expect(actual).toEqual(expected)); + }); +});