From e43238abaf91e70531f0653dc7d1776a6f21eccf Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 24 Sep 2024 12:52:57 +0200 Subject: [PATCH] fix: drop and create columns and indexes in the correct order (#21402) --- packages/core/database/src/schema/builder.ts | 35 ++++---- playwright.base.config.js | 2 +- .../edit-collection-type.spec.ts | 81 ++++++++++++++++--- 3 files changed, 88 insertions(+), 30 deletions(-) diff --git a/packages/core/database/src/schema/builder.ts b/packages/core/database/src/schema/builder.ts index 2e07e11539f..d9a2a78ab70 100644 --- a/packages/core/database/src/schema/builder.ts +++ b/packages/core/database/src/schema/builder.ts @@ -259,11 +259,6 @@ const createHelpers = (db: Database) => { dropForeignKey(tableBuilder, updatedForeignKey.object); } - for (const removedColumn of table.columns.removed) { - debug(`Dropping column ${removedColumn.name} on ${table.name}`); - dropColumn(tableBuilder, removedColumn); - } - // for mysql only, dropForeignKey also removes the index, so don't drop it twice const isMySQL = db.config.connection.client === 'mysql'; const ignoreForeignKeyNames = isMySQL @@ -287,7 +282,13 @@ const createHelpers = (db: Database) => { } } - // Update existing columns / foreign keys / indexes + // We drop columns after indexes to ensure that it doesn't cascade delete any indexes we expect to exist + for (const removedColumn of table.columns.removed) { + debug(`Dropping column ${removedColumn.name} on ${table.name}`); + dropColumn(tableBuilder, removedColumn); + } + + // Update existing columns for (const updatedColumn of table.columns.updated) { debug(`Updating column ${updatedColumn.name} on ${table.name}`); @@ -300,16 +301,7 @@ const createHelpers = (db: Database) => { } } - for (const updatedForeignKey of table.foreignKeys.updated) { - debug(`Recreating updated foreign key ${updatedForeignKey.name} on ${table.name}`); - createForeignKey(tableBuilder, updatedForeignKey.object); - } - - for (const updatedIndex of table.indexes.updated) { - debug(`Recreating updated index ${updatedIndex.name} on ${table.name}`); - createIndex(tableBuilder, updatedIndex.object); - } - + // Add any new columns for (const addedColumn of table.columns.added) { debug(`Creating column ${addedColumn.name} on ${table.name}`); @@ -321,6 +313,17 @@ const createHelpers = (db: Database) => { } } + // once the columns have all been updated, we can create indexes again + for (const updatedForeignKey of table.foreignKeys.updated) { + debug(`Recreating updated foreign key ${updatedForeignKey.name} on ${table.name}`); + createForeignKey(tableBuilder, updatedForeignKey.object); + } + + for (const updatedIndex of table.indexes.updated) { + debug(`Recreating updated index ${updatedIndex.name} on ${table.name}`); + createIndex(tableBuilder, updatedIndex.object); + } + for (const addedForeignKey of table.foreignKeys.added) { debug(`Creating foreign keys ${addedForeignKey.name} on ${table.name}`); createForeignKey(tableBuilder, addedForeignKey); diff --git a/playwright.base.config.js b/playwright.base.config.js index d68b5825256..358bfadcf8e 100644 --- a/playwright.base.config.js +++ b/playwright.base.config.js @@ -52,7 +52,7 @@ const createConfig = ({ port, testDir, appDir }) => ({ /* Fail the build on CI if you accidentally left test.only in the source code. */ forbidOnly: !!process.env.CI, /* Retry on CI only */ - retries: process.env.CI ? 3 : 0, + retries: process.env.CI ? 3 : 1, /* Opt out of parallel tests on CI. */ workers: 1, /* Reporter to use. See https://playwright.dev/docs/test-reporters */ diff --git a/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts b/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts index d7a654d665d..93537bb1886 100644 --- a/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts +++ b/tests/e2e/tests/content-type-builder/collection-type/edit-collection-type.spec.ts @@ -10,28 +10,56 @@ import { skipCtbTour, } from '../../../utils/shared'; -// TODO: fix the test so that it doesn't fail on CI -describeOnCondition(!process.env.CI)('Edit collection type', () => { +test.describe('Edit collection type', () => { + // very long timeout for these tests because they restart the server multiple times + test.describe.configure({ timeout: 300000 }); + // use a name with a capital and a space to ensure we also test the kebab-casing conversion for api ids const ctName = 'Secret Document'; + let dependentTestsInitialized = false; + test.beforeEach(async ({ page }) => { - await resetFiles(); - await resetDatabaseAndImportDataFromPath('with-admin.tar'); - await page.goto('/admin'); + // TODO: optimize and duplicate this logic in a standardized way other tests + // reduce each test run by having a fake `beforeAll` to create the content types only once + let loggedIn = false; + if (!dependentTestsInitialized) { + await resetFiles(); + await resetDatabaseAndImportDataFromPath('with-admin.tar'); + + await page.goto('/admin'); + + await login({ page }); + loggedIn = true; + await page.getByRole('link', { name: 'Content-Type Builder' }).click(); + + await skipCtbTour(page); + + // create a collection type to be used + await createCollectionType(page, { + name: ctName, + }); + + await createCollectionType(page, { + name: 'dog', + }); + await createCollectionType(page, { + name: 'owner', + }); + + dependentTestsInitialized = true; + } - await login({ page }); + if (!loggedIn) { + await page.goto('/admin'); - await page.getByRole('link', { name: 'Content-Type Builder' }).click(); + await login({ page }); + loggedIn = true; + } + await page.getByRole('link', { name: 'Content-Type Builder' }).click(); await skipCtbTour(page); - // TODO: create a "saveFileState" mechanism to be used so we don't have to do a full server restart before each test - // create a collection type to be used - await createCollectionType(page, { - name: ctName, - }); - await navToHeader(page, ['Content-Type Builder', ctName], ctName); }); @@ -41,6 +69,33 @@ describeOnCondition(!process.env.CI)('Edit collection type', () => { await resetFiles(); }); + // Tests for GH#21398 + test('Can update relation of type manyToOne to oneToOne', async ({ page }) => { + // Create dog owner relation in Content-Type Builder + await navToHeader(page, ['Content-Type Builder', 'Dog'], 'Dog'); + await page.getByRole('button', { name: /add another field to this/i }).click(); + await page.getByRole('button', { name: /relation/i }).click(); + await page.getByLabel('Basic settings').getByRole('button').nth(3).click(); + await page.getByRole('button', { name: /article/i }).click(); + await page.getByRole('menuitem', { name: /owner/i }).click(); + await page.getByRole('button', { name: 'Finish' }).click(); + await page.getByRole('button', { name: 'Save' }).click(); + + await waitForRestart(page); + + await expect(page.getByRole('cell', { name: 'owner', exact: true })).toBeVisible(); + + // update dog owner relation in Content-Type Builder to oneToOne + await page.getByRole('button', { name: /edit owner/i }).click(); + await page.getByLabel('Basic settings').getByRole('button').nth(0).click(); + await page.getByRole('button', { name: 'Finish' }).click(); + await page.getByRole('button', { name: 'Save' }).click(); + + await waitForRestart(page); + + await expect(page.getByRole('cell', { name: 'owner', exact: true })).toBeVisible(); + }); + test('Can toggle internationalization', async ({ page }) => { await page.getByRole('button', { name: 'Edit' }).first().click(); await page.getByRole('tab', { name: 'Advanced settings' }).click();