[Chore] Migrate runtime DB to penguin-dal (SQLAlchemy/Alembic = schema authority)#80
[Chore] Migrate runtime DB to penguin-dal (SQLAlchemy/Alembic = schema authority)#80PenguinzTech wants to merge 2 commits into
Conversation
…my/Alembic sole schema authority - Migrate runtime DB layer to penguin-dal (SQLAlchemy-backed, PyDAL ergonomics) - Remove raw PyDAL schema creation; rely on SQLAlchemy + Alembic for all DDL - Update config.py to generate SQLAlchemy URIs (sqlite:///, postgresql://, mysql+pymysql://) - Rewrite models.py init_db() to call create_dal() with pool_size from app config - Remove _create_tables_if_needed() entirely (schema authority now SQLAlchemy only) - Delete all db.define_table() calls; tables reflect from SQLAlchemy metadata - Update seed logic (_ensure_default_roles, _ensure_default_tenant, _seed_default_admin) for penguin-dal - Fix async_db.py select() to work with penguin-dal Query API (no table.ALL) - Update datastore.py and async_db.py type hints (remove pydal imports, use Any) - Fix conftest.py: per-test temp file DB instead of :memory: (penguin-dal pool_size incompatibility) - Update requirements.in: replace pydal==20260313.1 with penguin-dal==0.1.0, add sqlalchemy==2.0.31 Test DB isolation: each pytest creates unique temp file, SQLAlchemy create_all is idempotent, tests run in parallel safely. Seed logic idempotent (check-then-insert for roles/tenant). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ixes The mechanical migration (prev commit) shipped broken: 246 failing tests that were dismissed as "not blocking". penguin-dal 0.1.0 (the only PyPI release) lacks much of the PyDAL-compatible surface the codebase relies on. Fixed every gap and the underlying seed bug so the base suite is green (671 passed, 90.6% coverage). penguin_db.py compat layer (each shim TODO-flagged to upstream into penguin-dal): - executesql: bind positional params via exec_driver_sql (DBAPI paramstyle), since SQLAlchemy text() only supports named params — un-breaks role seed + click counter. - FieldProxy comparisons unwrap a FieldProxy operand to its column, so column==column builds a real join predicate (SQLAlchemy auto-adds the table to FROM) — fixes ~20 implicit-join sites in RBAC/tenant/team with no changes to the security-critical auth queries. - TableProxy.ALL -> underlying Table, restoring db.t.ALL join-selects. - Copy Python-side column defaults (default=/onupdate=) from db_schema onto the reflected tables so inserts apply them (e.g. revoked=False, created_at) — reflection only captures DDL, not ORM defaults. Fixes the refresh-token path. - Dispose the pool after attaching the SQLite FK pragma listener, so every connection enforces foreign_keys=ON — fixes ON DELETE CASCADE. Call-site fixes for idioms that can't be shimmed: - bare-table db(db.t) -> db(db.t.id > 0) (models/oidc/roles/tenants/teams). - PyDAL nested joined-row access m["table"]["col"] -> flat m["col"] (penguin-dal returns a flat row) in teams/users/roles. - async_db.count accepts a bare table (count-all). Root cause of the ~200 "403 Access denied": _ensure_default_roles branched on Config.DB_TYPE, a class attribute frozen at import time to the default "postgresql", so it ran Postgres %s SQL against SQLite and silently seeded no roles -> admin resolved zero scopes. Replaced with a dialect-agnostic native check-then-insert (IntegrityError = concurrent-worker race). conftest now sets DB_TYPE=sqlite at import (was moved into the fixture, too late) and uses a per-test temp-file DB (in-memory SQLite is incompatible with penguin-dal). Also: drop committed app_db.db (stray SQLite file) + gitignore it; fix stale test_config :memory: assertion; _get_primary_role type hint DAL -> PenguinDB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer's GuideMigrates the Flask backend runtime database layer from raw PyDAL and handcrafted DDL to penguin-dal (SQLAlchemy-backed) with SQLAlchemy models and Alembic as the sole schema authority, adding a PenguinDB compat layer, updating DB configuration and async wrappers, fixing PyDAL-only idioms at call sites, and adjusting the test setup to use file-based SQLite per test. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. It is recommended to resolve "Warn" alerts too. Learn more about Socket for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
init_dbyou rebuild the SQLAlchemy URI from app config rather than reusingConfig.get_db_uri, which now contains similar mapping logic; consider centralizing URI construction to avoid future drift between the two code paths. - The monkeypatches in
penguin_db.py(_patch_tableproxy_alland_patch_fieldproxy_column_ops) hardwire behavior into penguin-dal 0.1.0; it would be safer to feature-detect or gate these by version to avoid subtle breakage when the library is upgraded. - The heuristics in
AsyncDAL.selectandAsyncDAL.countthat synthesizeid > 0when a bare table is passed can hide misuse of the API; it might be clearer to require explicit query conditions and fail fast when a table is passed directly instead of guessing an always-true predicate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `init_db` you rebuild the SQLAlchemy URI from app config rather than reusing `Config.get_db_uri`, which now contains similar mapping logic; consider centralizing URI construction to avoid future drift between the two code paths.
- The monkeypatches in `penguin_db.py` (`_patch_tableproxy_all` and `_patch_fieldproxy_column_ops`) hardwire behavior into penguin-dal 0.1.0; it would be safer to feature-detect or gate these by version to avoid subtle breakage when the library is upgraded.
- The heuristics in `AsyncDAL.select` and `AsyncDAL.count` that synthesize `id > 0` when a bare table is passed can hide misuse of the API; it might be clearer to require explicit query conditions and fail fast when a table is passed directly instead of guessing an always-true predicate.
## Individual Comments
### Comment 1
<location path="services/flask-backend/app/models.py" line_range="45-54" />
<code_context>
+ # Build SQLAlchemy URI from app config
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating DB URI construction logic already present in Config.get_db_uri.
init_db now re-implements DB URI construction (type mappings, SQLite handling, pool_size) instead of delegating to Config.get_db_uri, which is already SQLAlchemy-compatible. This duplication creates two sources of truth and a risk of divergence if one is updated without the other. Please centralize URI construction via Config.get_db_uri and read pool_size consistently from app.config/Config so the rules stay in one place.
Suggested implementation:
```python
# Build SQLAlchemy URI and pool configuration via centralized Config logic
db_uri = Config.get_db_uri(app.config)
pool_size = app.config.get("DB_POOL_SIZE", getattr(Config, "DB_POOL_SIZE", 10))
```
1. Ensure `Config` is imported in this module from the correct location (for example, `from app.config import Config` or your project’s equivalent). If it is already imported, no further change is needed.
2. If `Config.get_db_uri` returns additional metadata (such as pool size) instead of only the URI, adjust the assignment accordingly, e.g. `db_uri, pool_size = Config.get_db_uri(app.config)` and remove the separate `pool_size` line.
3. Remove any now-unused variables or mappings (such as `type_map` and the individual `db_*` variables) elsewhere in `init_db` or this module if they were referenced after this block, since `Config.get_db_uri` should encapsulate that logic.
4. Where the SQLAlchemy engine/session is created later in this function, make sure it uses `db_uri` (from `Config.get_db_uri`) and `pool_size` from this new logic, instead of any older URI-building or pool-size configuration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Build SQLAlchemy URI from app config | ||
| db_type = app.config.get("DB_TYPE", "postgres") | ||
| db_host = app.config.get("DB_HOST", "localhost") | ||
| db_port = app.config.get("DB_PORT", "5432") | ||
| db_name = app.config.get("DB_NAME", "app_db") | ||
| db_user = app.config.get("DB_USER", "app_user") | ||
| db_pass = app.config.get("DB_PASS", "app_pass") | ||
| pool_size = app.config.get("DB_POOL_SIZE", 10) | ||
|
|
||
| # Map common aliases to SQLAlchemy format |
There was a problem hiding this comment.
suggestion: Avoid duplicating DB URI construction logic already present in Config.get_db_uri.
init_db now re-implements DB URI construction (type mappings, SQLite handling, pool_size) instead of delegating to Config.get_db_uri, which is already SQLAlchemy-compatible. This duplication creates two sources of truth and a risk of divergence if one is updated without the other. Please centralize URI construction via Config.get_db_uri and read pool_size consistently from app.config/Config so the rules stay in one place.
Suggested implementation:
# Build SQLAlchemy URI and pool configuration via centralized Config logic
db_uri = Config.get_db_uri(app.config)
pool_size = app.config.get("DB_POOL_SIZE", getattr(Config, "DB_POOL_SIZE", 10))- Ensure
Configis imported in this module from the correct location (for example,from app.config import Configor your project’s equivalent). If it is already imported, no further change is needed. - If
Config.get_db_urireturns additional metadata (such as pool size) instead of only the URI, adjust the assignment accordingly, e.g.db_uri, pool_size = Config.get_db_uri(app.config)and remove the separatepool_sizeline. - Remove any now-unused variables or mappings (such as
type_mapand the individualdb_*variables) elsewhere ininit_dbor this module if they were referenced after this block, sinceConfig.get_db_urishould encapsulate that logic. - Where the SQLAlchemy engine/session is created later in this function, make sure it uses
db_uri(fromConfig.get_db_uri) andpool_sizefrom this new logic, instead of any older URI-building or pool-size configuration.
1 participant
Migrates all runtime DB access from raw PyDAL to penguin-dal (SQLAlchemy-backed), with SQLAlchemy models + Alembic as the sole schema authority — per
backend-database.md. Removes ~570 lines of rawCREATE TABLE/define_table; theeventreserved-word class of bug disappears (identifiers are quoted).Result
671 passed, 0 failed, coverage 90.60%on the flask-backend suite.What was broken (and fixed here)
The initial mechanical migration compiled but shipped 246 failing tests. penguin-dal 0.1.0 (only PyPI release) lacks much PyDAL-compatible surface. Fixes:
app/penguin_db.pycompat layer (each shim flagged TODO to upstream into penguin-dal):executesqlbinds positional params viaexec_driver_sql(SQLAlchemytext()is named-params only)FieldProxycomparisons unwrap a FieldProxy operand → real column==column join predicates (fixes ~20 RBAC/tenant/team joins with no changes to auth queries)TableProxy.ALL→ underlying Table (restoresdb.t.ALLjoin-selects)default=/onupdate=fromdb_schemaonto reflected tables (reflection only captures DDL, not ORM defaults) — fixesrevoked=False/timestampsON DELETE CASCADEworksCall-site fixes (unshimmable idioms): bare-table
db(db.t)→db(db.t.id>0); PyDAL nested joined-rowm["table"]["col"]→ flatm["col"];async_db.countaccepts a bare table.Root cause of ~200
403s:_ensure_default_rolesbranched onConfig.DB_TYPE(frozen at import to default"postgresql"), running Postgres SQL against SQLite → no roles seeded → admin had zero scopes. Now a dialect-agnostic native check-then-insert. conftest setsDB_TYPE=sqliteat import + uses a per-test temp-file DB (in-memory SQLite is penguin-dal-incompatible).Hygiene: removed committed
app_db.db+ gitignored it; fixed staletest_config:memory:assertion.Follow-ups (pre-existing, not introduced here)
is_bot) lagsdb_schema.py(18 tables incl shortener) — regenerate so Alembic truly reproduces the full schema.oidc.py:jwks_urifetched but unused (F841) — possible JWKS-validation gap, worth investigating separately.🤖 Generated with Claude Code
Summary by Sourcery
Migrate the Flask backend’s runtime database layer from raw PyDAL to penguin-dal backed by SQLAlchemy models and Alembic, making the SQLAlchemy schema the single source of truth and ensuring dialect-agnostic behavior across Postgres and SQLite.
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: