[catalog-server] Handle invalid and not found packages better. (#1404)

Author: justinfagnani

packages/catalog-api/src/lib/schema.graphql

@@ -72,7 +72,7 @@ type DistTag {
 """
 The status of a package that has been successfully imported.
 
-A package may also has a status from the UnreadablePackageStatus enum.
+A package may also have a status from the UnreadablePackageStatus enum.
 """
 enum ReadablePackageStatus {
   """
@@ -90,7 +90,7 @@ enum ReadablePackageStatus {
 """
 The status of a package that has not been successfully imported.
 
-A package may also has a status from the ReadablePackageStatus enum.
+A package may also have a status from the ReadablePackageStatus enum.
 """
 enum UnreadablePackageStatus {
   """
@@ -104,7 +104,7 @@ enum UnreadablePackageStatus {
   NOT_FOUND
 
   """
-  An other recoverable error occured, such as network failure.
+  A recoverable error occured, such as network failure.
   """
   ERROR
 }

packages/catalog-server/src/lib/catalog.ts

@@ -8,6 +8,7 @@ import {distTagMapToList, getDistTagsForVersion} from './npm.js';
 import {validatePackage} from '@webcomponents/custom-elements-manifest-tools/lib/validate.js';
 import {getCustomElements} from '@webcomponents/custom-elements-manifest-tools';
 import {
+  HttpError,
   Package,
   PackageFiles,
 } from '@webcomponents/custom-elements-manifest-tools/lib/npm.js';
@@ -19,6 +20,8 @@ import {
   PackageInfo,
   ReadablePackageInfo,
   CustomElement,
+  UnreadablePackageStatus,
+  VersionStatus,
 } from '@webcomponents/catalog-api/lib/schema.js';
 import {
   Temporal,
@@ -100,20 +103,29 @@ export class Catalog {
 
     // Fetch package metadata from npm:
     console.log('Fetching package metadata...');
-    let newPackage: Package | undefined;
+    let newPackage: Package;
     try {
       newPackage = await this.#files.getPackageMetadata(packageName);
     } catch (e) {
-      await this.#repository.endPackageImportWithError(packageName);
-      return {};
+      console.error('Error fetching package metadata');
+      console.error(e);
+      let status: UnreadablePackageStatus = UnreadablePackageStatus.ERROR;
+      if ((e as HttpError).statusCode === 404) {
+        status = UnreadablePackageStatus.NOT_FOUND;
+      }
+      const packageInfo = await this.#repository.endPackageImportWithError(
+        packageName,
+        status
+      );
+      return {packageInfo};
+    } finally {
+      console.log(' done');
     }
-    console.log(' done');
 
     if (newPackage === undefined) {
-      await this.#repository.endPackageImportWithNotFound(packageName);
       // TODO (justinfagnani): a crazy edge case would be a package that was
       // previously found, but is not found now. Update package versions?
-      return {};
+      throw new Error(`Internal error importing package ${packageName}`);
     }
 
     const newDistTags = newPackage['dist-tags'];
@@ -176,7 +188,7 @@ export class Catalog {
 
     if (versionToImport !== undefined) {
       importResult = await this.importPackageVersion(
-        packageName,
+        newPackage,
         versionToImport
       );
     }
@@ -200,22 +212,44 @@ export class Catalog {
     };
   }
 
-  async importPackageVersion(packageName: string, version: string) {
+  async importPackageVersion(packageOrName: string | Package, version: string) {
+    const packageName =
+      typeof packageOrName === 'string' ? packageOrName : packageOrName.name;
+
     console.log('Marking package version as importing...');
     await this.#repository.startPackageVersionImport(packageName, version);
     console.log('  done');
 
+    let packageMetadata: Package;
+
+    if (typeof packageOrName === 'string') {
+      try {
+        packageMetadata = await this.#files.getPackageMetadata(packageName);
+      } catch (e) {
+        console.error(`Error fetching packageMetadata`);
+        console.error(e);
+        console.log('Marking package version as errored...');
+        // We mark this as a recoverable error, since another attempt might
+        // be able to fetch the package and therefore this package version.
+        const packageVersion =
+          await this.#repository.endPackageVersionImportWithError(
+            packageName,
+            version,
+            VersionStatus.ERROR
+          );
+        console.log('  done');
+        return {packageVersion};
+      }
+    } else {
+      packageMetadata = packageOrName;
+    }
+
     const {manifestData, manifestSource, problems} = await validatePackage({
       packageName,
       version,
       files: this.#files,
     });
 
-    // TODO (justinfagnani): If we're calling this from importPackage(), we'll
-    // already have package metadata, so either use that rather making another
-    // call, or cache the manifests in PackageFiles.
-    const packageMetadataPromise = this.#files.getPackageMetadata(packageName);
-
     if (problems.length > 0) {
       console.log('Writing problems...');
       await this.#repository.writeProblems(packageName, version, problems);
@@ -224,13 +258,15 @@ export class Catalog {
 
     if (manifestData === undefined) {
       console.error(`manifestData not found`);
-      console.log('Marking package version as errored...');
-      await this.#repository.endPackageVersionImportWithError(
-        packageName,
-        version
-      );
+      console.log('Marking package version as INVALID...');
+      const packageVersion =
+        await this.#repository.endPackageVersionImportWithError(
+          packageName,
+          version,
+          VersionStatus.INVALID
+        );
       console.log('  done');
-      return {problems};
+      return {packageVersion, problems};
     }
 
     const customElements = getCustomElements(
@@ -241,27 +277,15 @@ export class Catalog {
 
     if (customElements.length === 0) {
       console.error(`No customElements found`);
-      console.log(manifestSource);
-      console.log('Marking package version as errored...');
-      await this.#repository.endPackageVersionImportWithError(
-        packageName,
-        version
-      );
-      console.log('  done');
-      return {problems};
-    }
-
-    const packageMetadata = await packageMetadataPromise;
-
-    if (packageMetadata === undefined) {
-      console.error(`packageMetadata not found`);
-      console.log('Marking package version as errored...');
-      await this.#repository.endPackageVersionImportWithError(
-        packageName,
-        version
-      );
+      console.log('Marking package version as INVALID...');
+      const packageVersion =
+        await this.#repository.endPackageVersionImportWithError(
+          packageName,
+          version,
+          VersionStatus.INVALID
+        );
       console.log('  done');
-      return {problems};
+      return {packageVersion, problems};
     }
 
     // eslint-disable-next-line

packages/catalog-server/src/lib/firestore/firestore-repository.ts

@@ -9,6 +9,7 @@ import {
   Query,
   CollectionReference,
   CollectionGroup,
+  UpdateData,
 } from '@google-cloud/firestore';
 import {Firestore} from '@google-cloud/firestore';
 import firebase from 'firebase-admin';
@@ -29,6 +30,7 @@ import {
   ValidationProblem,
   ReadablePackageVersion,
   ReadablePackageInfo,
+  UnreadablePackageStatus,
 } from '@webcomponents/catalog-api/lib/schema.js';
 import {
   Package,
@@ -105,7 +107,10 @@ export class FirestoreRepository implements Repository {
     });
   }
 
-  async endPackageImportWithNotFound(packageName: string): Promise<void> {
+  async endPackageImportWithError(
+    packageName: string,
+    status: UnreadablePackageStatus
+  ): Promise<PackageInfo> {
     const packageRef = this.getPackageRef(packageName);
     await db.runTransaction(async (t) => {
       const packageDoc = await t.get(packageRef);
@@ -116,31 +121,20 @@ export class FirestoreRepository implements Repository {
       if (packageData.status !== PackageStatus.INITIALIZING) {
         throw new Error(`Unexpected package status: ${packageData.status}`);
       }
-      await t.set(packageRef, {
+      t.set(packageRef, {
         name: packageName,
-        status: PackageStatus.NOT_FOUND,
+        status,
         lastUpdate: FieldValue.serverTimestamp(),
       });
     });
-  }
-
-  async endPackageImportWithError(packageName: string): Promise<void> {
-    const packageRef = this.getPackageRef(packageName);
-    await db.runTransaction(async (t) => {
-      const packageDoc = await t.get(packageRef);
-      const packageData = packageDoc.data();
-      if (packageData === undefined) {
-        throw new Error(`Package not found: ${packageName}`);
-      }
-      if (packageData.status !== PackageStatus.INITIALIZING) {
-        throw new Error(`Unexpected package status: ${packageData.status}`);
-      }
-      await t.set(packageRef, {
-        name: packageName,
-        status: PackageStatus.ERROR,
-        lastUpdate: FieldValue.serverTimestamp(),
-      });
+    const packageInfo = await db.runTransaction(async (t) => {
+      // There doesn't seem to be a way to get a WriteResult and therefore
+      // a writeTime inside a transaction, so we read from the database to
+      // get the server timestamp.
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      return (await t.get(packageRef)).data()!;
     });
+    return packageInfo;
   }
 
   /**
@@ -266,10 +260,11 @@ export class FirestoreRepository implements Repository {
 
   async endPackageVersionImportWithError(
     packageName: string,
-    version: string
-  ): Promise<void> {
+    version: string,
+    status: VersionStatus
+  ): Promise<PackageVersion> {
+    const versionRef = this.getPackageVersionRef(packageName, version);
     await db.runTransaction(async (t) => {
-      const versionRef = this.getPackageVersionRef(packageName, version);
       const packageVersionDoc = await t.get(
         versionRef.withConverter(packageVersionConverter)
       );
@@ -282,11 +277,21 @@ export class FirestoreRepository implements Repository {
           `Unexpected package version status: ${packageVersionData.status}`
         );
       }
-      await t.update(versionRef, {
-        status: VersionStatus.ERROR,
+      // TODO (justinfagnani): figure out why we need the cast, since
+      // UpdateData<T> should allow a Partial<T>
+      t.update(versionRef, {
+        status,
         lastUpdate: FieldValue.serverTimestamp(),
-      });
+      } as UpdateData<PackageVersion> as PackageVersion);
     });
+    const packageVersion = await db.runTransaction(async (t) => {
+      // There doesn't seem to be a way to get a WriteResult and therefore
+      // a writeTime inside a transaction, so we read from the database to
+      // get the server timestamp.
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      return (await t.get(versionRef)).data()!;
+    });
+    return packageVersion;
   }
 
   async writeCustomElements(
@@ -477,7 +482,10 @@ export class FirestoreRepository implements Repository {
         packageName,
         versionOrTag
       );
-      version = packageVersion!.version;
+      if (packageVersion === undefined) {
+        return [];
+      }
+      version = packageVersion.version;
     } else {
       version = versionOrTag;
     }

packages/catalog-server/src/lib/repository.ts

@@ -10,7 +10,9 @@ import type {
   PackageVersion,
   ReadablePackageInfo,
   ReadablePackageVersion,
+  UnreadablePackageStatus,
   ValidationProblem,
+  VersionStatus,
 } from '@webcomponents/catalog-api/lib/schema';
 import type {CustomElementInfo} from '@webcomponents/custom-elements-manifest-tools';
 import type {
@@ -48,9 +50,10 @@ export interface Repository {
 
   startPackageUpdate(packageName: string): Promise<void>;
 
-  endPackageImportWithNotFound(packageName: string): Promise<void>;
-
-  endPackageImportWithError(packageName: string): Promise<void>;
+  endPackageImportWithError(
+    packageName: string,
+    status: UnreadablePackageStatus
+  ): Promise<PackageInfo>;
 
   endPackageImportWithReady(
     packageName: string,
@@ -85,13 +88,14 @@ export interface Repository {
   ): Promise<ReadablePackageVersion>;
 
   /**
-   * Updates a PackageVersion to status: ERROR. Verifies that the PackageVersion
-   * was in INITIALIZING status before the update.
+   * Updates a PackageVersion to the given status, which should be ERROR or INVALID.
+   * Verifies that the PackageVersion was in INITIALIZING status before the update.
    */
   endPackageVersionImportWithError(
     packageName: string,
-    version: string
-  ): Promise<void>;
+    version: string,
+    status: VersionStatus
+  ): Promise<PackageVersion>;
 
   writeCustomElements(
     packageVersionMetadata: Version,

packages/catalog-server/src/lib/server/routes/bootstrap-packages.ts

@@ -27,7 +27,7 @@ export const makeBootstrapPackagesRoute =
         async (
           packageName
         ): Promise<
-          | {error: unknown; packageName: string}
+          | {error: Error; packageName: string}
           | {
               packageName: string;
               elements: Array<CustomElement>;
@@ -51,7 +51,7 @@ export const makeBootstrapPackagesRoute =
           } catch (error) {
             return {
               packageName,
-              error,
+              error: error as Error,
             };
           }
         }
@@ -67,7 +67,7 @@ export const makeBootstrapPackagesRoute =
         if ('error' in result) {
           return `
             <h3>${packageName}</h3>
-            <code><pre>${result.error}</pre></code>
+            <code><pre>${result.error}\n${result.error.stack ?? ''}</pre></code>
           `;
         } else {
           const {elements} = result;