feat(db): implement Alembic migrations #164
Conversation
m-khan-97
left a comment
There was a problem hiding this comment.
Went through the full diff.
This is clean work — the separation of concerns is exactly right. create_app() no longer touches the DB at boot, startup.sh runs alembic upgrade head before Gunicorn starts, and since we're on set -euo pipefail a broken migration now fails the deploy loudly instead of silently corrupting schema. downgrade() is implemented, and the one-time alembic stamp head procedure for our existing production DB is documented clearly in docs/database-migrations.md. Nice side effect too — the test fixtures got simpler since we no longer need to monkeypatch DatabaseManager just to build the app.
Two small non-blocking notes for a follow-up:
- The baseline migration sets
scans.statusserver_default to'pending', but the oldALTER TABLEhad it as'completed'. Looks inert today since everyINSERTinfinding.pysetsstatusexplicitly, but worth a comment or fix for consistency so it doesn't bite us later. - Nothing in CI actually runs
alembic upgrade headagainst a real ephemeral Postgres — this was verified manually against Neon. Worth adding a Postgres service container to CI so a broken future migration gets caught before deploy rather than after. Could fold into the CI restructure work (#155).
Neither blocks this. Approving.
Thanks for the review, @m-khan-97. I really appreciate you taking the time to go through it. I agree with both points. The CI migration validation was actually something I had in mind before opening this PR, but I wanted to keep the scope focused on replacing the handwritten schema management with Alembic. I think it would fit well as part of the planned CI improvements in #155. For the scans.status server default, I’m happy to update it in this PR if you’d prefer to keep the baseline fully aligned with the previous schema. Otherwise, I’m equally happy to handle it as a small follow-up. Happy to go with whichever approach you think is best. |
|
Could you please hold on this PR merge once the new CI pipeline is merged to dev and then you can add the ones include followings into the CI steps : Provisions an ephemeral PostgreSQL service Which will save me from the extra work and new PR cycle |
|
@parthrohit22, conflicts to fix....do have a look before merge, @ritiksah141 CI based PR has been merged now, so probably working on this tmrw would be a reasonable idea. |
274ef67 to
05d23b0
Compare
|
Could you please add the things we talked about the CI part in this same PR that would be much helpful |
On It. |
|
Thanks for the feedback. @m-khan-97 - I’ve addressed both review points:
@ritiksah141 - the CI update discussed has now been included in this PR. @Vishnu2707 @m-khan-97 brahim - I’ve rebased onto the latest dev, resolved the merge conflicts, updated the PR with the requested changes, and all CI checks are now passing. Whenever you have time, I’d appreciate another review. |
|
@ritiksah141 , @m-khan-97 , touchbase with this PR pls. |
ritiksah141
left a comment
There was a problem hiding this comment.
I reviewed this end to end. The Alembic direction looks right, and the PR passes its own CI-style checks locally, but I think this needs changes before merge because the branch is stale against current dev and the baseline schema is no longer current.
Findings:
-
api/app.pyandapi/models/finding.pyconflict with currentdev.GitHub reports the PR as
CONFLICTING, and a local merge preview shows conflicts in both files. Currentdevnow includes the observability work from #167 and async scan state recovery from #169. This PR branch is still on the older versions of those files, so a rebase needs to keep:configure_logging(),init_sentry(),init_app(app),/ready,/metrics, and request IDs inapi/app.pyDatabaseManager.ping(),attempt_count, and retry-aware stale scan recovery inapi/models/finding.py
This is blocking because resolving the conflicts by taking the PR branch versions would regress merged behavior.
-
alembic/versions/b3f1a2c4d5e6_baseline_schema.py:23creates a baseline schema that is missingscans.attempt_count.After #169,
attempt_countis part of the scan state model. If this PR is rebased while keeping the latestDatabaseManagercode, fresh databases created withalembic upgrade headwill not have the column that the app expects. Please either addattempt_count INTEGER DEFAULT 0to the baseline if the baseline is meant to represent the current schema, or add a follow-up Alembic revision after the baseline to introduce it. -
alembic/versions/b3f1a2c4d5e6_baseline_schema.py:38defaultsscans.statusto'completed'.The queue semantics use pending scans as work items, and current schema creation on
devdefaults new scan rows to'pending'.create_pending_scan()currently inserts status explicitly, so this is not failing today, but the DB contract is surprising and can silently mark a scan complete if a future insert omits status. I would set the baseline default to'pending'and continue setting completed explicitly in completed-scan writes. -
startup.sh:10runsalembic upgrade headunconditionally, whiledocs/database-migrations.md:32says existing production DBs must be manually stamped first.The docs call this out clearly, but operationally this is still a sharp edge: if this deploy reaches an existing unversioned DB before
alembic stamp head, startup will try to create tables that already exist and the app will fail to boot. Please make sure the PR description includes a deployment checklist or that the rollout is coordinated before merge.
Validation I ran locally on the PR branch:
ruff check .
ruff format --check .
PYTHONPATH=/Users/ritiksah/openshield pytest --cov=. --cov-report=term-missing
DATABASE_URL=postgresql://ci:ci@localhost:5432/ci_db alembic upgrade head --sql
alembic heads
alembic history --verbose
bandit -r api/ scanner/ ai/ -ll
pip-audit -r requirements.txt --ignore-vuln PYSEC-2025-217 --ignore-vuln CVE-2026-1839 --ignore-vuln CVE-2026-4372Results:
ruff, format check, Bandit, and pip-audit passed.- Tests passed:
108 passed, 3 skipped. - Alembic offline SQL generation worked and showed the same schema issues above.
- I did not run a real online migration against Postgres locally because no local Postgres service was running.
5f6419b to
ca9c4a9
Compare
|
Addressed the review feedback from @ritiksah141 during the rebase onto the latest Resolved review items
Validation
I also investigated the failing Container Scan (Trivy) check after rebasing. Findings
+ alembic==1.18.5
To keep this PR focused on Alembic migrations, I haven't mixed unrelated dependency or container fixes into it. @Vishnu2707 - I'm happy to open a separate PR dedicated to investigating and resolving the inherited Trivy/container dependency issues so they can be reviewed independently. |
Summary
This PR replaces OpenShield's handwritten database schema management with Alembic, introducing version-controlled PostgreSQL migrations while preserving the existing persistence layer and application SQL queries.
The application no longer creates or manages database schema at runtime. Database initialization is now handled through Alembic migrations, providing a production-ready migration workflow for future schema evolution.
Additionally, the backend CI workflow now validates database migrations against a fresh PostgreSQL instance before executing the test suite, ensuring future migration issues are detected during CI.
Changes
Database
alembic.ini)alembic/)scansandfindingstablesscript.py.mako)alembic==1.18.5to project dependenciesscans.statusserver default with the historical schema (completed) for consistencyApplication
DatabaseManagerinit_db()as a deprecated warning-only no-op for backwards compatibilityDeployment
Updated
startup.shto execute:before starting the application.
CI
Updated the backend CI workflow to validate database migrations by:
before executing the backend test suite
This ensures new migrations are validated automatically against a clean database during CI.
Documentation
Updated:
README.mdCONTRIBUTING.mddocs/api-render-deploy.mddocs/azure-setup.mdAdded:
docs/database-migrations.mdDocumentation now includes:
Validation
Automated
tests/test_ai_hallucination_guard.py:39tests/test_ai_hallucination_guard.py:54tests/test_ai_hallucination_guard.py:95Manual
Validated against a fresh PostgreSQL database (Neon):
alembic upgrade headsuccessfully created the schemascans,findings, andalembic_versiontables createdheadExisting Deployments
Existing databases require a one-time baseline before future upgrades:
This migration path is documented in
docs/database-migrations.md.Related Issue
Closes #158
Checklist