From d9c388edefaf5dc672f2b1cbf8879eba992c1648 Mon Sep 17 00:00:00 2001 From: Wada Yusuke Date: Tue, 9 Jun 2026 22:32:49 +0900 Subject: [PATCH] =?UTF-8?q?refactor(backend):=20collector/=E3=82=A8?= =?UTF-8?q?=E3=83=A9=E3=83=BC=E3=82=B3=E3=83=BC=E3=83=89/worker=20?= =?UTF-8?q?=E3=81=AE=E4=BF=9D=E5=AE=88=E6=80=A7=E6=94=B9=E5=96=84=E3=81=A8?= =?UTF-8?q?=E3=83=86=E3=82=B9=E3=83=88=E8=A3=9C=E5=BC=B7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BE レビュー(report/BE_report_20260609_2152.md)の採用項目を実装。 - collector: 3 プラットフォーム fetcher の記事 dict 整形を _build_article に集約し、 日付切り出しと verify_user_exists のエンドポイント分岐を畳み込み（挙動不変） - github_link router: start/retry の GitHub 認可ガードを require_github_user 依存へ共通化 - worker: execute_task のタスク種別ハードコード分岐をハンドラレジストリ経由の データ駆動ディスパッチへ汎用化し「黙って completed」失敗モードを構造的に除去。 dispatch seam の patch 対象を GitHubLinkHandler.run へ移行、guard テストを registry 登録チェックへ書き換え - test: infer_error_code の全分岐回帰テストと messages.json 文言ドリフト結合ガードを追加 認可順序の変更により未連携ユーザーの /run 系は rate limit より前に 403 を返す （レスポンス契約は不変）。 Co-Authored-By: Claude Opus 4.8 --- backend/app/routers/github_link.py | 35 +++-- backend/app/services/blog/collector.py | 138 +++++++++++------- backend/app/services/tasks/worker.py | 17 ++- backend/tests/security/test_rate_limit.py | 10 +- backend/tests/test_blog_collector.py | 4 +- backend/tests/test_errors.py | 94 ++++++++++++ backend/tests/test_retry_flow.py | 13 +- .../tests/test_worker/test_execute_task.py | 43 +++--- 8 files changed, 239 insertions(+), 115 deletions(-) create mode 100644 backend/tests/test_errors.py diff --git a/backend/app/routers/github_link.py b/backend/app/routers/github_link.py index d1e83bbc..48abdb43 100644 --- a/backend/app/routers/github_link.py +++ b/backend/app/routers/github_link.py @@ -50,6 +50,21 @@ def _get_or_create_cache(db: Session, user_id: str) -> GitHubLinkCache: return cache +def require_github_user(user: User = Depends(get_current_user)) -> User: + """GitHub 連携には GitHub ログイン（``github_id`` 保持）が必須。未連携なら 403。 + + ``start`` / ``retry`` の両エンドポイントで共通の認可ガード。 + """ + if user.github_id is None: + raise_app_error( + status_code=403, + code=ErrorCode.AUTH_REQUIRED, + message=get_error("github_link.github_login_required"), + action="GitHub アカウントでログインし直してください", + ) + return user + + @router.get("/cache", response_model=CachedGitHubLinkResponse) def get_cache( user: User = Depends(get_current_user), @@ -104,18 +119,10 @@ async def start_github_link( request: Request, payload: GitHubLinkRequest, background_tasks: BackgroundTasks, - user: User = Depends(get_current_user), + user: User = Depends(require_github_user), db: Session = Depends(get_db), ): """GitHub 連携パイプラインをバックグラウンドで開始する。""" - if user.github_id is None: - raise_app_error( - status_code=403, - code=ErrorCode.AUTH_REQUIRED, - message=get_error("github_link.github_login_required"), - action="GitHub アカウントでログインし直してください", - ) - github_username = user.username # 進行中のタスクがあればそのステータスを返す @@ -151,7 +158,7 @@ async def retry_github_link( request: Request, background_tasks: BackgroundTasks, payload: GitHubLinkRequest | None = None, - user: User = Depends(get_current_user), + user: User = Depends(require_github_user), db: Session = Depends(get_db), ): """失敗した GitHub 連携タスクを手動で再実行する。 @@ -159,14 +166,6 @@ async def retry_github_link( ``dead_letter`` 状態のキャッシュのみ再実行可能。 ``retry_count`` を 0 にリセットし、ステータスを ``pending`` に戻して再ディスパッチする。 """ - if user.github_id is None: - raise_app_error( - status_code=403, - code=ErrorCode.AUTH_REQUIRED, - message=get_error("github_link.github_login_required"), - action="GitHub アカウントでログインし直してください", - ) - cache = db.query(GitHubLinkCache).filter_by(user_id=user.id).first() if not cache: raise_app_error( diff --git a/backend/app/services/blog/collector.py b/backend/app/services/blog/collector.py index c2cfcc4e..1c082a39 100644 --- a/backend/app/services/blog/collector.py +++ b/backend/app/services/blog/collector.py @@ -34,6 +34,51 @@ class BlogPlatformRequestError(RuntimeError): "qiita": {"qiita.com", "www.qiita.com"}, } +# verify_user_exists 用のユーザー存在確認エンドポイント。 +# (URL テンプレート, クエリパラメータ) を platform ごとに引く。 +_VERIFY_ENDPOINTS: dict[str, tuple[str, dict | None]] = { + "zenn": ("https://zenn.dev/api/users/{username}", None), + "note": ( + "https://note.com/api/v2/creators/{username}/contents", + {"kind": "note", "page": 1}, + ), + "qiita": ("https://qiita.com/api/v2/users/{username}", None), +} + + +def _truncate_date(value: str | None) -> str | None: + """ISO 日付文字列の先頭 10 文字（``YYYY-MM-DD``）を返す。空なら ``None``。""" + return value[:10] if value else None + + +def _build_article( + platform: str, + *, + external_id: str, + title: str, + url: str, + published_at: str | None, + likes_count: int, + tags: list[str], + summary: str = "", +) -> dict: + """各プラットフォーム共通の記事 dict を組み立てる。 + + この dict の形が記事取り込みの暗黙の契約であり、 + ``repositories.blog.BlogArticleRepository._apply_article_payload`` が同じキーを期待する。 + キーを増減する場合は両者を併せて更新すること。 + """ + return { + "platform": platform, + "external_id": external_id, + "title": title, + "url": url, + "published_at": published_at, + "likes_count": likes_count, + "summary": summary, + "tags": tags, + } + def _parse_platform_url(raw_username: str, allowed_hosts: set[str]): if raw_username.startswith(("http://", "https://")): @@ -121,18 +166,15 @@ async def fetch_zenn_articles(username: str) -> list[dict]: if isinstance(t, dict) ] articles.append( - { - "platform": "zenn", - "external_id": slug, - "title": item.get("title", ""), - "url": f"https://zenn.dev/{username}/articles/{slug}", - "published_at": ( - item.get("published_at", "")[:10] if item.get("published_at") else None - ), - "likes_count": item.get("liked_count", 0), - "summary": "", - "tags": tag_names, - } + _build_article( + "zenn", + external_id=slug, + title=item.get("title", ""), + url=f"https://zenn.dev/{username}/articles/{slug}", + published_at=_truncate_date(item.get("published_at")), + likes_count=item.get("liked_count", 0), + tags=tag_names, + ) ) next_page = data.get("next_page") @@ -163,11 +205,6 @@ async def fetch_note_articles(username: str) -> list[dict]: break for item in notes: - note_url = item.get("noteUrl", "") - external_id = str(item.get("id", "")) - published_at = ( - item.get("publishAt", "")[:10] if item.get("publishAt") else None - ) hashtags = item.get("hashtags", []) tag_names = [ h.get("hashtag", {}).get("name", "") @@ -177,16 +214,16 @@ async def fetch_note_articles(username: str) -> list[dict]: body = item.get("body") or "" articles.append( - { - "platform": "note", - "external_id": external_id, - "title": item.get("name", ""), - "url": note_url, - "published_at": published_at, - "likes_count": item.get("likeCount", 0), - "summary": body[:500], - "tags": [t for t in tag_names if t], - } + _build_article( + "note", + external_id=str(item.get("id", "")), + title=item.get("name", ""), + url=item.get("noteUrl", ""), + published_at=_truncate_date(item.get("publishAt")), + likes_count=item.get("likeCount", 0), + summary=body[:500], + tags=[t for t in tag_names if t], + ) ) is_last_page = data.get("data", {}).get("isLastPage", True) @@ -219,18 +256,15 @@ async def fetch_qiita_articles(username: str) -> list[dict]: for item in data: tag_names = [t.get("name", "") for t in item.get("tags", [])] articles.append( - { - "platform": "qiita", - "external_id": item.get("id", ""), - "title": item.get("title", ""), - "url": item.get("url", ""), - "published_at": ( - item.get("created_at", "")[:10] if item.get("created_at") else None - ), - "likes_count": item.get("likes_count", 0), - "summary": "", - "tags": tag_names, - } + _build_article( + "qiita", + external_id=item.get("id", ""), + title=item.get("title", ""), + url=item.get("url", ""), + published_at=_truncate_date(item.get("created_at")), + likes_count=item.get("likes_count", 0), + tags=tag_names, + ) ) if len(data) < per_page: @@ -250,23 +284,17 @@ async def verify_user_exists(platform: str, username: str) -> bool: except ValueError: return False - try: - if platform == "zenn": - async with httpx.AsyncClient(timeout=_VERIFY_TIMEOUT_SECONDS) as client: - resp = await client.get(f"https://zenn.dev/api/users/{normalized_username}") - return resp.status_code == 200 - if platform == "note": - async with httpx.AsyncClient(timeout=_VERIFY_TIMEOUT_SECONDS) as client: - resp = await client.get( - f"https://note.com/api/v2/creators/{normalized_username}/contents", - params={"kind": "note", "page": 1}, - ) - return resp.status_code == 200 - if platform == "qiita": - async with httpx.AsyncClient(timeout=_VERIFY_TIMEOUT_SECONDS) as client: - resp = await client.get(f"https://qiita.com/api/v2/users/{normalized_username}") - return resp.status_code == 200 + endpoint = _VERIFY_ENDPOINTS.get(platform) + if endpoint is None: raise UnsupportedBlogPlatformError(platform) + url_template, params = endpoint + + try: + async with httpx.AsyncClient(timeout=_VERIFY_TIMEOUT_SECONDS) as client: + resp = await client.get( + url_template.format(username=normalized_username), params=params + ) + return resp.status_code == 200 except (httpx.ConnectError, httpx.TimeoutException): logger.warning("プラットフォーム %s への接続に失敗しました", platform) raise BlogPlatformRequestError(platform) from None diff --git a/backend/app/services/tasks/worker.py b/backend/app/services/tasks/worker.py index 3b0bc97a..55943c8c 100644 --- a/backend/app/services/tasks/worker.py +++ b/backend/app/services/tasks/worker.py @@ -75,17 +75,18 @@ async def execute_task( }, ) - if get_handler(task_type) is None: + handler = get_handler(task_type) + if handler is None: logger.error("不明なタスク種別: %s", task_type) return try: - # 各 ``_run_*`` シムはテストからの patch ポイントとして残している。 - # 実体は ``services/tasks/handlers/`` 配下のハンドラ。 + # ハンドラレジストリ経由でディスパッチする（種別ごとの if 分岐は持たない）。 + # 種別を増やしてもここは変更不要で、ハンドラ未登録なら上で早期 return するため + # 「分岐の書き忘れで黙って completed になる」事故は構造的に起きない。 # ハンドラには SessionLocal (ファクトリ) を渡し、ハンドラ内で長時間処理の # 前後にセッションを開閉させる。 - if task_type == TaskType.GITHUB_LINK: - await _run_github_link(SessionLocal, payload) + await handler.run(SessionLocal, payload) duration_ms = _monotonic_ms_since(start) logger.info( @@ -231,9 +232,9 @@ def _finalize_retrying( ) -# ---------- ハンドラ薄ラッパー（後方互換）---------- -# テストや既存呼び出しから ``_run_github_link`` を直接呼べるよう、 -# ハンドラ実装への薄いシムを残す。引数は ``session_factory``。 +# ---------- ハンドラ薄ラッパー（直接呼び出し用）---------- +# ``execute_task`` はレジストリ経由で汎用ディスパッチするためこのシムを介さないが、 +# GitHub 連携ハンドラ単体をテスト等から直接呼ぶための入口として残す。引数は ``session_factory``。 async def _run_github_link(session_factory: SessionFactory, payload: dict) -> None: diff --git a/backend/tests/security/test_rate_limit.py b/backend/tests/security/test_rate_limit.py index d4e07904..edc1cc19 100644 --- a/backend/tests/security/test_rate_limit.py +++ b/backend/tests/security/test_rate_limit.py @@ -5,7 +5,6 @@ """ from app.main import limiter -from app.repositories import UserRepository from fastapi.testclient import TestClient from conftest import auth_header @@ -13,12 +12,9 @@ def test_github_link_run_rate_limited(client: TestClient) -> None: """POST /api/github-link/run が 5/分の上限超で 429 を返す。""" - headers = auth_header(client, "rl-gh-user") - # /run は連携済み GitHub アカウント（github_login）を要求するため事前にセットする - db = client._db_session - user = UserRepository(db).get_by_username("rl-gh-user") - user.github_login = "octocat" - db.commit() + # /run は GitHub 連携済みユーザー（github_id）を require_github_user で要求するため、 + # 認可で弾かれず rate limit まで到達するよう github_id 付きで作成する。 + headers = auth_header(client, "rl-gh-user", github_id=424242) limiter.reset() statuses: list[int] = [] diff --git a/backend/tests/test_blog_collector.py b/backend/tests/test_blog_collector.py index 36ae3d2b..c9be56e1 100644 --- a/backend/tests/test_blog_collector.py +++ b/backend/tests/test_blog_collector.py @@ -208,7 +208,9 @@ def test_verify_user_exists_normalizes_before_request() -> None: result = _run(verify_user_exists("zenn", "https://zenn.dev/testuser/articles/test")) assert result is True - mock_client.get.assert_awaited_once_with("https://zenn.dev/api/users/testuser") + mock_client.get.assert_awaited_once_with( + "https://zenn.dev/api/users/testuser", params=None + ) def test_verify_user_exists_zenn_user_not_found() -> None: diff --git a/backend/tests/test_errors.py b/backend/tests/test_errors.py new file mode 100644 index 00000000..afd65296 --- /dev/null +++ b/backend/tests/test_errors.py @@ -0,0 +1,94 @@ +"""``app.core.errors`` のエラーコード推定ロジックの回帰テスト。 + +``infer_error_code`` は status_code と detail（dict / str / None）から ErrorCode を +引く分岐の多い純粋関数で、FE へ返すエラーコード契約の要。特に +"GitHubユーザーが見つかりません" の部分文字列マッチは messages.json の文言変更で +無言の劣化（INTERNAL_ERROR へ fallback）を起こしうるため、結合を明示的に固定する。 +""" + +import pytest +from app.core.errors import ( + ErrorCode, + infer_async_error_code, + infer_error_code, + resolve_async_error_code, +) +from app.core.messages import get_error + + +@pytest.mark.parametrize( + ("status_code", "detail", "expected"), + [ + # dict + 有効な code 文字列 → その code をそのまま採用（status_code より優先） + (500, {"code": "RATE_LIMITED", "message": "x"}, ErrorCode.RATE_LIMITED), + # dict + 無効な code 文字列 → code を無視して message / status へ fall through + (404, {"code": "NOT_A_REAL_CODE"}, ErrorCode.VALIDATION_ERROR), + # dict + message のみ（code なし）→ message と status で判定 + (500, {"message": "なにかのエラー"}, ErrorCode.INTERNAL_ERROR), + # str detail → そのまま message として扱う + (401, "認証が必要です", ErrorCode.AUTH_REQUIRED), + # detail なし → status_code のみで判定 + (429, None, ErrorCode.RATE_LIMITED), + # GitHub ユーザー未検出の部分文字列は status より優先 + (500, "GitHubユーザーが見つかりません: foo", ErrorCode.GITHUB_USER_NOT_FOUND), + (200, "GitHubユーザーが見つかりません: foo", ErrorCode.GITHUB_USER_NOT_FOUND), + # status_code マッピング + (401, None, ErrorCode.AUTH_REQUIRED), + (429, None, ErrorCode.RATE_LIMITED), + (400, None, ErrorCode.VALIDATION_ERROR), + (404, None, ErrorCode.VALIDATION_ERROR), + (409, None, ErrorCode.VALIDATION_ERROR), + (422, None, ErrorCode.VALIDATION_ERROR), + # マッピングに無い status → INTERNAL_ERROR + (500, None, ErrorCode.INTERNAL_ERROR), + (418, None, ErrorCode.INTERNAL_ERROR), + ], +) +def test_infer_error_code(status_code: int, detail, expected: ErrorCode) -> None: + """status_code と detail の各組み合わせで期待する ErrorCode を返すこと。""" + assert infer_error_code(status_code, detail) is expected + + +def test_infer_error_code_dict_code_takes_precedence_over_github_message() -> None: + """dict の有効な code は GitHub 部分文字列マッチより優先されること。""" + detail = {"code": "VALIDATION_ERROR", "message": "GitHubユーザーが見つかりません: foo"} + assert infer_error_code(500, detail) is ErrorCode.VALIDATION_ERROR + + +def test_github_user_not_found_message_keeps_substring_contract() -> None: + """messages.json の実際の文言が GitHub 未検出の部分文字列契約を満たすこと。 + + ``infer_error_code`` がキーにしている "GitHubユーザーが見つかりません" を + messages.json 側の文言変更で取りこぼすと、FE で GITHUB_USER_NOT_FOUND の + リカバリ表示が出なくなる。文言が変わったら本テストで検知する。 + """ + message = get_error("github_link.github_user_not_found", username="someone") + assert infer_error_code(500, message) is ErrorCode.GITHUB_USER_NOT_FOUND + + +@pytest.mark.parametrize( + ("message", "expected"), + [ + (None, None), + ("", None), + ("GitHubユーザーが見つかりません: foo", ErrorCode.GITHUB_USER_NOT_FOUND), + ("不明なエラー", ErrorCode.INTERNAL_ERROR), + ], +) +def test_infer_async_error_code(message, expected) -> None: + """非同期タスクのエラーメッセージから ErrorCode を引く（空は None）。""" + assert infer_async_error_code(message) is expected + + +@pytest.mark.parametrize( + ("message", "expected"), + [ + (None, None), + ("", None), + ("GitHubユーザーが見つかりません: foo", "GITHUB_USER_NOT_FOUND"), + ("不明なエラー", "INTERNAL_ERROR"), + ], +) +def test_resolve_async_error_code(message, expected) -> None: + """``resolve_async_error_code`` は code の文字列値（または None）を返すこと。""" + assert resolve_async_error_code(message) == expected diff --git a/backend/tests/test_retry_flow.py b/backend/tests/test_retry_flow.py index 3f66a29b..dedbae7c 100644 --- a/backend/tests/test_retry_flow.py +++ b/backend/tests/test_retry_flow.py @@ -23,6 +23,7 @@ from app.repositories import UserRepository from app.services.tasks.base import TaskType from app.services.tasks.exceptions import NonRetryableError, RetryableError +from app.services.tasks.handlers.github_link import GitHubLinkHandler from app.services.tasks.worker import execute_task from fastapi.testclient import TestClient from sqlalchemy.orm import Session @@ -68,7 +69,7 @@ def _setup_github_link_test( ): """github_link worker テストの共通スキャフォールド。 - 元の各テストにあった「user 作成 → cache 作成 → worker.SessionLocal / _run_github_link / + 元の各テストにあった「user 作成 → cache 作成 → worker.SessionLocal / GitHubLinkHandler.run / _create_notification の 3 連 patch」を集約する。15 行 × 4 箇所のコピペを解消するための helper。 yield する mock_notify で `_create_notification` の呼び出し有無を検証できる。 """ @@ -84,8 +85,9 @@ def _setup_github_link_test( "app.services.tasks.worker.SessionLocal", return_value=_keep_open_session(db_session), ), - patch( - "app.services.tasks.worker._run_github_link", + patch.object( + GitHubLinkHandler, + "run", new_callable=AsyncMock, side_effect=side_effect, ), @@ -411,8 +413,9 @@ def test_is_final_at_last_attempt(db_session: Session): "app.services.tasks.worker.SessionLocal", return_value=_keep_open_session(db_session), ), - patch( - "app.services.tasks.worker._run_github_link", + patch.object( + GitHubLinkHandler, + "run", new_callable=AsyncMock, side_effect=RetryableError("still retrying"), ), diff --git a/backend/tests/test_worker/test_execute_task.py b/backend/tests/test_worker/test_execute_task.py index a0c81aa8..123f65a0 100644 --- a/backend/tests/test_worker/test_execute_task.py +++ b/backend/tests/test_worker/test_execute_task.py @@ -6,6 +6,8 @@ from app.models import GitHubLinkCache from app.repositories import UserRepository from app.services.tasks.base import TaskType +from app.services.tasks.handlers import get_handler +from app.services.tasks.handlers.github_link import GitHubLinkHandler from app.services.tasks.worker import ( _safe_rollback, execute_task, @@ -19,13 +21,14 @@ class TestExecuteTask: def test_known_task_type_routes_to_correct_handler(self, db_session: Session): - """GITHUB_LINK が _run_github_link に正しくディスパッチされることを確認する。""" + """GITHUB_LINK が登録ハンドラの run に正しくディスパッチされることを確認する。""" with ( patch("app.services.tasks.worker.SessionLocal", return_value=db_session), - patch( - "app.services.tasks.worker._run_github_link", + patch.object( + GitHubLinkHandler, + "run", new_callable=AsyncMock, - ) as mock_gh, + ) as mock_run, patch("app.services.tasks.worker._create_notification"), ): _run( @@ -35,23 +38,19 @@ def test_known_task_type_routes_to_correct_handler(self, db_session: Session): ) ) - mock_gh.assert_called_once() + mock_run.assert_called_once() - def test_all_task_types_have_dispatch_branch(self): - """TaskType に列挙された全種別に execute_task のディスパッチ分岐があること。 + def test_all_task_types_have_registered_handler(self): + """TaskType に列挙された全種別がハンドラレジストリに登録済みであること。 - 新しい TaskType を追加した際に worker.execute_task への分岐追加を - 忘れて「黙って completed になる」事故を防ぐためのガード。 + execute_task はレジストリ経由で汎用ディスパッチするため、種別を追加して + ハンドラ登録を忘れると未登録として早期 return される。「黙って completed に + なる」事故を防ぐため、全種別が登録済みであることをガードする。 """ - import inspect - - from app.services.tasks import worker - - source = inspect.getsource(worker.execute_task) - missing = [t.name for t in TaskType if f"TaskType.{t.name}" not in source] + missing = [t.name for t in TaskType if get_handler(t) is None] assert not missing, ( - f"execute_task に未対応の TaskType があります: {missing}。" - " 対応するハンドラシムを追加し if/elif に分岐を足してください。" + f"ハンドラ未登録の TaskType があります: {missing}。" + " services/tasks/handlers/__init__.py の _HANDLERS に登録してください。" ) def test_execute_task_marks_dead_letter_on_error(self, db_session: Session): @@ -72,8 +71,9 @@ def test_execute_task_marks_dead_letter_on_error(self, db_session: Session): "app.services.tasks.worker.SessionLocal", return_value=keep_open_session(db_session), ), - patch( - "app.services.tasks.worker._run_github_link", + patch.object( + GitHubLinkHandler, + "run", new_callable=AsyncMock, side_effect=RuntimeError("予期しないクラッシュ"), ), @@ -99,8 +99,9 @@ def test_execute_task_creates_notification_on_success(self, db_session: Session) with ( patch("app.services.tasks.worker.SessionLocal", mock_session_local), - patch( - "app.services.tasks.worker._run_github_link", + patch.object( + GitHubLinkHandler, + "run", new_callable=AsyncMock, return_value=None, ),