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

@@ -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,

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.

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<boolean, GenericNetworkError>;
+
+/**
+ * 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());
+      });
+  };
+}

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<boolean, CheckIsBuiltInPackageError>;
+
+/**
+ * Makes a {@link CheckIsBuiltInPackage} function.
+ */
+export function makeCheckIsBuiltInPackage(
+  checkIsUnityPackage: CheckIsUnityPackage,
+  fetchPackument: FetchPackument
+): CheckIsBuiltInPackage {
+  function checkExistsOnUnityRegistry(
+    packageName: DomainName,
+    version: SemanticVersion
+  ): AsyncResult<boolean, GenericNetworkError> {
+    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
+      );
+    });
+  };
+}

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,
           });

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<boolean, CheckIsUnityPackageError>;
+
+/**
+ * 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);
+  };
+}

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();
-    });
-  });
 });

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"]
+    );
+  });
 });

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"),
+      ])
+    );
+  });
+});

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)
+    );
+  });
+});