Refactor sync metadata to handle index creation if already exists #12757

guillim · 2025-06-20T09:19:43Z

In order to put back relations on track, we need to allow the preexisting indexes to be created in indexmetadata, but also to avoid failure when the migration runner will try to re-apply a pre-existing index

greptile-apps

PR Summary

Implements a temporary fix for index creation during version 55 to 56 migration by catching and logging index errors instead of throwing them. This change affects the workspace migration process but raises concerns about the implementation approach.

Added hardcoded boolean isv55tov56 in workspace-migration-runner.service.ts to handle index creation errors, which violates clean code principles
Using hardcoded version checks (v55tov56) instead of proper migration versioning raises maintainability concerns
This temporary solution bypasses proper error handling, potentially masking critical issues
Consider implementing a more robust versioning mechanism for handling pre-existing indexes

_{1 file reviewed, no comments}
_{Edit PR Review Bot Settings | Greptile}

guillim

ideally, we should check the workspace version to avoid this temp code. But i don't think we have access to this info here

prastoin

Looks good to me !
Would add maybe a comment with some tags such @guillim @prastoin

prastoin · 2025-06-20T09:22:38Z

...rc/engine/workspace-manager/workspace-migration-runner/workspace-migration-runner.service.ts

@@ -226,7 +226,16 @@ export class WorkspaceMigrationRunnerService {
      if (error.code === '42P07') {
        return;
      }
-      throw error;
+
+      const isv55tov56 = true;


NItpick: We could be checking APP_VERSION value directly ( ps: we're releasing 0.60 )

we should check the error messageFormat instead, it's not important to check the version IMO, + Add a // TODO

github-actions · 2025-06-20T09:34:05Z

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:12436

This environment will automatically shut down when the PR is closed or after 5 hours.

charlesBochet · 2025-06-20T09:35:37Z

...rc/engine/workspace-manager/workspace-migration-runner/workspace-migration-runner.service.ts

@@ -226,7 +226,14 @@ export class WorkspaceMigrationRunnerService {
      if (error.code === '42P07') {
        return;
      }
-      throw error;
+      // TODO: Remove this once we have re-indexed all our relations
+      if (error.message.includes('already exists')) {


i think we should also check for Index :p

that's being said we are in the create index function

Let's double check the functional behavior:

workspace with indexMetadata + IDX in db

workspace without indexMetadata and without index in db

workspace without indexMetadata but already index in db

first one checked locally
doing the other ones currently

does not seem to fix locally the issue. investigating

Note: Also very important to test on mulitple index for one objectmetadata to trigger the transaction failure

locally:
✅ workspace with indexMetadata + IDX in db
✅ workspace without indexMetadata and without index in db
✅ workspace without indexMetadata but already index in db

…enty into temproary-index-code

prastoin · 2025-06-20T11:57:28Z

...rc/engine/workspace-manager/workspace-migration-runner/workspace-migration-runner.service.ts

+        const whereClause = index.where ? `WHERE ${index.where}` : '';
+
+        await queryRunner.query(`
+          CREATE ${isUnique} INDEX IF NOT EXISTS "${index.name}" ON "${schemaName}"."${tableName}" (${quotedColumns.join(', ')}) ${whereClause}


Nitpick: Would still escape params here

updates

prastoin · 2025-06-20T11:58:00Z

@greptileai trigger

greptile-apps

PR Summary

Refactored index creation logic to use raw SQL queries with 'IF NOT EXISTS' clause instead of TypeORM's TableIndex, providing a more permanent solution for handling pre-existing indexes during migrations.

Removed direct TypeORM TableIndex usage in favor of raw SQL queries, improving flexibility and control over index creation
Implemented 'IF NOT EXISTS' clause for index creation to prevent failures with pre-existing indexes
Consider extracting SQL queries into a dedicated database utility service for better maintainability
Ensure consistent error logging and monitoring for index creation operations
Consider adding integration tests specifically for index recreation scenarios

_{1 file reviewed, no comments}
_{Edit PR Review Bot Settings | Greptile}

Weiko

Lgtm + @prastoin comment

In order to put back relations on track, we need to allow the preexisting indexes to be created in indexmetadata, but also to avoid failure when the migration runner will try to re-apply a pre-existing index

sentry-io · 2025-06-20T15:30:31Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ QueryFailedError: could not create unique index "IDX_UNIQUE_6d269465206d2f3e283ce479b2e" POST /metadata View Issue

_{Did you find this useful? React with a 👍 or 👎}

prastoin · 2025-06-20T15:50:26Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ QueryFailedError: could not create unique index "IDX_UNIQUE_6d269465206d2f3e283ce479b2e" POST /metadata View Issue

Did you find this useful? React with a 👍 or 👎

Coco is faster than you

temporary code

5f874b7

guillim self-assigned this Jun 20, 2025

guillim requested a review from prastoin June 20, 2025 09:19

greptile-apps bot reviewed Jun 20, 2025

View reviewed changes

guillim commented Jun 20, 2025

View reviewed changes

prastoin previously approved these changes Jun 20, 2025

View reviewed changes

changing the way we check errors

6b5d89a

adding toto

36d36cf

charlesBochet reviewed Jun 20, 2025

View reviewed changes

guillim and others added 4 commits June 20, 2025 13:49

better way

d0a1ba5

Merge branch 'main' into temproary-index-code

a170cc6

typo

51ab7b4

Merge branch 'temproary-index-code' of https://github.com/twentyhq/tw…

319a7e1

…enty into temproary-index-code

prastoin reviewed Jun 20, 2025

View reviewed changes

greptile-apps bot reviewed Jun 20, 2025

View reviewed changes

Weiko approved these changes Jun 20, 2025

View reviewed changes

guillim merged commit 3977506 into main Jun 20, 2025
42 checks passed

guillim deleted the temproary-index-code branch June 20, 2025 12:20

prastoin changed the title ~~temporary code~~ Refactor sync metadata to handle index creation if already exists Jun 20, 2025

Refactor sync metadata to handle index creation if already exists #12757

Refactor sync metadata to handle index creation if already exists #12757

Uh oh!

Conversation

guillim commented Jun 20, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

guillim left a comment

Choose a reason for hiding this comment

Uh oh!

prastoin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guillim Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prastoin commented Jun 20, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

Weiko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sentry-io bot commented Jun 20, 2025

Suspect Issues

Uh oh!

prastoin commented Jun 20, 2025

Suspect Issues

Uh oh!

Uh oh!

github-actions bot commented Jun 20, 2025 •

edited

Loading

guillim Jun 20, 2025 •

edited

Loading