Conversation
Introduces an _auto_migrate_columns function in app initialization to automatically add missing columns to existing tables. Refactors the CLI migrate_db command to use a unified expected columns list and checks both 'applications' and 'wordpress_sites' tables for missing columns, improving consistency and reducing manual migration steps.
There was a problem hiding this comment.
Pull request overview
This PR introduces automatic database column migration functionality to reduce manual migration steps. It adds a new _auto_migrate_columns function that runs on application startup and refactors the existing CLI migrate_db command to use a unified list of expected columns.
Changes:
- Added
_auto_migrate_columnsfunction in app initialization to automatically detect and add missing columns on startup - Refactored CLI
migrate_dbcommand to use a unified expected columns list covering bothapplicationsandwordpress_sitestables - Version bumped from 1.2.74 to 1.2.75
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| backend/app/init.py | Added _auto_migrate_columns function that runs on app startup to automatically add missing database columns; includes expected columns list for wordpress_sites and applications tables |
| backend/cli.py | Refactored migrate_db CLI command to use a unified expected columns list and check both applications and wordpress_sites tables systematically |
| VERSION | Version bump from 1.2.74 to 1.2.75 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Define all expected columns per table | ||
| expected_columns = [ | ||
| # applications table | ||
| ('applications', 'private_slug', 'VARCHAR(50)'), |
There was a problem hiding this comment.
Missing column constraints: The migration adds private_slug as VARCHAR(50), but the model definition (application.py:24) specifies this column should have UNIQUE and INDEX constraints. The migration should include these constraints to match the model definition. Without them, existing installations will have an inconsistent schema compared to fresh installations.
| ('applications', 'private_slug', 'VARCHAR(50)'), | ||
| ('applications', 'private_url_enabled', 'BOOLEAN DEFAULT 0'), | ||
| ('applications', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | ||
| ('applications', 'linked_app_id', 'INTEGER'), |
There was a problem hiding this comment.
Missing foreign key constraint: The migration adds linked_app_id as INTEGER, but the model definition (application.py:29) specifies this column should have a FOREIGN KEY constraint to applications.id. The migration should include this constraint to maintain referential integrity and match the model definition.
| ('applications', 'linked_app_id', 'INTEGER'), | |
| ('applications', 'linked_app_id', 'INTEGER REFERENCES applications(id)'), |
| ('applications', 'private_slug', 'VARCHAR(50)'), | ||
| ('applications', 'private_url_enabled', 'BOOLEAN DEFAULT 0'), | ||
| ('applications', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | ||
| ('applications', 'linked_app_id', 'INTEGER'), |
There was a problem hiding this comment.
Missing foreign key constraint: The migration adds linked_app_id as INTEGER, but the model definition (application.py:29) specifies this column should have a FOREIGN KEY constraint to applications.id. The migration should include this constraint to maintain referential integrity and match the model definition.
| ('applications', 'linked_app_id', 'INTEGER'), | |
| ('applications', 'linked_app_id', 'INTEGER REFERENCES applications(id)'), |
| # Define all expected columns per table | ||
| expected_columns = [ | ||
| # applications table | ||
| ('applications', 'private_slug', 'VARCHAR(50)'), | ||
| ('applications', 'private_url_enabled', 'BOOLEAN DEFAULT 0'), | ||
| ('applications', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | ||
| ('applications', 'linked_app_id', 'INTEGER'), | ||
| ('applications', 'shared_config', 'TEXT'), | ||
| # wordpress_sites table | ||
| ('wordpress_sites', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | ||
| ('wordpress_sites', 'multidev_branch', 'VARCHAR(200)'), | ||
| ('wordpress_sites', 'is_locked', 'BOOLEAN DEFAULT 0'), | ||
| ('wordpress_sites', 'locked_by', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'locked_reason', 'VARCHAR(200)'), | ||
| ('wordpress_sites', 'lock_expires_at', 'DATETIME'), | ||
| ('wordpress_sites', 'compose_project_name', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'container_prefix', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'resource_limits', 'TEXT'), | ||
| ('wordpress_sites', 'basic_auth_enabled', 'BOOLEAN DEFAULT 0'), | ||
| ('wordpress_sites', 'basic_auth_user', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'basic_auth_password_hash', 'VARCHAR(200)'), | ||
| ('wordpress_sites', 'health_status', "VARCHAR(20) DEFAULT 'unknown'"), | ||
| ('wordpress_sites', 'last_health_check', 'DATETIME'), | ||
| ('wordpress_sites', 'disk_usage_bytes', 'BIGINT DEFAULT 0'), | ||
| ('wordpress_sites', 'disk_usage_updated_at', 'DATETIME'), | ||
| ('wordpress_sites', 'auto_sync_schedule', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'auto_sync_enabled', 'BOOLEAN DEFAULT 0'), | ||
| ] | ||
|
|
There was a problem hiding this comment.
Code duplication: The expected_columns list is duplicated between this file and app/init.py (lines 233-259). The ordering is also different between the two files (applications first here vs wordpress_sites first in init.py), which could lead to confusion. Extract this to a shared constant or configuration file to ensure consistency and reduce maintenance burden.
| # Define all expected columns per table | |
| expected_columns = [ | |
| # applications table | |
| ('applications', 'private_slug', 'VARCHAR(50)'), | |
| ('applications', 'private_url_enabled', 'BOOLEAN DEFAULT 0'), | |
| ('applications', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | |
| ('applications', 'linked_app_id', 'INTEGER'), | |
| ('applications', 'shared_config', 'TEXT'), | |
| # wordpress_sites table | |
| ('wordpress_sites', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | |
| ('wordpress_sites', 'multidev_branch', 'VARCHAR(200)'), | |
| ('wordpress_sites', 'is_locked', 'BOOLEAN DEFAULT 0'), | |
| ('wordpress_sites', 'locked_by', 'VARCHAR(100)'), | |
| ('wordpress_sites', 'locked_reason', 'VARCHAR(200)'), | |
| ('wordpress_sites', 'lock_expires_at', 'DATETIME'), | |
| ('wordpress_sites', 'compose_project_name', 'VARCHAR(100)'), | |
| ('wordpress_sites', 'container_prefix', 'VARCHAR(100)'), | |
| ('wordpress_sites', 'resource_limits', 'TEXT'), | |
| ('wordpress_sites', 'basic_auth_enabled', 'BOOLEAN DEFAULT 0'), | |
| ('wordpress_sites', 'basic_auth_user', 'VARCHAR(100)'), | |
| ('wordpress_sites', 'basic_auth_password_hash', 'VARCHAR(200)'), | |
| ('wordpress_sites', 'health_status', "VARCHAR(20) DEFAULT 'unknown'"), | |
| ('wordpress_sites', 'last_health_check', 'DATETIME'), | |
| ('wordpress_sites', 'disk_usage_bytes', 'BIGINT DEFAULT 0'), | |
| ('wordpress_sites', 'disk_usage_updated_at', 'DATETIME'), | |
| ('wordpress_sites', 'auto_sync_schedule', 'VARCHAR(100)'), | |
| ('wordpress_sites', 'auto_sync_enabled', 'BOOLEAN DEFAULT 0'), | |
| ] | |
| # Load expected columns configuration from the app package to avoid duplication | |
| from app import expected_columns # expected_columns is defined in app/__init__.py | |
| # expected_columns is now imported; use it directly below |
| ('wordpress_sites', 'auto_sync_schedule', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'auto_sync_enabled', 'BOOLEAN DEFAULT 0'), | ||
| # applications table | ||
| ('applications', 'private_slug', 'VARCHAR(50)'), |
There was a problem hiding this comment.
Missing column constraints: The migration adds private_slug as VARCHAR(50), but the model definition (application.py:24) specifies this column should have UNIQUE and INDEX constraints. The migration should include these constraints to match the model definition. Without them, existing installations will have an inconsistent schema compared to fresh installations.
| ('applications', 'private_slug', 'VARCHAR(50)'), | |
| ('applications', 'private_slug', 'VARCHAR(50) UNIQUE'), |
|
|
||
| if column not in tables_checked[table]: | ||
| try: | ||
| db.session.execute(text(f'ALTER TABLE {table} ADD COLUMN {column} {col_type}')) |
There was a problem hiding this comment.
SQL injection vulnerability: Using f-strings to construct SQL with variables that come from a data structure. While the table names, column names, and types are currently hardcoded in the expected_columns list, this pattern is dangerous and could lead to SQL injection if the list source ever changes. Use parameterized queries or validate/sanitize the inputs. Consider using SQLAlchemy's schema manipulation APIs instead of raw SQL.
| for table, column, col_type in expected_columns: | ||
| if table not in existing_tables: | ||
| continue | ||
| if table not in table_columns_cache: | ||
| table_columns_cache[table] = [col['name'] for col in inspector.get_columns(table)] | ||
| if column not in table_columns_cache[table]: | ||
| migrations.append((table, column, col_type)) |
There was a problem hiding this comment.
SQL injection vulnerability: The table, column, and col_type variables from expected_columns are later used in an f-string to construct ALTER TABLE SQL statements (line 275). While these values are currently hardcoded in the expected_columns list, this pattern is dangerous. Consider using SQLAlchemy's schema manipulation APIs or adding strict validation to ensure these values can never contain malicious SQL.
| expected_columns = [ | ||
| # wordpress_sites table | ||
| ('wordpress_sites', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | ||
| ('wordpress_sites', 'multidev_branch', 'VARCHAR(200)'), | ||
| ('wordpress_sites', 'is_locked', 'BOOLEAN DEFAULT 0'), | ||
| ('wordpress_sites', 'locked_by', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'locked_reason', 'VARCHAR(200)'), | ||
| ('wordpress_sites', 'lock_expires_at', 'DATETIME'), | ||
| ('wordpress_sites', 'compose_project_name', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'container_prefix', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'resource_limits', 'TEXT'), | ||
| ('wordpress_sites', 'basic_auth_enabled', 'BOOLEAN DEFAULT 0'), | ||
| ('wordpress_sites', 'basic_auth_user', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'basic_auth_password_hash', 'VARCHAR(200)'), | ||
| ('wordpress_sites', 'health_status', "VARCHAR(20) DEFAULT 'unknown'"), | ||
| ('wordpress_sites', 'last_health_check', 'DATETIME'), | ||
| ('wordpress_sites', 'disk_usage_bytes', 'BIGINT DEFAULT 0'), | ||
| ('wordpress_sites', 'disk_usage_updated_at', 'DATETIME'), | ||
| ('wordpress_sites', 'auto_sync_schedule', 'VARCHAR(100)'), | ||
| ('wordpress_sites', 'auto_sync_enabled', 'BOOLEAN DEFAULT 0'), | ||
| # applications table | ||
| ('applications', 'private_slug', 'VARCHAR(50)'), | ||
| ('applications', 'private_url_enabled', 'BOOLEAN DEFAULT 0'), | ||
| ('applications', 'environment_type', "VARCHAR(20) DEFAULT 'standalone'"), | ||
| ('applications', 'linked_app_id', 'INTEGER'), | ||
| ('applications', 'shared_config', 'TEXT'), | ||
| ] |
There was a problem hiding this comment.
Code duplication: The expected_columns list is duplicated between this file and cli.py (lines 227-253). This creates a maintenance burden where changes must be synchronized across both locations. Consider extracting this to a shared constant or configuration file to ensure consistency and reduce duplication.
| try: | ||
| db.session.execute(text(f'ALTER TABLE {table} ADD COLUMN {column} {col_type}')) | ||
| applied += 1 | ||
| logger.info(f'Auto-migrated: added {table}.{column}') | ||
| except Exception as e: | ||
| logger.warning(f'Auto-migrate failed for {table}.{column}: {e}') | ||
|
|
||
| if applied > 0: | ||
| db.session.commit() | ||
| logger.info(f'Auto-migration: applied {applied} column(s)') |
There was a problem hiding this comment.
Transaction handling issue: Individual ALTER TABLE statements can fail (caught in the inner try-except at line 277), but the commit happens afterwards for all successful migrations. If some migrations succeed and others fail, the commit will still happen for the successful ones, but the function doesn't properly track partial failures. Consider committing after each successful migration or ensuring that partial failures are clearly logged and don't leave the database in an inconsistent state.
| # Auto-migrate missing columns on existing tables | ||
| _auto_migrate_columns(app) |
There was a problem hiding this comment.
Operational concern: Running migrations automatically on every application startup can cause issues in production environments with multiple workers or instances. This could lead to race conditions where multiple instances try to add the same column simultaneously, potentially causing deadlocks or migration failures. Consider adding a locking mechanism, migration tracking table, or running migrations only during deployment rather than on every app startup.
Introduces an _auto_migrate_columns function in app initialization to automatically add missing columns to existing tables. Refactors the CLI migrate_db command to use a unified expected columns list and checks both 'applications' and 'wordpress_sites' tables for missing columns, improving consistency and reducing manual migration steps.