fix: drop and create columns and indexes in the correct order (strapi…

…#21402)
nilsingwersen · Sep 24, 2024 · e43238a · e43238a
1 parent 8919752
commit e43238a
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 30 deletions.
diff --git 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
@@ -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
@@ -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();