feat: add database migration support and connection utilities#17
feat: add database migration support and connection utilities#17
Conversation
Add Alembic for database schema migrations and centralize database connection logic. ## Changes ### Database Migration (Alembic) - Add alembic.ini configuration file * Script location: alembic/ * File template: %%%(rev)s_%%%(slug)s * Timezone support: UTC - Add alembic/env.py migration environment * Async SQLAlchemy support * TODO: Connect target_metadata to models for autogenerate - Add alembic/script.py.mako migration template * Standard Alembic revision template ### Database Connection Module (app/database.py) - Add get_database_url() utility function * Centralized URL construction logic (DRY principle) * URL-encodes credentials to handle special characters * Supports DATABASE_URL env var or individual components * Security: Prevents credential leak in error messages ### Application Updates (app/main.py) - Refactor to use shared get_database_url() function - Remove duplicate database URL construction code - Improve error message clarity without exposing credentials ## Security Improvements - URL-encode database credentials (urllib.parse.quote_plus) - Prevent password exposure in error messages - Secure connection string handling ## Benefits - Consistent database URL handling across app - Support for passwords with special characters - Easy to test and maintain (single source of truth) - Ready for schema version control ## Usage ```bash # Create a new migration alembic revision -m "description" # Apply migrations alembic upgrade head # Rollback migrations alembic downgrade -1 ``` ## TODO - Implement lazy database initialization - Add environment variable support in alembic.ini - Connect target_metadata for autogenerate feature ## Files Changed (5 files) - alembic.ini: Alembic configuration - alembic/env.py: Migration environment setup - alembic/script.py.mako: Migration template - app/database.py: NEW - Database connection utilities - app/main.py: Refactored to use shared database module
There was a problem hiding this comment.
Pull request overview
Adds initial database migration scaffolding (Alembic) and centralizes database connection URL construction to support consistent DB configuration across the app.
Changes:
- Introduces Alembic configuration + environment + revision template files.
- Adds
app/database.pywithget_database_url()and SQLAlchemy async engine/session setup. - Refactors
app/main.pyto use the shared DB URL utility and updated service initialization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
app/main.py |
Uses centralized DB URL utility and updates app/service initialization; adjusts storage env var. |
app/database.py |
New DB utilities + SQLAlchemy async engine/session factory. |
alembic.ini |
Adds Alembic configuration scaffold (currently missing a usable URL configuration). |
alembic/env.py |
Adds Alembic environment script (currently sync engine setup and expects sqlalchemy.url). |
alembic/script.py.mako |
Adds Alembic revision file template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix DATABASE_PORT default from 54320 to standard 5432 - Update alembic/env.py to read DATABASE_URL from environment variables - Use sync engine in alembic for migrations (not async) - Add type assertions for mypy compatibility - Ignore DEP003 for app package in deptry config
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove unused logger import in app/database.py - Add URL encoding for credentials in alembic/env.py - Convert async DB URL to sync for Alembic migrations
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Normalize PostgreSQL URL schemes in app/database.py and alembic/env.py - Handle postgres://, postgresql://, postgresql+asyncpg:// formats - Convert all to postgresql+psycopg:// for consistency - Add URL encoding for credentials in alembic/env.py - Update alembic.ini comments to reflect actual implementation - Add backward compatibility for MEMU_STORAGE_DIR env var in app/main.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use quote(..., safe='') instead of quote_plus() for URL userinfo encoding (quote_plus encodes spaces as '+' which is incorrect for URL userinfo) - Add asyncpg to psycopg conversion in app/database.py (consistent with alembic/env.py, asyncpg is not a project dependency)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…erate - Create app/models/base.py with Base class (no DB connection on import) - Update app/database.py to import Base from app.models.base - Update alembic/env.py to set target_metadata = Base.metadata - This enables 'alembic revision --autogenerate' to detect model changes
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SQLAlchemy 2.x async_sessionmaker does not support the autocommit argument. Use explicit transaction scopes instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace 'in' operator with startswith() to avoid false positives when checking for asyncpg driver in database URLs. This ensures only URLs that actually start with postgresql+asyncpg:// are converted.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| db_host = os.getenv("DATABASE_HOST", "localhost") | ||
| db_port = os.getenv("DATABASE_PORT", "5432") | ||
| db_user = os.getenv("DATABASE_USER", "postgres") | ||
| db_pass = os.getenv("DATABASE_PASSWORD", "postgres") | ||
| db_name = os.getenv("DATABASE_NAME", "memu") |
There was a problem hiding this comment.
In get_sync_database_url(), the individual variable fallback uses hard-coded defaults (localhost/postgres/postgres/memu). This contradicts the PR description’s env var table (no defaults for host/user/password/name) and can cause Alembic to run against an unintended database when env vars are missing. Consider matching app.database.get_database_url() behavior (raise with a clear error) or document these defaults explicitly and ensure they’re safe.
Remove hard-coded defaults from get_sync_database_url() and add explicit validation for required environment variables. This ensures consistent behavior between Alembic migrations and the application, preventing accidental connections to unintended databases.
Summary
Add database configuration and Alembic migration setup for the memU server.
Changes
Database Configuration (
app/database.py)get_database_url()function to construct database URL from environment variablesDATABASE_URLdirect config and individual variablesurllib.parse.quote(..., safe="")for proper userinfo encodingpostgres://,postgresql://,postgresql+asyncpg://) topostgresql+psycopg://Model Base Class (
app/models/base.py)alembic/env.pywithout triggering DB connectionAlembic Migration Setup
alembic.iniconfiguration filealembic/env.pywith environment variable supporttarget_metadata = Base.metadatafor autogenerate supportpostgresql+psycopg://for migrationsalembic/versions/.gitkeepto preserve directory in gitEnvironment Variables
DATABASE_URLDATABASE_HOSTDATABASE_PORT5432DATABASE_USERDATABASE_PASSWORDDATABASE_NAMESTORAGE_PATH./dataMEMU_STORAGE_DIR)Known Limitations
The following are intentionally deferred to a future refactoring PR:
Lazy initialization -
DATABASE_URLand engine are created at import time. This is marked with TODO comments for future optimization.OPENAI_API_KEY validation at import time - Validation runs at module import. Moving to FastAPI lifespan is planned.
Testing
make checkpasses (lint, type check, dependency check)make testpasses