fix(api): fix logging issues with migration cli commands (#387)

* fix(api): fix logging issues with migration cli commands

* Fix issues with migration cli commands

* introduce env var based flag to ensure data imports are intentional

* refactor test suite to share common setup between test cases

apps/api/src/coscrad-cli/data-restore.cli-command.e2e.spec.ts

@@ -19,6 +19,8 @@ import buildTestDataInFlatFormat from '../test-data/buildTestDataInFlatFormat';
 import convertInMemorySnapshotToDatabaseFormat from '../test-data/utilities/convertInMemorySnapshotToDatabaseFormat';
 import { CoscradCliModule } from './coscrad-cli.module';
 
+const originalEnv = process.env;
+
 const cliCommandName = 'data-restore';
 
 const outputDir = `__cli-command-test-files__`;
@@ -62,38 +64,47 @@ describe(`CLI Command: **data-restore**`, () => {
         }
     });
 
+    beforeEach(async () => {
+        await testRepositoryProvider.testTeardown();
+
+        const testDataInFlatFormat = buildTestDataInFlatFormat();
+
+        const testDataWithUniqueKeys = Object.entries(testDataInFlatFormat).reduce(
+            (acc, [aggregateType, instances]) => ({
+                ...acc,
+                [aggregateType]: instances.map((instance) =>
+                    instance.clone({
+                        id: `${aggregateType.toUpperCase()}.${instance.id}`,
+                    })
+                ),
+            }),
+            {}
+        );
+
+        if (existsSync(fileToRestore)) unlinkSync(fileToRestore);
+
+        writeFileSync(
+            fileToRestore,
+            JSON.stringify(
+                convertInMemorySnapshotToDatabaseFormat(
+                    new DeluxeInMemoryStore(
+                        testDataWithUniqueKeys
+                    ).fetchFullSnapshotInLegacyFormat()
+                ),
+                null,
+                4
+            )
+        );
+        await testRepositoryProvider.testTeardown();
+    });
+
     describe(`when the command is valid`, () => {
         beforeEach(async () => {
-            await testRepositoryProvider.testTeardown();
-
-            const testDataInFlatFormat = buildTestDataInFlatFormat();
-
-            const testDataWithUniqueKeys = Object.entries(testDataInFlatFormat).reduce(
-                (acc, [aggregateType, instances]) => ({
-                    ...acc,
-                    [aggregateType]: instances.map((instance) =>
-                        instance.clone({
-                            id: `${aggregateType.toUpperCase()}.${instance.id}`,
-                        })
-                    ),
-                }),
-                {}
-            );
+            process.env.DATA_MODE = 'import';
+        });
 
-            if (existsSync(fileToRestore)) unlinkSync(fileToRestore);
-
-            writeFileSync(
-                fileToRestore,
-                JSON.stringify(
-                    convertInMemorySnapshotToDatabaseFormat(
-                        new DeluxeInMemoryStore(
-                            testDataWithUniqueKeys
-                        ).fetchFullSnapshotInLegacyFormat()
-                    ),
-                    null,
-                    4
-                )
-            );
+        afterEach(() => {
+            process.env = originalEnv;
         });
 
         describe(`when using the --filepath option to specify the input file`, () => {
@@ -191,4 +202,31 @@ describe(`CLI Command: **data-restore**`, () => {
             });
         });
     });
+
+    describe(`when the environment variable DATA_MODE is not set to 'import'`, () => {
+        beforeEach(async () => {
+            process.env.DATA_MODE = 'definitely_not_import';
+        });
+
+        afterEach(() => {
+            process.env = originalEnv;
+        });
+
+        it(`should not import any data`, async () => {
+            await CommandTestFactory.run(commandInstance, [
+                cliCommandName,
+                `--filepath=${fileToRestore}`,
+            ]);
+
+            const dataExporter = new ArangoDataExporter(new ArangoQueryRunner(databaseProvider));
+
+            const { document: allDocuments, edge: allEdges } = await dataExporter.fetchSnapshot();
+
+            const nonEmptyCollections = [allDocuments, allEdges].flatMap((keysAndDocuments) =>
+                Object.entries(keysAndDocuments).filter(([_key, documents]) => documents.length > 0)
+            );
+
+            expect(nonEmptyCollections).toEqual([]);
+        });
+    });
 });

apps/api/src/coscrad-cli/data-restore.cli-command.ts

@@ -16,7 +16,7 @@ export class DomainRestoreCliCommand extends CliCommandRunner {
     }
 
     async run(_passedParams: string[], { filepath }: { filepath: string }): Promise<void> {
-        return this.dataImporter.restore({ filepath });
+        return this.dataImporter.import({ filepath });
     }
 
     @CliCommandOption({

apps/api/src/coscrad-cli/list-migrations.cli-command.ts

@@ -24,6 +24,12 @@ export class ListMigrationsCliCommand extends CliCommandRunner {
             includeAlreadyRun: false,
         });
 
+        if (migrations.length === 0) {
+            this.logger.log(`no migrations available`);
+
+            return;
+        }
+
         this.logger.log(migrations);
     }
 }

apps/api/src/coscrad-cli/run-migrations.cli-command.ts

@@ -28,6 +28,12 @@ export class RunMigrationsCliCommand extends CliCommandRunner {
             includeAlreadyRun: false,
         });
 
+        if (migrationsToRun.length === 0) {
+            this.logger.log(`No migrations available. Exiting.`);
+
+            return;
+        }
+
         this.logger.log(`Running the following migrations: \n`.concat(migrationsToRun));
 
         await this.migrator.runAllAvailableMigrations(

apps/api/src/persistence/migrations/01/remove-base-digital-asset-url.migration.spec.ts

@@ -1,3 +1,4 @@
+import { isDeepStrictEqual } from 'util';
 import createTestModule from '../../../app/controllers/__tests__/createTestModule';
 import { Photograph } from '../../../domain/models/photograph/entities/photograph.entity';
 import { Term } from '../../../domain/models/term/entities/term.entity';
@@ -26,7 +27,7 @@ const baseDigitalAssetUrl = `https://www.mymedia.org/downloads/`;
 
 process.env[BASE_DIGITAL_ASSET_URL] = baseDigitalAssetUrl;
 
-type OldPhotograph = Omit<Photograph, 'imageUrl'> & { filename: string };
+type OldPhotograph = Omit<Photograph, 'imageUrl'> & { filename?: string };
 
 describe(`RemoveBaseDigitalAssetUrl`, () => {
     let testDatabaseProvider: ArangoDatabaseProvider;
@@ -78,8 +79,17 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
             contributorId: '55',
         };
 
+        const dtoForTermWithoutAudioFilename = {
+            term: `so bogus`,
+            termEnglish: `so bogus (English)`,
+            // audioFilename: MISSING!,
+            type: ResourceType.term,
+            published: true,
+            contributorId: '55',
+        };
+
         const originalTermDocumentsWithoutKeys: Omit<ArangoDatabaseDocument<DTO<Term>>, '_key'>[] =
-            [dtoForTermToCheckManually];
+            [dtoForTermToCheckManually, dtoForTermWithoutAudioFilename];
 
         const originalTermDocuments = originalTermDocumentsWithoutKeys.map((partialDto, index) => ({
             ...partialDto,
@@ -101,10 +111,24 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
             published: true,
         };
 
+        const dtoForPhotographWithoutFilename: Omit<
+            ArangoDatabaseDocument<DTO<OldPhotograph>>,
+            '_key'
+        > = {
+            type: ResourceType.photograph,
+            // filename: MISSING!,
+            photographer: `James Rames`,
+            dimensions: {
+                widthPX: 300,
+                heightPX: 400,
+            },
+            published: true,
+        };
+
         const originalPhotographDocumentsWithoutKeys: Omit<
             ArangoDatabaseDocument<DTO<OldPhotograph>>,
             '_key'
-        >[] = [dtoForPhotographToCheckManually];
+        >[] = [dtoForPhotographToCheckManually, dtoForPhotographWithoutFilename];
 
         const originalPhotographDocuments = originalPhotographDocumentsWithoutKeys.map(
             (partialDto, index) => ({
@@ -130,8 +154,12 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
 
         const idForTermToCheckManually = buildId(ResourceType.term, 0);
 
+        const idForTermWithoutAudioFilename = buildId(ResourceType.term, 1);
+
         const idForPhotographToCheckManually = buildId(ResourceType.photograph, 0);
 
+        const idForPhotographWithoutFilename = buildId(ResourceType.photograph, 1);
+
         it(`should apply the appropriate updates`, async () => {
             await migrationUnderTest.up(testQueryRunner);
 
@@ -142,7 +170,9 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
             expect(updatedTermDocuments.length).toBe(originalTermDocuments.length);
 
             const idsOfDocumentsWithoutBaseDigitalAssetUrl = updatedTermDocuments.filter(
-                ({ audioFilename }) => !audioFilename.includes(baseDigitalAssetUrl)
+                ({ audioFilename }) =>
+                    !isNullOrUndefined(audioFilename) &&
+                    !audioFilename.includes(baseDigitalAssetUrl)
             );
 
             expect(idsOfDocumentsWithoutBaseDigitalAssetUrl).toEqual([]);
@@ -155,6 +185,14 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
 
             expect(audioFilename).toBe(`https://www.mymedia.org/downloads/bogus.mp3`);
 
+            const migratedTermWithoutAudiofilename = (await testDatabaseProvider
+                .getDatabaseForCollection(ArangoCollectionId.terms)
+                .fetchById(idForTermWithoutAudioFilename)) as unknown as ArangoDatabaseDocument<
+                DTO<Term>
+            >;
+
+            expect(migratedTermWithoutAudiofilename.audioFilename).not.toBeTruthy();
+
             const { imageUrl } = (await testDatabaseProvider
                 .getDatabaseForCollection(ArangoCollectionId.photographs)
                 .fetchById(idForPhotographToCheckManually)) as unknown as ArangoDatabaseDocument<
@@ -163,6 +201,14 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
 
             expect(imageUrl).toBe(`https://www.mymedia.org/downloads/flowers.png`);
 
+            const dtoForPhotographWithoutFilename = (await testDatabaseProvider
+                .getDatabaseForCollection(ArangoCollectionId.photographs)
+                .fetchById(idForPhotographWithoutFilename)) as unknown as ArangoDatabaseDocument<
+                DTO<Photograph>
+            >;
+
+            expect(dtoForPhotographWithoutFilename.imageUrl).not.toBeTruthy();
+
             const updatedPhotographDocuments = (await testDatabaseProvider
                 .getDatabaseForCollection(ArangoCollectionId.photographs)
                 .fetchMany()) as DatabaseDTO<DTO<Photograph>>[];
@@ -171,7 +217,8 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
 
             const idsOfPhotographDocumentsWithoutBaseDigitalAssetUrl =
                 updatedPhotographDocuments.filter(
-                    ({ imageUrl }) => !imageUrl.includes(baseDigitalAssetUrl)
+                    ({ imageUrl }) =>
+                        !isNullOrUndefined(imageUrl) && !imageUrl.includes(baseDigitalAssetUrl)
                 );
 
             expect(idsOfPhotographDocumentsWithoutBaseDigitalAssetUrl).toEqual([]);
@@ -212,15 +259,26 @@ describe(`RemoveBaseDigitalAssetUrl`, () => {
                 };
             };
 
-            const snapshotBefore = await buildMiniSnapshot();
+            const { terms: termsBefore, photographs: photographsBefore } =
+                await buildMiniSnapshot();
 
             await migrationUnderTest.up(testQueryRunner);
 
             await migrationUnderTest.down(testQueryRunner);
 
-            const snapshotAfter = await buildMiniSnapshot();
+            const { terms: termsAfter, photographs: photographsAfter } = await buildMiniSnapshot();
+
+            const doArraysContainEqualMembers = <T, U>(a: T[], b: U[]) =>
+                a.length === b.length &&
+                a.every((element) =>
+                    b.some((elementFromB) => isDeepStrictEqual(element, elementFromB))
+                );
+
+            // TODO add a custom jest matcher for this
+            // TODO can we optimize this?
+            expect(doArraysContainEqualMembers(termsBefore, termsAfter)).toBe(true);
 
-            expect(snapshotBefore).toEqual(snapshotAfter);
+            expect(doArraysContainEqualMembers(photographsBefore, photographsAfter)).toBe(true);
         });
     });
 });

apps/api/src/persistence/migrations/01/remove-base-digital-asset-url.migration.ts

@@ -1,4 +1,4 @@
-import { isNonEmptyString } from '@coscrad/validation-constraints';
+import { isNonEmptyString, isNullOrUndefined } from '@coscrad/validation-constraints';
 import { Photograph } from '../../../domain/models/photograph/entities/photograph.entity';
 import { Term } from '../../../domain/models/term/entities/term.entity';
 import { InternalError } from '../../../lib/errors/InternalError';
@@ -60,17 +60,23 @@ export class RemoveBaseDigitalAssetUrl implements ICoscradMigration {
 
         await queryRunner.update<TermDocument, TermDocument>(
             ArangoCollectionId.terms,
-            ({ audioFilename }) => ({
-                audioFilename: `${this.baseDigitalAssetUrl}${audioFilename}.${defaultAudioExtension}`,
-            })
+            ({ audioFilename }) =>
+                isNullOrUndefined(audioFilename)
+                    ? {}
+                    : {
+                          audioFilename: `${this.baseDigitalAssetUrl}${audioFilename}.${defaultAudioExtension}`,
+                      }
         );
 
         await queryRunner.update<OldPhotographDocument, PhotographDocument>(
             ArangoCollectionId.photographs,
-            ({ filename }) => ({
-                imageUrl: `${this.baseDigitalAssetUrl}${filename}.${defaultPhotographExtension}`,
-                filename: null,
-            })
+            ({ filename }) =>
+                isNullOrUndefined(filename)
+                    ? {}
+                    : {
+                          imageUrl: `${this.baseDigitalAssetUrl}${filename}.${defaultPhotographExtension}`,
+                          filename: null,
+                      }
         );
     }
 
@@ -85,7 +91,7 @@ export class RemoveBaseDigitalAssetUrl implements ICoscradMigration {
         await queryRunner.update<TermDocument, TermDocument>(
             ArangoCollectionId.terms,
             ({ audioFilename }) => {
-                if (audioFilename.includes(this.baseDigitalAssetUrl)) {
+                if (audioFilename?.includes(this.baseDigitalAssetUrl)) {
                     return {
                         audioFilename: audioFilename
                             .replace(this.baseDigitalAssetUrl, '')
@@ -98,7 +104,7 @@ export class RemoveBaseDigitalAssetUrl implements ICoscradMigration {
         await queryRunner.update<PhotographDocument, OldPhotographDocument>(
             ArangoCollectionId.photographs,
             ({ imageUrl }) => {
-                if (imageUrl.includes(this.baseDigitalAssetUrl)) {
+                if (imageUrl?.includes(this.baseDigitalAssetUrl)) {
                     return {
                         filename: imageUrl
                             .replace(this.baseDigitalAssetUrl, '')

apps/api/src/persistence/migrations/migrator.ts

@@ -27,7 +27,7 @@ type MigrationVerificationStatus = typeof SUCCESS | typeof FAILURE;
 type MigrationVerificationCheck = {
     name: string;
     status: MigrationVerificationStatus;
-    errors: InternalError[];
+    errors: string[];
 };
 
 type MigrationVerificationReport = {
@@ -132,9 +132,9 @@ export class Migrator {
 
             if (status !== SUCCESS) {
                 logger.log(
-                    `Migration succeeded, but verification failed the following checks: ${checks.filter(
-                        ({ status }) => status === FAILURE
-                    )}`
+                    `Migration succeeded, but verification failed the following checks: ${checks
+                        .filter(({ status }) => status === FAILURE)
+                        .map(({ name }) => name)}`
                 );
             }
 
@@ -236,7 +236,7 @@ export class Migrator {
             {
                 name: 'invariant validation',
                 status: migrationStatus,
-                errors: invariantValidationErrors,
+                errors: invariantValidationErrors.map((error) => error.toString()),
             },
         ];