Feat/add postgres config#54
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Found four concrete issues: the configured Ruff gate currently fails, the database dependency crashes before yielding a session, connection URLs break for valid credentials containing reserved characters, and WINDUP_RELOAD=false enables reload.
Validation: uv run pytest -q passed (5 tests), uv run lint-imports passed, and uv run ruff check . failed on the inline-noted unused import.
| import os | ||
| from contextlib import asynccontextmanager | ||
|
|
||
| import windup_framework.db |
There was a problem hiding this comment.
CI: This import is unused, so the newly added uv run ruff check . step fails with F401. The health route already imports get_session, so this side-effect import is not needed; remove it (or otherwise make its purpose explicit without violating Ruff) so the PR's lint gate can pass.
|
|
||
| async def get_session() -> Iterator[Session]: | ||
| """FastAPI 依赖:每请求一个 session,请求结束自动关闭。""" | ||
| async with SessionLocal() as session: |
There was a problem hiding this comment.
Runtime bug: SessionLocal() creates a synchronous SQLAlchemy Session, which has neither async context-manager methods nor awaitable commit/rollback/close. Resolving /health/db therefore fails immediately with TypeError: 'Session' object does not support the asynchronous context manager protocol. Make this a synchronous generator using with and normal method calls, or switch the engine/session factory consistently to SQLAlchemy's async APIs; add a route/dependency test that actually advances the generator.
| def url(self) -> str: | ||
| """SQLAlchemy 连接串(psycopg3 驱动)。""" | ||
| return ( | ||
| f"postgresql+psycopg://{self.user}:{self.password}" |
There was a problem hiding this comment.
Configuration bug: Interpolating credentials directly produces an invalid URL when a valid username/password contains reserved characters such as @, :, /, or %. For example, POSTGRES_PASSWORD=p@ss:/word makes SQLAlchemy URL parsing fail before connecting. Construct the URL with sqlalchemy.engine.URL.create(...) (or correctly percent-encode each component) rather than concatenating raw values.
| factory=True, | ||
| host=os.getenv("WINDUP_HOST", "127.0.0.1"), | ||
| port=int(os.getenv("WINDUP_PORT", "8000")), | ||
| reload=bool(os.getenv("WINDUP_RELOAD")), |
There was a problem hiding this comment.
Configuration bug: bool() treats every non-empty string as true, so documented values such as WINDUP_RELOAD=false or WINDUP_RELOAD=0 still enable auto-reload. Parse an explicit boolean vocabulary (or use a settings model) so false values remain false.
1 participant
背景
添加 db 数据库配置信息