mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-30 01:46:01 +00:00
refactor(feedback): align the slice with the hexagonal spec
- command-ify the write use cases (RateRun / RetractRunRating; queries keep plain parameters, commands stay dumb data) - split the domain errors into exceptions.py, a peer of model.py (PEP 8 Error suffixes, AWS-style module name) - unify the aggregate->row mapping as _apply(row, feedback) so one explicit field list serves both the insert and the update path - drop the unused feedback.message_id column (migration 0011): feedback is bound to a run, nothing ever wrote or read the field - pin remove_for_run's equality semantics for user_id=None in the contract suite and fix the port docstring that contradicted both implementations
This commit is contained in:
parent
57ba19b877
commit
793169529c
@ -20,7 +20,8 @@ from sqlalchemy import select
|
||||
from sqlalchemy.exc import IntegrityError
|
||||
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker
|
||||
|
||||
from deerflow.domain.feedback.model import DuplicateFeedbackError, Feedback
|
||||
from deerflow.domain.feedback.exceptions import DuplicateFeedbackError
|
||||
from deerflow.domain.feedback.model import Feedback
|
||||
from deerflow.domain.feedback.ports import FeedbackRepository
|
||||
|
||||
# Transitional: the ORM row stays in the harness until PR-N moves engine,
|
||||
@ -53,31 +54,33 @@ class SqlFeedbackRepository(FeedbackRepository):
|
||||
thread_id=row.thread_id,
|
||||
rating=row.rating,
|
||||
user_id=row.user_id,
|
||||
message_id=row.message_id,
|
||||
comment=row.comment,
|
||||
tags=tuple(row.tags or ()),
|
||||
created_at=_tz_aware(row.created_at),
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _to_row(feedback: Feedback) -> FeedbackRow:
|
||||
"""Aggregate -> ORM row. Explicit field list: new columns stay
|
||||
def _apply(row: FeedbackRow, feedback: Feedback) -> None:
|
||||
"""Aggregate -> ORM row, in place. One explicit field list serves
|
||||
BOTH write paths (insert and update), so a new field cannot be
|
||||
stored on insert yet silently dropped on update. ``feedback_id``
|
||||
is deliberately absent: the surrogate key is fixed at insert and
|
||||
an upsert keeps the existing identity (see the port contract).
|
||||
Explicit field list rather than ``**asdict()``: new fields stay
|
||||
private until deliberately mapped here."""
|
||||
return FeedbackRow(
|
||||
feedback_id=feedback.feedback_id,
|
||||
run_id=feedback.run_id,
|
||||
thread_id=feedback.thread_id,
|
||||
user_id=feedback.user_id,
|
||||
message_id=feedback.message_id,
|
||||
rating=feedback.rating,
|
||||
comment=feedback.comment,
|
||||
tags=list(feedback.tags) or None,
|
||||
created_at=feedback.created_at,
|
||||
)
|
||||
row.thread_id = feedback.thread_id
|
||||
row.run_id = feedback.run_id
|
||||
row.user_id = feedback.user_id
|
||||
row.rating = feedback.rating
|
||||
row.comment = feedback.comment
|
||||
row.tags = list(feedback.tags) or None
|
||||
row.created_at = feedback.created_at
|
||||
|
||||
async def save(self, feedback: Feedback) -> Feedback:
|
||||
# Migrated from legacy ``upsert``: look up by aggregate identity,
|
||||
# then update in place or insert.
|
||||
# then apply the aggregate onto the existing or a fresh row. The
|
||||
# identity fields are rewritten with equal values on update --
|
||||
# idempotent, and the price of a single field list.
|
||||
async with self._session_factory() as session:
|
||||
stmt = select(FeedbackRow).where(
|
||||
FeedbackRow.thread_id == feedback.thread_id,
|
||||
@ -85,14 +88,10 @@ class SqlFeedbackRepository(FeedbackRepository):
|
||||
FeedbackRow.user_id == feedback.user_id,
|
||||
)
|
||||
row = (await session.execute(stmt)).scalar_one_or_none()
|
||||
if row is not None:
|
||||
row.rating = feedback.rating
|
||||
row.comment = feedback.comment
|
||||
row.tags = list(feedback.tags) or None
|
||||
row.created_at = feedback.created_at
|
||||
else:
|
||||
row = self._to_row(feedback)
|
||||
if row is None:
|
||||
row = FeedbackRow(feedback_id=feedback.feedback_id)
|
||||
session.add(row)
|
||||
self._apply(row, feedback)
|
||||
try:
|
||||
await session.commit()
|
||||
except IntegrityError as exc:
|
||||
|
||||
@ -24,6 +24,8 @@ from deerflow.domain.feedback import (
|
||||
Feedback,
|
||||
InvalidRatingError,
|
||||
InvalidTagError,
|
||||
RateRun,
|
||||
RetractRunRating,
|
||||
RunNotFoundError,
|
||||
)
|
||||
from deerflow.utils.time import coerce_iso
|
||||
@ -37,7 +39,14 @@ router = APIRouter(prefix="/api/threads", tags=["feedback"])
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class FeedbackUpsertRequest(BaseModel):
|
||||
class RateRunRequest(BaseModel):
|
||||
"""Request model of the RateRun use case (name = command name + Request).
|
||||
|
||||
Carries only what the client may supply: identity (user_id) never
|
||||
appears on a request model -- it is resolved server-side and injected
|
||||
through ``to_command``.
|
||||
"""
|
||||
|
||||
rating: int = Field(..., description="Feedback rating: +1 (positive) or -1 (negative)")
|
||||
comment: str | None = Field(default=None, description="Optional text feedback")
|
||||
tags: list[str] = Field(
|
||||
@ -45,33 +54,47 @@ class FeedbackUpsertRequest(BaseModel):
|
||||
description="Optional thumbs-down reason slugs (e.g. 'incorrect', 'slow')",
|
||||
)
|
||||
|
||||
def to_command(self, thread_id: str, run_id: str, user_id: str | None) -> RateRun:
|
||||
"""Body + path + resolved identity -> the use-case command.
|
||||
|
||||
The wire shape owns the inbound translation into domain vocabulary
|
||||
(wire types become domain types here, e.g. list -> tuple).
|
||||
"""
|
||||
return RateRun(
|
||||
thread_id=thread_id,
|
||||
run_id=run_id,
|
||||
rating=self.rating,
|
||||
user_id=user_id,
|
||||
comment=self.comment,
|
||||
tags=tuple(self.tags),
|
||||
)
|
||||
|
||||
|
||||
class FeedbackResponse(BaseModel):
|
||||
feedback_id: str
|
||||
run_id: str
|
||||
thread_id: str
|
||||
user_id: str | None = None
|
||||
message_id: str | None = None
|
||||
rating: int
|
||||
comment: str | None = None
|
||||
tags: list[str] = []
|
||||
created_at: str = ""
|
||||
|
||||
|
||||
def _to_response(feedback: Feedback) -> FeedbackResponse:
|
||||
"""Domain object -> wire shape. ``created_at`` keeps the legacy
|
||||
``coerce_iso`` serialization so the API output is byte-identical."""
|
||||
return FeedbackResponse(
|
||||
feedback_id=feedback.feedback_id,
|
||||
run_id=feedback.run_id,
|
||||
thread_id=feedback.thread_id,
|
||||
user_id=feedback.user_id,
|
||||
message_id=feedback.message_id,
|
||||
rating=feedback.rating,
|
||||
comment=feedback.comment,
|
||||
tags=list(feedback.tags),
|
||||
created_at=coerce_iso(feedback.created_at),
|
||||
)
|
||||
@classmethod
|
||||
def from_domain(cls, feedback: Feedback) -> FeedbackResponse:
|
||||
"""Domain object -> wire shape (allowlist). ``created_at`` keeps the
|
||||
legacy ``coerce_iso`` serialization so the API output is
|
||||
byte-identical."""
|
||||
return cls(
|
||||
feedback_id=feedback.feedback_id,
|
||||
run_id=feedback.run_id,
|
||||
thread_id=feedback.thread_id,
|
||||
user_id=feedback.user_id,
|
||||
rating=feedback.rating,
|
||||
comment=feedback.comment,
|
||||
tags=list(feedback.tags),
|
||||
created_at=coerce_iso(feedback.created_at),
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@ -84,21 +107,14 @@ def _to_response(feedback: Feedback) -> FeedbackResponse:
|
||||
async def upsert_feedback(
|
||||
thread_id: str,
|
||||
run_id: str,
|
||||
body: FeedbackUpsertRequest,
|
||||
body: RateRunRequest,
|
||||
request: Request,
|
||||
) -> FeedbackResponse:
|
||||
"""Set the current user's rating for a run (idempotent upsert)."""
|
||||
user_id = await get_current_user(request)
|
||||
service = get_feedback_service(request)
|
||||
try:
|
||||
feedback = await service.rate_run(
|
||||
thread_id,
|
||||
run_id,
|
||||
rating=body.rating,
|
||||
comment=body.comment,
|
||||
user_id=user_id,
|
||||
tags=body.tags,
|
||||
)
|
||||
feedback = await service.rate_run(body.to_command(thread_id, run_id, user_id))
|
||||
except InvalidRatingError:
|
||||
raise HTTPException(status_code=400, detail="rating must be +1 or -1")
|
||||
except InvalidTagError as exc:
|
||||
@ -109,7 +125,7 @@ async def upsert_feedback(
|
||||
# Lost a concurrent-upsert race (legacy behavior was a 500); the
|
||||
# client can simply retry.
|
||||
raise HTTPException(status_code=409, detail="Concurrent feedback update, please retry")
|
||||
return _to_response(feedback)
|
||||
return FeedbackResponse.from_domain(feedback)
|
||||
|
||||
|
||||
@router.delete("/{thread_id}/runs/{run_id}/feedback")
|
||||
@ -122,7 +138,7 @@ async def delete_run_feedback(
|
||||
"""Retract the current user's rating for a run."""
|
||||
user_id = await get_current_user(request)
|
||||
service = get_feedback_service(request)
|
||||
retracted = await service.retract_run_rating(thread_id, run_id, user_id=user_id)
|
||||
retracted = await service.retract_run_rating(RetractRunRating(thread_id=thread_id, run_id=run_id, user_id=user_id))
|
||||
if not retracted:
|
||||
raise HTTPException(status_code=404, detail="No feedback found for this run")
|
||||
return {"success": True}
|
||||
|
||||
@ -6,14 +6,15 @@ here; import ports (FeedbackRepository, RunLookup) from
|
||||
adapters and tests, not everyday call-site symbols.
|
||||
"""
|
||||
|
||||
from deerflow.domain.feedback.model import (
|
||||
from deerflow.domain.feedback.commands import RateRun, RetractRunRating
|
||||
from deerflow.domain.feedback.exceptions import (
|
||||
DuplicateFeedbackError,
|
||||
Feedback,
|
||||
FeedbackError,
|
||||
InvalidRatingError,
|
||||
InvalidTagError,
|
||||
RunNotFoundError,
|
||||
)
|
||||
from deerflow.domain.feedback.model import Feedback
|
||||
from deerflow.domain.feedback.service import FeedbackService
|
||||
|
||||
__all__ = [
|
||||
@ -23,5 +24,7 @@ __all__ = [
|
||||
"FeedbackService",
|
||||
"InvalidRatingError",
|
||||
"InvalidTagError",
|
||||
"RateRun",
|
||||
"RetractRunRating",
|
||||
"RunNotFoundError",
|
||||
]
|
||||
|
||||
@ -0,0 +1,38 @@
|
||||
"""Commands of the feedback context.
|
||||
|
||||
One frozen dataclass per state-changing use case -- the named carrier of
|
||||
"the information required to perform an operation on the domain" (AWS
|
||||
hexagonal guidance). Commands are dumb data on purpose: business
|
||||
validation stays on the aggregate (``Feedback.__post_init__``) and
|
||||
structural validation stays on the primary adapter's api model, so error
|
||||
attribution (invalid rating reported before an unknown run) is unchanged.
|
||||
|
||||
Queries are deliberately NOT commands: the read methods on
|
||||
``FeedbackService`` keep plain parameters -- a command expresses an
|
||||
intent to change state, and wrapping reads would be pure boilerplate.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from dataclasses import dataclass, field
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class RateRun:
|
||||
"""Set the user's current rating for a run (idempotent upsert)."""
|
||||
|
||||
thread_id: str
|
||||
run_id: str
|
||||
rating: int
|
||||
user_id: str | None
|
||||
comment: str | None = None
|
||||
tags: tuple[str, ...] = field(default=())
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class RetractRunRating:
|
||||
"""Withdraw the user's rating for a run (clicking the active button again)."""
|
||||
|
||||
thread_id: str
|
||||
run_id: str
|
||||
user_id: str | None
|
||||
@ -0,0 +1,27 @@
|
||||
"""The known errors of the feedback context.
|
||||
|
||||
One family under one base class, so the primary adapter can map the whole
|
||||
family onto protocol codes in a single table. Class names keep the PEP 8
|
||||
``Error`` suffix; the module is named ``exceptions`` after the AWS
|
||||
hexagonal guidance's domain folder of the same name.
|
||||
"""
|
||||
|
||||
|
||||
class FeedbackError(Exception):
|
||||
"""Base error for the feedback domain."""
|
||||
|
||||
|
||||
class InvalidRatingError(FeedbackError):
|
||||
"""Raised when a rating is not +1 or -1."""
|
||||
|
||||
|
||||
class InvalidTagError(FeedbackError):
|
||||
"""Raised when a feedback tag is not a known reason slug."""
|
||||
|
||||
|
||||
class DuplicateFeedbackError(FeedbackError):
|
||||
"""Raised when a concurrent upsert conflicts on the same run's feedback."""
|
||||
|
||||
|
||||
class RunNotFoundError(FeedbackError):
|
||||
"""Raised when the run does not exist or does not belong to the thread."""
|
||||
@ -4,6 +4,8 @@ import uuid
|
||||
from dataclasses import dataclass, field
|
||||
from datetime import UTC, datetime
|
||||
|
||||
from deerflow.domain.feedback import InvalidRatingError, InvalidTagError
|
||||
|
||||
VALID_RATINGS = (-1, 1)
|
||||
|
||||
# Language-neutral reason slugs for thumbs-down feedback. The UI translates
|
||||
@ -21,33 +23,12 @@ VALID_FEEDBACK_TAGS = frozenset(
|
||||
)
|
||||
|
||||
|
||||
class FeedbackError(Exception):
|
||||
"""Base error for the feedback domain."""
|
||||
|
||||
|
||||
class InvalidRatingError(FeedbackError):
|
||||
"""Raised when a rating is not +1 or -1."""
|
||||
|
||||
|
||||
class InvalidTagError(FeedbackError):
|
||||
"""Raised when a feedback tag is not a known reason slug."""
|
||||
|
||||
|
||||
class DuplicateFeedbackError(FeedbackError):
|
||||
"""Raised when a concurrent upsert conflicts on the same run's feedback."""
|
||||
|
||||
|
||||
class RunNotFoundError(FeedbackError):
|
||||
"""Raised when the run does not exist or does not belong to the thread."""
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class Feedback:
|
||||
"""A user's rating of a single run: at most one per (thread, run, user).
|
||||
|
||||
Single-entity aggregate. ``message_id`` optionally narrows the rating
|
||||
to one message within the run instead of the whole run. ``tags`` carry
|
||||
optional thumbs-down reason slugs from the feedback dialog.
|
||||
Single-entity aggregate. ``tags`` carry optional thumbs-down reason
|
||||
slugs from the feedback dialog.
|
||||
"""
|
||||
|
||||
feedback_id: str
|
||||
@ -55,7 +36,6 @@ class Feedback:
|
||||
thread_id: str
|
||||
rating: int
|
||||
user_id: str | None = None
|
||||
message_id: str | None = None
|
||||
comment: str | None = None
|
||||
tags: tuple[str, ...] = ()
|
||||
created_at: datetime = field(default_factory=lambda: datetime.now(UTC))
|
||||
@ -75,7 +55,6 @@ class Feedback:
|
||||
thread_id: str,
|
||||
rating: int,
|
||||
user_id: str | None = None,
|
||||
message_id: str | None = None,
|
||||
comment: str | None = None,
|
||||
tags: tuple[str, ...] | list[str] = (),
|
||||
) -> Feedback:
|
||||
@ -86,7 +65,6 @@ class Feedback:
|
||||
thread_id=thread_id,
|
||||
rating=rating,
|
||||
user_id=user_id,
|
||||
message_id=message_id,
|
||||
comment=comment,
|
||||
tags=tuple(tags),
|
||||
)
|
||||
|
||||
@ -44,8 +44,12 @@ class FeedbackRepository(Protocol):
|
||||
"""Retract the user's feedback for a run.
|
||||
|
||||
Returns True if an entry was removed, False if none existed.
|
||||
Ownership filter: a non-None user_id restricts results to the
|
||||
user's entries; None means no filtering (no-auth mode).
|
||||
Unlike the read methods, ownership here is an EQUALITY match, not a
|
||||
filter: a non-None user_id removes only that user's entry, and None
|
||||
matches only entries stored with a NULL owner (what no-auth mode
|
||||
writes) -- it does NOT mean "remove regardless of owner". A delete
|
||||
must never reach across owners, so the asymmetry with the reads'
|
||||
"None = no filtering" is deliberate.
|
||||
"""
|
||||
...
|
||||
|
||||
|
||||
@ -1,4 +1,6 @@
|
||||
from deerflow.domain.feedback.model import Feedback, RunNotFoundError
|
||||
from deerflow.domain.feedback.commands import RateRun, RetractRunRating
|
||||
from deerflow.domain.feedback.exceptions import RunNotFoundError
|
||||
from deerflow.domain.feedback.model import Feedback
|
||||
from deerflow.domain.feedback.ports import FeedbackRepository, RunLookup
|
||||
|
||||
|
||||
@ -8,8 +10,9 @@ class FeedbackService:
|
||||
Orchestrates use cases only: fetch/verify -> apply domain rules ->
|
||||
persist through output ports. Holds no business rules itself (those
|
||||
live on the Feedback aggregate) and knows nothing about HTTP or
|
||||
storage. user_id is always passed in explicitly -- resolving the
|
||||
current user is the primary adapter's job.
|
||||
storage. State-changing use cases take a command object (the handler
|
||||
is the method); queries take plain parameters. Resolving the current
|
||||
user is the primary adapter's job -- it arrives on the command.
|
||||
"""
|
||||
|
||||
def __init__(self, repository: FeedbackRepository, runs: RunLookup):
|
||||
@ -21,16 +24,7 @@ class FeedbackService:
|
||||
if await self._runs.thread_of(run_id) != thread_id:
|
||||
raise RunNotFoundError("Run does not belong to the specified thread")
|
||||
|
||||
async def rate_run(
|
||||
self,
|
||||
thread_id: str,
|
||||
run_id: str,
|
||||
*,
|
||||
rating: int,
|
||||
comment: str | None,
|
||||
user_id: str | None,
|
||||
tags: tuple[str, ...] | list[str] = (),
|
||||
) -> Feedback:
|
||||
async def rate_run(self, cmd: RateRun) -> Feedback:
|
||||
"""Set the user's current rating for a run (idempotent).
|
||||
|
||||
Backs the PUT endpoint: "my current verdict on this run is X".
|
||||
@ -48,20 +42,20 @@ class FeedbackService:
|
||||
to the given thread (cross-thread ids are rejected).
|
||||
"""
|
||||
feedback = Feedback.create(
|
||||
run_id=run_id,
|
||||
thread_id=thread_id,
|
||||
rating=rating,
|
||||
user_id=user_id,
|
||||
comment=comment,
|
||||
tags=tags,
|
||||
run_id=cmd.run_id,
|
||||
thread_id=cmd.thread_id,
|
||||
rating=cmd.rating,
|
||||
user_id=cmd.user_id,
|
||||
comment=cmd.comment,
|
||||
tags=cmd.tags,
|
||||
)
|
||||
await self._require_run(thread_id, run_id)
|
||||
await self._require_run(cmd.thread_id, cmd.run_id)
|
||||
return await self._repository.save(feedback)
|
||||
|
||||
async def retract_run_rating(self, thread_id: str, run_id: str, *, user_id: str | None) -> bool:
|
||||
async def retract_run_rating(self, cmd: RetractRunRating) -> bool:
|
||||
"""Withdraw the user's rating for a run (clicking the active button
|
||||
again). Returns False when there was nothing to retract."""
|
||||
return await self._repository.remove_for_run(thread_id, run_id, user_id=user_id)
|
||||
return await self._repository.remove_for_run(cmd.thread_id, cmd.run_id, user_id=cmd.user_id)
|
||||
|
||||
async def latest_per_run_in_thread(self, thread_id: str, *, user_id: str | None) -> dict[str, Feedback]:
|
||||
"""Current feedback per run across a thread -- powers the message-list
|
||||
|
||||
@ -19,9 +19,6 @@ class FeedbackRow(Base):
|
||||
run_id: Mapped[str] = mapped_column(String(64), nullable=False, index=True)
|
||||
thread_id: Mapped[str] = mapped_column(String(64), nullable=False, index=True)
|
||||
user_id: Mapped[str | None] = mapped_column(String(64), index=True)
|
||||
message_id: Mapped[str | None] = mapped_column(String(64))
|
||||
# message_id is an optional RunEventStore event identifier —
|
||||
# allows feedback to target a specific message or the entire run
|
||||
|
||||
rating: Mapped[int] = mapped_column(nullable=False)
|
||||
# +1 (thumbs-up) or -1 (thumbs-down)
|
||||
|
||||
@ -0,0 +1,33 @@
|
||||
"""Drop the unused feedback.message_id column.
|
||||
|
||||
Feedback is bound to a run, not to a single message: nothing ever wrote or
|
||||
read ``message_id`` (no API field, no frontend usage, no query), so the
|
||||
column is redundant design and the domain aggregate no longer carries it.
|
||||
|
||||
Revision ID: 0011_feedback_drop_message_id
|
||||
Revises: 0010_feedback_tags
|
||||
Create Date: 2026-07-29
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from collections.abc import Sequence
|
||||
|
||||
import sqlalchemy as sa
|
||||
|
||||
from deerflow.persistence.migrations._helpers import safe_add_column, safe_drop_column
|
||||
|
||||
revision: str = "0011_feedback_drop_message_id"
|
||||
down_revision: str | Sequence[str] | None = "0010_feedback_tags"
|
||||
branch_labels: str | Sequence[str] | None = None
|
||||
depends_on: str | Sequence[str] | None = None
|
||||
|
||||
|
||||
def upgrade() -> None:
|
||||
# Idempotent: a DB provisioned by create_all from the new metadata never
|
||||
# had the column, and a retried upgrade must not fail on the second run.
|
||||
safe_drop_column("feedback", "message_id")
|
||||
|
||||
|
||||
def downgrade() -> None:
|
||||
safe_add_column("feedback", sa.Column("message_id", sa.String(64), nullable=True))
|
||||
@ -88,6 +88,20 @@ class FeedbackRepositoryContract:
|
||||
assert await repo.remove_for_run("t1", "r1", user_id="u1") is True
|
||||
assert await repo.latest_per_run_in_thread("t1", user_id="u1") == {}
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_remove_for_run_none_user_matches_only_null_owner(self, repo):
|
||||
# Deletion is an equality match, not a filter: user_id=None targets
|
||||
# only the NULL-owner entry (no-auth writes) and must never reach
|
||||
# across into a named user's entry.
|
||||
await repo.save(Feedback.create(run_id="r1", thread_id="t1", rating=1, user_id="u1"))
|
||||
await repo.save(Feedback.create(run_id="r1", thread_id="t1", rating=-1, user_id=None))
|
||||
|
||||
assert await repo.remove_for_run("t1", "r1", user_id=None) is True
|
||||
assert await repo.remove_for_run("t1", "r1", user_id=None) is False
|
||||
|
||||
remaining = await repo.latest_per_run_in_thread("t1", user_id="u1")
|
||||
assert remaining["r1"].rating == 1
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_remove_for_run_nonexistent(self, repo):
|
||||
assert await repo.remove_for_run("t1", "r1", user_id="u1") is False
|
||||
|
||||
@ -1,13 +1,15 @@
|
||||
"""FeedbackService tests with injected fakes — zero IO, no engine.
|
||||
|
||||
The seam the ports create is what makes these tests possible: the service
|
||||
is exercised end-to-end against dict-backed doubles.
|
||||
is exercised end-to-end against dict-backed doubles. State-changing use
|
||||
cases go through command objects (RateRun / RetractRunRating); queries
|
||||
keep plain parameters.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
from feedback_fakes import FakeRunLookup, InMemoryFeedbackRepository
|
||||
|
||||
from deerflow.domain.feedback import FeedbackService, InvalidRatingError, RunNotFoundError
|
||||
from deerflow.domain.feedback import FeedbackService, InvalidRatingError, RateRun, RetractRunRating, RunNotFoundError
|
||||
|
||||
|
||||
def _service(runs: dict[str, str] | None = None) -> FeedbackService:
|
||||
@ -17,34 +19,38 @@ def _service(runs: dict[str, str] | None = None) -> FeedbackService:
|
||||
)
|
||||
|
||||
|
||||
def _rate(thread_id: str = "t1", run_id: str = "r1", *, rating: int = 1, user_id: str | None = "u1", comment: str | None = None, tags: tuple[str, ...] = ()) -> RateRun:
|
||||
return RateRun(thread_id=thread_id, run_id=run_id, rating=rating, user_id=user_id, comment=comment, tags=tags)
|
||||
|
||||
|
||||
class TestRateRun:
|
||||
@pytest.mark.anyio
|
||||
async def test_stores_rating(self):
|
||||
svc = _service()
|
||||
fb = await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
|
||||
fb = await svc.rate_run(_rate(rating=1))
|
||||
assert fb.rating == 1
|
||||
assert fb.user_id == "u1"
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_idempotent_upsert_keeps_identity(self):
|
||||
svc = _service()
|
||||
first = await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
|
||||
second = await svc.rate_run("t1", "r1", rating=-1, comment="meh", user_id="u1")
|
||||
first = await svc.rate_run(_rate(rating=1))
|
||||
second = await svc.rate_run(_rate(rating=-1, comment="meh"))
|
||||
assert second.feedback_id == first.feedback_id
|
||||
assert second.rating == -1
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_enrich_with_tags(self):
|
||||
svc = _service()
|
||||
await svc.rate_run("t1", "r1", rating=-1, comment=None, user_id="u1")
|
||||
fb = await svc.rate_run("t1", "r1", rating=-1, comment="wrong", user_id="u1", tags=["incorrect"])
|
||||
await svc.rate_run(_rate(rating=-1))
|
||||
fb = await svc.rate_run(_rate(rating=-1, comment="wrong", tags=("incorrect",)))
|
||||
assert fb.tags == ("incorrect",)
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_unknown_run_rejected(self):
|
||||
svc = _service({})
|
||||
with pytest.raises(RunNotFoundError):
|
||||
await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
|
||||
await svc.rate_run(_rate())
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_cross_thread_run_rejected(self):
|
||||
@ -52,7 +58,7 @@ class TestRateRun:
|
||||
this thread's URL (ownership check)."""
|
||||
svc = _service({"r1": "other-thread"})
|
||||
with pytest.raises(RunNotFoundError):
|
||||
await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
|
||||
await svc.rate_run(_rate())
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_invalid_rating_rejected_before_run_lookup(self):
|
||||
@ -61,29 +67,38 @@ class TestRateRun:
|
||||
# Expecting InvalidRatingError is what pins validation ahead of I/O.
|
||||
svc = _service()
|
||||
with pytest.raises(InvalidRatingError):
|
||||
await svc.rate_run("t1", "unknown-run", rating=0, comment=None, user_id="u1")
|
||||
await svc.rate_run(_rate(run_id="unknown-run", rating=0))
|
||||
|
||||
def test_command_is_dumb_data(self):
|
||||
# The command carries intent without validating it: business rules
|
||||
# stay on the aggregate, so error attribution (invalid rating before
|
||||
# unknown run) is owned by the handler's construction order, not by
|
||||
# the command's own constructor.
|
||||
cmd = _rate(rating=0)
|
||||
assert cmd.rating == 0
|
||||
|
||||
|
||||
class TestRetractAndReads:
|
||||
@pytest.mark.anyio
|
||||
async def test_retract(self):
|
||||
svc = _service()
|
||||
await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
|
||||
assert await svc.retract_run_rating("t1", "r1", user_id="u1") is True
|
||||
assert await svc.retract_run_rating("t1", "r1", user_id="u1") is False
|
||||
await svc.rate_run(_rate(rating=1))
|
||||
retract = RetractRunRating(thread_id="t1", run_id="r1", user_id="u1")
|
||||
assert await svc.retract_run_rating(retract) is True
|
||||
assert await svc.retract_run_rating(retract) is False
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_latest_per_run_in_thread(self):
|
||||
svc = _service({"r1": "t1", "r2": "t1"})
|
||||
await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
|
||||
await svc.rate_run("t1", "r2", rating=-1, comment=None, user_id="u1")
|
||||
await svc.rate_run(_rate(run_id="r1", rating=1))
|
||||
await svc.rate_run(_rate(run_id="r2", rating=-1))
|
||||
grouped = await svc.latest_per_run_in_thread("t1", user_id="u1")
|
||||
assert {run_id: fb.rating for run_id, fb in grouped.items()} == {"r1": 1, "r2": -1}
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_latest_for_runs(self):
|
||||
svc = _service({"r1": "t1", "r2": "t1"})
|
||||
await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
|
||||
await svc.rate_run("t1", "r2", rating=-1, comment=None, user_id="u1")
|
||||
await svc.rate_run(_rate(run_id="r1", rating=1))
|
||||
await svc.rate_run(_rate(run_id="r2", rating=-1))
|
||||
grouped = await svc.latest_for_runs("t1", {"r2"}, user_id="u1")
|
||||
assert set(grouped) == {"r2"}
|
||||
|
||||
@ -48,7 +48,7 @@ from deerflow.persistence.migrations._helpers import _normalize_default
|
||||
asyncio_test = pytest.mark.asyncio
|
||||
|
||||
|
||||
HEAD = "0010_feedback_tags"
|
||||
HEAD = "0011_feedback_drop_message_id"
|
||||
BASELINE = "0001_baseline"
|
||||
|
||||
|
||||
|
||||
@ -76,7 +76,7 @@ async def test_legacy_database_recovers_token_usage_column(tmp_path: Path) -> No
|
||||
cols = {row[1] for row in raw.execute("PRAGMA table_info(runs)").fetchall()}
|
||||
assert "token_usage_by_model" in cols
|
||||
version_row = raw.execute("SELECT version_num FROM alembic_version").fetchone()
|
||||
assert version_row[0] == "0010_feedback_tags"
|
||||
assert version_row[0] == "0011_feedback_drop_message_id"
|
||||
|
||||
# And the read path that originally 500'd must now succeed.
|
||||
sf = get_session_factory()
|
||||
@ -116,6 +116,6 @@ async def test_legacy_database_with_manual_alter_still_bootstraps(tmp_path: Path
|
||||
# No duplicate column -- list, not set, to catch dupes.
|
||||
assert cols.count("token_usage_by_model") == 1
|
||||
version_row = raw.execute("SELECT version_num FROM alembic_version").fetchone()
|
||||
assert version_row[0] == "0010_feedback_tags"
|
||||
assert version_row[0] == "0011_feedback_drop_message_id"
|
||||
finally:
|
||||
await close_engine()
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user