Skip to content

feat(db): implement Alembic migrations #164

Open
parthrohit22 wants to merge 2 commits into
openshield-org:devfrom
parthrohit22:infra/alembic-migrations
Open

feat(db): implement Alembic migrations #164
parthrohit22 wants to merge 2 commits into
openshield-org:devfrom
parthrohit22:infra/alembic-migrations

Conversation

@parthrohit22

@parthrohit22 parthrohit22 commented Jul 5, 2026

Copy link
Copy Markdown
Member

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

  • Added Alembic configuration (alembic.ini)
  • Added Alembic migration environment (alembic/)
  • Added a baseline migration for the scans and findings tables
  • Added the Alembic migration template (script.py.mako)
  • Added alembic==1.18.5 to project dependencies
  • Aligned the baseline scans.status server default with the historical schema (completed) for consistency

Application

  • Removed runtime schema creation from DatabaseManager
  • Removed schema management during Flask application startup
  • Kept init_db() as a deprecated warning-only no-op for backwards compatibility
  • Preserved all existing persistence logic and SQL queries

Deployment

Updated startup.sh to execute:

alembic upgrade head

before starting the application.

CI

Updated the backend CI workflow to validate database migrations by:

  • Provisioning a PostgreSQL service
  • Running:
alembic upgrade head

before executing the backend test suite

This ensures new migrations are validated automatically against a clean database during CI.

Documentation

Updated:

  • README.md
  • CONTRIBUTING.md
  • docs/api-render-deploy.md
  • docs/azure-setup.md

Added:

  • docs/database-migrations.md

Documentation now includes:

  • Local migration workflow
  • Contributor guidance
  • Production deployment process
  • Migration naming conventions
  • One-time production upgrade procedure

Validation

Automated

  • ✅ 108 tests passed
  • ⏭️ 3 AI hallucination guard tests skipped (expected) because the local vector store was not present:
    • tests/test_ai_hallucination_guard.py:39
    • tests/test_ai_hallucination_guard.py:54
    • tests/test_ai_hallucination_guard.py:95
  • ✅ Alembic SQL generation verified
  • ✅ Backend CI updated to execute Alembic migrations before running the test suite
  • ✅ Startup validation completed

Manual

Validated against a fresh PostgreSQL database (Neon):

  • alembic upgrade head successfully created the schema
  • scans, findings, and alembic_version tables created
  • ✅ Alembic revision recorded correctly
  • ✅ Primary key and foreign key relationships verified
  • ✅ Migration safely re-runs when already at head

Existing Deployments

Existing databases require a one-time baseline before future upgrades:

alembic stamp head

This migration path is documented in docs/database-migrations.md.


Related Issue

Closes #158


Checklist

  • Replaced handwritten schema management with Alembic migrations
  • Preserved existing persistence logic and SQL queries
  • Removed runtime schema creation from the application
  • Updated deployment workflow
  • Added baseline migration and Alembic configuration
  • Added backend CI migration validation against PostgreSQL
  • Aligned the baseline schema with the historical database default
  • Updated contributor and deployment documentation
  • Validated migrations against a real PostgreSQL database
  • Existing automated test suite passes

@parthrohit22 parthrohit22 self-assigned this Jul 5, 2026
@parthrohit22 parthrohit22 changed the title feat(db): implement Alembic migrations (#158) feat(db): implement Alembic migrations Jul 5, 2026
m-khan-97
m-khan-97 previously approved these changes Jul 5, 2026

@m-khan-97 m-khan-97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.status server_default to 'pending', but the old ALTER TABLE had it as 'completed'. Looks inert today since every INSERT in finding.py sets status explicitly, but worth a comment or fix for consistency so it doesn't bite us later.
  • Nothing in CI actually runs alembic upgrade head against 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.

@parthrohit22

Copy link
Copy Markdown
Member Author

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.status server_default to 'pending', but the old ALTER TABLE had it as 'completed'. Looks inert today since every INSERT in finding.py sets status explicitly, but worth a comment or fix for consistency so it doesn't bite us later.
  • Nothing in CI actually runs alembic upgrade head against 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 ([INFRA 2] Restructure CI: parallel jobs, lint/format gate, coverage reporting, frontend build check #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.

@ritiksah141

Copy link
Copy Markdown
Collaborator

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
Runs alembic upgrade head
Executes the test suite
Fails the workflow if the migration doesn’t apply cleanly

Which will save me from the extra work and new PR cycle

@Vishnu2707

Copy link
Copy Markdown
Member

@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.

@ritiksah141

Copy link
Copy Markdown
Collaborator

Could you please add the things we talked about the CI part in this same PR that would be much helpful

@parthrohit22

Copy link
Copy Markdown
Member Author

Could you please add the things we talked about the CI part in this same PR that would be much helpful

On It.

@parthrohit22

Copy link
Copy Markdown
Member Author

Thanks for the feedback.

@m-khan-97 - I’ve addressed both review points:

  • aligned the Alembic baseline scans.status server default with the historical schema (completed) for consistency;
  • added PostgreSQL migration validation to the backend CI by running alembic upgrade head before the test suite.

@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.

@Vishnu2707

Copy link
Copy Markdown
Member

@ritiksah141 , @m-khan-97 , touchbase with this PR pls.

@ritiksah141 ritiksah141 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. api/app.py and api/models/finding.py conflict with current dev.

    GitHub reports the PR as CONFLICTING, and a local merge preview shows conflicts in both files. Current dev now 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 in api/app.py
    • DatabaseManager.ping(), attempt_count, and retry-aware stale scan recovery in api/models/finding.py

    This is blocking because resolving the conflicts by taking the PR branch versions would regress merged behavior.

  2. alembic/versions/b3f1a2c4d5e6_baseline_schema.py:23 creates a baseline schema that is missing scans.attempt_count.

    After #169, attempt_count is part of the scan state model. If this PR is rebased while keeping the latest DatabaseManager code, fresh databases created with alembic upgrade head will not have the column that the app expects. Please either add attempt_count INTEGER DEFAULT 0 to 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.

  3. alembic/versions/b3f1a2c4d5e6_baseline_schema.py:38 defaults scans.status to 'completed'.

    The queue semantics use pending scans as work items, and current schema creation on dev defaults 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.

  4. startup.sh:10 runs alembic upgrade head unconditionally, while docs/database-migrations.md:32 says 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-4372

Results:

  • 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.

@parthrohit22 parthrohit22 force-pushed the infra/alembic-migrations branch from 5f6419b to ca9c4a9 Compare July 8, 2026 10:48
@parthrohit22

Copy link
Copy Markdown
Member Author

Addressed the review feedback from @ritiksah141 during the rebase onto the latest upstream/dev.

Resolved review items

  • Preserved the latest observability changes (/ready, /metrics, request IDs, logging, Sentry initialization).

  • Preserved the retry-aware scan recovery logic and attempt_count support.

  • Resolved merge conflicts in api/app.py and api/models/finding.py.

  • Updated the Alembic baseline to match the current application behaviour:

    • status default is pending
    • attempt_count default is 0
  • Added deployment guidance for existing databases (alembic stamp head).

  • Verified that Alembic remains the schema owner and removed the previous runtime migration logic from the application startup.

Validation

  • ruff check .

  • ruff format --check .

  • pytest143 passed, 3 skipped

  • ✅ Verified Alembic offline SQL generation

  • ✅ Confirmed the generated schema contains:

    • status DEFAULT 'pending'
    • attempt_count INTEGER DEFAULT 0

I also investigated the failing Container Scan (Trivy) check after rebasing.

Findings

  • The same Trivy container scan already fails on the current upstream/dev branch.
  • This PR changes only one runtime dependency:
+ alembic==1.18.5
  • The reported findings (jaraco.context, wheel, and transformers) are inherited from the current dependency/container state and are not introduced by the Alembic migration changes.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[INFRA 5] Adopt Alembic for database schema migrations

4 participants