fix(clickhouse): Unblock flagsmith migrate when ClickHouse is configured - #8445
fix(clickhouse): Unblock flagsmith migrate when ClickHouse is configured#8445matthewelwell wants to merge 1 commit into
flagsmith migrate when ClickHouse is configured#8445Conversation
…gured
On any install with `CLICKHOUSE_URL` or `CLICKHOUSE_HOST` set, `flagsmith
migrate` exits 1 partway through without finishing:
django.core.exceptions.FieldError: Cannot resolve keyword 'deleted'
into field. Choices are: app, applied, id, name
`_migrate()` walks `FLAGSMITH_MIGRATE_DATABASES` in a single process and
`clickhouse` is appended last, so `default` is always migrated first. That
resolves a Postgres `MigrationRecorder`, and Django's `classproperty` caches
the Postgres-shaped model on the *class*. `clickhouse-backend` applies its
monkeypatches as an import side effect of `clickhouse_backend.backend`,
which Django only imports when it opens the ClickHouse connection -- by
which point that cache is already populated. The patched `Migration`
property reads the stale class attribute, so the ClickHouse recorder is
handed a model with no `deleted` column, and `migration_qs`, which filters
on `deleted` for ClickHouse connections, raises.
Import the backend from the app's `ready()` so the patches are in place
before any recorder is resolved. Kept behind a check on the `clickhouse`
alias so Postgres-only installs stay unpatched, as they are today.
The same stale cache also breaks local test runs, where each xdist worker
sets up its databases lazily. It accounted for several hundred errors per
`make test`, varying with test order. CI escaped both symptoms: no workflow
runs migrations, and `--ci` builds every connection up front, which imports
the backend before any recorder exists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
|
I don't understand the need for this PR enough to stand behind it - it's pure slop. Closing and will discuss internally as needed. |
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20076 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #20076 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20076 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20076 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8445 +/- ##
=======================================
Coverage 98.81% 98.81%
=======================================
Files 1621 1622 +1
Lines 66128 66155 +27
=======================================
+ Hits 65344 65371 +27
Misses 784 784 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Visual Regression19 screenshots compared. See report for details. |
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
On any install with
CLICKHOUSE_URLorCLICKHOUSE_HOSTset,flagsmith migrateexits 1 partway through withFieldError: Cannot resolve keyword 'deleted' into field. Choices are: app, applied, id, name, so migrations never complete._migrate()walksFLAGSMITH_MIGRATE_DATABASESin one process withclickhouseappended last, sodefaultis migrated first. That resolves a PostgresMigrationRecorder, and Django'sclasspropertycaches the Postgres-shaped model on the class.clickhouse-backendapplies its monkeypatches as an import side effect ofclickhouse_backend.backend, which Django only imports when it opens the ClickHouse connection — too late. Its replacementMigrationproperty reads the stale class attribute, so the ClickHouse recorder gets a model with nodeletedcolumn andmigration_qs, which filters ondeletedfor ClickHouse connections, raises.This imports the backend from the app's
ready()so the patches land before any recorder is resolved, behind a check on theclickhousealias so Postgres-only installs stay unpatched as they are today.The same stale cache breaks local
make test, where each xdist worker sets up databases lazily — several hundred errors per run, varying with test order. CI never saw either symptom: no workflow runs migrations, and--cibuilds every connection up front, importing the backend before any recorder exists.How did you test this code?
Two new unit tests cover both branches of the alias guard.
Manually, against throwaway Postgres and ClickHouse databases:
CLICKHOUSE_HOST=localhost ... flagsmith migrateonmain→ exit 1, dies on the first migration recorded against theclickhousealias.Migrationmodel with fields['app', 'applied', 'id', 'name']and aFieldError; with this change it yields['app', 'applied', 'deleted', 'id', 'name'].CLICKHOUSE_*) leavesclickhouse_backend.backendunimported.Full suite locally with
-n auto: 5022 passed, 24 skipped, 0 failed, 0 errors — previously 1123 errors from this cause on the same machine.