feat(order): complete Stage 2 verification and Kafka e2e coverage#4
Open
jaehoon9875 wants to merge 9 commits into
Open
feat(order): complete Stage 2 verification and Kafka e2e coverage#4jaehoon9875 wants to merge 9 commits into
jaehoon9875 wants to merge 9 commits into
Conversation
- FastAPI 기반 order-service 전체 구조 구축 (router → service → repository → model 계층) - SQLAlchemy 2.0 비동기 모델 (Order, OrderItem) - Redis 캐시 연동 (주문 조회 캐시, 상태 변경 시 무효화) - Kafka 비동기 이벤트 발행 (order.created) - Alembic 마이그레이션 설정 - 단위/통합 테스트 코드 작성 - Python 3.12 버전 고정 (.python-version) 미완료: Alembic 첫 마이그레이션 생성 및 테스트 실행 (PLAN.md 참고) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix async response serialization and test fixture issues, add Alembic initial migration, and introduce Kafka-marked e2e test guidance so local validation and documentation align with Stage 2 completion criteria. Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough주문(order) 마이크로서비스가 새로 추가되었습니다. FastAPI 엔트리포인트, 비동기 DB 모델·레포지토리, Alembic 마이그레이션, Redis 캐시, aiokafka 프로듀서, Dockerfile, Python 3.12 규격과 단위·통합·E2E 테스트가 포함됩니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as FastAPI
participant Service as OrderService
participant Repo as OrderRepository
participant DB as PostgreSQL
participant Redis as Redis
participant Kafka as Kafka
Client->>API: POST /orders
API->>Service: create_order(payload)
Service->>Repo: create(order data)
Repo->>DB: INSERT orders, order_items
DB-->>Repo: persisted Order
Repo-->>Service: Order
Service->>Kafka: publish_order_created(order) (async)
Kafka-->>Service: ack
Service->>Redis: SET order:{id} (TTL)
Redis-->>Service: OK
Service-->>API: OrderResponse
API-->>Client: 201 + OrderResponse
Client->>API: GET /orders/{id}
API->>Service: get_order(id)
Service->>Redis: GET order:{id}
alt cache hit
Redis-->>Service: cached JSON
else cache miss
Service->>Repo: get_by_id(id)
Repo->>DB: SELECT ...
DB-->>Repo: Order
Repo-->>Service: Order
Service->>Redis: SET order:{id}
Redis-->>Service: OK
end
Service-->>API: OrderResponse
API-->>Client: 200 + OrderResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50분 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Drop an unused import in order repository to satisfy ruff F401 and unblock CI lint stage for the current PR. Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/CLAUDE.md`:
- Around line 16-23: Add a blank line before and after the fenced code block
that begins with ```bash so the markdown conforms to MD031; edit the CLAUDE.md
fenced block (the lines containing ```bash and the closing ```) to ensure there
is one empty line immediately above the opening ```bash and one empty line
immediately below the closing ``` to resolve the lint warning.
In `@apps/order-service/alembic/env.py`:
- Around line 29-37: The async migration flow in run_migrations_online splits
context.configure and context.run_migrations into separate connection.run_sync
calls and uses connection.begin() instead of Alembic's transaction pattern;
refactor run_migrations_online to follow the official async template by defining
a local do_run_migrations(conn) that calls context.configure(...,
connection=conn, target_metadata=target_metadata) and then uses
context.begin_transaction(): context.run_migrations() inside the same run_sync
invocation, then call create_async_engine(settings.DATABASE_URL), async with
engine.connect() as connection: await connection.run_sync(do_run_migrations) and
finally await engine.dispose() so configuration and migration run occur
atomically in one run_sync using context.begin_transaction().
In `@apps/order-service/alembic/script.py.mako`:
- Around line 8-17: The template uses typing.Union annotations; update the type
annotations to Python 3.10+ union syntax (|) for the variables revision,
down_revision, branch_labels, and depends_on (replace Union[str, None] with str
| None and Union[str, Sequence[str], None] with str | Sequence[str] | None), and
remove or adjust the import of Union in the top typing import accordingly so the
file compiles and new Alembic migrations are generated with modern syntax.
In `@apps/order-service/alembic/versions/9314af2d1ea2_create_orders_table.py`:
- Around line 42-45: The downgrade() currently drops order_items and orders but
does not remove the PostgreSQL Enum type used for the orders.orderstatus, which
leaves the custom type behind and breaks future upgrades; update downgrade()
(after op.drop_table('order_items')/op.drop_table('orders')) to explicitly
remove the enum type named "orderstatus" (for example via an op.execute("DROP
TYPE IF EXISTS orderstatus") or using sa.Enum(...).drop(op.get_bind(),
checkfirst=True) pattern) so the custom Enum is cleaned up.
In `@apps/order-service/app/dependencies.py`:
- Around line 31-44: The get_redis() dependency currently creates and closes a
new Redis client per request which is inefficient; change it to return a shared
pooled client instead: create a single aioredis.from_url (or aioredis.Redis
connection pool) instance at application startup (e.g., in an app startup
handler or a module-level initializer) and close it on shutdown, then modify
get_redis() to simply yield that shared pooled client (do not call from_url()
inside get_redis(), and do not aclose() it per-request); update any references
to get_redis to rely on the pooled client and ensure shutdown logic closes the
pool.
In `@apps/order-service/app/kafka/producer.py`:
- Around line 51-57: Wrap the call to _producer.send_and_wait (the async Kafka
publish) with a tenacity async retry so transient network errors are retried
before surfacing failure; use AsyncRetrying or a `@retry` decorator configured
with a limited number of attempts, exponential/backoff wait, and specific
retry-on exceptions (e.g., Kafka/network errors), and only log the
"order_created_event_published" failure after retries are exhausted so normal
transient failures are retried transparently while the existing service-layer
exception handling remains unchanged.
In `@apps/order-service/app/main.py`:
- Line 13: lifespan 함수에 반환 타입 힌트가 빠져 있습니다; `@asynccontextmanager` 패턴에 맞게 함수 정의
async def lifespan(app: FastAPI) 에 반환 타입 힌트를 추가하여 AsyncContextManager[None]으로
선언하세요 (즉 함수 시그니처를 lifespan(app: FastAPI) -> AsyncContextManager[None] 형태로 변경),
필요한 경우 typing에서 AsyncContextManager을 임포트하는 것도 추가하세요.
In `@apps/order-service/app/repositories/order.py`:
- Around line 57-65: update_status currently performs get_by_id → modify →
commit → get_by_id, causing three DB reads; replace the final get_by_id call
with await self.db.refresh(order) after commit to avoid re-querying the row,
i.e., keep the existing order instance updated via self.db.refresh(order) (use
symbols: update_status, get_by_id, self.db.commit, self.db.refresh, order and
order.items); if you need related items eager-loaded after refresh, either
eagerly load them in the initial get_by_id call or explicitly load order.items
after refresh (e.g., via selectinload in get_by_id or an explicit load),
otherwise the single refresh will reduce queries while preserving the updated
scalar fields.
In `@apps/order-service/app/routers/order.py`:
- Around line 16-27: Order creation must call inventory-service to reserve
stock: update OrderService.create_order (not the router) to perform an HTTP POST
to the inventory-service /inventory/reserve endpoint with the order item(s),
using tenacity to retry up to 3 times with exponential backoff; perform the
reserve call inside the same DB transaction/flow so that if the reserve
ultimately fails after retries you roll back the DB save and raise
HTTPException(status_code=503) back to the caller; ensure the
router.create_order continues to inject db: AsyncSession and redis:
aioredis.Redis (Depends(get_db), Depends(get_redis)) but the actual reserve
logic and retry should live in OrderService.create_order and use the injected db
transaction context and propagate errors so the router returns 503.
In `@apps/order-service/app/services/order.py`:
- Around line 70-90: The get_order method returns a dict on cache hits
(json.loads(cached)) but is declared to return Order | None, breaking type
consistency; update get_order to deserialize the cached JSON back into an Order
instance before returning (use the Order model's constructor or parsing method,
e.g., Order(**data) or Order.parse_obj(data)), ensuring the cache write uses
_order_to_dict and the cache read restores the same shape, and keep the
signature as Order | None (adjust _order_to_dict if needed to produce fields
required by Order).
In `@apps/order-service/Dockerfile`:
- Around line 18-20: Add a Docker HEALTHCHECK stanza to the Dockerfile (near
EXPOSE 8000 / before CMD ["uvicorn", "app.main:app", ...]) that periodically
probes the app (e.g., HTTP GET to http://localhost:8000/health or /ready) with a
sensible interval, timeout and retries so the orchestrator can detect unhealthy
containers; ensure the endpoint matches the app's health endpoint and adjust the
probe command (curl/wget) and values (interval, timeout, retries) accordingly.
- Around line 11-20: The Dockerfile runs as root; create and switch to a
non-root user by adding steps after COPY/CODE that create a uid/gid (e.g.,
adduser/addgroup or useradd), chown the WORKDIR (/app) and any installed files
(/usr/local) to that user, and set USER to that account before the CMD;
reference the existing WORKDIR, COPY --from=builder /install /usr/local, and CMD
["uvicorn", "app.main:app", ...] so the container executes uvicorn as the
non-root user.
In `@apps/order-service/README.md`:
- Around line 5-17: Fix the typo "실브로커" in the README section "1) 기본 테스트 (권장 기본
루틴)" by replacing it with a consistent, correct term such as "실 브로커" or "실제 브로커"
everywhere it appears (e.g., the sentence "- Kafka 실브로커 연동 테스트는 제외한다."), and
ensure the same chosen wording is used throughout the document for clarity.
In `@apps/order-service/requirements.txt`:
- Around line 13-17: The runtime image is installing test packages (pytest,
pytest-asyncio, pytest-cov, aiosqlite) from the current requirements.txt; split
test deps into a new requirements-dev.txt and remove
pytest/pytest-asyncio/pytest-cov/aiosqlite from the production requirements.txt,
then update the build/install flow to only pip install -r requirements.txt in
the production image and use requirements-dev.txt (or a separate dev build
stage) when running tests or building test artifacts so test deps do not end up
in the runtime image.
- Around line 14-15: Update the vulnerable pytest pin in requirements to a
secure minimum by replacing the current "pytest==8.3.3" entry with
"pytest>=9.0.3" (or a specific "pytest==9.0.3" if strict pinning is required);
ensure any dependency resolution or CI test environments are updated to install
the new pytest version and run tests to confirm compatibility, and also consider
updating "pytest-asyncio==0.24.0" only if compatibility issues arise with pytest
9.x.
In `@apps/order-service/tests/conftest.py`:
- Around line 13-35: Replace the in-memory SQLite create_all/drop_all approach
in db_session with a real PostgreSQL test database and per-test transaction
rollback: set TEST_DATABASE_URL to a Postgres test URI, create the async engine
via create_async_engine(TEST_DATABASE_URL), ensure Base.metadata.create_all is
run once on a connection before tests (use
connect.run_sync(Base.metadata.create_all)), and for each test in db_session
open a connection (engine.connect()), begin a transaction on that connection,
create an async_sessionmaker bound to the connection, yield a session, then
rollback the transaction and close the connection in teardown; stop using
Base.metadata.drop_all per-test. Reference symbols: TEST_DATABASE_URL,
db_session, create_async_engine, Base.metadata.create_all/drop_all,
async_sessionmaker.
- Around line 16-39: Add explicit async fixture return type hints: annotate
db_session as returning AsyncGenerator[AsyncSession, None] (import
AsyncGenerator from typing) and annotate async_client to return the async HTTP
client type used (e.g., AsyncClient from httpx) and ensure its db_session
parameter is typed as AsyncSession; update imports accordingly so the fixtures
read e.g. async def db_session() -> AsyncGenerator[AsyncSession, None] and async
def async_client(db_session: AsyncSession) -> AsyncClient.
In `@apps/order-service/tests/e2e/test_order_kafka_event.py`:
- Around line 59-62: The test currently catches the built-in TimeoutError;
update the exception handler to explicitly catch asyncio.TimeoutError to make
the intent clear and avoid ambiguity (change the except TimeoutError: in the
block that awaits consumer.getone() to except asyncio.TimeoutError:), ensuring
asyncio is referenced/imported where this occurs and leaving the rest of the
consumer.getone() wait_for logic unchanged.
In `@apps/order-service/tests/integration/test_order_router.py`:
- Around line 20-87: Add explicit return type hints (-> None) to all test
coroutine functions to satisfy the coding guideline; update each test function
definition (test_create_order, test_get_order, test_get_order_not_found,
test_update_order_status, test_health) to declare their return type as None
(e.g., async def test_create_order(async_client: AsyncClient) -> None:) while
leaving the async keyword, parameter type hints, and bodies unchanged.
- Around line 21-80: Add an integration test that verifies the failure path when
inventory reservation fails: copy the style of test_create_order but mock the
inventory reservation call (the service method that POSTs to /inventory/reserve
— e.g., patch "app.inventory.client.reserve" or the HTTP client call your order
service uses) to raise an exception or return a 5xx error even after retries,
also mock "app.kafka.producer.publish_order_created", then POST to "/orders"
with ORDER_PAYLOAD and assert the response status_code is 503 and no order was
persisted (e.g., response body indicates failure and a subsequent GET for the
returned/any id yields 404 or the DB contains no new order); ensure this test
covers the 3-retry behavior by asserting the reserve mock was called 3 times.
In `@apps/order-service/tests/unit/test_order_service.py`:
- Around line 53-100: Add a unit test that exercises OrderService.create_order’s
inventory reservation failure path: patch
app.services.order.OrderRepository.create to return fake_order, patch the
inventory client/called function (e.g., InventoryClient.reserve or whichever
symbol is used in create_order) to raise a transient exception and verify
tenacity retries are invoked 3 times with exponential backoff, then on final
failure ensure create_order maps the failure to a 503-style exception (or raises
the service-specific HTTPException) and that any DB side effects are rolled back
(i.e., repository.create was not persisted or a compensating delete was called);
use AsyncMock/side_effect for the inventory call and assert call_count == 3 and
the expected exception is raised/mapped.
- Around line 123-138: The test test_get_order_cache_miss_sets_cache only checks
that mock_redis.set was called but not that it used the expected key format or
TTL; update the assertion after invoking OrderService.get_order (and after
OrderRepository.get_by_id asserted) to assert mock_redis.set was called with the
key "order:{fake_order.id}" (use the same fake_order.id used in the test), the
correct serialized order value (or at least a value containing the order id),
and the expiry argument set to 300 seconds (e.g., ex=300) — reference the test
function test_get_order_cache_miss_sets_cache, OrderService.get_order path,
OrderRepository.get_by_id, and mock_redis.set when adding the more specific
assert_called_once_with check.
- Around line 15-153: Add missing return type hints: annotate fixtures mock_db()
-> AsyncMock, mock_redis() -> AsyncMock, fake_order() -> Order (ensure AsyncMock
and Order are imported or referenced), and add explicit return type -> None to
all async test functions (test_create_order_success,
test_create_order_kafka_failure_does_not_raise, test_get_order_cache_hit,
test_get_order_cache_miss_sets_cache, test_update_status_invalidates_cache).
Update imports if needed to include AsyncMock/Order typing symbols.
In `@docs/PLAN.md`:
- Around line 65-80: The fenced code block inside the blockquote in docs/PLAN.md
(the snippet that begins with the docker compose commands and includes the
alembic commands) needs blank lines placed immediately before the opening ```
and immediately after the closing ``` so the code fence is separated from the
surrounding blockquote text; update that blockquote so there is an empty line
before the code fence and another empty line after it to satisfy MD031.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70426cf5-0126-4274-a8a1-1c917c305c55
📒 Files selected for processing (34)
apps/CLAUDE.mdapps/order-service/.python-versionapps/order-service/Dockerfileapps/order-service/README.mdapps/order-service/alembic.iniapps/order-service/alembic/env.pyapps/order-service/alembic/script.py.makoapps/order-service/alembic/versions/9314af2d1ea2_create_orders_table.pyapps/order-service/app/__init__.pyapps/order-service/app/config.pyapps/order-service/app/dependencies.pyapps/order-service/app/kafka/__init__.pyapps/order-service/app/kafka/producer.pyapps/order-service/app/main.pyapps/order-service/app/models/__init__.pyapps/order-service/app/models/order.pyapps/order-service/app/repositories/__init__.pyapps/order-service/app/repositories/order.pyapps/order-service/app/routers/__init__.pyapps/order-service/app/routers/order.pyapps/order-service/app/schemas/__init__.pyapps/order-service/app/schemas/order.pyapps/order-service/app/services/__init__.pyapps/order-service/app/services/order.pyapps/order-service/pytest.iniapps/order-service/requirements.txtapps/order-service/tests/__init__.pyapps/order-service/tests/conftest.pyapps/order-service/tests/e2e/test_order_kafka_event.pyapps/order-service/tests/integration/__init__.pyapps/order-service/tests/integration/test_order_router.pyapps/order-service/tests/unit/__init__.pyapps/order-service/tests/unit/test_order_service.pydocs/PLAN.md
Comment on lines
+16
to
+23
| 로컬 환경 세팅: | ||
| ```bash | ||
| brew install python@3.12 # 아직 없으면 설치 | ||
| cd apps/order-service | ||
| python3.12 -m venv .venv # 반드시 python3.12 로 venv 생성 | ||
| source .venv/bin/activate | ||
| pip install -r requirements.txt | ||
| ``` |
There was a problem hiding this comment.
코드 블록 앞뒤 공백 규칙(MD031) 위반을 수정해주세요.
Line 17의 fenced code block 앞에 빈 줄 1줄이 없어 markdownlint 경고가 발생합니다.
✏️ 제안 수정안
로컬 환경 세팅:
+
```bash
brew install python@3.12 # 아직 없으면 설치
cd apps/order-service
python3.12 -m venv .venv # 반드시 python3.12 로 venv 생성
source .venv/bin/activate
pip install -r requirements.txt</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
로컬 환경 세팅:
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 17-17: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/CLAUDE.md` around lines 16 - 23, Add a blank line before and after the
fenced code block that begins with ```bash so the markdown conforms to MD031;
edit the CLAUDE.md fenced block (the lines containing ```bash and the closing
```) to ensure there is one empty line immediately above the opening ```bash and
one empty line immediately below the closing ``` to resolve the lint warning.
Comment on lines
+8
to
+17
| from typing import Sequence, Union | ||
|
|
||
| from alembic import op | ||
| import sqlalchemy as sa | ||
| ${imports if imports else ""} | ||
|
|
||
| revision: str = ${repr(up_revision)} | ||
| down_revision: Union[str, None] = ${repr(down_revision)} | ||
| branch_labels: Union[str, Sequence[str], None] = ${repr(branch_labels)} | ||
| depends_on: Union[str, Sequence[str], None] = ${repr(depends_on)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'from typing import .*Union|Union\[' apps/order-service/alembicRepository: jaehoon9875/sre-sample-app
Length of output: 956
Alembic 마이그레이션 템플릿을 Python 3.10+ 문법으로 업데이트하세요.
프로젝트 가이드라인에 따라 Union 대신 | 문법을 사용해야 합니다. 이 템플릿은 새로운 마이그레이션 파일을 생성하므로, 이를 수정하면 향후 마이그레이션이 모두 올바른 문법으로 생성됩니다.
♻️ 수정 방안
-from typing import Sequence, Union
+from collections.abc import Sequence
@@
-down_revision: Union[str, None] = ${repr(down_revision)}
-branch_labels: Union[str, Sequence[str], None] = ${repr(branch_labels)}
-depends_on: Union[str, Sequence[str], None] = ${repr(depends_on)}
+down_revision: str | None = ${repr(down_revision)}
+branch_labels: str | Sequence[str] | None = ${repr(branch_labels)}
+depends_on: str | Sequence[str] | None = ${repr(depends_on)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/order-service/alembic/script.py.mako` around lines 8 - 17, The template
uses typing.Union annotations; update the type annotations to Python 3.10+ union
syntax (|) for the variables revision, down_revision, branch_labels, and
depends_on (replace Union[str, None] with str | None and Union[str,
Sequence[str], None] with str | Sequence[str] | None), and remove or adjust the
import of Union in the top typing import accordingly so the file compiles and
new Alembic migrations are generated with modern syntax.
Comment on lines
+51
to
+57
| # send_and_wait: 브로커가 메시지를 받았다고 확인(ack)할 때까지 기다린다. | ||
| # 메시지는 bytes 로 직렬화해서 보낸다. | ||
| await _producer.send_and_wait( | ||
| "order.created", | ||
| json.dumps(payload).encode("utf-8"), | ||
| ) | ||
| logger.info("order_created_event_published", order_id=str(order.id)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Kafka 발행에 재시도 로직이 없습니다.
코딩 가이드라인에 따르면 외부 서비스 호출에는 tenacity를 사용한 재시도 로직이 필요합니다. send_and_wait는 네트워크 일시적 장애 시 실패할 수 있으므로 재시도가 권장됩니다.
서비스 레이어에서 예외를 catch하고 있지만, 재시도 후 실패 시에만 로깅하는 것이 더 안정적입니다.
♻️ tenacity 재시도 적용 예시
+from tenacity import retry, stop_after_attempt, wait_exponential, retry_if_exception_type
+from aiokafka.errors import KafkaError
+
+@retry(
+ stop=stop_after_attempt(3),
+ wait=wait_exponential(multiplier=1, min=1, max=10),
+ retry=retry_if_exception_type(KafkaError),
+ reraise=True,
+)
async def publish_order_created(order: Order) -> None:As per coding guidelines: "All external service calls must handle exceptions with appropriate retry and fallback logic using tenacity library with explicit configuration"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/order-service/app/kafka/producer.py` around lines 51 - 57, Wrap the call
to _producer.send_and_wait (the async Kafka publish) with a tenacity async retry
so transient network errors are retried before surfacing failure; use
AsyncRetrying or a `@retry` decorator configured with a limited number of
attempts, exponential/backoff wait, and specific retry-on exceptions (e.g.,
Kafka/network errors), and only log the "order_created_event_published" failure
after retries are exhausted so normal transient failures are retried
transparently while the existing service-layer exception handling remains
unchanged.
Comment on lines
+21
to
+80
| async def test_create_order(async_client: AsyncClient): | ||
| """ | ||
| POST /orders 가 주문을 생성하고 201 을 반환하는지 확인. | ||
| 실제 DB(인메모리 SQLite) 에 INSERT 가 일어난다. | ||
| """ | ||
| # Kafka 발행은 mock 처리 (테스트 환경에 Kafka 서버 없음) | ||
| with patch("app.kafka.producer.publish_order_created"): | ||
| response = await async_client.post("/orders", json=ORDER_PAYLOAD) | ||
|
|
||
| assert response.status_code == 201 | ||
| data = response.json() | ||
| assert data["user_id"] == 1 | ||
| assert data["status"] == "PENDING" | ||
| assert "id" in data | ||
| assert len(data["items"]) == 1 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_order(async_client: AsyncClient): | ||
| """ | ||
| GET /orders/{order_id} 가 생성된 주문을 올바르게 반환하는지 확인. | ||
| """ | ||
| with patch("app.kafka.producer.publish_order_created"): | ||
| create_response = await async_client.post("/orders", json=ORDER_PAYLOAD) | ||
|
|
||
| order_id = create_response.json()["id"] | ||
|
|
||
| response = await async_client.get(f"/orders/{order_id}") | ||
| assert response.status_code == 200 | ||
| assert response.json()["id"] == order_id | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_order_not_found(async_client: AsyncClient): | ||
| """ | ||
| 존재하지 않는 order_id 로 조회 시 404 를 반환하는지 확인. | ||
| """ | ||
| non_existent_id = str(uuid.uuid4()) | ||
| response = await async_client.get(f"/orders/{non_existent_id}") | ||
| assert response.status_code == 404 | ||
| assert response.json()["detail"] == "Order not found" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_order_status(async_client: AsyncClient): | ||
| """ | ||
| PATCH /orders/{order_id}/status 로 상태 변경이 정상 동작하는지 확인. | ||
| """ | ||
| with patch("app.kafka.producer.publish_order_created"): | ||
| create_response = await async_client.post("/orders", json=ORDER_PAYLOAD) | ||
|
|
||
| order_id = create_response.json()["id"] | ||
|
|
||
| response = await async_client.patch( | ||
| f"/orders/{order_id}/status", | ||
| json={"status": "CONFIRMED"}, | ||
| ) | ||
| assert response.status_code == 200 | ||
| assert response.json()["status"] == "CONFIRMED" | ||
|
|
There was a problem hiding this comment.
필수 시나리오(재고 예약 실패 시 503/롤백) 통합 테스트가 없습니다.
POST /orders의 성공 경로만 검증하고 있어, 재고 서비스 실패 시 주문 생성 롤백 및 503 반환 계약을 회귀 방지하지 못합니다. 이 실패 경로 테스트를 추가해야 합니다.
원하시면 해당 실패 시나리오(3회 재시도 포함) 통합 테스트 케이스 초안을 바로 만들어 드리겠습니다.
Based on learnings: Applies to apps/order-service/**/order.py : 주문 생성 시 inventory-service의 POST /inventory/reserve 엔드포인트 호출하여 재고를 차감해야 하며, 실패 시 주문 생성을 롤백하고 503을 반환.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/order-service/tests/integration/test_order_router.py` around lines 21 -
80, Add an integration test that verifies the failure path when inventory
reservation fails: copy the style of test_create_order but mock the inventory
reservation call (the service method that POSTs to /inventory/reserve — e.g.,
patch "app.inventory.client.reserve" or the HTTP client call your order service
uses) to raise an exception or return a 5xx error even after retries, also mock
"app.kafka.producer.publish_order_created", then POST to "/orders" with
ORDER_PAYLOAD and assert the response status_code is 503 and no order was
persisted (e.g., response body indicates failure and a subsequent GET for the
returned/any id yields 404 or the DB contains no new order); ensure this test
covers the 3-retry behavior by asserting the reserve mock was called 3 times.
Comment on lines
+53
to
+100
| @pytest.mark.asyncio | ||
| async def test_create_order_success(mock_db, mock_redis, fake_order): | ||
| """ | ||
| 주문 생성 성공 케이스. | ||
| - OrderRepository.create 가 호출되고 order 가 반환되는지 확인 | ||
| - Kafka 이벤트 발행이 시도되는지 확인 | ||
| """ | ||
| # patch: 특정 모듈 경로의 객체를 테스트 중에만 가짜로 교체한다. | ||
| # "app.services.order.OrderRepository" 를 mock 으로 교체해서 DB 호출을 막는다. | ||
| with patch("app.services.order.OrderRepository") as MockRepo, \ | ||
| patch("app.services.order.kafka_producer.publish_order_created") as mock_publish: | ||
|
|
||
| # create 메서드가 fake_order 를 반환하도록 설정 | ||
| MockRepo.return_value.create = AsyncMock(return_value=fake_order) | ||
| mock_publish.return_value = None # Kafka 발행은 아무것도 하지 않음 | ||
|
|
||
| service = OrderService(db=mock_db, redis=mock_redis) | ||
| data = OrderCreate(user_id=1, items=[OrderItemCreate(product_id=101, quantity=2, price=Decimal("15000"))]) | ||
|
|
||
| result = await service.create_order(data) | ||
|
|
||
| # 결과 검증 | ||
| assert result.user_id == 1 | ||
| assert result.status == OrderStatus.PENDING | ||
| MockRepo.return_value.create.assert_called_once_with(data) | ||
| mock_publish.assert_called_once_with(fake_order) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_create_order_kafka_failure_does_not_raise(mock_db, mock_redis, fake_order): | ||
| """ | ||
| Kafka 발행 실패 시 주문 생성이 롤백되지 않고 정상 반환되는지 확인. | ||
| 이벤트 유실은 허용하지만 주문 자체는 유지해야 한다. | ||
| """ | ||
| with patch("app.services.order.OrderRepository") as MockRepo, \ | ||
| patch("app.services.order.kafka_producer.publish_order_created") as mock_publish: | ||
|
|
||
| MockRepo.return_value.create = AsyncMock(return_value=fake_order) | ||
| # Kafka 발행에서 예외 발생하도록 설정 | ||
| mock_publish.side_effect = Exception("Kafka connection refused") | ||
|
|
||
| service = OrderService(db=mock_db, redis=mock_redis) | ||
| data = OrderCreate(user_id=1, items=[OrderItemCreate(product_id=101, quantity=2, price=Decimal("15000"))]) | ||
|
|
||
| # 예외가 전파되지 않고 order 가 정상 반환되어야 한다 | ||
| result = await service.create_order(data) | ||
| assert result is fake_order | ||
|
|
There was a problem hiding this comment.
재고 예약 실패/재시도/롤백 경로에 대한 서비스 단위 테스트가 필요합니다.
주문 생성 성공 및 Kafka 실패는 커버하지만, 필수 계약인 inventory reserve 실패 시 롤백 및 오류 처리 경로가 누락되어 있습니다.
원하시면 tenacity 3회 재시도와 최종 실패 시 예외 매핑(503)까지 검증하는 단위 테스트 케이스를 작성해 드리겠습니다.
Based on learnings: Applies to apps/order-service/**/order.py : inventory-service 호출 시 재시도는 3회 수행하며 exponential backoff를 적용.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/order-service/tests/unit/test_order_service.py` around lines 53 - 100,
Add a unit test that exercises OrderService.create_order’s inventory reservation
failure path: patch app.services.order.OrderRepository.create to return
fake_order, patch the inventory client/called function (e.g.,
InventoryClient.reserve or whichever symbol is used in create_order) to raise a
transient exception and verify tenacity retries are invoked 3 times with
exponential backoff, then on final failure ensure create_order maps the failure
to a 503-style exception (or raises the service-specific HTTPException) and that
any DB side effects are rolled back (i.e., repository.create was not persisted
or a compensating delete was called); use AsyncMock/side_effect for the
inventory call and assert call_count == 3 and the expected exception is
raised/mapped.
Comment on lines
+65
to
+80
| > **재개 방법**: docker-compose로 PostgreSQL을 먼저 기동한 뒤 진행한다. | ||
| > ```bash | ||
| > # 프로젝트 루트에서 | ||
| > docker compose up -d postgres | ||
| > | ||
| > # apps/order-service/.env 생성 (로컬용 — localhost 사용) | ||
| > cp .env.example .env | ||
| > # .env 의 DATABASE_URL 에서 postgres → localhost 로 변경 | ||
| > # Kafka 포트도 9092 → 29092 로 변경 (로컬 외부 노출 포트) | ||
| > | ||
| > # 마이그레이션 생성 및 적용 | ||
| > cd apps/order-service | ||
| > source .venv/bin/activate | ||
| > alembic revision --autogenerate -m "create orders table" | ||
| > alembic upgrade head | ||
| > ``` |
There was a problem hiding this comment.
코드 펜스 주변 공백 규칙(MD031)을 맞춰주세요.
블록 인용(>) 내부 코드블록 시작/종료 전후에 빈 줄이 없어 markdownlint 경고가 발생합니다.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 66-66: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/PLAN.md` around lines 65 - 80, The fenced code block inside the
blockquote in docs/PLAN.md (the snippet that begins with the docker compose
commands and includes the alembic commands) needs blank lines placed immediately
before the opening ``` and immediately after the closing ``` so the code fence
is separated from the surrounding blockquote text; update that blockquote so
there is an empty line before the code fence and another empty line after it to
satisfy MD031.
Use SettingsConfigDict for BaseSettings model_config and annotate settings initialization to avoid mypy typed-dict/call-arg false positives in CI. Made-with: Cursor
order-service의 Settings에 기본 연결 값을 넣어 테스트/타입체크 단계에서 초기화 오류가 나지 않게 정리했습니다. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/order-service/app/repositories/order.py`:
- Around line 20-43: The create method currently ignores a possible None from
get_by_id using "type: ignore"; fix it by ensuring create always returns an
Order instance: after self.db.add(order) call flush/commit and refresh or
re-query the created order, then if get_by_id(order.id) returns None raise a
clear exception (e.g., ValueError or a domain-specific exception) instead of
silencing it; remove the "type: ignore" and have create return the validated
Order (references: create, get_by_id, Order, OrderItem, self.db.add,
self.db.commit / self.db.flush + self.db.refresh).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a27129be-bf83-4bea-8ab9-4c0b69c3580f
📒 Files selected for processing (2)
apps/order-service/app/config.pyapps/order-service/app/repositories/order.py
Alembic 비동기 마이그레이션 패턴과 enum downgrade 정리를 보강하고, Redis/Kafka 테스트 경로의 수명주기 누수를 줄여 회귀 가능성을 낮췄다. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
apps/order-service/tests/conftest.py (2)
37-60: 🧹 Nitpick | 🔵 Trivial
async_client픽스처에 반환 타입 힌트를 추가하세요.제안 수정
`@pytest_asyncio.fixture` -async def async_client(db_session: AsyncSession): +async def async_client(db_session: AsyncSession) -> AsyncGenerator[AsyncClient, None]: """ 테스트용 HTTP 클라이언트.As per coding guidelines:
apps/**/*.py: All functions must include type hints for both parameters and return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/tests/conftest.py` around lines 37 - 60, The fixture async_client lacks a return type hint; update its signature to include an async generator return type (e.g., async def async_client(db_session: AsyncSession) -> AsyncGenerator[AsyncClient, None]:), add the corresponding import from typing (AsyncGenerator) at the top of the file, and ensure AsyncClient is imported or referenced consistently so tools enforce the apps/**/*.py type-hint rule; keep the existing logic using app.dependency_overrides, get_db, get_redis, AsyncMock, and ASGITransport unchanged.
15-34: 🧹 Nitpick | 🔵 Trivial픽스처 함수에 반환 타입 힌트를 추가하세요.
db_session과async_client픽스처 모두 반환 타입 힌트가 없어 코딩 가이드라인을 위반합니다.제안 수정
+from collections.abc import AsyncGenerator + `@pytest_asyncio.fixture` -async def db_session(): +async def db_session() -> AsyncGenerator[AsyncSession, None]: """ 테스트마다 새 인메모리 DB 를 만들고 테스트 후 삭제한다. 각 테스트가 독립적인 DB 상태에서 실행되므로 서로 간섭하지 않는다. """As per coding guidelines:
apps/**/*.py: All functions must include type hints for both parameters and return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/tests/conftest.py` around lines 15 - 34, Add explicit return type hints to the async fixtures: annotate db_session as returning AsyncGenerator[AsyncSession, None] and annotate async_client as returning AsyncGenerator[AsyncClient, None]; update imports accordingly (e.g. from typing import AsyncGenerator, from sqlalchemy.ext.asyncio import AsyncSession, and the AsyncClient type from your test HTTP client like httpx) so both fixture signatures include proper return type hints (e.g., async def db_session() -> AsyncGenerator[AsyncSession, None]: and async def async_client() -> AsyncGenerator[AsyncClient, None]:).apps/order-service/app/main.py (1)
13-14: 🧹 Nitpick | 🔵 Trivial
lifespan함수에 반환 타입 힌트를 추가하세요.
@asynccontextmanager데코레이터를 사용하는 함수는AsyncIterator[None]을 반환 타입으로 명시해야 합니다.제안 수정
+from collections.abc import AsyncIterator + `@asynccontextmanager` -async def lifespan(app: FastAPI): +async def lifespan(app: FastAPI) -> AsyncIterator[None]:As per coding guidelines:
apps/**/*.py: All functions must include type hints for both parameters and return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/app/main.py` around lines 13 - 14, Add an explicit return type hint to the asynccontextmanager function by changing the signature of lifespan to return AsyncIterator[None] (i.e., async def lifespan(app: FastAPI) -> AsyncIterator[None]:) and ensure AsyncIterator is imported (from typing import AsyncIterator) so the function complies with the project's typing guideline; keep the existing app: FastAPI parameter type as-is.apps/order-service/tests/unit/test_order_service.py (2)
123-138:⚠️ Potential issue | 🟠 Major캐시 계약 회귀를 잡기엔 assertion이 너무 느슨합니다.
redis.set호출 여부만 보면order:{order_id}키 형식이나 TTL 300초가 바뀌어도 테스트가 통과합니다. 이 케이스는 call args까지 검증해야 합니다.Based on learnings: Applies to apps/order-service/**/order.py : 주문 조회 시 Redis 캐시를 우선 조회하며, order:{order_id} 키 형식을 사용하고 TTL은 300초로 설정.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/tests/unit/test_order_service.py` around lines 123 - 138, The test test_get_order_cache_miss_sets_cache is too loose because it only asserts mock_redis.set was called; update it to assert the exact call args: verify OrderService.get_order sets Redis with key formatted as f"order:{fake_order.id}", the expected serialized value (or fake_order if your service stores the object directly) and the TTL of 300 seconds; use mock_redis.set.assert_called_once_with(...) or inspect mock_redis.set.call_args to check (refer to test symbols mock_redis, fake_order, and the OrderService.get_order flow that uses OrderRepository.get_by_id).
15-27:⚠️ Potential issue | 🟠 Major픽스처와 테스트 함수 반환 타입 힌트가 아직 빠져 있습니다.
mock_db,mock_redis, 그리고 각 async 테스트에-> AsyncMock/-> None같은 명시적 반환 타입을 넣어 주세요.As per coding guidelines:
apps/**/*.py: All functions must include type hints for both parameters and return values.Also applies to: 53-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/tests/unit/test_order_service.py` around lines 15 - 27, Add explicit return type hints to the fixtures and async test functions: update the mock_db fixture signature to "def mock_db() -> AsyncMock" and mock_redis to "def mock_redis() -> AsyncMock", and for each async test coroutine in this file (tests between lines ~53-154) add a return type hint "-> None" (e.g., "async def test_xyz(...) -> None"). Ensure AsyncMock is imported or referenced consistently where used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/order-service/alembic/env.py`:
- Line 29: Add a type hint for the connection parameter on the function
do_run_migrations to satisfy the codebase typing rule; change the signature to
accept a sqlalchemy Connection (e.g., Connection from sqlalchemy.engine) and
keep the existing -> None return type, and add the corresponding import (from
sqlalchemy.engine import Connection) at the top of the module so the annotation
resolves.
In `@apps/order-service/app/main.py`:
- Around line 21-29: The code logs "order_service_started" before confirming
Kafka started and calls close_redis() in finally even if kafka_producer.start()
fails; change the startup sequence so init_redis() runs first, then attempt
kafka_producer.start() inside a try/except, only emit
logger.info("order_service_started") after kafka_producer.start() succeeds, and
on failure log the kafka error (include exception details) before cleaning up;
also track whether kafka_producer was started (e.g., started_flag) so
kafka_producer.stop() is only called if started, and always call close_redis()
in the finally block to ensure Redis is cleaned up.
In `@apps/order-service/app/services/order.py`:
- Around line 75-86: The Redis calls in the order service should not raise
service errors to callers; wrap self.redis.get(cache_key) in a try/except so any
exception is logged (use logger.warn/info) and treated as a cache miss (proceed
to repo.get_by_id), and wrap self.redis.set(cache_key,
json.dumps(_order_to_dict(order)), ex=CACHE_TTL) (and any redis.delete calls
around lines 96-99) in try/except so failures are logged but ignored; keep
returning OrderResponse.model_validate_json(cached) on successful cache read,
fall back to repo.get_by_id when get fails, and never let redis exceptions
propagate from methods in this module.
- Around line 57-69: The create_order method currently persists the order before
reserving inventory; change the flow in create_order to call the
inventory-service POST /inventory/reserve first (using the inventory client),
and only if that reserve succeeds then call self.repo.create(order_data) to
persist; if the reserve call fails, raise/return a 503 and do not create the
order. Also handle the edge where repo.create fails after a successful reserve
by calling the inventory release/cancel endpoint (or compensating reserve
rollback) to avoid leaked reservations, and keep existing
kafka_producer.publish_order_created in the happy path (catching publish errors
but not masking reserve/create failures). Reference symbols: create_order,
self.repo.create, kafka_producer.publish_order_created, and the inventory
reserve/release endpoints.
In `@apps/order-service/tests/e2e/test_order_kafka_event.py`:
- Around line 10-12: The test function definition
test_order_created_event_published_to_kafka should include an explicit return
type hint; update the async def signature for
test_order_created_event_published_to_kafka to async def
test_order_created_event_published_to_kafka() -> None: so it complies with the
project's rule that all functions in apps/**/*.py include parameter and return
type hints.
In `@docs/TEMP-PR4-REVIEW-TODO.md`:
- Around line 58-63: Add explicit HTML TODO comments next to each incomplete
checklist item in the docs so they follow the coding guideline: insert <!--
TODO: describe what remains --> immediately after each unchecked box line (e.g.,
the entries referencing apps/order-service/README.md, apps/CLAUDE.md,
docs/PLAN.md and the e2e timeout note) in the TEMP-PR4 checklist content; ensure
the comment briefly states the required action (e.g., "fix typo", "apply MD031
rule", "add type hints", "use asyncio.TimeoutError") and preserves the existing
checkbox formatting.
---
Duplicate comments:
In `@apps/order-service/app/main.py`:
- Around line 13-14: Add an explicit return type hint to the asynccontextmanager
function by changing the signature of lifespan to return AsyncIterator[None]
(i.e., async def lifespan(app: FastAPI) -> AsyncIterator[None]:) and ensure
AsyncIterator is imported (from typing import AsyncIterator) so the function
complies with the project's typing guideline; keep the existing app: FastAPI
parameter type as-is.
In `@apps/order-service/tests/conftest.py`:
- Around line 37-60: The fixture async_client lacks a return type hint; update
its signature to include an async generator return type (e.g., async def
async_client(db_session: AsyncSession) -> AsyncGenerator[AsyncClient, None]:),
add the corresponding import from typing (AsyncGenerator) at the top of the
file, and ensure AsyncClient is imported or referenced consistently so tools
enforce the apps/**/*.py type-hint rule; keep the existing logic using
app.dependency_overrides, get_db, get_redis, AsyncMock, and ASGITransport
unchanged.
- Around line 15-34: Add explicit return type hints to the async fixtures:
annotate db_session as returning AsyncGenerator[AsyncSession, None] and annotate
async_client as returning AsyncGenerator[AsyncClient, None]; update imports
accordingly (e.g. from typing import AsyncGenerator, from sqlalchemy.ext.asyncio
import AsyncSession, and the AsyncClient type from your test HTTP client like
httpx) so both fixture signatures include proper return type hints (e.g., async
def db_session() -> AsyncGenerator[AsyncSession, None]: and async def
async_client() -> AsyncGenerator[AsyncClient, None]:).
In `@apps/order-service/tests/unit/test_order_service.py`:
- Around line 123-138: The test test_get_order_cache_miss_sets_cache is too
loose because it only asserts mock_redis.set was called; update it to assert the
exact call args: verify OrderService.get_order sets Redis with key formatted as
f"order:{fake_order.id}", the expected serialized value (or fake_order if your
service stores the object directly) and the TTL of 300 seconds; use
mock_redis.set.assert_called_once_with(...) or inspect mock_redis.set.call_args
to check (refer to test symbols mock_redis, fake_order, and the
OrderService.get_order flow that uses OrderRepository.get_by_id).
- Around line 15-27: Add explicit return type hints to the fixtures and async
test functions: update the mock_db fixture signature to "def mock_db() ->
AsyncMock" and mock_redis to "def mock_redis() -> AsyncMock", and for each async
test coroutine in this file (tests between lines ~53-154) add a return type hint
"-> None" (e.g., "async def test_xyz(...) -> None"). Ensure AsyncMock is
imported or referenced consistently where used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca2f3a9b-ca33-406b-9376-89b101028447
📒 Files selected for processing (11)
apps/order-service/alembic/env.pyapps/order-service/alembic/versions/9314af2d1ea2_create_orders_table.pyapps/order-service/app/config.pyapps/order-service/app/dependencies.pyapps/order-service/app/main.pyapps/order-service/app/repositories/order.pyapps/order-service/app/services/order.pyapps/order-service/tests/conftest.pyapps/order-service/tests/e2e/test_order_kafka_event.pyapps/order-service/tests/unit/test_order_service.pydocs/TEMP-PR4-REVIEW-TODO.md
Comment on lines
+57
to
+69
| async def create_order(self, data: OrderCreate) -> Order: | ||
| # 1. DB 에 주문 저장 | ||
| order = await self.repo.create(data) | ||
| logger.info("order_created", order_id=str(order.id), user_id=order.user_id) | ||
|
|
||
| # 2. Kafka 이벤트 발행. | ||
| # 실패해도 주문 자체는 유지한다 (이벤트 유실은 별도로 처리할 운영 이슈). | ||
| try: | ||
| await kafka_producer.publish_order_created(order) | ||
| except Exception as e: | ||
| logger.error("kafka_publish_failed", order_id=str(order.id), error=str(e)) | ||
|
|
||
| return order |
There was a problem hiding this comment.
재고 예약 없이 주문을 저장하면 Stage 2 계약을 깨뜨립니다.
create_order()가 바로 DB 저장으로 들어가서 inventory-service의 /inventory/reserve 호출, 실패 시 롤백, 503 반환 경로가 전혀 없습니다. 지금 상태에서는 재고 차감 없이 주문이 생성되고, reserve 실패를 되돌릴 트랜잭션 경계도 없습니다.
As per coding guidelines: apps/order-service/**/*order*.py: 주문 생성 시 inventory-service의 POST /inventory/reserve 엔드포인트 호출하여 재고를 차감해야 하며, 실패 시 주문 생성을 롤백하고 503을 반환.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/order-service/app/services/order.py` around lines 57 - 69, The
create_order method currently persists the order before reserving inventory;
change the flow in create_order to call the inventory-service POST
/inventory/reserve first (using the inventory client), and only if that reserve
succeeds then call self.repo.create(order_data) to persist; if the reserve call
fails, raise/return a 503 and do not create the order. Also handle the edge
where repo.create fails after a successful reserve by calling the inventory
release/cancel endpoint (or compensating reserve rollback) to avoid leaked
reservations, and keep existing kafka_producer.publish_order_created in the
happy path (catching publish errors but not masking reserve/create failures).
Reference symbols: create_order, self.repo.create,
kafka_producer.publish_order_created, and the inventory reserve/release
endpoints.
Alembic·lifespan·테스트 타입 힌트를 보강하고 Redis 캐시 예외를 안전하게 흡수해 서비스 실패 전파를 줄였다. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
apps/order-service/app/services/order.py (1)
57-69:⚠️ Potential issue | 🔴 Critical재고 reserve 없이 주문을 저장하면 Stage 2 계약이 계속 깨집니다.
create_order()가 여전히repo.create()부터 수행해서 재고 차감 실패를 막을 수 없습니다. 여기서 inventory-service의/inventory/reserve를 먼저 호출하고, 3회 exponential backoff 재시도 후 최종 실패를 503으로 매핑해야 합니다. reserve 성공 뒤 DB 저장이 실패하는 경우의 보상 해제까지 같이 있어야 데이터가 일관됩니다.As per coding guidelines:
apps/order-service/**/*order*.py: 주문 생성 시 inventory-service의 POST /inventory/reserve 엔드포인트 호출하여 재고를 차감해야 하며, 실패 시 주문 생성을 롤백하고 503을 반환 /apps/**/services/**/*.py: Usetenacitylibrary for explicit retry and fallback handling in inter-service HTTP calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/app/services/order.py` around lines 57 - 69, create_order currently persists the order before reserving inventory; change the flow to call inventory_client.reserve(...) (use the existing inventory client or create one) before calling repo.create(order) and use tenacity to perform 3 retries with exponential backoff on reserve; if the reserve still fails after retries, raise an HTTP 503 (map to the service error) and do not create the order. Additionally, if reserve succeeds but repo.create(...) later fails, call inventory_client.release(...) as a compensating action to undo the reserve, then raise the error; keep the existing kafka_producer.publish_order_created(...) call after successful DB save as-is. Ensure you reference and update create_order, repo.create, kafka_producer.publish_order_created, inventory_client.reserve and inventory_client.release when making these changes.apps/order-service/tests/unit/test_order_service.py (1)
54-54:⚠️ Potential issue | 🟡 Minor테스트 함수 파라미터 타입 힌트도 채워 주세요.
반환 타입은 추가됐지만
mock_db,mock_redis,fake_order파라미터가 아직 untyped입니다. 이 파일도 공통 규칙 대상이라 각 테스트 시그니처에AsyncMock/Order타입을 명시하는 편이 맞습니다.As per coding guidelines:
apps/**/*.py: Add type hints to all function parameters and return values.Also applies to: 82-82, 103-103, 123-123, 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/tests/unit/test_order_service.py` at line 54, The test function test_create_order_success has untyped parameters; add parameter type hints: mock_db: AsyncMock, mock_redis: AsyncMock, fake_order: Order and keep the return type -> None; import AsyncMock from unittest.mock (or typing) and Order from the module that defines the Order model used in tests (e.g., your orders schema/module). Apply the same change to the other test signatures flagged (the other test functions in this file) so all test parameters and returns follow the apps/**/*.py typing guideline.apps/order-service/tests/conftest.py (1)
11-36:⚠️ Potential issue | 🟠 Major통합 테스트가 실제 DB 경로를 검증하지 못합니다.
인메모리 SQLite +
create_all/drop_all은 Alembic/PostgreSQL 경로와 타입 차이를 우회해서 Stage 2 검증 신뢰도가 떨어집니다. 통합 테스트는 실제 Postgres 테스트 DB를 사용하고, 테스트 단위 트랜잭션 롤백으로 격리하는 구성이 필요합니다.Based on learnings: Integration tests must use actual database with transaction rollback for isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/tests/conftest.py` around lines 11 - 36, The fixture currently uses an in-memory SQLite (TEST_DATABASE_URL) and Base.metadata.create_all/drop_all per test, which doesn't validate the real Postgres/Alembic path; change db_session to use a real Postgres test database URL (from env/config) by creating a create_async_engine(TEST_DATABASE_URL) that points to Postgres, perform schema setup once (Base.metadata.create_all) before the test run (not per test), and then for each test open a single connection and start a top-level transaction and a nested SAVEPOINT (use connection.begin() + connection.begin_nested()/session.begin_nested()) to provide isolation and rollback after the test; keep using async_sessionmaker but bind sessions to the test connection so each test rolls back instead of creating/dropping DB objects. Ensure references: TEST_DATABASE_URL, db_session fixture, create_async_engine, async_sessionmaker, and Base.metadata.create_all/drop_all are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/order-service/alembic/env.py`:
- Around line 36-40: The run_migrations_online function must guarantee engine
disposal on all paths; modify it so the async engine is created before a try
block (engine = create_async_engine(settings.DATABASE_URL)), run the
connection/await connection.run_sync(do_run_migrations) inside the try (keeping
async with engine.connect()), and put await engine.dispose() in a finally block
(guarding if engine is not None) so disposal happens even if do_run_migrations
or connection creation raises; update function run_migrations_online and
references to engine/do_run_migrations accordingly.
In `@apps/order-service/app/main.py`:
- Around line 22-30: Move the call to init_redis() into the same guarded
start-up try block so Redis initialization failures are caught, logged with the
same "order_service_start_failed" message, and trigger the existing cleanup
path; specifically, wrap init_redis() together with await kafka_producer.start()
inside the try that sets kafka_started and logs "order_service_started", ensure
the except block catches exceptions from init_redis() and
kafka_producer.start(), calls logger.error(..., error=str(exc)) and re-raises so
existing teardown logic runs, and keep the kafka_started flag management
(kafka_started variable, kafka_producer.start) unchanged.
- Around line 46-49: Change the /health endpoint to return a Pydantic v2 model
instead of a raw dict: define a small Pydantic model (e.g., class
HealthResponse(BaseModel) with field status: str) and update the `@app.get`
decorator to use response_model=HealthResponse on the health function; update
the function signature to return HealthResponse (or
HealthResponse.model_validate if constructing from dict) and import BaseModel
from pydantic v2 so the endpoint follows the project's response_model pattern.
In `@docs/TEMP-PR4-REVIEW-TODO.md`:
- Around line 33-43: The markdown numbered list uses mixed numbers (1), 2), 3),
4)) and violates MD029; update the three list items that start with "2)", "3)",
and "4)" so they each use "1)" like the first item (i.e., normalize all four
bullets to "1) ...") in the document section that contains the
Alembic/versions/services/dependencies checklist lines, then run the markdown
linter to confirm MD029 compliance.
---
Duplicate comments:
In `@apps/order-service/app/services/order.py`:
- Around line 57-69: create_order currently persists the order before reserving
inventory; change the flow to call inventory_client.reserve(...) (use the
existing inventory client or create one) before calling repo.create(order) and
use tenacity to perform 3 retries with exponential backoff on reserve; if the
reserve still fails after retries, raise an HTTP 503 (map to the service error)
and do not create the order. Additionally, if reserve succeeds but
repo.create(...) later fails, call inventory_client.release(...) as a
compensating action to undo the reserve, then raise the error; keep the existing
kafka_producer.publish_order_created(...) call after successful DB save as-is.
Ensure you reference and update create_order, repo.create,
kafka_producer.publish_order_created, inventory_client.reserve and
inventory_client.release when making these changes.
In `@apps/order-service/tests/conftest.py`:
- Around line 11-36: The fixture currently uses an in-memory SQLite
(TEST_DATABASE_URL) and Base.metadata.create_all/drop_all per test, which
doesn't validate the real Postgres/Alembic path; change db_session to use a real
Postgres test database URL (from env/config) by creating a
create_async_engine(TEST_DATABASE_URL) that points to Postgres, perform schema
setup once (Base.metadata.create_all) before the test run (not per test), and
then for each test open a single connection and start a top-level transaction
and a nested SAVEPOINT (use connection.begin() +
connection.begin_nested()/session.begin_nested()) to provide isolation and
rollback after the test; keep using async_sessionmaker but bind sessions to the
test connection so each test rolls back instead of creating/dropping DB objects.
Ensure references: TEST_DATABASE_URL, db_session fixture, create_async_engine,
async_sessionmaker, and Base.metadata.create_all/drop_all are updated
accordingly.
In `@apps/order-service/tests/unit/test_order_service.py`:
- Line 54: The test function test_create_order_success has untyped parameters;
add parameter type hints: mock_db: AsyncMock, mock_redis: AsyncMock, fake_order:
Order and keep the return type -> None; import AsyncMock from unittest.mock (or
typing) and Order from the module that defines the Order model used in tests
(e.g., your orders schema/module). Apply the same change to the other test
signatures flagged (the other test functions in this file) so all test
parameters and returns follow the apps/**/*.py typing guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 386c66ef-8fb3-4d25-a048-dc56625f5729
📒 Files selected for processing (7)
apps/order-service/alembic/env.pyapps/order-service/app/main.pyapps/order-service/app/services/order.pyapps/order-service/tests/conftest.pyapps/order-service/tests/e2e/test_order_kafka_event.pyapps/order-service/tests/unit/test_order_service.pydocs/TEMP-PR4-REVIEW-TODO.md
마이그레이션 리소스 정리 보장을 추가하고 앱 시작/헬스 응답 및 테스트 타입 힌트를 정리해 CodeRabbit의 저비용 지적사항을 우선 해소했다. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/order-service/tests/unit/test_order_service.py (1)
53-108:⚠️ Potential issue | 🟠 Major
create_order필수 계약(재고 예약 실패/재시도/503 매핑) 테스트가 여전히 비어 있습니다.현재 테스트는 성공/Kafka 실패만 검증합니다. 재고 예약 실패 시나리오(3회 재시도 + 최종 실패 매핑 + 롤백/보상 동작)도 단위 테스트로 고정해야 회귀를 막을 수 있습니다.
Based on learnings: Applies to apps/order-service/**/order.py : 주문 생성 시 inventory-service의 POST /inventory/reserve 엔드포인트 호출하여 재고를 차감해야 하며, 실패 시 주문 생성을 롤백하고 503을 반환, inventory-service 호출 시 재시도는 3회 수행하며 exponential backoff를 적용.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/tests/unit/test_order_service.py` around lines 53 - 108, Add a new unit test for OrderService.create_order that simulates inventory-service POST /inventory/reserve failing so you can verify the 3-retry + rollback + 503 mapping contract: patch the inventory client used by create_order (the same symbol the service calls) so its reserve method raises an exception on every call, assert it was invoked exactly 3 times with the expected reserve payload (to validate retries and retry count), assert OrderRepository.create does not persist the order (or that a compensating method like OrderRepository.delete/Rollback was called) and assert create_order surfaces a 503-mapped error (e.g., raises HTTPException with status_code 503 or returns the mapped response), and ensure any Kafka publishing is not attempted in this failure path by asserting publish_order_created was not called; use the same symbols from the diff: OrderService.create_order, OrderRepository.create (and delete/rollback if available), and kafka_producer.publish_order_created.apps/order-service/app/main.py (1)
28-42:⚠️ Potential issue | 🟠 Major시작 실패 경로에서 Redis 정리가 누락됩니다.
Line 33-35에서 예외를 재발생시키면 Line 36의
yield구간으로 진입하지 않아 Line 41 정리가 실행되지 않습니다. 특히 Line 29 성공 후 Line 30 실패 시 Redis가 열린 채 남을 수 있습니다.🔧 제안 수정
async def lifespan(app: FastAPI) -> AsyncIterator[None]: """ @@ """ kafka_started = False + redis_started = False try: await init_redis() + redis_started = True await kafka_producer.start() kafka_started = True logger.info("order_service_started") except Exception as exc: logger.error("order_service_start_failed", error=str(exc)) + if redis_started: + await close_redis() raise try: yield finally: if kafka_started: await kafka_producer.stop() - await close_redis() + if redis_started: + await close_redis() logger.info("order_service_stopped")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/order-service/app/main.py` around lines 28 - 42, The startup try/except currently re-raises before cleanup, leaving Redis open if init_redis() succeeded but kafka_producer.start() fails; modify the startup sequence in the init block (symbols: init_redis, kafka_producer.start, kafka_started, close_redis, kafka_producer.stop, yield) so that any exception during kafka_producer.start() triggers immediate cleanup of Redis (call close_redis()) before re-raising, and only mark kafka_started True after kafka_producer.start() succeeds; alternatively, move close_redis() into the exception handler when kafka_started is False to ensure Redis is always closed on startup failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/order-service/alembic/env.py`:
- Line 37: The migration engine is created without NullPool which can cause
connection reuse/cleanup issues for one-off Alembic runs; update the engine
creation by importing NullPool from sqlalchemy.pool and pass poolclass=NullPool
into create_async_engine(...) where the variable engine is assigned (using
settings.DATABASE_URL) so the line becomes
create_async_engine(settings.DATABASE_URL, poolclass=NullPool).
In `@apps/order-service/tests/unit/test_order_service.py`:
- Around line 125-131: The test is asserting and stubbing
OrderRepository.get_by_id but OrderService calls self.repo.get(order_id), so
update the mocks to match the real method: use MockRepo.return_value.get (set
its return value for the cache-hit case and assert
MockRepo.return_value.get.assert_not_called() for verification) and replace all
other get_by_id references (also in the similar block around lines 148-157) with
get so the test correctly verifies DB access via OrderRepository.get when
OrderService.get_order is exercised.
---
Duplicate comments:
In `@apps/order-service/app/main.py`:
- Around line 28-42: The startup try/except currently re-raises before cleanup,
leaving Redis open if init_redis() succeeded but kafka_producer.start() fails;
modify the startup sequence in the init block (symbols: init_redis,
kafka_producer.start, kafka_started, close_redis, kafka_producer.stop, yield) so
that any exception during kafka_producer.start() triggers immediate cleanup of
Redis (call close_redis()) before re-raising, and only mark kafka_started True
after kafka_producer.start() succeeds; alternatively, move close_redis() into
the exception handler when kafka_started is False to ensure Redis is always
closed on startup failure.
In `@apps/order-service/tests/unit/test_order_service.py`:
- Around line 53-108: Add a new unit test for OrderService.create_order that
simulates inventory-service POST /inventory/reserve failing so you can verify
the 3-retry + rollback + 503 mapping contract: patch the inventory client used
by create_order (the same symbol the service calls) so its reserve method raises
an exception on every call, assert it was invoked exactly 3 times with the
expected reserve payload (to validate retries and retry count), assert
OrderRepository.create does not persist the order (or that a compensating method
like OrderRepository.delete/Rollback was called) and assert create_order
surfaces a 503-mapped error (e.g., raises HTTPException with status_code 503 or
returns the mapped response), and ensure any Kafka publishing is not attempted
in this failure path by asserting publish_order_created was not called; use the
same symbols from the diff: OrderService.create_order, OrderRepository.create
(and delete/rollback if available), and kafka_producer.publish_order_created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3bb2eb2-cdc2-40d6-bae7-6455688fb402
📒 Files selected for processing (4)
apps/order-service/alembic/env.pyapps/order-service/app/main.pyapps/order-service/tests/unit/test_order_service.pydocs/TEMP-PR4-REVIEW-TODO.md
Comment on lines
+125
to
+131
| with patch("app.services.order.OrderRepository") as MockRepo: | ||
| service = OrderService(db=mock_db, redis=mock_redis) | ||
| result = await service.get_order(fake_order.id) | ||
|
|
||
| # 캐시 히트 → DB 조회 없음 | ||
| MockRepo.return_value.get_by_id.assert_not_called() | ||
| assert result is not None |
There was a problem hiding this comment.
OrderRepository 목킹 메서드명이 실제 서비스 호출과 불일치합니다.
캐시 테스트에서 get_by_id를 설정/검증하고 있는데, 서비스 구현은 self.repo.get(order_id)를 호출합니다. 이 상태면 DB 호출 검증이 빗나가거나 테스트가 잘못 통과할 수 있습니다.
🔧 제안 수정
`@pytest.mark.asyncio`
async def test_get_order_cache_hit(
@@
with patch("app.services.order.OrderRepository") as MockRepo:
service = OrderService(db=mock_db, redis=mock_redis)
result = await service.get_order(fake_order.id)
# 캐시 히트 → DB 조회 없음
- MockRepo.return_value.get_by_id.assert_not_called()
+ MockRepo.return_value.get.assert_not_called()
assert result is not None
@@
`@pytest.mark.asyncio`
async def test_get_order_cache_miss_sets_cache(
@@
with patch("app.services.order.OrderRepository") as MockRepo:
- MockRepo.return_value.get_by_id = AsyncMock(return_value=fake_order)
+ MockRepo.return_value.get = AsyncMock(return_value=fake_order)
@@
- MockRepo.return_value.get_by_id.assert_called_once_with(fake_order.id)
+ MockRepo.return_value.get.assert_called_once_with(fake_order.id)
mock_redis.set.assert_called_once_with(
f"order:{fake_order.id}",
json.dumps(_order_to_dict(fake_order)),
ex=CACHE_TTL,
)Also applies to: 148-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/order-service/tests/unit/test_order_service.py` around lines 125 - 131,
The test is asserting and stubbing OrderRepository.get_by_id but OrderService
calls self.repo.get(order_id), so update the mocks to match the real method: use
MockRepo.return_value.get (set its return value for the cache-hit case and
assert MockRepo.return_value.get.assert_not_called() for verification) and
replace all other get_by_id references (also in the similar block around lines
148-157) with get so the test correctly verifies DB access via
OrderRepository.get when OrderService.get_order is exercised.
앱 시작 실패 경로의 Redis 정리를 보강하고 Alembic 엔진 풀 설정을 안전하게 조정했으며, 최신 리뷰 대응 범위를 문서에 명확히 분리했다. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
order-servicewith initial Alembic migration and documentation updates.Test Plan
pytest tests/ -m "not kafka" -vpytest tests/ -v --cov=app --cov-report=term-missing(82%)pytest tests/e2e/test_order_kafka_event.py -m kafka -v -rs(PASS)alembic upgrade headSummary by CodeRabbit
새 기능
마이그레이션
테스트
문서