KRX 로그인 기능 추가 및 pytest 수정#282
Conversation
There was a problem hiding this comment.
Pull request overview
KRX 데이터 포털의 로그인 필수 정책 변경에 대응하기 위해, (환경변수 기반) 로그인 세션을 생성/사용하도록 네트워크 레이어를 확장하고 테스트/CI 구성을 조정하는 PR입니다.
Changes:
requests.Session기반의 KRX 로그인 세션 생성 로직(auth.py) 추가 및webio에서 세션 사용 지원- pytest VCR 초기화/공통 cassette 처리 로직 보강
- GitHub Actions CI 매트릭스에서 Windows 제거 및 테스트 실행 시 KRX 자격증명 env 주입
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
pykrx/website/comm/auth.py |
KRX 로그인/세션 생성 유틸 추가 |
pykrx/website/comm/webio.py |
GET/POST 요청 시 전역 세션을 사용하도록 변경 |
pykrx/website/comm/__init__.py |
세션/인증 관련 심볼을 패키지 레벨로 재-export |
tests/conftest.py |
VCR matcher/공통 cassette 병합 및 singleton 초기화 로직 추가 |
.github/workflows/ci.yml |
Windows 테스트 제거, pytest 실행 시 KRX_ID/KRX_PW 주입 |
| resp = session.post(LOGIN_URL, data=payload, headers=headers, timeout=15) | ||
| data = resp.json() | ||
| error_code = data.get("_error_code", "") |
There was a problem hiding this comment.
login_krx() assumes the login response is JSON (resp.json()), but KRX can return non-JSON (HTML/error page) on auth/rate-limit/region blocking, which would raise and potentially break imports/requests. Please check resp.ok / Content-Type and handle JSON decode failures with a clear error (or return False) before calling .json().
| from pykrx.website.comm.auth import build_krx_session | ||
|
|
||
| _session = build_krx_session() | ||
|
|
There was a problem hiding this comment.
_session = build_krx_session() runs at import time. When KRX_ID/KRX_PW are set this will perform network calls during module import, which can break environments without network access and can bypass VCR in tests. Consider lazy-initializing the session inside get_session() (or on first request) instead of doing it at import time.
| from pykrx.website.comm.auth import build_krx_session, login_krx, warmup_krx_session | ||
| from pykrx.website.comm.util import dataframe_empty_handler, singleton | ||
| from pykrx.website.comm.webio import get_session, set_session |
There was a problem hiding this comment.
Re-exporting get_session/set_session via from ...webio import ... forces pykrx.website.comm imports to import webio, which currently initializes the KRX session at import time (potentially making network calls). To avoid unexpected side effects, consider lazy re-exports (import inside functions) or avoid importing webio from __init__.py.
| # Register custom matchers globally for ALL VCR instances | ||
| # This needs to be done at module level before pytest-vcr creates VCR instances | ||
| _global_vcr = vcr.VCR() | ||
| _global_vcr = vcrpy.VCR() | ||
| _global_vcr.register_matcher("uri_ignore_dates", uri_without_dates) | ||
| _global_vcr.register_matcher("body_ignore_dates", form_body_matcher) | ||
|
|
There was a problem hiding this comment.
_global_vcr is created and matchers are registered on it, but that instance is never used (each fixture creates/receives a different VCR instance). As written this doesn’t actually “register globally” and is likely dead code; consider removing it or wiring pytest-vcr to use this instance explicitly.
| - name: Run tests with pytest | ||
| env: | ||
| KRX_ID: ${{ secrets.KRX_ID }} | ||
| KRX_PW: ${{ secrets.KRX_PW }} |
There was a problem hiding this comment.
Passing KRX_ID/KRX_PW into the test job environment will trigger real KRX login requests during import (with the current webio import-time session initialization), which bypasses VCR and can make CI flaky and non-deterministic. If tests are intended to be VCR-only, consider removing these env vars from CI (or only setting them for explicitly-marked live tests).
| session = requests.Session() | ||
| warmup_krx_session(session) | ||
| login_krx(session, login_id, login_pw) | ||
| return session |
There was a problem hiding this comment.
build_krx_session() ignores the boolean result of login_krx(). If credentials are wrong or login fails, the code still returns a Session that will be used for subsequent requests, leading to confusing downstream errors. Consider returning None/falling back to plain requests (or raising a helpful exception) when login_krx() returns False.
| # Use plain requests inside VCR context so cassettes (recorded without auth) | ||
| # match correctly on all platforms including Windows. | ||
| original_session = _webio.get_session() | ||
| _webio.set_session(None) |
There was a problem hiding this comment.
This fixture temporarily disables _webio’s session to make cassette matching deterministic, but if webio created/logged-in a session during import (when KRX_ID/KRX_PW are set) the initial login requests already happened outside any VCR cassette. Fixing this likely requires making session creation lazy (no network at import time) rather than only calling set_session(None) here.
| env: | ||
| KRX_ID: ${{ secrets.KRX_ID }} | ||
| KRX_PW: ${{ secrets.KRX_PW }} |
There was a problem hiding this comment.
Same issue as above: exporting KRX_ID/KRX_PW for test-quick will cause real login attempts outside VCR if session creation happens at import time. Consider keeping CI test runs hermetic by not injecting secrets unless running dedicated live-network jobs.
| env: | |
| KRX_ID: ${{ secrets.KRX_ID }} | |
| KRX_PW: ${{ secrets.KRX_PW }} |
| def build_krx_session( | ||
| login_id: str | None = None, login_pw: str | None = None | ||
| ) -> requests.Session: | ||
| if login_id is None: | ||
| login_id = os.getenv("KRX_ID") | ||
| if login_pw is None: | ||
| login_pw = os.getenv("KRX_PW") | ||
|
|
||
| if not (login_id and login_pw): | ||
| return None |
There was a problem hiding this comment.
build_krx_session() is annotated to return requests.Session but it returns None when credentials are missing. Please change the return type to requests.Session | None (or Optional[requests.Session]) so the API contract matches actual behavior and type-checkers/callers aren’t misled.
| def login_krx(session: requests.Session, login_id: str, login_pw: str) -> bool: | ||
| warmup_krx_session(session) | ||
|
|
There was a problem hiding this comment.
login_krx() calls warmup_krx_session(session), and build_krx_session() also warms up before calling login_krx(). This results in duplicate warmup requests when building a session; consider doing the warmup in only one place to avoid unnecessary extra HTTP calls.
7ac2c80 to
98c8bb5
Compare
98c8bb5 to
3f284e5
Compare
krx 홈페이지에서 작년 말 로그인을 필수로 하도록 정책이 바뀌었습니다.
ISSUE-276에서 제공해주신 login session을 생성하는 방식으로 수정했습니다.
KRX_ID/KRX_PW를 환경 변수로 등록하고 사용하시면 됩니다.ID/PWD를 사용하는 만큼 개인 계정 정보가 노출되는 것은 사용자의 몫입니다. 일부 기능은 계정 없이도 사용가능하니 선택하여 사용하시길 바랍니다.
이슈
getDataJson에 access하지 못하는 이슈가 있어 github ci를 수정했습니다. (remove: windows-latest)