feat: rollback if patch exists before applying

elibosley · elibosley · commit c2f4e8d4e5c7 · 2025-02-04T12:05:56.000-05:00
diff --git a/api/src/unraid-api/unraid-file-modifier/file-modification.ts b/api/src/unraid-api/unraid-file-modifier/file-modification.ts
@@ -4,7 +4,6 @@ import { access, readFile, unlink, writeFile } from 'fs/promises';
 import { basename, dirname, join } from 'path';
 
 import { applyPatch, parsePatch, reversePatch } from 'diff';
-import { patch } from 'semver';
 
 export interface ShouldApplyWithReason {
     shouldApply: boolean;
@@ -28,11 +27,16 @@ export abstract class FileModification {
     }
 
     private async savePatch(patchResult: string): Promise<void> {
-        const patchFile = this.getPatchFilePath(this.filePath);
-        await writeFile(patchFile, patchResult, 'utf8');
+        const patchFilePath = this.getPatchFilePath(this.filePath);
+        await writeFile(patchFilePath, patchResult, 'utf8');
     }
 
-    private async loadSavedPatch(targetFile: string): Promise<string | null> {
+    /**
+     * Loads the applied patch for the target file if it exists
+     * @param targetFile - The path to the file to be patched
+     * @returns The patch contents if it exists (targetFile.patch), null otherwise
+     */
+    private async loadPatchedFilePatch(targetFile: string): Promise<string | null> {
         const patchFile = this.getPatchFilePath(targetFile);
         try {
             await access(patchFile, constants.R_OK);
@@ -80,9 +84,13 @@ export abstract class FileModification {
         await writeFile(this.filePath, results);
     }
 
-    // Default implementation of apply that uses the patch
     async apply(): Promise<void> {
         try {
+            const savedPatch = await this.loadPatchedFilePatch(this.filePath);
+            if (savedPatch) {
+                // Rollback the saved patch before applying the new patch
+                await this.rollback();
+            }
             // First attempt to apply the patch that was generated
             const staticPatch = await this.getPregeneratedPatch();
             if (staticPatch) {
@@ -104,12 +112,11 @@ export abstract class FileModification {
         }
     }
 
-    // Update rollback to use the shared utility
     async rollback(): Promise<void> {
         let patch: string;
 
         // Try to load saved patch first
-        const savedPatch = await this.loadSavedPatch(this.filePath);
+        const savedPatch = await this.loadPatchedFilePatch(this.filePath);
         if (savedPatch) {
             this.logger.debug(`Using saved patch file for ${this.id}`);
             patch = savedPatch;
@@ -141,15 +148,37 @@ export abstract class FileModification {
             await access(patchFile, constants.W_OK);
             await unlink(patchFile);
         } catch {
-            // Ignore errors when trying to delete the patch file
+            this.logger.warn(`Failed to delete patch file for ${this.id}`);
         }
     }
 
     // Default implementation that can be overridden if needed
     async shouldApply(): Promise<ShouldApplyWithReason> {
-        return {
-            shouldApply: true,
-            reason: 'Default behavior is to always apply modifications',
-        };
+        try {
+            if (!this.filePath || !this.id) {
+                throw new Error('Invalid file modification configuration');
+            }
+
+            const fileExists = await access(this.filePath, constants.R_OK | constants.W_OK)
+                .then(() => true)
+                .catch(() => false);
+
+            if (!fileExists) {
+                return {
+                    shouldApply: false,
+                    reason: `Target file ${this.filePath} does not exist or is not accessible`,
+                };
+            }
+            return {
+                shouldApply: true,
+                reason: 'Default behavior is to apply modifications if the file exists',
+            };
+        } catch (err) {
+            this.logger.error(`Failed to check if file ${this.filePath} should be applied: ${err}`);
+            return {
+                shouldApply: false,
+                reason: `Failed to check if file ${this.filePath} should be applied: ${err}`,
+            };
+        }
     }
 }
diff --git a/api/src/unraid-api/unraid-file-modifier/modifications/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time b/api/src/unraid-api/unraid-file-modifier/modifications/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time
@@ -1 +1 @@
-1738614986599
+1738681678475
diff --git a/api/src/unraid-api/unraid-file-modifier/modifications/__fixtures__/downloaded/Notifications.page.last-download-time b/api/src/unraid-api/unraid-file-modifier/modifications/__fixtures__/downloaded/Notifications.page.last-download-time
@@ -1 +1 @@
-1738614986764
+1738681678771
diff --git a/api/src/unraid-api/unraid-file-modifier/modifications/__fixtures__/downloaded/auth-request.php.last-download-time b/api/src/unraid-api/unraid-file-modifier/modifications/__fixtures__/downloaded/auth-request.php.last-download-time
@@ -1 +1 @@
-1738614987214
+1738681679144
diff --git a/api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts b/api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts
@@ -2,21 +2,22 @@ import { Logger } from '@nestjs/common';
 import { readFile, writeFile } from 'fs/promises';
 import { basename, resolve } from 'path';
 
-import { describe, expect, test } from 'vitest';
+import { describe, expect, test, vi } from 'vitest';
 
 import { FileModification } from '@app/unraid-api/unraid-file-modifier/file-modification';
 import AuthRequestModification from '@app/unraid-api/unraid-file-modifier/modifications/auth-request.modification';
 import DefaultPageLayoutModification from '@app/unraid-api/unraid-file-modifier/modifications/default-page-layout.modification';
 import { LogRotateModification } from '@app/unraid-api/unraid-file-modifier/modifications/log-rotate.modification';
 import NotificationsPageModification from '@app/unraid-api/unraid-file-modifier/modifications/notifications-page.modification';
 import SSOFileModification from '@app/unraid-api/unraid-file-modifier/modifications/sso.modification';
+import { afterEach } from 'node:test';
 
 interface ModificationTestCase {
     ModificationClass: new (...args: ConstructorParameters<typeof FileModification>) => FileModification;
     fileUrl: string;
     fileName: string;
 }
-
+let patcher: FileModification;
 const testCases: ModificationTestCase[] = [
     {
         ModificationClass: DefaultPageLayoutModification,
@@ -80,7 +81,7 @@ async function testModification(testCase: ModificationTestCase) {
     const filePath = resolve(__dirname, `../__fixtures__/downloaded/${fileName}`);
     const originalContent = await downloadOrRetrieveOriginalFile(filePath, testCase.fileUrl);
     const logger = new Logger();
-    const patcher = await new testCase.ModificationClass(logger);
+    patcher = await new testCase.ModificationClass(logger);
     const originalPath = patcher.filePath;
     // @ts-expect-error - Ignore for testing purposes
     patcher.filePath = filePath;
@@ -103,8 +104,43 @@ async function testModification(testCase: ModificationTestCase) {
     await expect(revertedContent).toMatch(originalContent);
 }
 
+async function testInvalidModification(testCase: ModificationTestCase) {
+    const mockLogger = {
+        log: vi.fn(),
+        error: vi.fn(),
+        warn: vi.fn(),
+        debug: vi.fn(),
+        verbose: vi.fn(),
+    };
+
+    patcher = new testCase.ModificationClass(mockLogger as unknown as Logger);
+
+    // @ts-expect-error - Testing invalid pregenerated patches
+    patcher.getPregeneratedPatch = vi.fn().mockResolvedValue('I AM NOT A VALID PATCH');
+
+    const path = patcher.filePath;
+    const filePath = resolve(__dirname, `../__fixtures__/downloaded/${testCase.fileName}`);
+
+    // @ts-expect-error - Testing invalid pregenerated patches
+    patcher.filePath = filePath;
+    await patcher.apply();
+
+    expect(mockLogger.error.mock.calls[0][0]).toContain(`Failed to apply static patch to ${filePath}`);
+
+    expect(mockLogger.error.mock.calls.length).toBe(1);
+    await patcher.rollback();
+}
+
 describe('File modifications', () => {
     test.each(testCases)(`$fileName modifier correctly applies to fresh install`, async (testCase) => {
         await testModification(testCase);
     });
+
+    test.each(testCases)(`$fileName modifier correctly handles invalid content`, async (testCase) => {
+        await testInvalidModification(testCase);
+    });
+
+    afterEach(async () => {
+        await patcher?.rollback();
+    });
 });
