Skip to content

feat: add postgres db config#55

Open
johnnyzhang-eng wants to merge 4 commits into
1024XEngineer:mainfrom
johnnyzhang-eng:feat/add-postgres-config
Open

feat: add postgres db config#55
johnnyzhang-eng wants to merge 4 commits into
1024XEngineer:mainfrom
johnnyzhang-eng:feat/add-postgres-config

Conversation

@johnnyzhang-eng

Copy link
Copy Markdown

背景

在 framework 层添加 PostgreSQL 数据库配置与基础设施(engine / session / Base),供后续领域模型与仓储层使用。本 PR 是已关闭 #54 的修复版:#54 因两处 CI 门禁失败被关闭,这里已修复并在本地验证全部门禁通过。

内容

  • framework/config/database.py:pydantic-settings 读取数据库连接配置(host / port / db / url)
  • framework/db/:SQLAlchemy Base(DeclarativeBase)、engineSessionLocalget_session
  • tests/test_db_config.py:配置加载 / url 组装 / engine 与 session 工厂 / Base 声明式 —— 5 项单测(不连真实库)

修复了 #54 的两处门禁失败

  • Commit messages 门禁:原提交标题带前导空格 feat: add db config → 去掉空格
  • Ruff F401:bootstrap/app.py 引入了未使用的 import windup_framework.db(DB 尚未接入 app 装配)→ 删除该行;DB 模块由单测独立覆盖,app 装配本期不接 DB

本地验证(与 CI 门禁一致)

  • Ruff:All checks passed
  • Import-linter:2 contracts kept, 0 broken
  • Pytest:5 passed
  • Commit messages / Branch name:本地按同一正则复核通过

说明

Refs #3

@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review Summary

Found four concrete runtime/configuration issues in the new backend foundation. The configured Ruff and import-linter checks pass, and the existing test suite reports 5 passed, but it does not exercise the database dependency, special-character credentials, reload parsing, or documented module entry point.

View job run


async def get_session() -> Iterator[Session]:
"""FastAPI 依赖:每请求一个 session,请求结束自动关闭。"""
async with SessionLocal() as session:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] SessionLocal creates a synchronous Session, so it has neither the async context-manager protocol nor awaitable commit/rollback/close methods. Calling /health/db currently fails before executing the query with TypeError: 'Session' object does not support the asynchronous context manager protocol. Make this a synchronous generator using with and ordinary method calls, or switch the engine/session factory consistently to SQLAlchemy's async APIs; please also cover the endpoint in a test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. get_session was rewritten from a broken async def async-generator (which used async with SessionLocal() and await session.commit()/rollback()/close()) into a plain synchronous generator. Because SessionLocal is a sync sessionmaker whose Session has no __aenter__/__aexit__ and non-awaitable commit/rollback/close, the old code raised TypeError on entry and returned 500 on GET /health/db; the Iterator[Session] annotation is now correct and the redundant finally: session.close() was dropped since the with block already closes the session. A new test tests/test_health.py::test_health_db_ok drives the endpoint via TestClient with a fake sync session override and asserts a 200 {'status': 'ok'}.

def url(self) -> str:
"""SQLAlchemy 连接串(psycopg3 驱动)。"""
return (
f"postgresql+psycopg://{self.user}:{self.password}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Interpolating credentials directly into the URL makes valid database passwords containing reserved characters unusable. For example, POSTGRES_PASSWORD=p@ss:word produces a URL that SQLAlchemy cannot parse (ValueError while interpreting the port). Construct the URL with sqlalchemy.URL.create(...) (or otherwise percent-encode each component) so deployment secrets are handled safely.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. The url property no longer interpolates the DSN with a raw f-string; it now builds it via URL.create(drivername='postgresql+psycopg', username, password, host, port, database).render_as_string(hide_password=False), with from sqlalchemy import URL added to the imports. Reserved characters in the password (@, :, /) are now escaped instead of being mis-parsed as the port, and the property still returns a str so the existing contract holds. A new test tests/test_db_config.py::test_url_escapes_reserved_password_chars round-trips a p@ss:word/! password through make_url and asserts the parsed password/port/host/database; it fails on the old implementation and passes now.

factory=True,
host=os.getenv("WINDUP_HOST", "127.0.0.1"),
port=int(os.getenv("WINDUP_PORT", "8000")),
reload=bool(os.getenv("WINDUP_RELOAD")),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] bool() treats every non-empty environment string as true, so documented values such as WINDUP_RELOAD=false or WINDUP_RELOAD=0 still enable auto-reload. Parse this setting as a real boolean (for example through the settings model) before passing it to Uvicorn.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. Added a module-level _env_flag(name) helper returning os.getenv(name, '').strip().lower() in {'1', 'true', 'yes', 'on'} and changed the call site to reload=_env_flag('WINDUP_RELOAD'). The previous bool(os.getenv('WINDUP_RELOAD')) was truthy for any non-empty string, so WINDUP_RELOAD=false/0/no/off still enabled auto-reload; these values now correctly disable it, with unset defaulting to off. A new parametrized test tests/test_bootstrap_reload_flag.py covers the truthy/falsy cases, including the regression guards that 'false' and '0' resolve to False.

``create_app`` 负责创建 FastAPI 实例并挂载路由 / 中间件 / 异常处理,
是整个 web 服务的唯一装配点(composition root)。

``main`` 是开发启动入口:``python -m windup_app`` 或 ``windup`` 命令。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] This advertised startup command does not work because the package has no windup_app/__main__.py; python -m windup_app exits with No module named windup_app.__main__. Add the module entry point delegating to main(), or remove this command from the documentation and use the installed windup script only.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. Added windup_app/__main__.py delegating to bootstrap.app:main under an if __name__ == '__main__' guard, so the docstring-advertised python -m windup_app entry point now exists instead of raising No module named windup_app.__main__. It reuses the same main() as the windup console script, so there is no duplicated logic and importing the module has no side effects. No dedicated regression test was added because invoking python -m windup_app starts uvicorn and blocks; the delegated main()/app assembly is exercised by tests/test_health.py, and the full suite stays green (20 passed).

… -m entrypoint)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@johnnyzhang-eng

Copy link
Copy Markdown
Author

Thanks for the review. All four findings are addressed and the full suite passes (20 passed; ruff and import-linter green).

  • F1 (db/session.py): get_session rewritten from a broken async def async-generator over a synchronous SessionLocal into a plain sync generator (with SessionLocal(), session.commit()/rollback()), fixing the TypeError/500 on GET /health/db; the Iterator[Session] annotation is now correct and the redundant session.close() was dropped.
  • F2 (config/database.py): DSN now built via URL.create(...).render_as_string(hide_password=False) (added from sqlalchemy import URL) so passwords with reserved chars (@ : /) are escaped rather than mis-parsed as the port; still returns str.
  • F3 (bootstrap/app.py): added _env_flag() and use reload=_env_flag('WINDUP_RELOAD'), so false/0/no/off/unset correctly disable auto-reload instead of every non-empty string being truthy.
  • F4 (bootstrap/app.py + new __main__.py): added windup_app/__main__.py delegating to bootstrap.app:main under an if __name__ == '__main__' guard, so the advertised python -m windup_app interface now works.

New tests: tests/test_health.py (test_health_ok, test_health_db_ok via TestClient with a fake sync session override, both assert 200), tests/test_bootstrap_reload_flag.py (parametrized truthy/falsy parsing incl. false/0 → False, plus unset → False), and tests/test_db_config.py::test_url_escapes_reserved_password_chars (reserved-char password round-trip via make_url).

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.

2 participants