From a12181a5e062c3235b2e1f4fefeca4ad2b3349f3 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Tue, 4 Feb 2025 10:15:08 -0500 Subject: [PATCH] feat: rollback if patch exists before applying --- .../unraid-file-modifier/file-modification.ts | 53 ++++++++++++++----- .../DefaultPageLayout.php.last-download-time | 2 +- .../Notifications.page.last-download-time | 2 +- .../auth-request.php.last-download-time | 2 +- .../__test__/generic-modification.spec.ts | 42 +++++++++++++-- 5 files changed, 83 insertions(+), 18 deletions(-) diff --git a/api/src/unraid-api/unraid-file-modifier/file-modification.ts b/api/src/unraid-api/unraid-file-modifier/file-modification.ts index c22441aee..c03fff685 100644 --- 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 { - 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 { + /** + * 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 { 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 { 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 { 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 { - 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 index 50ff2c43b..29da400b7 100644 --- 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 \ No newline at end of file +1738681678475 \ No newline at end of file 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 index f07add427..3a7b5a6ee 100644 --- 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 \ No newline at end of file +1738681678771 \ No newline at end of file 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 index e3f4f1aa9..9f49493e8 100644 --- 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 \ No newline at end of file +1738681679144 \ No newline at end of file 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 index e3b8265f1..dcba02a97 100644 --- 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,7 +2,7 @@ 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'; @@ -10,13 +10,14 @@ import DefaultPageLayoutModification from '@app/unraid-api/unraid-file-modifier/ 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) => 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(); + }); });