diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d1a3fa..0e557ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ ### 수정 +- asyncio 전환 재검증을 위한 2인 적대적 리뷰어 서브에이전트(동시성/자원관리 관점, 보안/데이터 + 무결성 관점) 감사에서 발견·검증된 버그 수정: `ApiHubClient.aiter_pages()`/ + `AsyncApiHubClient.iter_pages()`가 공용 `pagination.aiter_pages()` 헬퍼를 거치지 않고 + 자체 루프를 재구현해, `max_pages` 안전장치에 걸려 더 가져올 페이지가 남았을 때 동기 + `iter_pages()`와 달리 `PaginationLimitWarning` 없이 조용히 데이터를 잘라내고, `start_page`/ + `max_pages`/`max_items` 입력값 검증도 건너뛰던 문제 수정. 동기 `DataGoKrClient.aiter_pages()`가 + 이미 따르던 것과 동일하게 공용 헬퍼에 위임하도록 정렬. 두 리뷰어 모두 다른 관점(동시성 안전성, + 자격증명 마스킹, result-code 처리, 응답 검증 대칭성)에서는 실제 버그를 찾지 못함(재시도/백오프 + 로직, 자격증명 마스킹, `resultCode` 예외 매핑은 동기/비동기 경로가 동일한 공용 함수를 공유함을 + 확인). - 4인 전문 리뷰어 서브에이전트의 적대적 코드 리뷰로 발견·검증된 버그 수정: `iter_pages()`가 응답 body의 `pageNo`를 그대로 신뢰해 다음 페이지를 계산하다가 그 값이 없거나 항상 고정값이면 같은 페이지를 최대 `max_pages`번 중복 재요청하며 서로 다른 페이지인 것처럼 반환하던 문제, `pageNo`/ diff --git a/docs/journal.md b/docs/journal.md index c8d99fc..50d72c5 100644 --- a/docs/journal.md +++ b/docs/journal.md @@ -2,6 +2,37 @@ 새 항목은 항상 파일 맨 위에 추가(역시간순). 기존 항목은 절대 수정하지 않는다 — 잘못된 결정조차 기록으로 남는 것이 가치다. +## 2026-09-11 (claude, asyncio 재검증 2인 적대적 리뷰 — ApiHubClient.aiter_pages 수정) + +**작업**: 기존 asyncio 전환(PR #25에서 이미 완료·머지됨)이 실제로 안전한지 재검증하기 위해 +독립된 서브에이전트 2명에게 서로 다른 관점(동시성/자원관리, 보안/데이터 무결성)으로 `src/kma` +전체 비동기 경로를 다시 감사시켰다. + +**발견**: 두 리뷰어 모두 독립적으로 동일한 버그를 발견했다 — `ApiHubClient.aiter_pages()` +(`AsyncApiHubClient.iter_pages()`로도 노출)가 공용 `pagination.aiter_pages()` 헬퍼를 거치지 +않고 `for offset in range(max_pages)` 루프를 직접 구현하고 있었다. `DataGoKrClient.aiter_pages()` +는 이미 `pagination.aiter_pages()`에 위임하는데(동기/비동기 대칭 원칙), APIHub 쪽만 예외였다. +그 결과 `max_pages`에 도달했는데 더 가져올 페이지가 남아있어도 동기 `iter_pages()`와 달리 +`PaginationLimitWarning`을 내지 않고 조용히 데이터를 잘랐고, `start_page`/`max_pages`/`max_items` +입력값 검증도 없었다. `tests/*.py`에 `aiter_pages` 테스트가 전무해 CI로는 잡히지 않았다. + +**구현 상세**: +- `apihub.py`에 `from .pagination import aiter_pages as _aiter_pages` 추가, `aiter_pages()`를 + `datagokr.py`의 패턴과 동일하게 `_aiter_pages(...)`에 위임하도록 재작성. +- 더 이상 쓰이지 않게 된 `_has_next_page` import와 로컬 `_body_item_count()` 헬퍼 제거 + (동일 로직이 `pagination._item_count()`에 이미 있었음). +- `tests/test_apihub.py`에 `PagingFakeSession`/`AsyncPagingFakeSession` fixture와 4개 테스트 + 추가 — 동기/비동기 페이지 수집 대칭성, `PaginationLimitWarning` 발생 대칭성, 인자 검증 + 대칭성. 수정 전 코드로 되돌려 새 테스트 2개가 실제로 실패함을 확인(회귀 방지 검증). +- 두 번째 관점(보안/자격증명/result-code 처리)에서는 실제 버그 없음 — 재시도·백오프·자격증명 + 마스킹·`resultCode` 예외 매핑이 동기/비동기 경로에서 동일한 공용 함수를 공유함을 확인. + (informational, 이번 작업 범위 아님) `DataGoKrClient`의 타입화 helper 다수가 async facade에 + 대응 메서드가 없다는 completeness gap도 보고됐으나 버그/취약점은 아니라서 미조치. + +**검증**: `pytest -q` 153 passed·12 live skipped(기존 149→153, 신규 4개), ruff/mypy 통과, +`KMA_RUN_LIVE=1`로 live e2e 재실행 — 9 passed·3 skipped(기존 결과와 동일, 3개는 서비스키 구독 +범위 밖이라 이미 문서화된 스킵). + ## 2026-08-18 (codex, quota 22 비재시도 분류 + XML 200-body 경로) **작업**: 일일 한도 초과 `resultCode=22`를 `failure_kind="quota"`, diff --git a/docs/resume.md b/docs/resume.md index f87fbd4..c8193eb 100644 --- a/docs/resume.md +++ b/docs/resume.md @@ -2,7 +2,7 @@ 새 에이전트 세션이 시작될 때 "지금 어디까지 했고, 다음은 뭐 하면 되나"를 한 화면에서 답한다. -## 현재 진척도 (2026-05-27 갱신) +## 현재 진척도 (2026-09-11 갱신) - ✅ Windows 기준 고정 worktree alias 복구 및 `.codegraph/` Git 상태 노이즈 제거 - ✅ `KmaClient` 타입화 단기예보 4개 endpoint (`getUltraSrtNcst`, `getUltraSrtFcst`, `getVilageFcst`, `getFcstVersion`) @@ -17,7 +17,7 @@ - ✅ `ForecastTimepoint` 피벗 + `pivot_forecast_items()` 시계열 helper - ✅ 예외 계층 (`KmaError` → `Auth`/`Request`/`Server`/`Parse`) - ✅ 인증값 보안 (redaction, sanitize, `.env` 로딩) -- ✅ 161개 테스트 (149 mock + 12 live, 라이브는 키 구독에 따라 일부 skip), ruff/mypy 통과 +- ✅ 165개 테스트 (153 mock + 12 live, 라이브는 키 구독에 따라 일부 skip), ruff/mypy 통과 - ✅ httpx async facade (`build_session`, `build_async_client`, sync/async retry) - ✅ `_parsing.py` 공유 파싱 도우미 추출 (PR #3) - ✅ `maplibre-vworld-js` 에이전트 스타일, 고정 worktree 규칙, AI용 가이드 문서, MCP 설정 도입 및 PR 머지 완료 @@ -34,6 +34,8 @@ - ✅ data.go.kr NO_DATA(03)를 빈 결과로 정규화 (#18, PR #19) - ✅ 중기예보 `MidForecastItem.tm_fc` live 결측 수정 — 요청 `tmFc` 폴백 (#20) - ✅ `resultCode=22` 일일 quota 비재시도 분류 + HTTP 200 XML 오류 envelope 경로 고정 +- ✅ asyncio 전환 재검증 2인 적대적 리뷰 — `ApiHubClient.aiter_pages()`가 공용 `pagination.aiter_pages()` + 를 우회해 `PaginationLimitWarning`/입력 검증을 누락하던 동기·비동기 비대칭 버그 수정 (2026-09-11) ## 다음 한 작업 (1시간 이내 분량) diff --git a/src/kma/apihub.py b/src/kma/apihub.py index 18557a8..1e0d331 100644 --- a/src/kma/apihub.py +++ b/src/kma/apihub.py @@ -37,7 +37,7 @@ redact_credentials_in_text, request_params_from_url, ) -from .pagination import has_next_page as _has_next_page +from .pagination import aiter_pages as _aiter_pages from .pagination import iter_pages as _iter_pages APIHUB_BASE_URL = "https://apihub.kma.go.kr" @@ -547,9 +547,8 @@ async def aiter_pages( """Asynchronously iterate paginated APIHub `open_api` response bodies.""" endpoint = f"/api/typ02/openApi/{service.strip('/')}/{operation.strip('/')}" - items_seen = 0 - for offset in range(max_pages): - page_no = start_page + offset + + async def _fetch_page(page_no: int) -> Mapping[str, Any]: response = await self.aopen_api( service, operation, @@ -558,13 +557,15 @@ async def aiter_pages( page_no=page_no, num_of_rows=num_of_rows, ) - body = _apihub_open_api_body(response, endpoint=endpoint) + return _apihub_open_api_body(response, endpoint=endpoint) + + async for body in _aiter_pages( + _fetch_page, + start_page=start_page, + max_pages=max_pages, + max_items=max_items, + ): yield body - items_seen += _body_item_count(body) - if max_items is not None and items_seen >= max_items: - return - if not _has_next_page(body): - return def _portal_get(self, path: str, params: Mapping[str, Any]) -> ApiHubResponse: return self._get(path, params) @@ -1063,18 +1064,6 @@ def _apihub_open_api_body(response: ApiHubResponse, *, endpoint: str) -> Mapping return body -def _body_item_count(body: Mapping[str, Any]) -> int: - items = body.get("items") - if not isinstance(items, Mapping): - return 0 - raw = items.get("item") - if isinstance(raw, list): - return len(raw) - if isinstance(raw, Mapping): - return 1 - return 0 - - def _normalize_apihub_path(path: str) -> str: parts = urlsplit(path) clean = parts.path if parts.scheme or parts.netloc else path diff --git a/tests/test_apihub.py b/tests/test_apihub.py index 0009b0f..ad2831e 100644 --- a/tests/test_apihub.py +++ b/tests/test_apihub.py @@ -1,6 +1,8 @@ from __future__ import annotations import asyncio +import json +import warnings from typing import Any, Callable import httpx @@ -16,6 +18,7 @@ redact_url_credentials, ) from kma.exceptions import KmaAuthError +from kma.pagination import PaginationLimitWarning class FakeResponse: @@ -89,6 +92,55 @@ def get(self, url: str, *, params: dict[str, Any] | None, timeout: float) -> Fak return FakeErrorResponse("error") +def _open_api_page_body(*, page_no: int, num_of_rows: int, total_count: int) -> str: + start = (page_no - 1) * num_of_rows + 1 + end = min(page_no * num_of_rows, total_count) + items = [{"id": f"item-{i}"} for i in range(start, end + 1)] + payload = { + "response": { + "header": {"resultCode": "00", "resultMsg": "NORMAL_SERVICE"}, + "body": { + "pageNo": page_no, + "numOfRows": num_of_rows, + "totalCount": total_count, + "items": {"item": items}, + }, + } + } + return json.dumps(payload) + + +class PagingFakeSession: + """`pageNo` 요청 파라미터에 따라 서로 다른 body를 돌려주는 fake open_api session.""" + + def __init__(self, *, total_count: int, num_of_rows: int = 10) -> None: + self.total_count = total_count + self.num_of_rows = num_of_rows + self.calls: list[dict[str, Any]] = [] + + def get(self, url: str, *, params: dict[str, Any] | None, timeout: float) -> FakeResponse: + self.calls.append({"url": url, "params": params, "timeout": timeout}) + assert params is not None + page_no = int(params["pageNo"]) + body = _open_api_page_body( + page_no=page_no, + num_of_rows=self.num_of_rows, + total_count=self.total_count, + ) + return FakeResponse(body, url=url, content_type="application/json") + + +class AsyncPagingFakeSession(PagingFakeSession): + async def get( + self, + url: str, + *, + params: dict[str, Any] | None, + timeout: float, + ) -> FakeResponse: + return super().get(url, params=params, timeout=timeout) + + def assert_raises(exc_type: type[BaseException], func: Callable[[], object]) -> BaseException: try: func() @@ -348,3 +400,101 @@ def test_apihub_discover_services_and_endpoints_use_portal_pages() -> None: assert session.calls[0]["url"] == "https://apihub.kma.go.kr/apiList.do" assert session.calls[0]["params"] == {"seqApi": 10} assert session.calls[1]["params"] == {"seqApi": 10, "seqApiSub": 288} + + +def test_apihub_iter_pages_collects_all_pages_without_warning() -> None: + session = PagingFakeSession(total_count=25, num_of_rows=10) + client = ApiHubClient("hub-key", session=session) + + with warnings.catch_warnings(): + warnings.simplefilter("error", PaginationLimitWarning) + pages = list( + client.iter_pages("MidFcstInfoService", "getMidFcst", num_of_rows=10) + ) + + assert [page["pageNo"] for page in pages] == [1, 2, 3] + assert len(pages[-1]["items"]["item"]) == 5 + + +def test_apihub_aiter_pages_collects_all_pages_without_warning() -> None: + async def run() -> list[dict[str, Any]]: + session = AsyncPagingFakeSession(total_count=25, num_of_rows=10) + client = ApiHubClient("hub-key", async_session=session) + pages = [] + async for page in client.aiter_pages( + "MidFcstInfoService", "getMidFcst", num_of_rows=10 + ): + pages.append(page) + return pages + + with warnings.catch_warnings(): + warnings.simplefilter("error", PaginationLimitWarning) + pages = asyncio.run(run()) + + assert [page["pageNo"] for page in pages] == [1, 2, 3] + assert len(pages[-1]["items"]["item"]) == 5 + + +def test_apihub_aiter_pages_warns_on_truncation_like_sync_iter_pages() -> None: + """비동기 aiter_pages는 동기 iter_pages와 동일하게 max_pages 절단을 경고해야 한다. + + 회귀 방지 대상: 이전에는 aiter_pages가 pagination.aiter_pages를 거치지 않고 + 직접 루프를 구현해 PaginationLimitWarning을 내지 않고 조용히 데이터를 잘랐다. + """ + + sync_session = PagingFakeSession(total_count=50, num_of_rows=10) + sync_client = ApiHubClient("hub-key", session=sync_session) + with warnings.catch_warnings(record=True) as sync_caught: + warnings.simplefilter("always") + sync_pages = list( + sync_client.iter_pages( + "MidFcstInfoService", "getMidFcst", num_of_rows=10, max_pages=2 + ) + ) + + async def run_async() -> tuple[list[dict[str, Any]], list[warnings.WarningMessage]]: + async_session = AsyncPagingFakeSession(total_count=50, num_of_rows=10) + async_client = ApiHubClient("hub-key", async_session=async_session) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + pages = [ + page + async for page in async_client.aiter_pages( + "MidFcstInfoService", "getMidFcst", num_of_rows=10, max_pages=2 + ) + ] + return pages, caught + + async_pages, async_caught = asyncio.run(run_async()) + + assert len(sync_pages) == 2 + assert len(async_pages) == 2 + assert any(issubclass(w.category, PaginationLimitWarning) for w in sync_caught) + assert any(issubclass(w.category, PaginationLimitWarning) for w in async_caught) + + +def test_apihub_aiter_pages_validates_arguments_like_sync_iter_pages() -> None: + session = PagingFakeSession(total_count=10, num_of_rows=10) + sync_client = ApiHubClient("hub-key", session=session) + sync_error = assert_raises( + ValueError, + lambda: list( + sync_client.iter_pages("MidFcstInfoService", "getMidFcst", max_pages=0) + ), + ) + assert "max_pages" in str(sync_error) + + async def run() -> BaseException: + async_session = AsyncPagingFakeSession(total_count=10, num_of_rows=10) + async_client = ApiHubClient("hub-key", async_session=async_session) + try: + async for _ in async_client.aiter_pages( + "MidFcstInfoService", "getMidFcst", max_pages=0 + ): + pass + except ValueError as exc: + return exc + raise AssertionError("expected ValueError") + + async_error = asyncio.run(run()) + assert "max_pages" in str(async_error)