Skip to content

UpdateTaskOnDeleteActionCommand - Add logs#17479

Merged
etiennejouan merged 2 commits intomainfrom
ej/add-log-on-command
Jan 27, 2026
Merged

UpdateTaskOnDeleteActionCommand - Add logs#17479
etiennejouan merged 2 commits intomainfrom
ej/add-log-on-command

Conversation

@etiennejouan
Copy link
Copy Markdown
Contributor

No description provided.

isSystemBuild: true,
});
} catch (error) {
this.logger.debug(`Error details: ${JSON.stringify(error)}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rethrow

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Jan 27, 2026

Greptile Overview

Greptile Summary

Added try-catch error handling around the fieldMetadataService.updateOneField call in updateTaskRelationOnDeleteAction method.

Critical Issues:

  • The error is caught and logged at debug level, but execution continues to log "Successfully updated..." even when the update fails at line 1-16-update-task-on-delete-action.command.ts:142-143
  • Using logger.debug for errors in a migration command means failures will be invisible in production logs
  • This creates misleading logs that indicate success when the operation actually failed, making it impossible to diagnose migration failures

Recommendation:
Either rethrow the error to propagate the failure, move the success log inside the try block (after the update call), or at minimum: log errors with logger.error() and return early to prevent the misleading success message.

Confidence Score: 1/5

  • This PR introduces a critical bug that silently swallows migration errors and logs false success messages
  • The error handling logic is fundamentally broken - it catches errors but continues to log success, making it impossible to detect when migrations fail. This will hide real failures in production and create data inconsistency issues.
  • The single changed file 1-16-update-task-on-delete-action.command.ts requires immediate attention to fix the error handling logic

Important Files Changed

Filename Overview
packages/twenty-server/src/database/commands/upgrade-version-command/1-16/1-16-update-task-on-delete-action.command.ts Added try-catch around field metadata update but silently swallows errors and logs success even on failure

Sequence Diagram

sequenceDiagram
    participant Command as UpdateTaskOnDeleteActionCommand
    participant Cache as WorkspaceCacheService
    participant FieldService as FieldMetadataService
    participant Logger as Logger

    Command->>Command: runOnWorkspace(workspaceId, options)
    Command->>Logger: log("Running UpdateTaskOnDeleteActionCommand...")
    Command->>Command: updateTaskRelationOnDeleteAction(workspaceId, isDryRun)
    
    Command->>Cache: getOrRecompute(workspaceId, ['flatFieldMetadataMaps', 'flatObjectMetadataMaps'])
    Cache-->>Command: return metadata maps
    
    Command->>Command: find taskTarget object metadata
    
    alt taskTarget not found
        Command->>Logger: warn("TaskTarget object metadata not found")
        Command-->>Command: return early
    end
    
    Command->>Command: find task field in taskTarget
    
    alt task field not found
        Command->>Logger: warn("Task field not found")
        Command-->>Command: return early
    end
    
    alt onDelete already CASCADE
        Command->>Logger: log("Task relation already has CASCADE onDelete")
        Command-->>Command: return early
    end
    
    Command->>Logger: log("Updating task relation onDelete...")
    
    alt not dry run
        Command->>FieldService: updateOneField({id, settings, workspaceId})
        
        alt update succeeds
            FieldService-->>Command: success
        else update fails
            FieldService-->>Command: throw error
            Command->>Logger: debug("Error details: ...")
            Note over Command: Error is caught but execution continues
        end
        
        Command->>Logger: log("Successfully updated...") 
        Note over Command,Logger: This logs success even if update failed!
    else dry run
        Command->>Logger: log("DRY RUN: Would update...")
    end
    
    Command->>Command: deleteOrphanedTaskTargets(workspaceId, isDryRun)
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +129 to 144
try {
await this.fieldMetadataService.updateOneField({
updateFieldInput: {
id: taskField.id,
settings: updatedSettings,
},
workspaceId,
isSystemBuild: true,
});
} catch (error) {
this.logger.debug(`Error details: ${JSON.stringify(error)}`);

throw error;
}

this.logger.log(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error is caught and logged but execution continues to log "Successfully updated..." even when the update fails. This creates misleading logs and hides failures. Either rethrow the error, or move the success log inside the try block, or at minimum log an error (not debug) and return early.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-server/src/database/commands/upgrade-version-command/1-16/1-16-update-task-on-delete-action.command.ts
Line: 129:144

Comment:
Error is caught and logged but execution continues to log "Successfully updated..." even when the update fails. This creates misleading logs and hides failures. Either rethrow the error, or move the success log inside the try block, or at minimum log an error (not debug) and return early.

How can I resolve this? If you propose a fix, please make it concise.

isSystemBuild: true,
});
} catch (error) {
this.logger.debug(`Error details: ${JSON.stringify(error)}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logger.debug is too low a level for errors in a migration command. Use logger.error or logger.warn to ensure errors are visible in production logs.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-server/src/database/commands/upgrade-version-command/1-16/1-16-update-task-on-delete-action.command.ts
Line: 139:139

Comment:
`logger.debug` is too low a level for errors in a migration command. Use `logger.error` or `logger.warn` to ensure errors are visible in production logs.

How can I resolve this? If you propose a fix, please make it concise.

@etiennejouan etiennejouan added this pull request to the merge queue Jan 27, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 27, 2026

🚀 Preview Environment Ready!

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

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

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Merged via the queue into main with commit f532a51 Jan 27, 2026
59 checks passed
@etiennejouan etiennejouan deleted the ej/add-log-on-command branch January 27, 2026 14:15
@twenty-eng-sync
Copy link
Copy Markdown

Hey @etiennejouan! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants