fix(server): resolve foreign key violation blocking application uninstall (#22502)
Fixes [sonarly issue #54192](https://sonarly.com/issue/54192) ## Problem Uninstalling an application fails with a DB error when its `packageJsonFileId` / `yarnLockFileId` columns are populated: update or delete on table "file" violates foreign key constraint "FK_3818380258798f9ffa9963b6dc4" on table "application" Storage was also wiped before the failing DB delete, leaving the app half-uninstalled. ## Root cause `application` and `file` reference each other through `ON DELETE RESTRICT` FKs (`application.packageJsonFileId/yarnLockFileId → file.id` and `file.applicationId → application.id`), so no deletion order works on its own. The deferrable-FK migration doesn't help: in Postgres, `RESTRICT` fires immediately even on `DEFERRABLE INITIALLY DEFERRED` constraints (only `NO ACTION` honors deferral). Uninstall deleted file rows first, in autocommit statements. ## Fix `ApplicationService.delete()` now runs in a single transaction: 1. Clear `packageJsonFileId` / `yarnLockFileId` (breaks the FK cycle) 2. Delete the app's `file` rows 3. Delete the `application` row Storage cleanup moved after commit and made non-fatal, so a failure can no longer leave partial state. `deleteApplicationFiles` is split into `deleteApplicationFileRows` (DB, transactional) and `deleteApplicationFilesFromStorage` (blobs). The test cleanup util had the same file-first ordering bug and is fixed the same way. ## Questions / Follow-ups - **Should the FK cycle be resolved at the schema level?** Both legs could be switched to `ON DELETE NO ACTION DEFERRABLE INITIALLY DEFERRED`, which appears to be what the deferrable-FK migration intended — deferral would then actually apply to deletes, making transactional deletion order-independent. Happy to open a separate PR if there's interest. - **Should the marketplace install path set the package file FKs?** It stores `package.json` in the `file` table but never populates `application.packageJsonFileId` / `yarnLockFileId` — today only workspace creation and `application:rebuild-default-deps` set them. Marketplace packages also don't ship a `yarn.lock`, so this needs a product decision. <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/22502?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This commit is contained in:
@@ -1,21 +1,21 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { InjectRepository } from '@nestjs/typeorm';
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
import { InjectDataSource, InjectRepository } from '@nestjs/typeorm';
|
||||
|
||||
import { FileFolder } from 'twenty-shared/types';
|
||||
import { isDefined } from 'twenty-shared/utils';
|
||||
import { type QueryRunner, type Repository } from 'typeorm';
|
||||
import { type DataSource, type QueryRunner, type Repository } from 'typeorm';
|
||||
import { v4 } from 'uuid';
|
||||
|
||||
import { ApplicationEntity } from 'src/engine/core-modules/application/application.entity';
|
||||
import {
|
||||
ApplicationException,
|
||||
ApplicationExceptionCode,
|
||||
} from 'src/engine/core-modules/application/application.exception';
|
||||
import { getDefaultApplicationPackageFields } from 'src/engine/core-modules/application/application-package/utils/get-default-application-package-fields.util';
|
||||
import { parseAvailablePackagesFromPackageJsonAndYarnLock } from 'src/engine/core-modules/application/application-package/utils/parse-available-packages-from-package-json-and-yarn-lock.util';
|
||||
import { ApplicationRegistrationEntity } from 'src/engine/core-modules/application/application-registration/application-registration.entity';
|
||||
import { ApplicationRegistrationSourceType } from 'src/engine/core-modules/application/application-registration/enums/application-registration-source-type.enum';
|
||||
import { ApplicationVariableEntity } from 'src/engine/core-modules/application/application-variable/application-variable.entity';
|
||||
import { ApplicationEntity } from 'src/engine/core-modules/application/application.entity';
|
||||
import {
|
||||
ApplicationException,
|
||||
ApplicationExceptionCode,
|
||||
} from 'src/engine/core-modules/application/application.exception';
|
||||
import { WORKSPACE_CUSTOM_APPLICATION_NAME } from 'src/engine/core-modules/application/constants/workspace-custom-application.constant';
|
||||
import { type FlatApplication } from 'src/engine/core-modules/application/types/flat-application.type';
|
||||
import { FileStorageService } from 'src/engine/core-modules/file-storage/file-storage.service';
|
||||
@@ -34,7 +34,11 @@ import { TWENTY_STANDARD_APPLICATION } from 'src/engine/workspace-manager/twenty
|
||||
|
||||
@Injectable()
|
||||
export class ApplicationService {
|
||||
private readonly logger = new Logger(ApplicationService.name);
|
||||
|
||||
constructor(
|
||||
@InjectDataSource()
|
||||
private readonly coreDataSource: DataSource,
|
||||
@InjectRepository(ApplicationEntity)
|
||||
private readonly applicationRepository: Repository<ApplicationEntity>,
|
||||
@InjectRepository(ApplicationRegistrationEntity)
|
||||
@@ -584,15 +588,50 @@ export class ApplicationService {
|
||||
);
|
||||
}
|
||||
|
||||
await this.fileStorageService.deleteApplicationFiles({
|
||||
workspaceId,
|
||||
applicationUniversalIdentifier: universalIdentifier,
|
||||
});
|
||||
const queryRunner = this.coreDataSource.createQueryRunner();
|
||||
|
||||
await this.applicationRepository.delete({
|
||||
universalIdentifier,
|
||||
workspaceId,
|
||||
});
|
||||
await queryRunner.connect();
|
||||
await queryRunner.startTransaction();
|
||||
|
||||
try {
|
||||
await queryRunner.manager.update(
|
||||
ApplicationEntity,
|
||||
{ id: application.id },
|
||||
{ packageJsonFileId: null, yarnLockFileId: null },
|
||||
);
|
||||
|
||||
await this.fileStorageService.deleteApplicationFileRows({
|
||||
applicationId: application.id,
|
||||
workspaceId,
|
||||
queryRunner,
|
||||
});
|
||||
|
||||
await queryRunner.manager.delete(ApplicationEntity, {
|
||||
universalIdentifier,
|
||||
workspaceId,
|
||||
});
|
||||
|
||||
await queryRunner.commitTransaction();
|
||||
} catch (error) {
|
||||
if (queryRunner.isTransactionActive) {
|
||||
await queryRunner.rollbackTransaction();
|
||||
}
|
||||
|
||||
throw error;
|
||||
} finally {
|
||||
await queryRunner.release();
|
||||
}
|
||||
|
||||
try {
|
||||
await this.fileStorageService.deleteApplicationFilesFromStorage({
|
||||
workspaceId,
|
||||
applicationUniversalIdentifier: universalIdentifier,
|
||||
});
|
||||
} catch (error) {
|
||||
this.logger.warn(
|
||||
`Failed to delete storage folder for application ${universalIdentifier} in workspace ${workspaceId}: ${error instanceof Error ? error.message : String(error)}`,
|
||||
);
|
||||
}
|
||||
|
||||
await this.workspaceCacheService.invalidateAndRecompute(workspaceId, [
|
||||
'flatApplicationMaps',
|
||||
|
||||
+17
-12
@@ -228,29 +228,34 @@ export class FileStorageService {
|
||||
});
|
||||
}
|
||||
|
||||
async deleteApplicationFiles({
|
||||
async deleteApplicationFileRows({
|
||||
applicationId,
|
||||
workspaceId,
|
||||
queryRunner,
|
||||
}: {
|
||||
applicationId: string;
|
||||
workspaceId: string;
|
||||
queryRunner?: QueryRunner;
|
||||
}) {
|
||||
const fileRepository = queryRunner
|
||||
? this.fileRepository.withManager(queryRunner.manager)
|
||||
: this.fileRepository;
|
||||
|
||||
await fileRepository.delete(workspaceId, { applicationId });
|
||||
}
|
||||
|
||||
async deleteApplicationFilesFromStorage({
|
||||
applicationUniversalIdentifier,
|
||||
workspaceId,
|
||||
}: {
|
||||
applicationUniversalIdentifier: string;
|
||||
workspaceId: string;
|
||||
}) {
|
||||
const application = await this.applicationRepository.findOneOrFail({
|
||||
where: {
|
||||
universalIdentifier: applicationUniversalIdentifier,
|
||||
workspaceId: workspaceId,
|
||||
},
|
||||
});
|
||||
|
||||
const driver = this.fileStorageDriverFactory.getCurrentDriver();
|
||||
|
||||
await driver.delete({
|
||||
folderPath: `${workspaceId}/${applicationUniversalIdentifier}/`,
|
||||
});
|
||||
|
||||
await this.fileRepository.delete(workspaceId, {
|
||||
applicationId: application.id,
|
||||
});
|
||||
}
|
||||
|
||||
async deleteFile(params: ResourceIdentifier): Promise<void> {
|
||||
|
||||
+105
@@ -0,0 +1,105 @@
|
||||
import { buildBaseManifest } from 'test/integration/metadata/suites/application/utils/build-base-manifest.util';
|
||||
import { cleanupApplicationAndAppRegistration } from 'test/integration/metadata/suites/application/utils/cleanup-application-and-app-registration.util';
|
||||
import { setupApplicationForSync } from 'test/integration/metadata/suites/application/utils/setup-application-for-sync.util';
|
||||
import { syncApplication } from 'test/integration/metadata/suites/application/utils/sync-application.util';
|
||||
import { uninstallApplication } from 'test/integration/metadata/suites/application/utils/uninstall-application.util';
|
||||
import { uploadApplicationFile } from 'test/integration/metadata/suites/application/utils/upload-application-file.util';
|
||||
import { v4 as uuidv4 } from 'uuid';
|
||||
|
||||
const TEST_APP_ID = uuidv4();
|
||||
const TEST_ROLE_ID = uuidv4();
|
||||
|
||||
describe('Uninstall application with package file FKs populated', () => {
|
||||
beforeEach(async () => {
|
||||
await setupApplicationForSync({
|
||||
applicationUniversalIdentifier: TEST_APP_ID,
|
||||
name: 'Test Application',
|
||||
description: 'App for testing uninstall with package file FKs',
|
||||
sourcePath: 'test-uninstall-package-file-fks',
|
||||
});
|
||||
|
||||
await syncApplication({
|
||||
manifest: buildBaseManifest({
|
||||
appId: TEST_APP_ID,
|
||||
roleId: TEST_ROLE_ID,
|
||||
}),
|
||||
expectToFail: false,
|
||||
});
|
||||
}, 60000);
|
||||
|
||||
afterEach(async () => {
|
||||
await cleanupApplicationAndAppRegistration({
|
||||
applicationUniversalIdentifier: TEST_APP_ID,
|
||||
});
|
||||
});
|
||||
|
||||
it('uninstalls the application and deletes its file rows in one pass', async () => {
|
||||
jest.useRealTimers();
|
||||
|
||||
await uploadApplicationFile({
|
||||
applicationUniversalIdentifier: TEST_APP_ID,
|
||||
fileFolder: 'Dependencies',
|
||||
filePath: 'yarn.lock',
|
||||
fileBuffer: Buffer.from('# test yarn.lock\n'),
|
||||
filename: 'yarn.lock',
|
||||
contentType: 'text/plain',
|
||||
expectToFail: false,
|
||||
});
|
||||
|
||||
jest.useFakeTimers();
|
||||
|
||||
const [application] = await globalThis.testDataSource.query(
|
||||
`SELECT id FROM core."application" WHERE "universalIdentifier" = $1`,
|
||||
[TEST_APP_ID],
|
||||
);
|
||||
|
||||
const applicationId: string = application.id;
|
||||
|
||||
await globalThis.testDataSource.query(
|
||||
`UPDATE core."application" a
|
||||
SET "packageJsonFileId" = (
|
||||
SELECT f.id FROM core."file" f
|
||||
WHERE f."applicationId" = a.id
|
||||
AND f.path = 'dependencies/package.json'
|
||||
),
|
||||
"yarnLockFileId" = (
|
||||
SELECT f.id FROM core."file" f
|
||||
WHERE f."applicationId" = a.id
|
||||
AND f.path = 'dependencies/yarn.lock'
|
||||
)
|
||||
WHERE a.id = $1`,
|
||||
[applicationId],
|
||||
);
|
||||
|
||||
const [applicationBeforeUninstall] = await globalThis.testDataSource.query(
|
||||
`SELECT "packageJsonFileId", "yarnLockFileId"
|
||||
FROM core."application" WHERE id = $1`,
|
||||
[applicationId],
|
||||
);
|
||||
|
||||
expect(applicationBeforeUninstall.packageJsonFileId).not.toBeNull();
|
||||
expect(applicationBeforeUninstall.yarnLockFileId).not.toBeNull();
|
||||
|
||||
const { data, errors } = await uninstallApplication({
|
||||
universalIdentifier: TEST_APP_ID,
|
||||
expectToFail: false,
|
||||
});
|
||||
|
||||
expect(errors).toBeUndefined();
|
||||
expect(data?.uninstallApplication).toBe(true);
|
||||
|
||||
const applicationsAfterUninstall = await globalThis.testDataSource.query(
|
||||
`SELECT id FROM core."application" WHERE "universalIdentifier" = $1`,
|
||||
[TEST_APP_ID],
|
||||
);
|
||||
|
||||
expect(applicationsAfterUninstall).toHaveLength(0);
|
||||
|
||||
const fileRowsAfterUninstall = await globalThis.testDataSource.query(
|
||||
`SELECT id FROM core."file" WHERE "applicationId" = $1`,
|
||||
[applicationId],
|
||||
);
|
||||
|
||||
expect(fileRowsAfterUninstall).toHaveLength(0);
|
||||
}, 60000);
|
||||
});
|
||||
+7
@@ -14,6 +14,13 @@ export const cleanupApplicationAndAppRegistration = async ({
|
||||
// May fail if the sync never succeeded
|
||||
}
|
||||
|
||||
await globalThis.testDataSource.query(
|
||||
`UPDATE core."application"
|
||||
SET "packageJsonFileId" = NULL, "yarnLockFileId" = NULL
|
||||
WHERE "universalIdentifier" = $1`,
|
||||
[applicationUniversalIdentifier],
|
||||
);
|
||||
|
||||
await globalThis.testDataSource.query(
|
||||
`DELETE FROM core."file" WHERE "applicationId" IN (
|
||||
SELECT id FROM core."application" WHERE "universalIdentifier" = $1
|
||||
|
||||
Reference in New Issue
Block a user