fix(template): wire async DB layer + Alembic and make generated projects gate-clean#96
fix(template): wire async DB layer + Alembic and make generated projects gate-clean#96williaby wants to merge 2 commits into
Conversation
- middleware/security.py + correlation.py: type call_next as RequestResponseEndpoint, use del for MutableHeaders, annotate list[str], drop redundant isinstance, collapse implicit string concat. - api/health.py: ReadinessCheck uses Field(default=None) so basedpyright does not treat optional fields as required. - tests/test_example.py: gate TestCLI behind include_cli==yes so disabling the CLI no longer leaves tests importing the removed cli.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ects gate-clean Generated projects with API + database selections started red (missing DB module, no driver, and a pydoclint config stricter than the shipped Google docstrings). This makes the default generations pass their own gates. Database: - New core/database.py (async engine, async_sessionmaker, Base, get_session) shipped when include_database != none; removed by post-gen otherwise. - database_url added to core/config.py with a dialect-correct default URL. - New 'database' optional-dependency group gated on include_database, with the async driver per dialect (asyncpg/aiomysql/aiosqlite) and alembic for the migration variants; sqlalchemy/alembic removed from the unconditional api extra. - health.py readiness check uses SQLAlchemy 2.0 text(). - Minimal async Alembic scaffold (alembic.ini, migrations/env.py, script.py.mako, versions/.gitkeep) for sqlalchemy_migrations/_ledger; *.mako added to _copy_without_render; migrations/ excluded from ruff/pydoclint/ interrogate; post-gen removes the scaffold for non-migration choices. Docstrings / gates: - Brought all shipped src docstrings into strict pydoclint compliance (typed Args/Returns, removed __init__ docstrings in favor of class Args, documented class Attributes with types) across config, exceptions, health, logging, security, correlation. - ruff force-exclude=true so excluded dirs are honored under pre-commit. Verified across 3 combos (no-api/no-db; api+sqlalchemy+sqlite+cli; api+migrations+postgres): pydoclint 0, ruff clean, basedpyright 0 errors, tests pass, and pre-commit green except the pre-existing end-of-file-fixer auto-fix on 3 files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughAdds async SQLAlchemy + Alembic migration scaffolding as a conditional cookiecutter feature: new ChangesDatabase/Alembic template feature
Middleware type annotations and docstring polish
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def _cleanup_database_files() -> None: | ||
| """Remove database and migration files based on cookiecutter choices.""" | ||
| # No database selected: drop the engine/session module entirely. | ||
| if "{{ cookiecutter.include_database }}" == "none": |
| def _cleanup_database_files() -> None: | ||
| """Remove database and migration files based on cookiecutter choices.""" | ||
| # No database selected: drop the engine/session module entirely. | ||
| if "{{ cookiecutter.include_database }}" == "none": |
| if "{{ cookiecutter.include_database }}" not in ( | ||
| "sqlalchemy_migrations", | ||
| "sqlalchemy_ledger", | ||
| ): |
| if "{{ cookiecutter.include_database }}" not in ( | ||
| "sqlalchemy_migrations", | ||
| "sqlalchemy_ledger", | ||
| ): |
There was a problem hiding this comment.
Pull request overview
This PR updates the cookiecutter template so generated projects (especially API + DB combinations) include a properly wired async SQLAlchemy layer and (optionally) an Alembic scaffold, while also tightening template output to pass the repo’s “quality gates” (ruff/pydoclint/pyright/tests) across common flag combinations.
Changes:
- Adds an async SQLAlchemy database module and threads a dialect-correct
database_urlthrough settings, with optional-dependency wiring for drivers + Alembic. - Introduces an Alembic migration scaffold that is conditionally kept/removed by the post-gen hook, and excludes migrations from lint/doc tooling.
- Brings multiple docstrings and middleware type hints into strict pydoclint compliance and gates CLI tests behind
include_cli.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hooks/post_gen_project.py | Adds post-gen cleanup for DB/migrations scaffolding based on cookiecutter flags. |
| cookiecutter.json | Adds *.mako to _copy_without_render for Alembic templates. |
| {{cookiecutter.project_slug}}/tests/test_example.py | Wraps TestCLI in a cookiecutter conditional so tests don’t import removed CLI code. |
| {{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/utils/logging.py | Docstring tightening for pydoclint (but see stored bug comment). |
| {{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/middleware/security.py | Type-hints middleware endpoints more strictly; adjusts header deletion semantics. |
| {{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/middleware/correlation.py | Tightens typing and docstrings for correlation middleware utilities. |
| {{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/exceptions.py | Updates docstrings to strict pydoclint format across exception hierarchy. |
| {{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/database.py | Introduces async engine/session plumbing for DB-enabled generations. |
| {{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/config.py | Adds conditional database_url setting with dialect-specific defaults. |
| {{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/api/health.py | Uses SQLAlchemy text() for the readiness DB probe. |
| {{cookiecutter.project_slug}}/pyproject.toml | Moves DB deps into a database optional group; excludes migrations from tooling; sets ruff.force-exclude=true. |
| {{cookiecutter.project_slug}}/migrations/versions/.gitkeep | Ensures versions directory exists for Alembic scaffold. |
| {{cookiecutter.project_slug}}/migrations/script.py.mako | Adds Alembic revision template. |
| {{cookiecutter.project_slug}}/migrations/env.py | Adds async Alembic env wired to app settings + async engine. |
| {{cookiecutter.project_slug}}/alembic.ini | Adds Alembic config scaffold (URL injected at runtime). |
| {{cookiecutter.project_slug}}/.pre-commit-config.yaml | Excludes migrations from pydoclint hook runs. |
| Args: | ||
| level: Logging level (DEBUG, INFO, WARNING, ERROR, CRITICAL). | ||
| level (str): Logging level (DEBUG, INFO, WARNING, ERROR, CRITICAL). | ||
| Defaults to INFO. | ||
| json_logs: If True, output JSON logs (production mode). If False, | ||
| json_logs (bool): If True, output JSON logs (production mode). If False, | ||
| use rich console formatting (development mode). Defaults to False. | ||
| include_timestamp: Whether to include timestamps in log output. | ||
| include_timestamp (bool): Whether to include timestamps in log output. | ||
| Defaults to True. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @{{cookiecutter.project_slug}}/migrations/env.py:
- Around line 24-26: Add RAD (Risky Assumptions Documentation) tags to the
migration environment setup in env.py to document critical assumptions that
could cause production failures. Before or near the lines where
config.set_main_option is called with settings.database_url and where
target_metadata is assigned from Base.metadata, add comments that clearly tag
the three assumptions: that settings.database_url is valid and accessible at
import time, that Base.metadata contains all models because all model modules
have been imported, and that the async database driver is installed and
compatible with the configuration.
- Around line 52-57: In the run_migrations_online() function, remove the await
connectable.dispose() call at the end. The engine returned by get_engine() is a
shared singleton from core.database, and disposing it closes the connection pool
globally, breaking the application's ability to perform database operations
after migrations run. The async context manager for connectable.connect()
already handles resource cleanup automatically, so explicit disposal is
unnecessary and harmful.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/api/health.py:
- Around line 92-103: The check_database function makes several assumptions
about database availability that could cause production failures if not met, but
these assumptions are not documented with RAD tags. Add RAD tags to the
check_database function to document the assumptions that: the core.database
module exists (which is only true when include_database is not set to "none"),
the database session can be successfully created, and the database URL is valid.
These tags should be placed at the beginning of the function to document the
contract and help with debugging if assumptions are violated.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/config.py:
- Around line 36-48: The database_url variable assignments for the different
database dialects (postgresql, mysql, sqlite) contain production-impacting
assumptions such as hardcoded localhost addresses, default credentials, and
local file paths, but are missing the required assumption annotation tags per
coding guidelines. Add inline `#ASSUME` or `#CRITICAL` tags to each database_url
assignment to explicitly document these development-specific assumptions and
make the production risks reviewable.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/database.py:
- Around line 50-57: The get_session() function directly returns
_session_factory() without error handling, which defers any engine
initialization failures until the session is actually used, making debugging
harder. Wrap the _session_factory() call in a try-except block to catch any
exceptions that may occur due to misconfiguration or uninitialized engine, and
raise or log a clear error message that indicates the database engine may not
have been properly initialized, providing better error context at the point of
session creation rather than later during session usage.
- Around line 30-33: The module-level _engine initialization using
create_async_engine() contains critical assumptions that could cause unclear
import-time failures but lacks documentation. Add RAD
(Risk/Assumption/Dependency) tags as comments above or within the _engine
creation block to explicitly document the three critical assumptions: that
settings.database_url is valid and accessible at import time, that the
configured async database driver (asyncpg/aiomysql/aiosqlite) is installed, and
that the URL format matches the expected dialect. These tags should clearly
indicate that if any assumption fails, the entire module import will fail with
an unclear exception, helping future maintainers understand the critical nature
of this initialization.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/middleware/security.py:
- Around line 413-414: The SSRF validation logic in the middleware is
incomplete. While the docstring claims to validate query parameters, form data,
and JSON body, the actual implementation only scans query parameters (as
indicated in lines 425-427). Extend the validation logic to also inspect form
data and JSON request bodies for potential SSRF attempts targeting internal
resources, ensuring that URL-bearing fields in all three input sources are
properly scanned before allowing the request to proceed.
In `@hooks/post_gen_project.py`:
- Around line 327-330: The templated variable include_database is being used
directly in the membership check at lines 327-330, causing static analysis
failures. Extract the templated include_database variable into a normalized
constant at the module level (following the pattern of other include flags),
then replace the direct template string "{{ cookiecutter.include_database }}" in
the conditional statement with the new constant. Apply the same refactoring to
the similar comparison at line 323 to maintain consistency throughout the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68458b08-ce48-4398-a335-b9e9a1a001a3
📒 Files selected for processing (16)
cookiecutter.jsonhooks/post_gen_project.py{{cookiecutter.project_slug}}/.pre-commit-config.yaml{{cookiecutter.project_slug}}/alembic.ini{{cookiecutter.project_slug}}/migrations/env.py{{cookiecutter.project_slug}}/migrations/script.py.mako{{cookiecutter.project_slug}}/migrations/versions/.gitkeep{{cookiecutter.project_slug}}/pyproject.toml{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/api/health.py{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/config.py{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/database.py{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/exceptions.py{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/middleware/correlation.py{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/middleware/security.py{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/utils/logging.py{{cookiecutter.project_slug}}/tests/test_example.py
| # Inject the application's database URL and model metadata. | ||
| config.set_main_option("sqlalchemy.url", settings.database_url) | ||
| target_metadata = Base.metadata |
There was a problem hiding this comment.
Add RAD tags for critical assumptions in migration environment.
Per coding guidelines, tag assumptions that could cause production failures. The migration environment assumes:
settings.database_urlis valid and accessible at import timeBase.metadatacontains all models (all model modules have been imported)- The async database driver is installed and compatible
🏷️ Proposed RAD tag additions
# Inject the application's database URL and model metadata.
+# `#CRITICAL`: configuration: settings.database_url must be valid at migration time
+# `#CRITICAL`: metadata: Base.metadata must include all ORM models
config.set_main_option("sqlalchemy.url", settings.database_url)
target_metadata = Base.metadata🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @{{cookiecutter.project_slug}}/migrations/env.py around lines 24 - 26, Add
RAD (Risky Assumptions Documentation) tags to the migration environment setup in
env.py to document critical assumptions that could cause production failures.
Before or near the lines where config.set_main_option is called with
settings.database_url and where target_metadata is assigned from Base.metadata,
add comments that clearly tag the three assumptions: that settings.database_url
is valid and accessible at import time, that Base.metadata contains all models
because all model modules have been imported, and that the async database driver
is installed and compatible with the configuration.
Source: Coding guidelines
| async def run_migrations_online() -> None: | ||
| """Run migrations in 'online' mode using the application's async engine.""" | ||
| connectable = get_engine() | ||
| async with connectable.connect() as connection: | ||
| await connection.run_sync(do_run_migrations) | ||
| await connectable.dispose() |
There was a problem hiding this comment.
Do not dispose the shared application engine after migrations.
get_engine() returns the singleton _engine from core.database, which is shared across the entire application. Calling await connectable.dispose() at line 57 closes all pooled connections and renders the engine unusable. If the application attempts database operations after running migrations programmatically, it will fail with connection errors.
🛡️ Recommended fix: remove engine disposal
async def run_migrations_online() -> None:
"""Run migrations in 'online' mode using the application's async engine."""
connectable = get_engine()
async with connectable.connect() as connection:
await connection.run_sync(do_run_migrations)
- await connectable.dispose()The connection context manager handles cleanup automatically. The engine remains available for subsequent application use.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def run_migrations_online() -> None: | |
| """Run migrations in 'online' mode using the application's async engine.""" | |
| connectable = get_engine() | |
| async with connectable.connect() as connection: | |
| await connection.run_sync(do_run_migrations) | |
| await connectable.dispose() | |
| async def run_migrations_online() -> None: | |
| """Run migrations in 'online' mode using the application's async engine.""" | |
| connectable = get_engine() | |
| async with connectable.connect() as connection: | |
| await connection.run_sync(do_run_migrations) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @{{cookiecutter.project_slug}}/migrations/env.py around lines 52 - 57, In the
run_migrations_online() function, remove the await connectable.dispose() call at
the end. The engine returned by get_engine() is a shared singleton from
core.database, and disposing it closes the connection pool globally, breaking
the application's ability to perform database operations after migrations run.
The async context manager for connectable.connect() already handles resource
cleanup automatically, so explicit disposal is unnecessary and harmful.
| ReadinessCheck: Database status and measured latency. | ||
| """ | ||
| start = time.time() | ||
| try: | ||
| # Import here to avoid circular dependencies | ||
| from sqlalchemy import text | ||
|
|
||
| from {{ cookiecutter.project_slug }}.core.database import get_session | ||
|
|
||
| async with get_session() as session: | ||
| # Simple query to check connectivity | ||
| await session.execute("SELECT 1") | ||
| await session.execute(text("SELECT 1")) |
There was a problem hiding this comment.
Add RAD tags for database availability assumption.
Per coding guidelines, tag assumptions that could cause production failures. The check_database function assumes:
- The
core.databasemodule exists (only true wheninclude_database != "none") - The database session can be created
- The database URL is valid
While template conditionals should prevent this code from being generated without database support, tagging the assumption documents the contract.
🏷️ Proposed RAD tag addition
start = time.time()
try:
+ # `#CRITICAL`: dependencies: core.database module must be available
# Import here to avoid circular dependencies
from sqlalchemy import text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ReadinessCheck: Database status and measured latency. | |
| """ | |
| start = time.time() | |
| try: | |
| # Import here to avoid circular dependencies | |
| from sqlalchemy import text | |
| from {{ cookiecutter.project_slug }}.core.database import get_session | |
| async with get_session() as session: | |
| # Simple query to check connectivity | |
| await session.execute("SELECT 1") | |
| await session.execute(text("SELECT 1")) | |
| ReadinessCheck: Database status and measured latency. | |
| """ | |
| start = time.time() | |
| try: | |
| # `#CRITICAL`: dependencies: core.database module must be available | |
| # Import here to avoid circular dependencies | |
| from sqlalchemy import text | |
| from {{ cookiecutter.project_slug }}.core.database import get_session | |
| async with get_session() as session: | |
| # Simple query to check connectivity | |
| await session.execute(text("SELECT 1")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/api/health.py
around lines 92 - 103, The check_database function makes several assumptions
about database availability that could cause production failures if not met, but
these assumptions are not documented with RAD tags. Add RAD tags to the
check_database function to document the assumptions that: the core.database
module exists (which is only true when include_database is not set to "none"),
the database session can be successfully created, and the database URL is valid.
These tags should be placed at the beginning of the function to document the
contract and help with debugging if assumptions are violated.
Source: Coding guidelines
| {%- if cookiecutter.include_database != "none" %} | ||
| {%- if cookiecutter.database_dialect == "postgresql" %} | ||
| database_url: str = ( | ||
| "postgresql+asyncpg://postgres:postgres@localhost:5432/{{ cookiecutter.project_slug }}" | ||
| ) | ||
| {%- elif cookiecutter.database_dialect == "mysql" %} | ||
| database_url: str = ( | ||
| "mysql+aiomysql://root:root@localhost:3306/{{ cookiecutter.project_slug }}" | ||
| ) | ||
| {%- else %} | ||
| database_url: str = "sqlite+aiosqlite:///./{{ cookiecutter.project_slug }}.db" | ||
| {%- endif %} | ||
| {%- endif %} |
There was a problem hiding this comment.
Add required assumption tags for dialect-specific default DB URLs.
These defaults encode production-impacting assumptions (localhost, default creds, local file DB) but the required assumption annotations are missing. Please tag them inline per the repo rule so risk is explicit and reviewable.
Suggested patch
{%- if cookiecutter.include_database != "none" %}
{%- if cookiecutter.database_dialect == "postgresql" %}
+ `#CRITICAL`: configuration: assumes local PostgreSQL with default credentials;
+ `#VERIFY`: require environment override for production deployments
database_url: str = (
"postgresql+asyncpg://postgres:postgres@localhost:5432/{{ cookiecutter.project_slug }}"
)
{%- elif cookiecutter.database_dialect == "mysql" %}
+ `#CRITICAL`: configuration: assumes local MySQL with default credentials;
+ `#VERIFY`: require environment override for production deployments
database_url: str = (
"mysql+aiomysql://root:root@localhost:3306/{{ cookiecutter.project_slug }}"
)
{%- else %}
+ `#ASSUME`: configuration: assumes writable local filesystem path for SQLite
+ `#VERIFY`: validate deployment path/permissions or override via env
database_url: str = "sqlite+aiosqlite:///./{{ cookiecutter.project_slug }}.db"
{%- endif %}
{%- endif %}As per coding guidelines, "**/*.py: Tag assumptions that could cause production failures ... Tag non-critical assumptions ... Tag edge cases ... with corresponding #CRITICAL/#ASSUME/#VERIFY/#EDGE comments."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/config.py
around lines 36 - 48, The database_url variable assignments for the different
database dialects (postgresql, mysql, sqlite) contain production-impacting
assumptions such as hardcoded localhost addresses, default credentials, and
local file paths, but are missing the required assumption annotation tags per
coding guidelines. Add inline `#ASSUME` or `#CRITICAL` tags to each database_url
assignment to explicitly document these development-specific assumptions and
make the production risks reviewable.
Source: Coding guidelines
| _engine: AsyncEngine = create_async_engine( | ||
| settings.database_url, | ||
| pool_pre_ping=True, | ||
| ) |
There was a problem hiding this comment.
Add RAD tags for critical assumptions in engine initialization.
Per coding guidelines, tag critical assumptions that could cause production failures. The module-level engine creation assumes:
settings.database_urlis valid and accessible at import time- The configured async driver (asyncpg/aiomysql/aiosqlite) is installed
- The URL format matches the expected dialect
If any assumption fails, the import will raise an unclear exception before the application starts.
🏷️ Proposed RAD tag additions
+# `#CRITICAL`: initialization: settings.database_url must be valid at import time
+# `#CRITICAL`: dependencies: async driver (asyncpg/aiomysql/aiosqlite) must be installed
_engine: AsyncEngine = create_async_engine(
settings.database_url,
pool_pre_ping=True,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _engine: AsyncEngine = create_async_engine( | |
| settings.database_url, | |
| pool_pre_ping=True, | |
| ) | |
| # `#CRITICAL`: initialization: settings.database_url must be valid at import time | |
| # `#CRITICAL`: dependencies: async driver (asyncpg/aiomysql/aiosqlite) must be installed | |
| _engine: AsyncEngine = create_async_engine( | |
| settings.database_url, | |
| pool_pre_ping=True, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/database.py
around lines 30 - 33, The module-level _engine initialization using
create_async_engine() contains critical assumptions that could cause unclear
import-time failures but lacks documentation. Add RAD
(Risk/Assumption/Dependency) tags as comments above or within the _engine
creation block to explicitly document the three critical assumptions: that
settings.database_url is valid and accessible at import time, that the
configured async database driver (asyncpg/aiomysql/aiosqlite) is installed, and
that the URL format matches the expected dialect. These tags should clearly
indicate that if any assumption fails, the entire module import will fail with
an unclear exception, helping future maintainers understand the critical nature
of this initialization.
Source: Coding guidelines
| def get_session() -> AsyncSession: | ||
| """Return a new async session for use as an async context manager. | ||
|
|
||
| Returns: | ||
| AsyncSession: A new session bound to the shared engine. Use it as | ||
| ``async with get_session() as session: ...``. | ||
| """ | ||
| return _session_factory() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider wrapping session factory call for better error context.
get_session() directly returns _session_factory(), which will fail at session usage time if the engine was never properly initialized. While the docstring correctly instructs users to use it as a context manager, adding a minimal try-except could provide clearer error messages for misconfiguration scenarios.
♻️ Optional defensive wrapper
def get_session() -> AsyncSession:
"""Return a new async session for use as an async context manager.
Returns:
AsyncSession: A new session bound to the shared engine. Use it as
``async with get_session() as session: ...``.
"""
+ # `#VERIFY`: defensive code required
return _session_factory()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/core/database.py
around lines 50 - 57, The get_session() function directly returns
_session_factory() without error handling, which defers any engine
initialization failures until the session is actually used, making debugging
harder. Wrap the _session_factory() call in a try-except block to catch any
exceptions that may occur due to misconfiguration or uninitialized engine, and
raise or log a clear error message that indicates the database engine may not
have been properly initialized, providing better error context at the point of
session creation rather than later during session usage.
| Validates query parameters, form data, and JSON body for potential | ||
| SSRF attempts targeting internal resources. |
There was a problem hiding this comment.
SSRF protection is bypassable for JSON/form payloads.
Lines 413-414 state form/JSON are validated, but Lines 425-427 only scan query parameters. URL-bearing fields in JSON or form bodies will bypass this middleware.
Suggested direction
async def dispatch(
self, request: Request, call_next: RequestResponseEndpoint
) -> Response:
@@
- # Check query parameters for URLs
+ # Check query parameters for URLs
for param, value in request.query_params.items():
if "://" in value or value.startswith("//"):
if self._is_blocked_url(value):
return JSONResponse(...)
+
+ # Also inspect JSON and form payloads when present.
+ # Ensure body is buffered/replayed so downstream handlers still receive it.
+ # (Implementation should preserve request stream semantics.)Also applies to: 425-427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@{{cookiecutter.project_slug}}/src/{{cookiecutter.project_slug}}/middleware/security.py
around lines 413 - 414, The SSRF validation logic in the middleware is
incomplete. While the docstring claims to validate query parameters, form data,
and JSON body, the actual implementation only scans query parameters (as
indicated in lines 425-427). Extend the validation logic to also inspect form
data and JSON request bodies for potential SSRF attempts targeting internal
resources, ensuring that URL-bearing fields in all three input sources are
properly scanned before allowing the request to proceed.
| if "{{ cookiecutter.include_database }}" not in ( | ||
| "sqlalchemy_migrations", | ||
| "sqlalchemy_ledger", | ||
| ): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether include-database checks are still done via direct templated literals
# (expected after fix: no matches in hooks/post_gen_project.py)
rg -n 'if\s+"\{\{\s*cookiecutter\.include_database\s*\}\}"\s*(==|not in|in)' hooks/post_gen_project.py
# Verify normalized constant is defined and used
rg -n 'INCLUDE_DATABASE' hooks/post_gen_project.pyRepository: ByronWilliamsCPA/cookiecutter-python-template
Length of output: 207
Normalize templated variables to fix static analysis failures on membership checks.
Lines 323 and 327 compare templated literals directly, causing static analysis to flag these as always-true conditions and fail CI. Extract include_database into a normalized constant at module level, then use that constant in comparisons—following the same pattern as other include flags.
Suggested patch
+INCLUDE_DATABASE = "{{ cookiecutter.include_database }}".strip().lower()
+
def _cleanup_database_files() -> None:
"""Remove database and migration files based on cookiecutter choices."""
# No database selected: drop the engine/session module entirely.
- if "{{ cookiecutter.include_database }}" == "none":
+ if INCLUDE_DATABASE == "none":
remove_file(Path("src/{{ cookiecutter.project_slug }}/core/database.py"))
# Alembic only ships with migration-capable database choices.
- if "{{ cookiecutter.include_database }}" not in (
+ if INCLUDE_DATABASE not in (
"sqlalchemy_migrations",
"sqlalchemy_ledger",
):
remove_file(Path("alembic.ini"))
remove_dir(Path("migrations"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "{{ cookiecutter.include_database }}" not in ( | |
| "sqlalchemy_migrations", | |
| "sqlalchemy_ledger", | |
| ): | |
| INCLUDE_DATABASE = "{{ cookiecutter.include_database }}".strip().lower() | |
| def _cleanup_database_files() -> None: | |
| """Remove database and migration files based on cookiecutter choices.""" | |
| # No database selected: drop the engine/session module entirely. | |
| if INCLUDE_DATABASE == "none": | |
| remove_file(Path("src/{{ cookiecutter.project_slug }}/core/database.py")) | |
| # Alembic only ships with migration-capable database choices. | |
| if INCLUDE_DATABASE not in ( | |
| "sqlalchemy_migrations", | |
| "sqlalchemy_ledger", | |
| ): | |
| remove_file(Path("alembic.ini")) | |
| remove_dir(Path("migrations")) |
🧰 Tools
🪛 GitHub Actions: Validate Template / 0_validate (3.14).txt
[error] 327-327: basedpyright: Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Actions: Validate Template / 1_validate (3.11).txt
[error] 327-327: basedpyright: Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Actions: Validate Template / 4_validate (3.10).txt
[error] 327-327: basedpyright: Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Actions: Validate Template / validate (3.10)
[error] 327-327: basedpyright: Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains).
🪛 GitHub Actions: Validate Template / validate (3.11)
[error] 327-327: basedpyright: Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Actions: Validate Template / validate (3.14)
[error] 327-327: basedpyright: Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains).
🪛 GitHub Check: Code Quality Checks
[failure] 327-327:
Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Check: CodeQL
[warning] 327-330: Constant in conditional expression or statement
Testing a constant will always give the same result.
[warning] 327-330: Comparison of constants
Comparison of constants; use 'True' or 'False' instead.
🪛 GitHub Check: Test Hook Files
[failure] 327-327:
Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Check: validate (3.10)
[failure] 327-327:
Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Check: validate (3.11)
[failure] 327-327:
Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🪛 GitHub Check: validate (3.14)
[failure] 327-327:
Expression will always evaluate to True since the types "Literal['{{ cookiecutter.include_database }}']" and "Literal['sqlalchemy_migrations', 'sqlalchemy_ledger']" have no overlap (reportUnnecessaryContains)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/post_gen_project.py` around lines 327 - 330, The templated variable
include_database is being used directly in the membership check at lines
327-330, causing static analysis failures. Extract the templated
include_database variable into a normalized constant at the module level
(following the pattern of other include flags), then replace the direct template
string "{{ cookiecutter.include_database }}" in the conditional statement with
the new constant. Apply the same refactoring to the similar comparison at line
323 to maintain consistency throughout the file.
Source: Linters/SAST tools
Changes
Makes default generations pass their own quality gates. Generated projects with
API + database selections previously started red: a missing DB module, no async
driver, and a
pydoclintconfig stricter than the shipped Google docstrings.Database layer (was half-wired)
core/database.py(async engine,async_sessionmaker,Base,get_session), shipped wheninclude_database != none.database_urladded tocore/config.pywith a dialect-correct default.databaseoptional-dependency group gated oninclude_databasewith theasync driver per dialect (
asyncpg/aiomysql/aiosqlite) andalembicforthe migration variants;
sqlalchemy/alembicremoved from the unconditionalapiextra.health.pyreadiness check uses SQLAlchemy 2.0text().alembic.ini,migrations/env.py,script.py.mako,versions/.gitkeep) forsqlalchemy_migrations/_ledger;*.makoadded to_copy_without_render;migrations/excluded fromruff/pydoclint/interrogate; post-gen removes the scaffold for non-migration
choices.
Strict gates
srcdocstrings brought into strictpydoclintcompliance(typed
Args:/Returns:,__init__docstrings folded into classArgs:,typed class
Attributes:) across config, exceptions, health, logging,security, correlation.
call_next: RequestResponseEndpoint,MutableHeadersdel,list[str], redundantisinstance).TestCLIgated behindinclude_cliso disabling the CLI no longer leavestests importing the removed
cli.py.ruff force-exclude = trueso excluded dirs are honored under pre-commit.Template Testing
Verified by generating three flag combinations with
cookiecutterand runningthe full gate set on each:
errors, 79 tests pass.
sqlalchemy(no migrations) + sqlite + cli: pydoclint 0, ruffclean, basedpyright 0 errors, 115 tests pass.
sqlalchemy_migrations+ postgres: pydoclint 0, ruff clean,basedpyright 0 errors, 103 tests pass, full
pre-commit run --all-filesgreenexcept the pre-existing
end-of-file-fixerauto-fix (see below).Known / out of scope
end-of-file-fixerauto-fixes 3 files (.env.example,.gitignore,.pre-commit-config.yaml) on first run because they render with extratrailing blank lines from Jinja
{% endif %}. Self-healing (re-stage +commit); pre-existing and unrelated to these fixes.
Breaking Changes
in a
databaseextra (gated oninclude_database) instead of theapiextra; install with
uv sync --all-extras.Summary by CodeRabbit
New Features
Bug Fixes
Refactor