Skip to content

Null - Second command - Cleaning remaining empty values#16241

Merged
charlesBochet merged 3 commits intomainfrom
ej/null-command-2
Dec 2, 2025
Merged

Null - Second command - Cleaning remaining empty values#16241
charlesBochet merged 3 commits intomainfrom
ej/null-command-2

Conversation

@etiennejouan
Copy link
Copy Markdown
Contributor

Because of this code I forget to clean,

  • workspace created between 1.12.0 and 1.12.2 (from friday 28 nov PM > monday 1 dec PM) have standard objects with empty string default value set on related TEXT type column (but default value null on field metadata) (ex: jobTitle on person)
  • custom objects created between 1.12.0 and 1.12.2 (from friday 28 nov PM > monday 1 dec PM) have "name" TEXT field with empty string default value set + NOT NULL constraint on column

Command cleans data and updates table structure

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Dec 2, 2025

Greptile Overview

Greptile Summary

Adds migration command to clean up empty string defaults in TEXT fields introduced by PR #16217. The command handles two scenarios: standard objects with empty string defaults at DB level (with null in metadata), and custom objects' name field with empty string defaults plus NOT NULL constraint. The fix converts empty strings to NULL and removes incorrect defaults/constraints.

Key changes:

  • Created CleanEmptyStringNullInTextFieldsCommand that processes all workspace objects
  • Checks database column defaults via information_schema and cleans mismatched empty string defaults
  • Updates both database schema and field metadata for custom object name fields
  • Includes dry-run mode for safe testing

Issues found:

  • Missing transaction wrappers around multi-step DDL operations could leave database in inconsistent state if partial failure occurs
  • Method naming (cleanUpEmptyStringDefaultsAndSetNullableInNameFieldInCustomObjects) suggests it only applies to custom objects, but it's called for all objects

Confidence Score: 3/5

  • PR fixes a real data consistency issue but lacks transaction safety for critical database operations
  • The migration logic correctly identifies and fixes the empty string default value issue, and includes proper logging and dry-run support. However, multiple sequential DDL operations (ALTER TABLE + UPDATE) lack transaction wrappers, which could leave the database in an inconsistent state if any operation fails mid-execution. This is especially concerning for production migrations. The pattern used in similar migrations (e.g., 1-12-add-messages-import-scheduled-sync-stage.command.ts) demonstrates proper transaction handling that should be applied here.
  • packages/twenty-server/src/database/commands/upgrade-version-command/1-13/1-13-clean-empty-string-null-in-text-fields.command.ts - needs transaction safety

Important Files Changed

File Analysis

Filename Score Overview
packages/twenty-server/src/database/commands/upgrade-version-command/1-13/1-13-clean-empty-string-null-in-text-fields.command.ts 3/5 Migration command that cleans empty string defaults in TEXT fields and converts to NULL for both standard and custom objects; lacks transaction safety for DB operations
packages/twenty-server/src/database/commands/upgrade-version-command/1-13/1-13-upgrade-version-command.module.ts 5/5 Standard NestJS module configuration that properly imports dependencies and exports the command
packages/twenty-server/src/database/commands/upgrade-version-command/upgrade-version-command.module.ts 5/5 Added V1_13 module import to the main upgrade command module

Sequence Diagram

sequenceDiagram
    participant CMD as CleanEmptyStringCommand
    participant ORM as ObjectMetadataRepository
    participant DS as DataSource
    participant DB as PostgreSQL Database
    participant FM as FieldMetadataRepository

    CMD->>ORM: find({ workspaceId, relations: ['fields'] })
    ORM-->>CMD: objectMetadataItems[]
    
    loop For each object
        CMD->>CMD: computeObjectTargetTable(objectMetadata)
        
        alt !isCustom (Standard Object)
            CMD->>CMD: cleanUpEmptyStringDefaultsInTextFieldsInStandardObjects()
            
            loop For each TEXT field with isNullable=true, defaultValue=null
                CMD->>DB: SELECT column_default FROM information_schema
                DB-->>CMD: columnDefault
                
                alt columnDefault is empty string
                    CMD->>DS: ALTER TABLE DROP DEFAULT
                    DS->>DB: Execute DDL
                    CMD->>DS: UPDATE SET field = NULL WHERE field = ''
                    DS->>DB: Execute DML
                end
            end
        end
        
        CMD->>CMD: cleanUpEmptyStringDefaultsAndSetNullableInNameFieldInCustomObjects()
        
        alt Has name field with defaultValue=''
            CMD->>DS: ALTER TABLE DROP DEFAULT, DROP NOT NULL
            DS->>DB: Execute DDL
            CMD->>DS: UPDATE SET name = NULL WHERE name = ''
            DS->>DB: Execute DML
            CMD->>FM: update(nameField.id, {defaultValue: null, isNullable: true})
            FM->>DB: Update metadata
        end
    end
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.

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 2, 2025

🚀 Preview Environment Ready!

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

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

@charlesBochet charlesBochet merged commit 00e970b into main Dec 2, 2025
58 of 59 checks passed
@charlesBochet charlesBochet deleted the ej/null-command-2 branch December 2, 2025 12:39
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants