feat(db) Replace the hand-rolled migration runner with Alembic, and allow Postgres - #768
Open
ioanalytica wants to merge 16 commits into
Open
feat(db) Replace the hand-rolled migration runner with Alembic, and allow Postgres#768ioanalytica wants to merge 16 commits into
ioanalytica wants to merge 16 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change introduces Alembic schema management, a baseline migration, SQLite-to-Postgres upgrades, environment-based database URL resolution, boot-time schema enforcement, database-agnostic scripts, expanded tests, model index updates, and case-insensitive API searches. ChangesDatabase migration and storage transition
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Entrypoint
participant DatabaseConfig
participant UpgradeEngine
participant SQLite
participant PostgreSQL
Entrypoint->>DatabaseConfig: resolve database URL
Entrypoint->>UpgradeEngine: run upgrade_from_settings
UpgradeEngine->>SQLite: inspect or build baseline
UpgradeEngine->>PostgreSQL: transfer rows when Postgres is selected
UpgradeEngine->>SQLite: finalize rollback backup
UpgradeEngine-->>Entrypoint: return upgrade report
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/backup_plans.py (1)
195-205: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the uniqueness checks case-insensitive end to end.
ilikefinds case-insensitive matches, butcandidate not in existing_namesand the suffix loop remain case-sensitive. IfNightlyalready exists, requestingnightlystill returnsnightly, allowing names that differ only by case. Normalize both the candidate and existing names before the membership checks.Proposed fix
existing_names = { name for (name,) in db.query(BackupPlan.name) .filter(BackupPlan.name.ilike(f"{candidate}%")) .all() } - if candidate not in existing_names: + normalized_names = {name.casefold() for name in existing_names} + if candidate.casefold() not in normalized_names: return candidate suffix = 2 - while f"{candidate} ({suffix})" in existing_names: + while f"{candidate} ({suffix})".casefold() in normalized_names: suffix += 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/backup_plans.py` around lines 195 - 205, Update the uniqueness logic in the backup-plan name generation flow to normalize both candidate and database names consistently before membership checks. Ensure the initial candidate check and the suffix loop in the surrounding name-generation function reject names differing only by case while preserving the existing suffix behavior.
🧹 Nitpick comments (1)
tests/unit/test_main.py (1)
24-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: flatten to parenthesized context managers for consistency.
The other two tests in this class use a single parenthesized
with (...)group; this one keeps the deeply nested style. Aligning them improves readability. Functionally correct as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_main.py` around lines 24 - 71, Optionally refactor the nested patch context managers in the startup_event test around startup_event into one parenthesized with group, matching the style used by the other tests in the class. Preserve all patches, ordering, mocks, and assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/api/backup_plans.py`:
- Around line 195-205: Update the uniqueness logic in the backup-plan name
generation flow to normalize both candidate and database names consistently
before membership checks. Ensure the initial candidate check and the suffix loop
in the surrounding name-generation function reject names differing only by case
while preserving the existing suffix behavior.
---
Nitpick comments:
In `@tests/unit/test_main.py`:
- Around line 24-71: Optionally refactor the nested patch context managers in
the startup_event test around startup_event into one parenthesized with group,
matching the style used by the other tests in the class. Preserve all patches,
ordering, mocks, and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2de1f8cd-a1a3-4d88-afbf-cf7110d285f2
📒 Files selected for processing (160)
.github/workflows/tests.ymlDockerfilealembic.iniapp/api/backup_plans.pyapp/api/scripts_library.pyapp/config.pyapp/database/alembic/README.mdapp/database/alembic/env.pyapp/database/alembic/script.py.makoapp/database/alembic/versions/f6c46c665fd3_baseline_schema.pyapp/database/database.pyapp/database/db_upgrade.pyapp/database/migrations/004_add_progress_fields.pyapp/database/migrations/005_add_backup_speed.pyapp/database/migrations/006_add_scheduled_jobs.pyapp/database/migrations/007_add_size_progress_fields.pyapp/database/migrations/008_add_scheduled_job_id_to_backups.pyapp/database/migrations/009_add_restore_jobs.pyapp/database/migrations/010_add_backup_hooks.pyapp/database/migrations/011_add_must_change_password.pyapp/database/migrations/012_add_prune_compact_to_scheduled_jobs.pyapp/database/migrations/013_add_maintenance_status_to_backup_jobs.pyapp/database/migrations/014_add_check_compact_jobs.pyapp/database/migrations/015_add_last_check_compact_to_repositories.pyapp/database/migrations/016_add_max_duration_to_check_jobs.pyapp/database/migrations/017_add_pid_tracking.pyapp/database/migrations/018_remove_config_file.pyapp/database/migrations/019_add_archive_name_template.pyapp/database/migrations/020_add_repository_mode.pyapp/database/migrations/021_add_custom_flags.pyapp/database/migrations/022_add_ssh_key_default_path.pyapp/database/migrations/023_add_notify_on_backup_start.pyapp/database/migrations/024_add_hourly_quarterly_prune.pyapp/database/migrations/025_add_check_notifications_and_scheduling.pyapp/database/migrations/027_add_script_library.pyapp/database/migrations/028_split_hook_timeouts.pyapp/database/migrations/029_add_continue_on_error.pyapp/database/migrations/030_add_repository_path_to_check_compact_jobs.pyapp/database/migrations/031_add_prune_jobs_table.pyapp/database/migrations/032_add_storage_and_mount_to_ssh_connections.pyapp/database/migrations/033_add_log_management_settings.pyapp/database/migrations/034_add_scheduled_prune_field.pyapp/database/migrations/034_convert_check_interval_to_cron.pyapp/database/migrations/035_add_prune_job_log_fields.pyapp/database/migrations/036_add_compact_job_log_fields.pyapp/database/migrations/037_add_multi_repo_schedules.pyapp/database/migrations/038_add_keyfile_support.pyapp/database/migrations/039_add_repository_notification_filters.pyapp/database/migrations/040_add_cache_settings.pyapp/database/migrations/041_add_redis_url.pyapp/database/migrations/043_add_check_job_log_fields.pyapp/database/migrations/044_add_remote_backup_support.pyapp/database/migrations/045_add_repository_connection_id.pyapp/database/migrations/046_add_use_new_wizard_setting.pyapp/database/migrations/047_add_browse_memory_limits.pyapp/database/migrations/048_fix_ssh_connection_cascade.pyapp/database/migrations/049_add_delete_archive_jobs.pyapp/database/migrations/050_add_source_connection_to_repositories.pyapp/database/migrations/051_add_analytics_enabled.pyapp/database/migrations/052_add_analytics_consent_given.pyapp/database/migrations/053_add_operation_timeouts.pyapp/database/migrations/054_add_stats_refresh_interval.pyapp/database/migrations/055_add_bypass_lock.pyapp/database/migrations/056_add_last_stats_refresh.pyapp/database/migrations/057_add_script_parameters.pyapp/database/migrations/057_populate_connection_id_for_legacy_repos.pyapp/database/migrations/058_add_notification_enhancements.pyapp/database/migrations/059_add_use_sftp_mode_to_ssh_connections.pyapp/database/migrations/060_add_speed_eta_to_restore_jobs.pyapp/database/migrations/061_add_bypass_lock_on_info.pyapp/database/migrations/062_add_unique_constraint_schedule_repo.pyapp/database/migrations/063_add_schedule_script_parameters.pyapp/database/migrations/064_add_show_restore_tab.pyapp/database/migrations/065_cleanup_schedule_duplicates.pyapp/database/migrations/066_add_ssh_path_prefix.pyapp/database/migrations/067_add_cascade_delete_to_script_executions.pyapp/database/migrations/068_add_remote_restore_fields.pyapp/database/migrations/069_add_bypass_lock_on_list.pyapp/database/migrations/070_add_source_size_timeout.pyapp/database/migrations/071_add_mqtt_beta_enabled.pyapp/database/migrations/072_add_mqtt_base_topic.pyapp/database/migrations/073_fix_delete_archive_jobs_cascade.pyapp/database/migrations/074_add_use_sudo_to_ssh_connections.pyapp/database/migrations/074_remove_mqtt_base_topic.pyapp/database/migrations/075_add_skip_on_failure.pyapp/database/migrations/075_fix_schedule_script_id_set_null.pyapp/database/migrations/076_fix_package_install_jobs_cascade.pyapp/database/migrations/077_fix_script_executions_fk_actions.pyapp/database/migrations/078_fix_scripts_created_by_user_fk.pyapp/database/migrations/079_add_borg_version.pyapp/database/migrations/080_add_plan.pyapp/database/migrations/081_add_role_and_api_tokens.pyapp/database/migrations/082_add_user_profile_fields.pyapp/database/migrations/083_add_deployment_profile.pyapp/database/migrations/084_add_user_repository_permissions.pyapp/database/migrations/085_add_all_repositories_role.pyapp/database/migrations/086_add_licensing_state.pyapp/database/migrations/087_add_repository_inline_script_parameters.pyapp/database/migrations/088_add_metrics_settings.pyapp/database/migrations/089_add_backup_job_archive_name.pyapp/database/migrations/090_add_borg2_fast_browse_beta_enabled.pyapp/database/migrations/091_add_totp_to_users.pyapp/database/migrations/092_add_passkey_credentials.pyapp/database/migrations/093_add_backup_warning_notifications.pyapp/database/migrations/094_add_scheduler_concurrency_limits.pyapp/database/migrations/095_add_schedule_timezones.pyapp/database/migrations/096_add_oidc_settings.pyapp/database/migrations/097_add_oidc_state_and_auth_events.pyapp/database/migrations/098_add_auth_rate_limit_buckets.pyapp/database/migrations/099_harden_oidc_exchange_and_admin_mapping.pyapp/database/migrations/100_add_oidc_linking_and_token_auth.pyapp/database/migrations/101_add_restore_check_jobs.pyapp/database/migrations/102_add_restore_check_canary_enabled.pyapp/database/migrations/103_add_restore_check_notifications.pyapp/database/migrations/104_add_agent_machines.pyapp/database/migrations/104_add_backup_plans.pyapp/database/migrations/105_add_agent_jobs.pyapp/database/migrations/105_add_dashboard_health_threshold_settings.pyapp/database/migrations/106_add_agent_repository_links.pyapp/database/migrations/106_add_schedule_enabled_flags.pyapp/database/migrations/107_add_check_extra_flags.pyapp/database/migrations/107_add_source_locations.pyapp/database/migrations/108_add_managed_agents_beta_enabled.pyapp/database/migrations/109_add_repository_wipe_jobs.pyapp/database/migrations/110_add_repository_executor_type.pyapp/database/migrations/111_add_backup_job_route_strategy.pyapp/database/migrations/111_add_backup_monitoring_reports.pyapp/database/migrations/112_add_backup_report_delivery_schedule.pyapp/database/migrations/112_agent_delete_and_nullable_enrollment_expiry.pyapp/database/migrations/113_add_rclone_storage.pyapp/database/migrations/114_add_scheduled_rclone_mirror_jobs.pyapp/database/migrations/115_add_rclone_oauth_provider_credentials.pyapp/database/migrations/116_add_agent_default_path.pyapp/database/migrations/117_add_backup_plan_database_template_id.pyapp/database/migrations/117_add_rclone_sync_job_operation.pyapp/database/migrations/118_add_backup_plan_scripts.pyapp/database/migrations/119_add_backup_retry_lineage.pyapp/database/migrations/120_add_lock_breaking_enabled.pyapp/database/migrations/121_add_rclone_oauth_provider_credentials_table.pyapp/database/migrations/122_add_rclone_remote_storage.pyapp/database/migrations/122_add_remote_backend_clients.pyapp/database/migrations/123_add_repository_upload_ratelimit.pyapp/database/migrations/124_add_backup_plan_upload_ratelimit_policies.pyapp/database/migrations/125_add_prune_keep_within.pyapp/database/migrations/126_add_agent_script_to_plan_hooks.pyapp/database/migrations/126_clear_persisted_archive_browse_listings.pyapp/database/migrations/127_add_agent_job_to_script_executions.pyapp/database/migrations/__init__.pyapp/database/models.pyapp/main.pyapp/scripts/deploy_ssh_key.pyentrypoint.shrequirements.txttests/fixtures/api.pytests/unit/test_api_rclone.pytests/unit/test_config.pytests/unit/test_database_operations.pytests/unit/test_db_upgrade.pytests/unit/test_dockerfile_database_url.pytests/unit/test_main.py
💤 Files with no reviewable changes (139)
- app/database/migrations/038_add_keyfile_support.py
- app/database/migrations/046_add_use_new_wizard_setting.py
- app/database/migrations/init.py
- app/database/migrations/055_add_bypass_lock.py
- app/database/migrations/064_add_show_restore_tab.py
- app/database/migrations/117_add_rclone_sync_job_operation.py
- app/database/migrations/085_add_all_repositories_role.py
- app/database/migrations/049_add_delete_archive_jobs.py
- app/database/migrations/072_add_mqtt_base_topic.py
- app/database/migrations/107_add_source_locations.py
- app/database/migrations/045_add_repository_connection_id.py
- app/database/migrations/125_add_prune_keep_within.py
- app/database/migrations/004_add_progress_fields.py
- app/database/migrations/111_add_backup_job_route_strategy.py
- app/database/migrations/127_add_agent_job_to_script_executions.py
- app/database/migrations/014_add_check_compact_jobs.py
- app/database/migrations/110_add_repository_executor_type.py
- app/database/migrations/071_add_mqtt_beta_enabled.py
- app/database/migrations/017_add_pid_tracking.py
- app/database/migrations/122_add_remote_backend_clients.py
- app/database/migrations/107_add_check_extra_flags.py
- app/database/migrations/086_add_licensing_state.py
- app/database/migrations/043_add_check_job_log_fields.py
- app/database/migrations/102_add_restore_check_canary_enabled.py
- app/database/migrations/108_add_managed_agents_beta_enabled.py
- app/database/migrations/057_populate_connection_id_for_legacy_repos.py
- app/database/migrations/090_add_borg2_fast_browse_beta_enabled.py
- app/database/migrations/020_add_repository_mode.py
- app/database/migrations/006_add_scheduled_jobs.py
- app/database/migrations/070_add_source_size_timeout.py
- app/database/migrations/106_add_agent_repository_links.py
- app/database/migrations/109_add_repository_wipe_jobs.py
- app/database/migrations/022_add_ssh_key_default_path.py
- app/database/migrations/032_add_storage_and_mount_to_ssh_connections.py
- app/database/migrations/063_add_schedule_script_parameters.py
- app/database/migrations/082_add_user_profile_fields.py
- app/database/migrations/036_add_compact_job_log_fields.py
- app/database/migrations/087_add_repository_inline_script_parameters.py
- app/database/migrations/005_add_backup_speed.py
- app/database/migrations/066_add_ssh_path_prefix.py
- app/database/migrations/051_add_analytics_enabled.py
- app/database/migrations/061_add_bypass_lock_on_info.py
- app/database/migrations/103_add_restore_check_notifications.py
- app/database/migrations/124_add_backup_plan_upload_ratelimit_policies.py
- app/database/migrations/047_add_browse_memory_limits.py
- app/database/migrations/118_add_backup_plan_scripts.py
- app/database/migrations/029_add_continue_on_error.py
- app/database/migrations/089_add_backup_job_archive_name.py
- app/database/migrations/117_add_backup_plan_database_template_id.py
- app/database/migrations/088_add_metrics_settings.py
- app/database/migrations/104_add_agent_machines.py
- app/database/migrations/104_add_backup_plans.py
- app/database/migrations/091_add_totp_to_users.py
- app/database/migrations/122_add_rclone_remote_storage.py
- app/database/migrations/105_add_agent_jobs.py
- app/database/migrations/096_add_oidc_settings.py
- app/database/migrations/048_fix_ssh_connection_cascade.py
- app/database/migrations/054_add_stats_refresh_interval.py
- app/database/migrations/053_add_operation_timeouts.py
- app/database/migrations/039_add_repository_notification_filters.py
- app/database/migrations/021_add_custom_flags.py
- app/database/migrations/093_add_backup_warning_notifications.py
- app/database/migrations/030_add_repository_path_to_check_compact_jobs.py
- app/database/migrations/013_add_maintenance_status_to_backup_jobs.py
- app/database/migrations/097_add_oidc_state_and_auth_events.py
- app/database/migrations/023_add_notify_on_backup_start.py
- app/database/migrations/018_remove_config_file.py
- app/database/migrations/111_add_backup_monitoring_reports.py
- app/database/migrations/028_split_hook_timeouts.py
- app/database/migrations/079_add_borg_version.py
- app/database/migrations/123_add_repository_upload_ratelimit.py
- app/database/migrations/080_add_plan.py
- app/database/migrations/115_add_rclone_oauth_provider_credentials.py
- app/database/migrations/092_add_passkey_credentials.py
- app/database/migrations/009_add_restore_jobs.py
- app/database/migrations/084_add_user_repository_permissions.py
- app/database/migrations/114_add_scheduled_rclone_mirror_jobs.py
- app/database/migrations/083_add_deployment_profile.py
- app/database/migrations/116_add_agent_default_path.py
- app/database/migrations/057_add_script_parameters.py
- app/database/migrations/078_fix_scripts_created_by_user_fk.py
- app/database/migrations/031_add_prune_jobs_table.py
- app/database/migrations/105_add_dashboard_health_threshold_settings.py
- app/database/migrations/074_add_use_sudo_to_ssh_connections.py
- app/database/migrations/012_add_prune_compact_to_scheduled_jobs.py
- app/database/migrations/019_add_archive_name_template.py
- app/database/migrations/056_add_last_stats_refresh.py
- app/database/migrations/112_add_backup_report_delivery_schedule.py
- app/database/migrations/041_add_redis_url.py
- app/database/migrations/094_add_scheduler_concurrency_limits.py
- app/database/migrations/121_add_rclone_oauth_provider_credentials_table.py
- app/database/migrations/011_add_must_change_password.py
- app/database/migrations/040_add_cache_settings.py
- app/database/migrations/025_add_check_notifications_and_scheduling.py
- app/database/migrations/008_add_scheduled_job_id_to_backups.py
- app/database/migrations/034_add_scheduled_prune_field.py
- app/database/migrations/027_add_script_library.py
- app/database/migrations/075_fix_schedule_script_id_set_null.py
- app/database/migrations/033_add_log_management_settings.py
- app/database/migrations/034_convert_check_interval_to_cron.py
- app/database/migrations/062_add_unique_constraint_schedule_repo.py
- app/database/migrations/126_add_agent_script_to_plan_hooks.py
- app/database/migrations/126_clear_persisted_archive_browse_listings.py
- app/database/migrations/099_harden_oidc_exchange_and_admin_mapping.py
- app/database/migrations/076_fix_package_install_jobs_cascade.py
- app/database/migrations/010_add_backup_hooks.py
- app/database/migrations/077_fix_script_executions_fk_actions.py
- app/database/migrations/058_add_notification_enhancements.py
- app/database/migrations/073_fix_delete_archive_jobs_cascade.py
- app/database/migrations/120_add_lock_breaking_enabled.py
- app/database/migrations/095_add_schedule_timezones.py
- app/database/migrations/067_add_cascade_delete_to_script_executions.py
- app/database/migrations/015_add_last_check_compact_to_repositories.py
- app/database/migrations/037_add_multi_repo_schedules.py
- app/database/migrations/101_add_restore_check_jobs.py
- app/database/migrations/052_add_analytics_consent_given.py
- app/database/migrations/113_add_rclone_storage.py
- app/database/migrations/081_add_role_and_api_tokens.py
- app/database/migrations/050_add_source_connection_to_repositories.py
- app/database/migrations/044_add_remote_backup_support.py
- app/database/migrations/059_add_use_sftp_mode_to_ssh_connections.py
- app/database/migrations/100_add_oidc_linking_and_token_auth.py
- app/database/migrations/074_remove_mqtt_base_topic.py
- app/database/migrations/069_add_bypass_lock_on_list.py
- app/database/migrations/016_add_max_duration_to_check_jobs.py
- app/database/migrations/065_cleanup_schedule_duplicates.py
- app/database/migrations/068_add_remote_restore_fields.py
- app/database/migrations/112_agent_delete_and_nullable_enrollment_expiry.py
- app/database/migrations/060_add_speed_eta_to_restore_jobs.py
- app/database/migrations/007_add_size_progress_fields.py
- app/database/migrations/024_add_hourly_quarterly_prune.py
- app/database/migrations/035_add_prune_job_log_fields.py
- app/database/migrations/075_add_skip_on_failure.py
- app/database/migrations/098_add_auth_rate_limit_buckets.py
- app/database/migrations/106_add_schedule_enabled_flags.py
- app/database/migrations/119_add_backup_retry_lineage.py
- tests/unit/test_api_rclone.py
- tests/unit/test_database_operations.py
- app/main.py
Contributor
Author
|
Thanks — both findings looked at, one is answered with tests rather than a change, and CI turned up something real that is now fixed. CI: smoke tests were failing, and rightly soNot Postgres — every unit, integration, lint and frontend job passed. The three smoke jobs failed with Cause: this branch moved schema creation into the container entrypoint, so the application no longer prepared its own database. The smoke jobs start it with Fixed in Four tests, three on the function and one on the call, since the regression was a missing invocation rather than a broken function.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_db_upgrade_ensure_schema.py (1)
87-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer nested functions or mocks over lambda exception hacks.
Using an unstarted generator's
.throw()method to raise an exception inside alambdaexpression is a cryptic workaround. Using a standard nested function (orunittest.mock.Mock(side_effect=...)) achieves the same result transparently and aligns with Python and pytest idioms.
tests/unit/test_db_upgrade_ensure_schema.py#L87-L91: Replace the lambda with a nesteddefthat raisesRuntimeError("table already exists").tests/unit/test_db_upgrade_ensure_schema.py#L100-L104: Replace the lambda with a nesteddefthat raisesRuntimeError("disk is full").♻️ Proposed refactor for both tests
For
test_a_lost_race_is_not_an_error:- monkeypatch.setattr( - db_upgrade, - "_upgrade_to_head", - lambda *a, **k: (_ for _ in ()).throw(RuntimeError("table already exists")), - ) + def mock_upgrade_table_exists(*args, **kwargs): + raise RuntimeError("table already exists") + + monkeypatch.setattr(db_upgrade, "_upgrade_to_head", mock_upgrade_table_exists)For
test_a_build_that_really_failed_still_raises:- monkeypatch.setattr( - db_upgrade, - "_upgrade_to_head", - lambda *a, **k: (_ for _ in ()).throw(RuntimeError("disk is full")), - ) + def mock_upgrade_disk_full(*args, **kwargs): + raise RuntimeError("disk is full") + + monkeypatch.setattr(db_upgrade, "_upgrade_to_head", mock_upgrade_disk_full)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_db_upgrade_ensure_schema.py` around lines 87 - 91, Replace the exception-raising lambda patched into db_upgrade._upgrade_to_head in tests/unit/test_db_upgrade_ensure_schema.py:87-91 with a nested function that raises RuntimeError("table already exists"). Make the same change at tests/unit/test_db_upgrade_ensure_schema.py:100-104, using a nested function that raises RuntimeError("disk is full"); preserve both tests’ existing behavior and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_db_upgrade_ensure_schema.py`:
- Around line 87-91: Replace the exception-raising lambda patched into
db_upgrade._upgrade_to_head in tests/unit/test_db_upgrade_ensure_schema.py:87-91
with a nested function that raises RuntimeError("table already exists"). Make
the same change at tests/unit/test_db_upgrade_ensure_schema.py:100-104, using a
nested function that raises RuntimeError("disk is full"); preserve both tests’
existing behavior and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 534610f2-8538-4c0d-b6a5-ceb966b24806
📒 Files selected for processing (2)
app/database/db_upgrade.pytests/unit/test_db_upgrade_ensure_schema.py
🚧 Files skipped from review as they are similar to previous changes (1)
- app/database/db_upgrade.py
Contributor
Author
|
CI is green now — including the three smoke jobs that caught this, which were the only failures before. Two commits since the review:
Also |
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 20, 2026 17:52
24a1d47 to
268ef6b
Compare
Contributor
Author
|
Taken — the generator Checked that the tests still discriminate after the rewrite, since a clearer test that no longer fails would be a bad trade: widening the guard to a blanket catch still produces |
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 22, 2026 16:36
268ef6b to
c772a31
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/scripts/startup_packages.py (1)
40-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared "SQLite file absent" guard. The same
make_url(...) → backend == "sqlite" → not Path(url.database).exists()predicate is reimplemented in three scripts; a single shared helper would prevent the checks from drifting apart.
app/scripts/startup_packages.py#L40-L41: keep_database_absent()but have it delegate to a shared helper (e.g. inapp.database).app/scripts/reset_password.py#L55-L58: replace the inline guard with the shared helper.app/scripts/delete_ssh_key.py#L34-L37: replace the inline guard with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/scripts/startup_packages.py` around lines 40 - 41, Extract the shared SQLite-file-absent predicate into a helper in app.database, then update _database_absent() in app/scripts/startup_packages.py to delegate to it. Replace the inline make_url/backend/Path checks in app/scripts/reset_password.py lines 55-58 and app/scripts/delete_ssh_key.py lines 34-37 with the shared helper, preserving the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/scripts/startup_packages.py`:
- Around line 40-41: Extract the shared SQLite-file-absent predicate into a
helper in app.database, then update _database_absent() in
app/scripts/startup_packages.py to delegate to it. Replace the inline
make_url/backend/Path checks in app/scripts/reset_password.py lines 55-58 and
app/scripts/delete_ssh_key.py lines 34-37 with the shared helper, preserving the
existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5275df7-052b-4374-81c6-f024cddeb61a
📒 Files selected for processing (12)
app/config.pyapp/database/alembic/README.mdapp/database/db_upgrade.pyapp/main.pyapp/scripts/delete_ssh_key.pyapp/scripts/reset_password.pyapp/scripts/startup_packages.pytests/unit/test_admin_db_scripts.pytests/unit/test_api_backup_plans.pytests/unit/test_db_upgrade_ensure_schema.pytests/unit/test_main.pytests/unit/test_startup_packages.py
🚧 Files skipped from review as they are similar to previous changes (5)
- app/database/alembic/README.md
- app/main.py
- tests/unit/test_main.py
- tests/unit/test_db_upgrade_ensure_schema.py
- app/database/db_upgrade.py
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 22, 2026 17:14
c772a31 to
a619f99
Compare
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 22, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
Contributor
Author
|
The shared guard now lives in @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/database/url_utils.py`:
- Around line 19-20: Update the URL-checking function around make_url and
get_backend_name to handle SQLite in-memory URLs before constructing
Path(url.database): return false for empty database values and the ":memory:"
database, while preserving the missing-file check for SQLite URLs with a
filesystem database.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4e4dc4e-a0da-484f-bae5-84cee1489280
📒 Files selected for processing (13)
app/config.pyapp/database/alembic/README.mdapp/database/db_upgrade.pyapp/database/url_utils.pyapp/main.pyapp/scripts/delete_ssh_key.pyapp/scripts/reset_password.pyapp/scripts/startup_packages.pytests/unit/test_admin_db_scripts.pytests/unit/test_api_backup_plans.pytests/unit/test_db_upgrade_ensure_schema.pytests/unit/test_main.pytests/unit/test_startup_packages.py
🚧 Files skipped from review as they are similar to previous changes (10)
- app/database/alembic/README.md
- app/config.py
- tests/unit/test_startup_packages.py
- app/scripts/reset_password.py
- tests/unit/test_main.py
- app/main.py
- app/scripts/delete_ssh_key.py
- app/database/db_upgrade.py
- app/scripts/startup_packages.py
- tests/unit/test_db_upgrade_ensure_schema.py
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 22, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 22, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 22, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 22, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 23, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 23, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
ioanalytica
added a commit
to ioanalytica/borg-ui
that referenced
this pull request
Jul 23, 2026
CodeRabbit on karanhudia#768: the same make_url -> backend == sqlite -> file missing predicate was reimplemented in three scripts. One helper in app/database/url_utils.py (import-side-effect free) now serves all of them. Mirrors the amended commits on feat/alembic-and-postgres.
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 24, 2026 06:27
a619f99 to
4de3fdb
Compare
Contributor
Author
|
Addressed the @coderabbitai review |
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 24, 2026 06:48
4de3fdb to
756ab75
Compare
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/test_main.py`:
- Around line 24-71: Mock app.database.db_upgrade.ensure_schema in each startup
test setup before invoking startup_event: tests/unit/test_main.py lines 24-71,
82-98, and 132-153. Keep the existing dependency mocks and assertions unchanged
so all startup tests avoid real schema initialization and database mutation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8522d45-2bf3-444f-869d-bd9a88243aab
📒 Files selected for processing (169)
.github/workflows/tests.ymlDockerfilealembic.iniapp/api/backup_plans.pyapp/api/scripts_library.pyapp/config.pyapp/database/alembic/README.mdapp/database/alembic/env.pyapp/database/alembic/script.py.makoapp/database/alembic/versions/f6c46c665fd3_baseline_schema.pyapp/database/database.pyapp/database/db_upgrade.pyapp/database/migrations/004_add_progress_fields.pyapp/database/migrations/005_add_backup_speed.pyapp/database/migrations/006_add_scheduled_jobs.pyapp/database/migrations/007_add_size_progress_fields.pyapp/database/migrations/008_add_scheduled_job_id_to_backups.pyapp/database/migrations/009_add_restore_jobs.pyapp/database/migrations/010_add_backup_hooks.pyapp/database/migrations/011_add_must_change_password.pyapp/database/migrations/012_add_prune_compact_to_scheduled_jobs.pyapp/database/migrations/013_add_maintenance_status_to_backup_jobs.pyapp/database/migrations/014_add_check_compact_jobs.pyapp/database/migrations/015_add_last_check_compact_to_repositories.pyapp/database/migrations/016_add_max_duration_to_check_jobs.pyapp/database/migrations/017_add_pid_tracking.pyapp/database/migrations/018_remove_config_file.pyapp/database/migrations/019_add_archive_name_template.pyapp/database/migrations/020_add_repository_mode.pyapp/database/migrations/021_add_custom_flags.pyapp/database/migrations/022_add_ssh_key_default_path.pyapp/database/migrations/023_add_notify_on_backup_start.pyapp/database/migrations/024_add_hourly_quarterly_prune.pyapp/database/migrations/025_add_check_notifications_and_scheduling.pyapp/database/migrations/027_add_script_library.pyapp/database/migrations/028_split_hook_timeouts.pyapp/database/migrations/029_add_continue_on_error.pyapp/database/migrations/030_add_repository_path_to_check_compact_jobs.pyapp/database/migrations/031_add_prune_jobs_table.pyapp/database/migrations/032_add_storage_and_mount_to_ssh_connections.pyapp/database/migrations/033_add_log_management_settings.pyapp/database/migrations/034_add_scheduled_prune_field.pyapp/database/migrations/034_convert_check_interval_to_cron.pyapp/database/migrations/035_add_prune_job_log_fields.pyapp/database/migrations/036_add_compact_job_log_fields.pyapp/database/migrations/037_add_multi_repo_schedules.pyapp/database/migrations/038_add_keyfile_support.pyapp/database/migrations/039_add_repository_notification_filters.pyapp/database/migrations/040_add_cache_settings.pyapp/database/migrations/041_add_redis_url.pyapp/database/migrations/043_add_check_job_log_fields.pyapp/database/migrations/044_add_remote_backup_support.pyapp/database/migrations/045_add_repository_connection_id.pyapp/database/migrations/046_add_use_new_wizard_setting.pyapp/database/migrations/047_add_browse_memory_limits.pyapp/database/migrations/048_fix_ssh_connection_cascade.pyapp/database/migrations/049_add_delete_archive_jobs.pyapp/database/migrations/050_add_source_connection_to_repositories.pyapp/database/migrations/051_add_analytics_enabled.pyapp/database/migrations/052_add_analytics_consent_given.pyapp/database/migrations/053_add_operation_timeouts.pyapp/database/migrations/054_add_stats_refresh_interval.pyapp/database/migrations/055_add_bypass_lock.pyapp/database/migrations/056_add_last_stats_refresh.pyapp/database/migrations/057_add_script_parameters.pyapp/database/migrations/057_populate_connection_id_for_legacy_repos.pyapp/database/migrations/058_add_notification_enhancements.pyapp/database/migrations/059_add_use_sftp_mode_to_ssh_connections.pyapp/database/migrations/060_add_speed_eta_to_restore_jobs.pyapp/database/migrations/061_add_bypass_lock_on_info.pyapp/database/migrations/062_add_unique_constraint_schedule_repo.pyapp/database/migrations/063_add_schedule_script_parameters.pyapp/database/migrations/064_add_show_restore_tab.pyapp/database/migrations/065_cleanup_schedule_duplicates.pyapp/database/migrations/066_add_ssh_path_prefix.pyapp/database/migrations/067_add_cascade_delete_to_script_executions.pyapp/database/migrations/068_add_remote_restore_fields.pyapp/database/migrations/069_add_bypass_lock_on_list.pyapp/database/migrations/070_add_source_size_timeout.pyapp/database/migrations/071_add_mqtt_beta_enabled.pyapp/database/migrations/072_add_mqtt_base_topic.pyapp/database/migrations/073_fix_delete_archive_jobs_cascade.pyapp/database/migrations/074_add_use_sudo_to_ssh_connections.pyapp/database/migrations/074_remove_mqtt_base_topic.pyapp/database/migrations/075_add_skip_on_failure.pyapp/database/migrations/075_fix_schedule_script_id_set_null.pyapp/database/migrations/076_fix_package_install_jobs_cascade.pyapp/database/migrations/077_fix_script_executions_fk_actions.pyapp/database/migrations/078_fix_scripts_created_by_user_fk.pyapp/database/migrations/079_add_borg_version.pyapp/database/migrations/080_add_plan.pyapp/database/migrations/081_add_role_and_api_tokens.pyapp/database/migrations/082_add_user_profile_fields.pyapp/database/migrations/083_add_deployment_profile.pyapp/database/migrations/084_add_user_repository_permissions.pyapp/database/migrations/085_add_all_repositories_role.pyapp/database/migrations/086_add_licensing_state.pyapp/database/migrations/087_add_repository_inline_script_parameters.pyapp/database/migrations/088_add_metrics_settings.pyapp/database/migrations/089_add_backup_job_archive_name.pyapp/database/migrations/090_add_borg2_fast_browse_beta_enabled.pyapp/database/migrations/091_add_totp_to_users.pyapp/database/migrations/092_add_passkey_credentials.pyapp/database/migrations/093_add_backup_warning_notifications.pyapp/database/migrations/094_add_scheduler_concurrency_limits.pyapp/database/migrations/095_add_schedule_timezones.pyapp/database/migrations/096_add_oidc_settings.pyapp/database/migrations/097_add_oidc_state_and_auth_events.pyapp/database/migrations/098_add_auth_rate_limit_buckets.pyapp/database/migrations/099_harden_oidc_exchange_and_admin_mapping.pyapp/database/migrations/100_add_oidc_linking_and_token_auth.pyapp/database/migrations/101_add_restore_check_jobs.pyapp/database/migrations/102_add_restore_check_canary_enabled.pyapp/database/migrations/103_add_restore_check_notifications.pyapp/database/migrations/104_add_agent_machines.pyapp/database/migrations/104_add_backup_plans.pyapp/database/migrations/105_add_agent_jobs.pyapp/database/migrations/105_add_dashboard_health_threshold_settings.pyapp/database/migrations/106_add_agent_repository_links.pyapp/database/migrations/106_add_schedule_enabled_flags.pyapp/database/migrations/107_add_check_extra_flags.pyapp/database/migrations/107_add_source_locations.pyapp/database/migrations/108_add_managed_agents_beta_enabled.pyapp/database/migrations/109_add_repository_wipe_jobs.pyapp/database/migrations/110_add_repository_executor_type.pyapp/database/migrations/111_add_backup_job_route_strategy.pyapp/database/migrations/111_add_backup_monitoring_reports.pyapp/database/migrations/112_add_backup_report_delivery_schedule.pyapp/database/migrations/112_agent_delete_and_nullable_enrollment_expiry.pyapp/database/migrations/113_add_rclone_storage.pyapp/database/migrations/114_add_scheduled_rclone_mirror_jobs.pyapp/database/migrations/115_add_rclone_oauth_provider_credentials.pyapp/database/migrations/116_add_agent_default_path.pyapp/database/migrations/117_add_backup_plan_database_template_id.pyapp/database/migrations/117_add_rclone_sync_job_operation.pyapp/database/migrations/118_add_backup_plan_scripts.pyapp/database/migrations/119_add_backup_retry_lineage.pyapp/database/migrations/120_add_lock_breaking_enabled.pyapp/database/migrations/121_add_rclone_oauth_provider_credentials_table.pyapp/database/migrations/122_add_rclone_remote_storage.pyapp/database/migrations/122_add_remote_backend_clients.pyapp/database/migrations/123_add_repository_upload_ratelimit.pyapp/database/migrations/124_add_backup_plan_upload_ratelimit_policies.pyapp/database/migrations/125_add_prune_keep_within.pyapp/database/migrations/126_add_agent_script_to_plan_hooks.pyapp/database/migrations/126_clear_persisted_archive_browse_listings.pyapp/database/migrations/127_add_agent_job_to_script_executions.pyapp/database/migrations/__init__.pyapp/database/models.pyapp/database/url_utils.pyapp/main.pyapp/scripts/delete_ssh_key.pyapp/scripts/deploy_ssh_key.pyapp/scripts/reset_password.pyapp/scripts/startup_packages.pyentrypoint.shrequirements.txttests/fixtures/api.pytests/unit/test_admin_db_scripts.pytests/unit/test_api_backup_plans.pytests/unit/test_api_rclone.pytests/unit/test_config.pytests/unit/test_database_operations.pytests/unit/test_db_upgrade.pytests/unit/test_db_upgrade_ensure_schema.pytests/unit/test_dockerfile_database_url.pytests/unit/test_main.pytests/unit/test_startup_packages.pytests/unit/test_url_utils.py
💤 Files with no reviewable changes (138)
- app/database/migrations/019_add_archive_name_template.py
- app/database/migrations/014_add_check_compact_jobs.py
- app/database/migrations/108_add_managed_agents_beta_enabled.py
- app/database/migrations/047_add_browse_memory_limits.py
- app/database/migrations/006_add_scheduled_jobs.py
- app/database/migrations/038_add_keyfile_support.py
- app/database/migrations/086_add_licensing_state.py
- app/database/migrations/079_add_borg_version.py
- app/database/migrations/066_add_ssh_path_prefix.py
- app/database/migrations/012_add_prune_compact_to_scheduled_jobs.py
- app/database/migrations/005_add_backup_speed.py
- app/database/migrations/083_add_deployment_profile.py
- app/database/migrations/092_add_passkey_credentials.py
- app/database/migrations/117_add_backup_plan_database_template_id.py
- app/database/migrations/074_remove_mqtt_base_topic.py
- app/database/migrations/095_add_schedule_timezones.py
- app/database/migrations/010_add_backup_hooks.py
- app/database/migrations/118_add_backup_plan_scripts.py
- app/database/migrations/init.py
- app/database/migrations/111_add_backup_monitoring_reports.py
- app/database/migrations/109_add_repository_wipe_jobs.py
- app/database/migrations/093_add_backup_warning_notifications.py
- app/database/migrations/057_populate_connection_id_for_legacy_repos.py
- app/database/migrations/024_add_hourly_quarterly_prune.py
- app/database/migrations/094_add_scheduler_concurrency_limits.py
- app/database/migrations/041_add_redis_url.py
- app/database/migrations/061_add_bypass_lock_on_info.py
- app/database/migrations/106_add_agent_repository_links.py
- app/database/migrations/072_add_mqtt_base_topic.py
- app/database/migrations/023_add_notify_on_backup_start.py
- app/database/migrations/046_add_use_new_wizard_setting.py
- app/database/migrations/055_add_bypass_lock.py
- app/database/migrations/008_add_scheduled_job_id_to_backups.py
- app/database/migrations/028_split_hook_timeouts.py
- app/database/migrations/063_add_schedule_script_parameters.py
- app/database/migrations/085_add_all_repositories_role.py
- app/database/migrations/020_add_repository_mode.py
- app/database/migrations/074_add_use_sudo_to_ssh_connections.py
- app/database/migrations/082_add_user_profile_fields.py
- app/database/migrations/075_add_skip_on_failure.py
- app/database/migrations/106_add_schedule_enabled_flags.py
- app/database/migrations/070_add_source_size_timeout.py
- app/database/migrations/107_add_source_locations.py
- app/database/migrations/018_remove_config_file.py
- app/database/migrations/104_add_backup_plans.py
- app/database/migrations/051_add_analytics_enabled.py
- app/database/migrations/105_add_dashboard_health_threshold_settings.py
- app/database/migrations/004_add_progress_fields.py
- app/database/migrations/125_add_prune_keep_within.py
- app/database/migrations/043_add_check_job_log_fields.py
- app/database/migrations/013_add_maintenance_status_to_backup_jobs.py
- app/database/migrations/007_add_size_progress_fields.py
- app/database/migrations/048_fix_ssh_connection_cascade.py
- app/database/migrations/049_add_delete_archive_jobs.py
- app/database/migrations/060_add_speed_eta_to_restore_jobs.py
- app/database/migrations/121_add_rclone_oauth_provider_credentials_table.py
- app/database/migrations/054_add_stats_refresh_interval.py
- app/database/migrations/045_add_repository_connection_id.py
- app/database/migrations/100_add_oidc_linking_and_token_auth.py
- app/database/migrations/103_add_restore_check_notifications.py
- app/database/migrations/099_harden_oidc_exchange_and_admin_mapping.py
- app/database/migrations/009_add_restore_jobs.py
- app/database/migrations/062_add_unique_constraint_schedule_repo.py
- app/database/migrations/021_add_custom_flags.py
- app/database/migrations/031_add_prune_jobs_table.py
- app/database/migrations/052_add_analytics_consent_given.py
- app/database/migrations/017_add_pid_tracking.py
- app/database/migrations/119_add_backup_retry_lineage.py
- app/database/migrations/126_add_agent_script_to_plan_hooks.py
- app/database/migrations/034_convert_check_interval_to_cron.py
- app/database/migrations/111_add_backup_job_route_strategy.py
- app/database/migrations/116_add_agent_default_path.py
- app/database/migrations/076_fix_package_install_jobs_cascade.py
- app/database/migrations/034_add_scheduled_prune_field.py
- app/database/migrations/114_add_scheduled_rclone_mirror_jobs.py
- app/database/migrations/065_cleanup_schedule_duplicates.py
- app/database/migrations/033_add_log_management_settings.py
- app/database/migrations/077_fix_script_executions_fk_actions.py
- app/database/migrations/032_add_storage_and_mount_to_ssh_connections.py
- app/database/migrations/027_add_script_library.py
- app/database/migrations/112_add_backup_report_delivery_schedule.py
- app/database/migrations/029_add_continue_on_error.py
- app/database/migrations/124_add_backup_plan_upload_ratelimit_policies.py
- app/database/migrations/050_add_source_connection_to_repositories.py
- app/database/migrations/057_add_script_parameters.py
- app/database/migrations/096_add_oidc_settings.py
- app/database/migrations/069_add_bypass_lock_on_list.py
- app/database/migrations/115_add_rclone_oauth_provider_credentials.py
- app/database/migrations/039_add_repository_notification_filters.py
- app/database/migrations/071_add_mqtt_beta_enabled.py
- app/database/migrations/107_add_check_extra_flags.py
- app/database/migrations/123_add_repository_upload_ratelimit.py
- app/database/migrations/068_add_remote_restore_fields.py
- app/database/migrations/089_add_backup_job_archive_name.py
- app/database/migrations/090_add_borg2_fast_browse_beta_enabled.py
- app/database/migrations/081_add_role_and_api_tokens.py
- app/database/migrations/064_add_show_restore_tab.py
- app/database/migrations/101_add_restore_check_jobs.py
- app/database/migrations/073_fix_delete_archive_jobs_cascade.py
- app/database/migrations/037_add_multi_repo_schedules.py
- app/database/migrations/044_add_remote_backup_support.py
- app/database/migrations/011_add_must_change_password.py
- app/database/migrations/122_add_rclone_remote_storage.py
- app/database/migrations/035_add_prune_job_log_fields.py
- app/database/migrations/091_add_totp_to_users.py
- app/database/migrations/102_add_restore_check_canary_enabled.py
- app/database/migrations/056_add_last_stats_refresh.py
- app/database/migrations/059_add_use_sftp_mode_to_ssh_connections.py
- app/database/migrations/030_add_repository_path_to_check_compact_jobs.py
- app/database/migrations/080_add_plan.py
- app/database/migrations/113_add_rclone_storage.py
- app/database/migrations/110_add_repository_executor_type.py
- app/database/migrations/127_add_agent_job_to_script_executions.py
- app/database/migrations/036_add_compact_job_log_fields.py
- app/database/migrations/040_add_cache_settings.py
- app/database/migrations/122_add_remote_backend_clients.py
- app/database/migrations/087_add_repository_inline_script_parameters.py
- app/database/migrations/015_add_last_check_compact_to_repositories.py
- app/database/migrations/016_add_max_duration_to_check_jobs.py
- app/database/migrations/075_fix_schedule_script_id_set_null.py
- app/database/migrations/104_add_agent_machines.py
- app/database/migrations/067_add_cascade_delete_to_script_executions.py
- app/database/migrations/105_add_agent_jobs.py
- app/database/migrations/053_add_operation_timeouts.py
- app/database/migrations/025_add_check_notifications_and_scheduling.py
- app/database/migrations/126_clear_persisted_archive_browse_listings.py
- app/database/migrations/078_fix_scripts_created_by_user_fk.py
- app/database/migrations/097_add_oidc_state_and_auth_events.py
- app/database/migrations/084_add_user_repository_permissions.py
- app/database/migrations/120_add_lock_breaking_enabled.py
- app/database/migrations/022_add_ssh_key_default_path.py
- app/database/migrations/058_add_notification_enhancements.py
- app/database/migrations/117_add_rclone_sync_job_operation.py
- app/database/migrations/112_agent_delete_and_nullable_enrollment_expiry.py
- app/database/migrations/088_add_metrics_settings.py
- app/database/migrations/098_add_auth_rate_limit_buckets.py
- tests/unit/test_database_operations.py
- tests/unit/test_api_rclone.py
🚧 Files skipped from review as they are similar to previous changes (25)
- alembic.ini
- tests/unit/test_dockerfile_database_url.py
- app/database/url_utils.py
- Dockerfile
- app/database/alembic/script.py.mako
- requirements.txt
- app/database/database.py
- app/api/scripts_library.py
- app/api/backup_plans.py
- app/database/alembic/env.py
- entrypoint.sh
- app/database/alembic/README.md
- tests/fixtures/api.py
- app/scripts/deploy_ssh_key.py
- app/main.py
- app/scripts/delete_ssh_key.py
- app/config.py
- app/scripts/reset_password.py
- tests/unit/test_db_upgrade_ensure_schema.py
- tests/unit/test_startup_packages.py
- app/database/alembic/versions/f6c46c665fd3_baseline_schema.py
- tests/unit/test_db_upgrade.py
- app/scripts/startup_packages.py
- app/database/models.py
- app/database/db_upgrade.py
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 24, 2026 07:11
756ab75 to
0dbb9c7
Compare
Contributor
Author
|
Addressed the test-isolation finding: the three startup unit tests (mqtt, cache, licensing) now patch @coderabbitai review |
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
Contributor
Author
|
Heads-up on the red Rather than pull an unrelated dependency bump into this Alembic/Postgres PR, I've opened #776 to fix it repo-wide — a single minor bump |
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 24, 2026 11:11
c4685cc to
0debf13
Compare
Contributor
Author
|
Thanks for merging #776 and syncing this branch! I've replaced the The code state is byte-for-byte identical to the merge — I verified the resulting tree hashes match exactly ( (force-pushed to our fork branch; nothing else changed.) |
SQLite's LIKE is case-insensitive by default, Postgres' is not. The script library search therefore returned no matches on Postgres for any query whose case did not match the stored name. The backup-plan name lookup is a prefilter compared exactly in Python afterwards, so its result is unaffected either way; it is converted for consistency, keeping "no raw LIKE" a greppable rule.
…mbic models.py described a schema no install runs: create_all alone vs create_all + migrations x2 differed in 44 semantic ways (the migration-built schema reproduces a live install with 0 diffs, so the model is what was wrong). - declares the 28 indexes the migrations create and the model never mentioned, plus ON DELETE SET NULL on scheduled_jobs.pre/post_backup_script_id and a func.now() server_default on scheduled_job_repositories.created_at - AUTOINCREMENT on every single-integer-PK table (46 of 48), derived from the PK so a new model cannot forget it; verified alembic autogenerate renders it - a naming_convention on the metadata so every constraint has a deliberate name (SQLite cannot ALTER an unnamed constraint; batch migrations need the names) Left as deliberate repairs: 8 columns no model class has, and a duplicate index. Changes nothing for existing installs (create_all skips existing tables); it only shapes what the Alembic baseline is cut from.
The baseline is generated from the reconciled models.py: 48 tables, 131 indexes, 77 named foreign keys, verified to produce a schema identical to create_all's, and to apply unchanged to both SQLite and Postgres 16 (alembic check reports no drift against either). It lives under app/ because the image only copies app/; env.py takes the URL from settings and renders every migration as batch so one body serves SQLite (table rebuild) and Postgres (plain ALTER) alike. alembic_init(sqlite_db, postgres_conn=None) is the one-time upgrade: both databases exist at once, rows are copied by column name, the source is renamed to *_bak as the rollback. Postgres is just a different URL on the same path. Self-references transfer in two passes, tables go in topological order (Postgres checks every FK at insert), sequences are advanced past the data (SQLite derives the next id itself; a SQLite-only test can never catch a missing setval, so the unit tests run against a real Postgres in CI). The transfer engine enforces foreign keys on both dialects, so the SQLite target cannot accept what Postgres rejects. Verified against a real 98k-row database into both dialects.
…tions Startup replayed all 135 migrations on every boot and built the schema with create_all at import time. Both are gone. The entrypoint brings the database to the alembic head once, before the app is imported (gunicorn forks workers that would race over the same database), as the user that will own the file, and fails the boot instead of starting on a database in an unknown state. The legacy migrations are deleted: nothing reaches them any more, and the baseline was cut from the schema they actually produced. With them goes their whole class of problem -- a schema that depended on restart count, migrations hardcoding a primary-key form the model never asked for, and one silently switching foreign-key enforcement off for the life of the process. The upgrade target is built with synchronous=OFF: it is disposable until the swap, so durability while building it buys nothing, and over NFS the fsync per CREATE INDEX was the whole cost. The live database keeps FULL. Progress is logged to stdout (schema build, per-table row counts, alembic's own output) so kubectl logs shows a migration running instead of a silent pod.
The server can store its own database in Postgres instead of the SQLite file. resolve_database_url assembles the DSN from DB_HOST/DB_PORT/DB_USER/DB_PASSWORD/ DB_NAME (quoting the password, which a generated one needs), DATABASE_URL still wins where set, and DB_HOST without credentials fails loudly rather than silently running on SQLite. The image no longer bakes a DATABASE_URL default, which would otherwise always win over DB_HOST; the app derives the SQLite URL from DATA_DIR itself. Boot is decided by the alembic stamp: once Postgres is at head it IS the database, whatever SQLite file sits in the data dir (a kept rollback, or one an entrypoint script recreated empty) -- so a restart never stalls on it. The database is upgraded before the SSH-key deploy that reads from it, and deploy_ssh_key goes through the ORM so it follows DATABASE_URL to Postgres and creates no stray SQLite file. Verified end to end on a real cluster: SQLite migrates into Postgres, the app reports the postgres dialect and reads the transferred rows.
The startup script opened /data/borg.db with raw sqlite3 and wrote back
with SQLite-only SQL (datetime('now')). With an external database there
is no such file: package auto-install silently never ran, and any stray
empty borg.db turned every boot into a "no such table:
installed_packages" traceback. Route the script through the
application's own engine instead. A fresh SQLite install still skips
without creating the database file, and a present-but-empty database
degrades to "no packages" rather than an error.
reset_password and delete_ssh_key opened /data/borg.db with raw sqlite3, so on installs using DATABASE_URL they silently targeted the wrong (or a nonexistent) database. Same treatment as the startup packages script: route them through the application's engine. reset_password keeps BORG_DB_PATH as a legacy SQLite override; both refuse to run against a missing SQLite file instead of implicitly creating one, and the delete_ssh_key mutation now runs in a single transaction. deploy_ssh_key needed nothing - the Postgres series already moved it to the ORM.
The Settings Config carried a v1-style fields={...} block mapping CORS_ORIGINS /
TRUSTED_PROXIES / OIDC_ALLOWED_RETURN_ORIGINS onto private fields. pydantic v2
removed that config key: it warned on every settings import and ignored the
mapping, so those env vars already had no effect (verified). Removing it
silences the warning and changes nothing at runtime; a comment records that
wiring them back up needs Field(validation_alias=) on public fields.
Moving schema creation into the container entrypoint left the application unable to prepare its own database. Started any other way — `uvicorn app.main:app`, a development run, a test harness — it came up against an empty database and failed every request that read from it, which is what the smoke tests found: authentication returned 500, because there was no users table. Startup now calls ensure_schema() before anything opens a session. It is cheap when there is nothing to do — one query for the recorded revision — and builds the baseline when the database is empty. A database that predates Alembic is deliberately left untouched, with a log line naming the tool. Migrating one copies every row and can change where the database lives; that is a one-time operation with a rollback, not something to do behind a server that is already coming up. Three tests cover ensure_schema itself and one covers the call, since the regression was not a broken function but a missing invocation: without it the new test fails with "Expected 'ensure_schema' to have been called once".
The prefix query in _unique_backup_plan_name was converted from LIKE to ILIKE so both dialects behave alike — SQLite's LIKE is already case-insensitive, Postgres' is not. Review asked whether that leaves the following comparisons inconsistent, since they are exact. It does not, and these tests say why: plan names are unique case-sensitively (`name = Column(String, unique=True)`), so a name differing only in case is genuinely free and suffixing it would be wrong. Widening the candidate set cannot change an answer decided by exact comparison. Four cases: a case variant is free, an exact collision is suffixed, suffixes skip what exists, and a case variant does not consume a suffix.
ensure_schema() runs per process, so with more than one worker two of them can find an empty database and both start building it. One wins; the other used to take its worker down with "table already exists". The loser now waits for the winner to finish before deciding, because a database mid-build is not yet at head and a single check would call a won race lost. Only a build that is still absent after that surfaces — a failure with a real cause, a full disk say, is not swallowed. The entrypoint still builds the schema once before gunicorn forks, so workers normally find it already there. This is for the starts that do not go through it, and for --workers > 1 later.
ioanalytica
force-pushed
the
feat/alembic-and-postgres
branch
from
July 24, 2026 14:23
0debf13 to
4a2eac6
Compare
…losing data The one-time upgrade copies rows by column name; it does not re-run the old migrations. That is safe for schema drift, but some old migrations transform data and drop the source column -- e.g. 034 converts check_interval_days into a cron expression and drops it. A database that skipped those has the data in the old shape, and a structural copy would drop the column and silently lose it. Guard against it: before copying, refuse a source that still carries a data-transform's dropped column, naming what would be lost and pointing at the supported path (upgrade to a 2.2.5 release first, which runs the full migration chain). A curated sentinel list keeps this to the destructive cases; the report still lists every dropped column for the rest. This touches no migration and no already-migrated database -- it only turns a silent, version-skipping data loss into an actionable error.
ioanalytica
changed the title
Copying rows by column name cannot reproduce the data transforms old migrations performed (an interval became a cron expression, a timeout was split), so copying a pre-2.2.5 database straight onto the baseline would silently drop that data. Keep the 135 pre-Alembic migrations as a frozen catch-up ladder and run them, once, on a throwaway copy of a legacy database before its rows are copied. Any supported version now upgrades directly to the latest -- and into Postgres in the same step -- with no manual intermediate release. The source is never touched; it stays as the rollback. The old guard is kept as a safety net: if the lenient ladder leaves a required transform undone, the upgrade refuses rather than lose data. Add the durable fork the long coexistence needs: a database that already has alembic_version is only ever moved forward by Alembic, never dragged through the legacy ladder -- the one correct behaviour once a second Alembic revision exists.
…ransform A faithful "database at version N" cannot be reconstructed: the pre-Alembic startup built its schema with create_all before running migrations, so a historical schema is not recoverable from the current models alone. Cover the real risk surface instead. - Run the whole 135-migration ladder against a representative schema and assert none raises -- the automated twin of running the upgrade against a real 197 MB production database, which reproduced it with zero row loss. - Prove the 028 hook-timeout split is backfilled from a pre-028 source. An audit of every migration that drops a column found 034 (interval->cron) is the only transform that moves data into a column the baseline dropped, so it stays the sole sentinel; 028 keeps its source column, so that data survives regardless.
Contributor
Author
|
Update addressing your feedback on the intermediate-version step. The upgrade no longer refuses a pre-2.2.5 database and asks the operator to stop on an intermediate release. It also adds the durable fork the long coexistence needs: once a database has Validated end-to-end against a real 197 MB production database (227k rows → transferred, zero row loss). Key commits: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detailed write-up sent separately; this is the short version for review.
What changes
Startup schema management moves from the hand-rolled migration runner to Alembic with a single baseline revision (
f6c46c665fd3, 48 tables). New schema changes are Alembic revisions from here on.The 135 pre-Alembic migration scripts are kept, frozen, for one job: catching an existing database up (see "Existing installations"). They are no longer run at startup.
Why
The old runner had no record of what it had already applied, so it re-ran everything on every start; whether a given database ended up correct depended on how often the container had restarted. It also only ever spoke SQLite.
Verification you can reproduce
Building the schema both ways —
Base.metadata.create_all()frommodels.py, andalembic upgrade headfrom the baseline — yields the same 48 tables with zero differences in columns, indexes and foreign keys.That check matters because the baseline was not produced by
alembic revision --autogenerate.models.pyhad drifted from the schema the migrations actually built, so autogenerating from it would have baked the drift in. The baseline describes the real schema, andmodels.pywas corrected to match it first — that is what the separate reconcile commit does. The two agreeing is therefore a result, not a tautology.Existing installations — direct upgrade from any version
db_upgrade.pyperforms a one-time move onto the baseline, run automatically by the entrypoint before the app starts:alembic_versionis only ever moved forward by Alembic (alembic upgrade head) — never the legacy path. This is the durable fork: however many Alembic revisions accrue, a managed database follows Alembic alone.Both databases exist at the same time, so nothing is serialised; every deviation between source and target is reported rather than absorbed.
Postgres
Opt-in, and it falls out of the above rather than being a separate feature: the same upgrade path with a different URL. SQLite remains the default and is unaffected. One behavioural fix was needed —
.like()is case-sensitive on Postgres where SQLite is not, so name searches use.ilike().For future migrations
A short guide under
docs/engineering/covers the handful of places where dialects actually differ — batch operations for SQLite's limitedALTER TABLE, booleans, autoincrement — so one migration file stays correct on both.Validation
Run against a real 197 MB production SQLite database (227k rows): transferred onto the baseline with zero row loss. Tests cover the data transforms (interval→cron, the hook-timeout split), the full-ladder run, the safety net, and the Alembic-managed fork.
Self-contained commits in dependency order.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests / CI