Migrate attachments to morph relations + fix morph join column filtering#17381
Migrate attachments to morph relations + fix morph join column filtering#17381FelixMalfait merged 10 commits intomainfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 31 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-migrate-attachment-to-morph-relations.command.ts">
<violation number="1" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-migrate-attachment-to-morph-relations.command.ts:223">
P1: Feature flag enabling and cache operations after transaction commit can leave workspace in inconsistent state if they fail. Since the transaction is already committed, a failure here means database changes are persisted but the feature flag isn't set, causing the next migration attempt to fail on already-renamed columns. Consider enabling the feature flag before committing, or making the column renames idempotent by checking column existence first.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| this.logger.log(`✅ Successfully migrated attachment fieldmetadata`); | ||
|
|
||
| await queryRunner.commitTransaction(); |
There was a problem hiding this comment.
P1: Feature flag enabling and cache operations after transaction commit can leave workspace in inconsistent state if they fail. Since the transaction is already committed, a failure here means database changes are persisted but the feature flag isn't set, causing the next migration attempt to fail on already-renamed columns. Consider enabling the feature flag before committing, or making the column renames idempotent by checking column existence first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/database/commands/upgrade-version-command/1-17/1-17-migrate-attachment-to-morph-relations.command.ts, line 223:
<comment>Feature flag enabling and cache operations after transaction commit can leave workspace in inconsistent state if they fail. Since the transaction is already committed, a failure here means database changes are persisted but the feature flag isn't set, causing the next migration attempt to fail on already-renamed columns. Consider enabling the feature flag before committing, or making the column renames idempotent by checking column existence first.</comment>
<file context>
@@ -0,0 +1,258 @@
+
+ this.logger.log(`✅ Successfully migrated attachment fieldmetadata`);
+
+ await queryRunner.commitTransaction();
+
+ await this.featureFlagService.enableFeatureFlags(
</file context>
There was a problem hiding this comment.
1-13-migrate-timeline-activity-to-morph-relations.command.ts followed the same pattern.
Once executed, I believe the command will never run again on a specific server, so we do not need to over-engineer imo.
Greptile OverviewGreptile SummaryThis PR successfully migrates attachment relations from individual foreign keys to a unified morph relation pattern, following the TimelineActivity approach. The migration is controlled by the Key Changes:
Technical Implementation:
The feature flag ensures backward compatibility during rollout, and new workspaces have the flag enabled by default. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant FeatureFlag
participant AttachmentHook
participant GraphQL
participant Migration
participant Database
Note over User,Database: Attachment Upload Flow
User->>Frontend: Upload attachment file
Frontend->>FeatureFlag: Check IS_ATTACHMENT_MIGRATED
FeatureFlag-->>Frontend: Feature flag status
Frontend->>AttachmentHook: getActivityTargetObjectFieldIdName()
alt IS_ATTACHMENT_MIGRATED enabled
AttachmentHook-->>Frontend: targetCompanyId
else Legacy mode
AttachmentHook-->>Frontend: companyId
end
Frontend->>GraphQL: createOneAttachment({targetCompanyId: id})
GraphQL->>Database: INSERT with target* column
Database-->>GraphQL: Attachment record
GraphQL-->>Frontend: Created attachment
Note over User,Database: Attachment Query Flow
User->>Frontend: View attachments
Frontend->>FeatureFlag: Check IS_ATTACHMENT_MIGRATED
FeatureFlag-->>Frontend: Feature flag status
Frontend->>AttachmentHook: Compute filter field name
Frontend->>GraphQL: findManyAttachments(filter: {targetCompanyId: {eq: id}})
GraphQL->>Database: Query with morph relation join
Note over GraphQL,Frontend: Filter Matching (Optimistic Cache)
GraphQL-->>Frontend: Attachment metadata + morphRelations[]
Frontend->>Frontend: doesMorphRelationJoinColumnMatch()
Frontend->>Frontend: Compute all valid join columns from morphRelations
Frontend->>Frontend: Match filterKey against computed names
Frontend-->>User: Display attachments
Note over User,Database: Migration Process
Migration->>Database: BEGIN TRANSACTION
Migration->>Database: ALTER TABLE: Rename columns (e.g., companyId → targetCompanyId)
Migration->>Database: UPDATE fieldMetadata: Set type=MORPH_RELATION, morphId=shared_id
Migration->>Database: COMMIT
Migration->>FeatureFlag: Enable IS_ATTACHMENT_MIGRATED
Migration->>Database: Invalidate workspace cache
Migration-->>User: Migration complete
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:49584 This environment will automatically shut down when the PR is closed or after 5 hours. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
.../commands/upgrade-version-command/1-17/1-17-migrate-attachment-to-morph-relations.command.ts
Show resolved
Hide resolved
- Rename doesMorphRelationJoinColumnMatch to isMorphRelationJoinColumnKey - Reuse existing computePossibleMorphGqlFieldForFieldName utility - Fix inconsistency: use morphRelation.type instead of settings.relationType - Organize imports properly
FelixMalfait
left a comment
FelixMalfait
left a comment
There was a problem hiding this comment.
Great work!!!
Let's do taskTarget/noteTarget but only consider these as MORPH for now (as if noteTarget was a normal object, don't try to solve the migration to MANY-TO-MANY in the same PR, we will try it late). So noteTarget+taskTarket should be very similar to this PR (maybe one PR for both is enough)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Transaction not rolled back on early return path
Medium Severity
When attachmentObjectMetadata is not found, the function returns early at line 124 without rolling back the transaction that was started at line 73. The finally block will call queryRunner.release(), but the transaction remains in an undefined state—neither committed nor explicitly rolled back. This is inconsistent with the catch block's explicit rollbackTransaction() call and could cause issues with connection pooling or leave the database in an unexpected state depending on the driver.
|
Thanks @mabdullahabaid for your contribution! |
prastoin
left a comment
prastoin
left a comment
There was a problem hiding this comment.
Hey there ! Solid work over there
We should be sure to test ( sorry if redundant also a reminder to myself ):
- False Feature flag implem is working as expected on existing workspaces
- True Feature flag implem is working as expected on new workspaces
- Refactor as possible using new universalIdentifier the backfill command on a prod extract ( lets reach out on monday )
| (objectMetadata) => objectMetadata.nameSingular, | ||
| ); | ||
|
|
||
| const fieldNameMigrations = [ |
There was a problem hiding this comment.
Suggestion: We can directly identify such field, at least if existing for the given workspace, using their universalIdentifier
| }), | ||
| ); | ||
|
|
||
| const attachmentObjectMetadata = |
| ); | ||
|
|
||
| await this.workspaceCacheService.invalidateAndRecompute(workspaceId, [ | ||
| 'flatFieldMetadataMaps', |
There was a problem hiding this comment.
Suggestion: We should use such pattern in order to not forget any related cache entries
const relatedMetadataNames = getMetadataRelatedMetadataNames('viewField');
const relatedCacheKeysToInvalidate = relatedMetadataNames.map(
getMetadataFlatEntityMapsKey,
);
this.logger.log(
`Invalidating caches: ${relatedCacheKeysToInvalidate.join(' ')}`,
);
if (!options.dryRun) {
await this.workspaceCacheService.invalidateAndRecompute(workspaceId, [
'flatViewFieldMaps',
...relatedCacheKeysToInvalidate,
]);
}|
|
||
| export const DEFAULT_FEATURE_FLAGS = [ | ||
| FeatureFlagKey.IS_TIMELINE_ACTIVITY_MIGRATED, | ||
| FeatureFlagKey.IS_ATTACHMENT_MIGRATED, |
|
@prastoin thank you for the review and suggestions, will look into them tomorrow! |
…ing (twentyhq#17381) Closes [1744](twentyhq/core-team-issues#1744). This PR migrates attachments to morph relations behind a feature flag, following the TimelineActivity pattern. it introduces the `IS_ATTACHMENT_MIGRATED` flag, updates standard field metadata and indexes to use morph relations, adds a workspace migration that renames `attachment.*Id` columns to `target*Id` and converts the corresponding field metadata to `MORPH_RELATION` with a shared `morphId`. On the frontend, attachment read/write paths now switch to `target*Id` when the flag is enabled. It also fixes optimistic filtering for morph join columns. The metadata API deduplicates morph fields, so attachments now expose a single target field of type `MORPH_RELATION` plus a `morphRelations` array listing each target object. Because only one `settings.joinColumnName` is returned (e.g. `targetRocketId`), filters like `targetCompanyId` don’t map to any field and the optimistic cache code throws. `doesMorphRelationJoinColumnMatch` resolves this by computing all valid join column names from `morphRelations` using `computeMorphRelationFieldName` and comparing them to the filter key. That makes filters like `targetCompanyId` resolvable even with a single target field, so attachment uploads and list matching no longer crash. <img width="477" height="474" alt="image" src="https://github.com/user-attachments/assets/50e19418-3438-4d1e-9f1f-1bc1a03174a9" /> <br /> <br /> Today the metadata API returns one morph field called `target` and a list of possible targets (`morphRelations`), but it does not tell us the join column for each target. That’s why the Frontend had to compute join column names. If we want to fix this at the API level, there are two options: - Add join column names to each target in `morphRelations` (e.g. company → `targetCompanyId`). This is additive and low‑risk. - Return each target as its own field (`targetCompany`, `targetPerson`, etc.) instead of a single target. This is a larger change because it changes the shape of metadata and would require more UI updates. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Introduces morph relations for attachments behind `IS_ATTACHMENT_MIGRATED`, aligning server schema/metadata and frontend behavior. > > - Adds `IS_ATTACHMENT_MIGRATED` flag (frontend/server) and seeds/defaults; updates generated GraphQL enums > - New workspace upgrade `1.17` command migrates data: renames `attachment.*Id` → `target*Id` and converts related fields to `MORPH_RELATION` with shared `morphId` > - Updates standard field metadata and indexes to `target*` (attachment + related objects), dev seeds, snapshots, and workspace entity types > - Frontend: switches attachment read/write filters via `getActivityTargetObjectFieldIdName` using the feature flag; updates hooks/components (`useAttachments`, `useUploadAttachmentFile`, editors); expands `Attachment` type > - Fixes optimistic cache filtering to recognize morph join columns in `isRecordMatchingFilter` by computing valid join-column keys from `morphRelations` > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit f208fa2. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Félix Malfait <felix@twenty.com>
3 participants
Closes 1744.
This PR migrates attachments to morph relations behind a feature flag, following the TimelineActivity pattern. it introduces the
IS_ATTACHMENT_MIGRATEDflag, updates standard field metadata and indexes to use morph relations, adds a workspace migration that renamesattachment.*Idcolumns totarget*Idand converts the corresponding field metadata toMORPH_RELATIONwith a sharedmorphId. On the frontend, attachment read/write paths now switch totarget*Idwhen the flag is enabled.It also fixes optimistic filtering for morph join columns. The metadata API deduplicates morph fields, so attachments now expose a single target field of type
MORPH_RELATIONplus amorphRelationsarray listing each target object. Because only onesettings.joinColumnNameis returned (e.g.targetRocketId), filters liketargetCompanyIddon’t map to any field and the optimistic cache code throws.doesMorphRelationJoinColumnMatchresolves this by computing all valid join column names frommorphRelationsusingcomputeMorphRelationFieldNameand comparing them to the filter key. That makes filters liketargetCompanyIdresolvable even with a single target field, so attachment uploads and list matching no longer crash.Today the metadata API returns one morph field called
targetand a list of possible targets (morphRelations), but it does not tell us the join column for each target. That’s why the Frontend had to compute join column names.If we want to fix this at the API level, there are two options:
morphRelations(e.g. company →targetCompanyId). This is additive and low‑risk.targetCompany,targetPerson, etc.) instead of a single target. This is a larger change because it changes the shape of metadata and would require more UI updates.Note
Introduces morph relations for attachments behind
IS_ATTACHMENT_MIGRATED, aligning server schema/metadata and frontend behavior.IS_ATTACHMENT_MIGRATEDflag (frontend/server) and seeds/defaults; updates generated GraphQL enums1.17command migrates data: renamesattachment.*Id→target*Idand converts related fields toMORPH_RELATIONwith sharedmorphIdtarget*(attachment + related objects), dev seeds, snapshots, and workspace entity typesgetActivityTargetObjectFieldIdNameusing the feature flag; updates hooks/components (useAttachments,useUploadAttachmentFile, editors); expandsAttachmenttypeisRecordMatchingFilterby computing valid join-column keys frommorphRelationsWritten by Cursor Bugbot for commit f208fa2. This will update automatically on new commits. Configure here.