From 51e643fff174e4daf02eea87a623333990adf9ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9D=9E=E6=B3=95=E6=93=8D=E4=BD=9C?= Date: Mon, 26 Oct 2026 09:46:11 +0000 Subject: [PATCH] refactor(api): unify message feedback across app entrypoints (#43748) --- api/controllers/common/errors.py | 16 + api/controllers/console/app/message.py | 132 ++-- api/controllers/console/explore/error.py | 6 - api/controllers/console/explore/message.py | 19 +- api/controllers/service_api/app/message.py | 82 ++- .../service_api/flask_admission.py | 87 ++- api/controllers/web/message.py | 44 +- .../application_services/installed_app.py | 3 +- api/extensions/ext_application_services.py | 3 + api/openapi/markdown/console-openapi.md | 5 +- api/openapi/markdown/service-openapi.md | 15 +- api/openapi/markdown/web-openapi.md | 11 +- .../app_definition_query_repository.py | 21 +- .../installed_app_message_repository.py | 66 +- api/repositories/message_repository.py | 143 ++++- api/services/app_definition_query_service.py | 6 + api/services/errors/message.py | 4 + .../installed_app_message_adapters.py | 30 +- api/services/installed_app_message_service.py | 55 +- api/services/message_feedback_service.py | 140 +++++ api/services/message_service.py | 106 ---- .../services/test_message_service.py | 241 +------- .../console/test_message_feedback.py | 311 ++++++++++ .../service_api/app/test_message_feedback.py | 484 +++++++++++++++ .../controllers/web/test_message_feedback.py | 224 +++++++ .../controllers/console/agent/test_roster.py | 44 -- .../controllers/console/app/test_message.py | 48 -- .../console/explore/test_message.py | 144 +---- .../service_api/app/test_message.py | 111 +--- .../controllers/web/test_message.py | 74 +-- .../test_installed_app_message_repository.py | 130 +--- .../test_installed_app_message_adapters.py | 45 +- .../test_installed_app_message_service.py | 78 --- .../services/test_message_feedback_service.py | 575 ++++++++++++++++++ .../services/test_message_service.py | 127 ---- .../generated/api/console/agent/errors.gen.ts | 14 +- .../generated/api/console/agent/orpc.gen.ts | 7 +- .../generated/api/console/agent/types.gen.ts | 6 + .../generated/api/console/agent/zod.gen.ts | 12 + .../generated/api/console/apps/errors.gen.ts | 14 +- .../generated/api/console/apps/orpc.gen.ts | 7 +- .../generated/api/console/apps/types.gen.ts | 7 + .../generated/api/console/apps/zod.gen.ts | 12 + .../api/console/installed-apps/errors.gen.ts | 1 + .../api/console/installed-apps/orpc.gen.ts | 1 + .../api/console/installed-apps/types.gen.ts | 2 +- .../generated/api/service/errors.gen.ts | 27 + .../generated/api/service/orpc.gen.ts | 18 + .../generated/api/service/types.gen.ts | 24 +- .../generated/api/service/zod.gen.ts | 12 + .../contracts/generated/api/web/errors.gen.ts | 13 + .../contracts/generated/api/web/orpc.gen.ts | 5 + .../contracts/generated/api/web/types.gen.ts | 8 +- .../contracts/generated/api/web/zod.gen.ts | 12 + 54 files changed, 2377 insertions(+), 1455 deletions(-) create mode 100644 api/services/message_feedback_service.py create mode 100644 api/tests/e2e_tests/contracts/controllers/console/test_message_feedback.py create mode 100644 api/tests/e2e_tests/contracts/controllers/service_api/app/test_message_feedback.py create mode 100644 api/tests/e2e_tests/contracts/controllers/web/test_message_feedback.py create mode 100644 api/tests/unit_tests/services/test_message_feedback_service.py diff --git a/api/controllers/common/errors.py b/api/controllers/common/errors.py index a8ea4264934..a0ce8fc9e41 100644 --- a/api/controllers/common/errors.py +++ b/api/controllers/common/errors.py @@ -4,6 +4,7 @@ from typing import cast from flask import current_app, got_request_exception +from core.logging.context import get_request_id from libs.exception import BaseHTTPException from libs.external_api import ExternalApi from services.errors.base import NoPermissionError @@ -92,6 +93,21 @@ class NotFoundError(BaseHTTPException): code = 404 +class MessageFeedbackRatingRequiredError(BaseHTTPException): + error_code = "message_feedback_rating_required" + description = "A rating is required when there is no existing feedback to remove." + code = HTTPStatus.BAD_REQUEST + + def __init__(self) -> None: + super().__init__() + self.data = { + "code": self.error_code, + "message": self.description, + "status": self.code, + "details": {"request_id": get_request_id()}, + } + + class UnauthorizedError(BaseHTTPException): error_code = "unauthorized" code = HTTPStatus.UNAUTHORIZED diff --git a/api/controllers/console/app/message.py b/api/controllers/console/app/message.py index 9cbee4cd09d..5267a846f7e 100644 --- a/api/controllers/console/app/message.py +++ b/api/controllers/console/app/message.py @@ -10,7 +10,7 @@ from werkzeug.exceptions import NotFound from controllers.common.controller_schemas import MessageFeedbackPayload as _MessageFeedbackPayloadBase from controllers.common.errors import InternalServerError as InternalServerHTTPError -from controllers.common.errors import NotFoundError, UnauthorizedError +from controllers.common.errors import MessageFeedbackRatingRequiredError, NotFoundError, UnauthorizedError from controllers.common.fields import SimpleResultResponse from controllers.common.rbac import AgentId, PlainApp, RBACCheck from controllers.common.schema import query_params_from_model, register_response_schema_models, register_schema_models @@ -55,8 +55,8 @@ from libs.infinite_scroll_pagination import InfiniteScrollPagination from libs.login import login_required from machinery.context import RequestContext from models.account import Account -from models.enums import FeedbackFromSource, FeedbackRating -from models.model import App, AppMode, Conversation, Message, MessageAnnotation, MessageFeedback +from models.enums import FeedbackRating +from models.model import App, AppMode, Conversation, Message, MessageAnnotation from services.agent.errors import AgentNotFoundError, AgentVersionNotFoundError from services.app.agent_app_contracts import AgentAppNotFoundError from services.app.console_service import ConsoleAppNotFoundError @@ -65,6 +65,7 @@ from services.conversation_service import ConversationService from services.entities.message_entities import MessageAccount from services.errors.conversation import ConversationNotExistsError from services.errors.message import ( + FeedbackRatingRequiredError, MessageActorNotFoundError, MessageNotExistsError, SuggestedQuestionsAfterAnswerDisabledError, @@ -198,64 +199,45 @@ class AgentChatMessageListApi(Resource): @console_ns.route("/apps//feedbacks") class MessageFeedbackApi(Resource): + @error_responses(AppNotFoundError, AppUnavailableError, MessageFeedbackRatingRequiredError) @console_ns.doc("create_message_feedback") @console_ns.doc(description="Create or update message feedback (like/dislike)") @console_ns.doc(params={"app_id": "Application ID"}) @console_ns.expect(console_ns.models[AppMessageFeedbackPayload.__name__]) - @console_ns.response(200, "Feedback updated successfully", console_ns.models[SimpleResultResponse.__name__]) - @console_ns.response(404, "Message not found") - @setup_required - @login_required - @account_initialization_required - @with_current_user - @with_session - @get_app_model + @console_ns.response( + HTTPStatus.OK, "Feedback updated successfully", console_ns.models[SimpleResultResponse.__name__] + ) + @console_ns.response(HTTPStatus.NOT_FOUND, "Message not found") + @console_ns.response(HTTPStatus.FORBIDDEN, "Insufficient permissions") + @console_account_admission() @model_validate(AppMessageFeedbackPayload) - def post(self, req_data: AppMessageFeedbackPayload, session: Session, current_user: Account, app_model: App): - return _update_message_feedback( - args=req_data, - session=session, - current_user=current_user, - app_model=app_model, - ) + def post(self, req_data: AppMessageFeedbackPayload, context: RequestContext, app_id: UUID) -> dict[str, object]: + try: + app = application_services().apps.console.get_reference(context, str(app_id)) + except ConsoleAppNotFoundError as exc: + raise AppNotFoundError() from exc + return _update_message_feedback(args=req_data, context=context, app_id=app.id) @console_ns.route("/agent//feedbacks") class AgentMessageFeedbackApi(Resource): - @error_responses(AgentNotFoundError) + @error_responses(AgentNotFoundError, AppUnavailableError, MessageFeedbackRatingRequiredError) @console_ns.doc("create_agent_message_feedback") @console_ns.doc(description="Create or update Agent App message feedback") @console_ns.doc(params={"agent_id": "Agent ID"}) @console_ns.expect(console_ns.models[AppMessageFeedbackPayload.__name__]) - @console_ns.response(200, "Feedback updated successfully", console_ns.models[SimpleResultResponse.__name__]) - @console_ns.response(404, "Agent or message not found") - @setup_required - @login_required - @account_initialization_required - @rbac_permission_required(RBACCheck(RBACPermission.AGENT_TEST_AND_RUN, AgentId())) - @with_current_user - @with_current_tenant_id - @with_session + @console_ns.response( + HTTPStatus.OK, "Feedback updated successfully", console_ns.models[SimpleResultResponse.__name__] + ) + @console_ns.response(HTTPStatus.NOT_FOUND, "Agent or message not found") + @console_account_admission(rbac_checks=(RBACCheck(RBACPermission.AGENT_TEST_AND_RUN, AgentId()),)) @model_validate(AppMessageFeedbackPayload) - def post( - self, - req_data: AppMessageFeedbackPayload, - session: Session, - current_tenant_id: str, - current_user: Account, - agent_id: UUID, - ): - app_model = resolve_agent_runtime_app_model( - session=session, - tenant_id=current_tenant_id, - agent_id=agent_id, - ) - return _update_message_feedback( - args=req_data, - session=session, - current_user=current_user, - app_model=app_model, - ) + def post(self, req_data: AppMessageFeedbackPayload, context: RequestContext, agent_id: UUID) -> dict[str, object]: + try: + app_id = application_services().agent_apps.access.resolve_existing_runtime_app_id(context, str(agent_id)) + except AgentAppNotFoundError as exc: + raise AgentNotFoundError() from exc + return _update_message_feedback(args=req_data, context=context, app_id=app_id) @console_ns.route("/apps//annotations/count") @@ -479,46 +461,26 @@ def _list_chat_messages( def _update_message_feedback( - *, - args: AppMessageFeedbackPayload, - session: Session, - current_user: Account, - app_model: App, -): - message_id = args.message_id - - message = session.scalar(select(Message).where(Message.id == message_id, Message.app_id == app_model.id).limit(1)) - - if not message: - raise NotFound("Message Not Exists.") - - feedback = message.admin_feedback_with_session(session=session) - - if not args.rating and feedback: - session.delete(feedback) - elif args.rating and feedback: - feedback.rating = FeedbackRating(args.rating) - feedback.content = args.content - elif not args.rating and not feedback: - raise ValueError("rating cannot be None when feedback not exists") - else: - rating_value = args.rating - if rating_value is None: - raise ValueError("rating is required to create feedback") - feedback = MessageFeedback( - app_id=app_model.id, - conversation_id=message.conversation_id, - message_id=message.id, - rating=FeedbackRating(rating_value), + *, args: AppMessageFeedbackPayload, context: RequestContext, app_id: str +) -> dict[str, object]: + try: + application_services().message_feedbacks.set_admin_feedback( + app_id=app_id, + app_owner_tenant_id=context.active_workspace_id, + account_id=context.account_id, + message_id=args.message_id, + rating=FeedbackRating(args.rating) if args.rating is not None else None, content=args.content, - from_source=FeedbackFromSource.ADMIN, - from_account_id=current_user.id, ) - session.add(feedback) - - session.commit() - - return SimpleResultResponse(result="success").model_dump(mode="json") + except AppDefinitionUnavailableError as exc: + raise AppUnavailableError() from exc + except MessageActorNotFoundError as exc: + raise UnauthorizedError("Account no longer exists") from exc + except MessageNotExistsError as exc: + raise NotFoundError("Message Not Exists.") from exc + except FeedbackRatingRequiredError as exc: + raise MessageFeedbackRatingRequiredError() from exc + return dump_response(SimpleResultResponse, {"result": "success"}) def _get_message_suggested_questions( diff --git a/api/controllers/console/explore/error.py b/api/controllers/console/explore/error.py index 16818c7fda9..730c0a6a08a 100644 --- a/api/controllers/console/explore/error.py +++ b/api/controllers/console/explore/error.py @@ -79,12 +79,6 @@ class MessageCursorNotFoundHTTPError(InstalledAppHTTPError): code = HTTPStatus.NOT_FOUND -class MessageFeedbackRatingRequiredHTTPError(InstalledAppHTTPError): - error_code = "message_feedback_rating_required" - description = "A rating is required when there is no existing feedback to remove." - code = HTTPStatus.BAD_REQUEST - - class NotCompletionAppError(BaseHTTPException): error_code = "not_completion_app" description = "Not Completion App" diff --git a/api/controllers/console/explore/message.py b/api/controllers/console/explore/message.py index 405f3277bc7..f3cee2cd6c5 100644 --- a/api/controllers/console/explore/message.py +++ b/api/controllers/console/explore/message.py @@ -12,7 +12,7 @@ from flask_restx import Resource from pydantic import BaseModel from controllers.common.controller_schemas import MessageFeedbackPayload, MessageListQuery -from controllers.common.errors import InternalServerError, UnauthorizedError +from controllers.common.errors import InternalServerError, MessageFeedbackRatingRequiredError, UnauthorizedError from controllers.common.schema import query_params_from_model, register_response_schema_models, register_schema_models from controllers.console import console_ns from controllers.console.app.error import ( @@ -28,7 +28,6 @@ from controllers.console.explore.error import ( ConversationNotFoundHTTPError, InstalledAppNotFoundHTTPError, MessageCursorNotFoundHTTPError, - MessageFeedbackRatingRequiredHTTPError, MessageNotFoundHTTPError, NotChatAppError, NotCompletionAppError, @@ -45,6 +44,7 @@ from libs import helper from libs.exception import BaseHTTPException from libs.external_api import error_responses from machinery.context import RequestContext +from models.enums import FeedbackRating from services.account_errors import AccountNotFoundError from services.app_definition_query_service import AppDefinitionUnavailableError from services.entities.message_entities import MessageAccount @@ -53,13 +53,14 @@ from services.errors.app_model_config import AppModelConfigBrokenError from services.errors.base import BaseServiceError from services.errors.conversation import ConversationNotExistsError from services.errors.message import ( + FeedbackRatingRequiredError, FirstMessageNotExistsError, MessageActorNotFoundError, MessageNotExistsError, SuggestedQuestionsAfterAnswerDisabledError, ) from services.installed_app_access_service import InstalledAppNotFoundError, InstalledAppRef -from services.installed_app_message_service import FeedbackRatingRequiredError, MessageNotChatAppError +from services.installed_app_message_service import MessageNotChatAppError from services.message_more_like_this_service import ( MoreLikeThisConfigNotFoundError, MoreLikeThisNotCompletionError, @@ -103,7 +104,7 @@ def _message_errors[**P, R](view: Callable[P, R]) -> Callable[P, R]: except ConversationNotExistsError as error: raise ConversationNotFoundHTTPError() from error except FeedbackRatingRequiredError as error: - raise MessageFeedbackRatingRequiredHTTPError() from error + raise MessageFeedbackRatingRequiredError() from error except MoreLikeThisDisabledError as error: raise AppMoreLikeThisDisabledError() from error except SuggestedQuestionsAfterAnswerDisabledError as error: @@ -155,7 +156,7 @@ class MessageListApi(Resource): endpoint="installed_app_message_feedback", ) class MessageFeedbackApi(Resource): - @error_responses(MessageFeedbackRatingRequiredHTTPError, MessageNotFoundHTTPError) + @error_responses(AppUnavailableError, MessageFeedbackRatingRequiredError, MessageNotFoundHTTPError) @console_ns.expect(console_ns.models[MessageFeedbackPayload.__name__]) @console_ns.response(HTTPStatus.OK, "Feedback submitted successfully", console_ns.models[ResultResponse.__name__]) @console_account_admission() @@ -169,11 +170,13 @@ class MessageFeedbackApi(Resource): installed_app: InstalledAppRef, message_id: UUID, ) -> dict[str, object]: - application_services().installed_apps.messages.set_feedback( + application_services().message_feedbacks.set_feedback( + app_id=installed_app.app_id, + app_owner_tenant_id=installed_app.app_owner_tenant_id, + actor=MessageAccount(account_id=request_context.account_id), installed_app=installed_app, - account_id=request_context.account_id, message_id=str(message_id), - rating=payload.rating, + rating=FeedbackRating(payload.rating) if payload.rating is not None else None, content=payload.content, ) return helper.dump_response(ResultResponse, {"result": "success"}) diff --git a/api/controllers/service_api/app/message.py b/api/controllers/service_api/app/message.py index 39ba3ba505c..b6449d6513a 100644 --- a/api/controllers/service_api/app/message.py +++ b/api/controllers/service_api/app/message.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, Field, TypeAdapter, WithJsonSchema import services from controllers.common.controller_schemas import MessageFeedbackPayload, MessageListQuery -from controllers.common.errors import InternalServerError, NotFoundError +from controllers.common.errors import InternalServerError, MessageFeedbackRatingRequiredError, NotFoundError from controllers.common.fields import SimpleResultStringListResponse from controllers.common.schema import query_params_from_model, register_response_schema_models, register_schema_models from controllers.console.wraps import model_validate @@ -22,7 +22,7 @@ from controllers.service_api.app.error import ( ProviderNotInitializeError, ProviderQuotaExceededError, ) -from controllers.service_api.flask_admission import service_api_end_user_admission +from controllers.service_api.flask_admission import service_api_app_admission, service_api_end_user_admission from controllers.service_api.schema import expect_with_user from controllers.service_api.wraps import FetchUserArg, WhereisUserArg, validate_app_token from core.errors.error import ModelCurrentlyNotSupportError, ProviderTokenNotInitError, QuotaExceededError @@ -34,7 +34,7 @@ from fields.message_fields import MessageInfiniteScrollPagination, MessageListIt from graphon.model_runtime.errors.invoke import InvokeError from libs.external_api import error_responses from libs.helper import dump_response -from machinery.context import ServiceApiEndUserContext +from machinery.context import AppRequestContext, ServiceApiEndUserContext from models.enums import FeedbackRating from models.model import App, AppMode, EndUser from services.agent.errors import AgentVersionNotFoundError @@ -42,6 +42,7 @@ from services.app_definition_query_service import AppDefinitionUnavailableError from services.entities.message_entities import MessageEndUser from services.errors.conversation import ConversationNotExistsError from services.errors.message import ( + FeedbackRatingRequiredError, FirstMessageNotExistsError, MessageActorNotFoundError, MessageNotExistsError, @@ -156,8 +157,11 @@ class MessageListApi(Resource): @service_api_ns.route("/messages//feedbacks") class MessageFeedbackApi(Resource): + @error_responses(AppUnavailableError, MessageFeedbackRatingRequiredError) @expect_with_user(service_api_ns, MessageFeedbackPayload) - @service_api_ns.response(200, "Feedback submitted successfully", service_api_ns.models[ResultResponse.__name__]) + @service_api_ns.response( + HTTPStatus.OK, "Feedback submitted successfully", service_api_ns.models[ResultResponse.__name__] + ) @service_api_ns.doc("create_message_feedback") @service_api_ns.doc( summary="Submit Message Feedback", @@ -167,22 +171,30 @@ class MessageFeedbackApi(Resource): ), tags=["Feedback"], responses={ - 404: "`not_found` : Message does not exist.", + HTTPStatus.NOT_FOUND: "`not_found` : Message does not exist.", }, ) @service_api_ns.doc(description="Submit feedback for a message") @service_api_ns.doc(params={"message_id": "Message ID."}) @service_api_ns.doc( responses={ - 200: "Feedback submitted successfully", - 400: "Bad request - invalid feedback payload", - 401: "Unauthorized - invalid API token", - 404: "Message not found", + HTTPStatus.OK: "Feedback submitted successfully", + HTTPStatus.BAD_REQUEST: ( + "`message_feedback_rating_required`: Cannot revoke feedback that does not exist. " + "`app_unavailable`: App is no longer available." + ), + HTTPStatus.UNAUTHORIZED: "Unauthorized - invalid API token", + HTTPStatus.NOT_FOUND: "Message not found", } ) - @validate_app_token(fetch_user_arg=FetchUserArg(fetch_from=WhereisUserArg.JSON, required=True)) + @service_api_end_user_admission(fetch_user_arg=FetchUserArg(fetch_from=WhereisUserArg.JSON, required=True)) @model_validate(MessageFeedbackPayload) - def post(self, payload: MessageFeedbackPayload, app_model: App, end_user: EndUser, message_id: UUID): + def post( + self, + payload: MessageFeedbackPayload, + context: ServiceApiEndUserContext, + message_id: UUID, + ) -> dict[str, object]: """Submit feedback for a message. Allows users to rate messages as like/dislike and provide optional feedback content. @@ -190,22 +202,29 @@ class MessageFeedbackApi(Resource): message_id_str = str(message_id) try: - MessageService.create_feedback( - app_model=app_model, + application_services().message_feedbacks.set_feedback( + app_id=context.app_id, + app_owner_tenant_id=context.tenant_id, message_id=message_id_str, - user=end_user, + actor=MessageEndUser(end_user_id=context.end_user_id), rating=FeedbackRating(payload.rating) if payload.rating else None, content=payload.content, - session=db.session(), ) - except MessageNotExistsError as caught_error: - raise NotFoundError("Message Not Exists.") from caught_error + except AppDefinitionUnavailableError as error: + raise AppUnavailableError() from error + except MessageActorNotFoundError as error: + raise NotFoundError("End user not found") from error + except MessageNotExistsError as error: + raise NotFoundError("Message Not Exists.") from error + except FeedbackRatingRequiredError as error: + raise MessageFeedbackRatingRequiredError() from error - return ResultResponse(result="success").model_dump(mode="json") + return dump_response(ResultResponse, {"result": "success"}) @service_api_ns.route("/app/feedbacks") class AppGetFeedbacksApi(Resource): + @error_responses(AppUnavailableError) @service_api_ns.doc("get_app_feedbacks") @service_api_ns.doc( summary="List App Feedbacks", @@ -215,33 +234,40 @@ class AppGetFeedbacksApi(Resource): ), tags=["Feedback"], responses={ - 200: "A list of application feedbacks.", + HTTPStatus.OK: "A list of application feedbacks.", }, ) @service_api_ns.doc(params=query_params_from_model(FeedbackListQuery)) @service_api_ns.doc(description="Get all feedbacks for the application") @service_api_ns.doc( responses={ - 200: "Feedbacks retrieved successfully", - 401: "Unauthorized - invalid API token", + HTTPStatus.OK: "Feedbacks retrieved successfully", + HTTPStatus.BAD_REQUEST: "`app_unavailable`: App is no longer available.", + HTTPStatus.UNAUTHORIZED: "Unauthorized - invalid API token", } ) @service_api_ns.response( - 200, + HTTPStatus.OK, "Feedbacks retrieved successfully", service_api_ns.models[AppFeedbackListResponse.__name__], ) - @validate_app_token + @service_api_app_admission @model_validate(FeedbackListQuery) - def get(self, query_args: FeedbackListQuery, app_model: App): + def get(self, query_args: FeedbackListQuery, context: AppRequestContext) -> dict[str, object]: """Get all feedbacks for the application. Returns paginated list of all feedback submitted for messages in this app. """ - feedbacks = MessageService.get_all_messages_feedbacks( - app_model, page=query_args.page, limit=query_args.limit, session=db.session() - ) - return AppFeedbackListResponse(data=feedbacks).model_dump(mode="json") + try: + feedbacks = application_services().message_feedbacks.get_feedbacks( + app_id=context.app_id, + app_owner_tenant_id=context.tenant_id, + page=query_args.page, + limit=query_args.limit, + ) + except AppDefinitionUnavailableError as error: + raise AppUnavailableError() from error + return dump_response(AppFeedbackListResponse, {"data": feedbacks}) @service_api_ns.route("/messages//suggested") diff --git a/api/controllers/service_api/flask_admission.py b/api/controllers/service_api/flask_admission.py index 06d93d31571..c3e98ea5bc4 100644 --- a/api/controllers/service_api/flask_admission.py +++ b/api/controllers/service_api/flask_admission.py @@ -6,6 +6,7 @@ from typing import Concatenate from flask import request +from controllers.common.errors import UnauthorizedError from controllers.common.resource_access_token_errors import resource_access_token_errors from controllers.service_api.app import error as http_errors from controllers.service_api.wraps import ( @@ -17,8 +18,8 @@ from controllers.service_api.wraps import ( ) from extensions.ext_application_services import application_services from libs.external_api import document_errors -from machinery.context import ServiceApiEndUserContext -from services.app_definition_query_service import AppDefinitionUnavailableError +from machinery.context import AppRequestContext, ServiceApiEndUserContext +from services.app_definition_query_service import AppDefinitionUnavailableError, ServiceApiAppRecord from services.auth.resource_access_token_contracts import is_resource_access_token from services.errors.app import AppAbnormalStatusError, AppApiDisabledError from services.errors.workspace import WorkspaceArchivedError, WorkspaceNotFoundError @@ -43,31 +44,7 @@ def service_api_end_user_admission[T, **P, R]( ) -> Callable[Concatenate[T, P], R]: @wraps(view) def admitted(self: T, /, *args: P.args, **kwargs: P.kwargs) -> R: - auth_token = peek_service_api_bearer_token() - tenant_id = None - if auth_token and is_resource_access_token(auth_token): - with resource_access_token_errors(): - grant = application_services().resource_access_tokens.resolve_app_for_service_api( - token=auth_token, - requested_app_id=request.headers.get("X-Dify-App-ID"), - ) - app_id = next(iter(grant.app_ids)) - tenant_id = grant.tenant_id - else: - token = validate_and_get_api_token("app") - app_id = token.app_id - try: - app = application_services().app_definitions.get_service_api_app(app_id, tenant_id=tenant_id) - except AppDefinitionUnavailableError as error: - raise http_errors.AppNotFoundError() from error - except AppAbnormalStatusError as error: - raise http_errors.AppAbnormalStatusError() from error - except AppApiDisabledError as error: - raise http_errors.AppApiDisabledError() from error - except WorkspaceNotFoundError as error: - raise http_errors.WorkspaceNotFoundError() from error - except WorkspaceArchivedError as error: - raise http_errors.WorkspaceArchivedError() from error + app = _admit_service_api_app() end_user = resolve_service_api_end_user( tenant_id=app.tenant_id, app_id=app.app_id, fetch_user_arg=fetch_user_arg @@ -92,3 +69,59 @@ def service_api_end_user_admission[T, **P, R]( return admitted return decorator + + +def service_api_app_admission[T, **P, R]( + view: Callable[Concatenate[T, AppRequestContext, P], R], +) -> Callable[Concatenate[T, P], R]: + """Admit app-token queries without an end user or request-scoped ORM session. + + Retain the legacy active-workspace/owner requirement without logging in that + owner: handlers consume only the token's app scope, not an Account identity. + """ + + @wraps(view) + def admitted(self: T, /, *args: P.args, **kwargs: P.kwargs) -> R: + app = _admit_service_api_app() + if not application_services().app_definitions.has_service_api_owner(app.tenant_id): + raise UnauthorizedError("Tenant owner account not found or tenant is not active.") + return view(self, AppRequestContext(tenant_id=app.tenant_id, app_id=app.app_id), *args, **kwargs) + + document_app_token_contract(admitted, None) + document_errors( + admitted, + http_errors.AppNotFoundError, + http_errors.AppAbnormalStatusError, + http_errors.AppApiDisabledError, + http_errors.WorkspaceNotFoundError, + http_errors.WorkspaceArchivedError, + ) + return admitted + + +def _admit_service_api_app() -> ServiceApiAppRecord: + auth_token = peek_service_api_bearer_token() + tenant_id = None + if auth_token and is_resource_access_token(auth_token): + with resource_access_token_errors(): + grant = application_services().resource_access_tokens.resolve_app_for_service_api( + token=auth_token, + requested_app_id=request.headers.get("X-Dify-App-ID"), + ) + app_id = next(iter(grant.app_ids)) + tenant_id = grant.tenant_id + else: + token = validate_and_get_api_token("app") + app_id = token.app_id + try: + return application_services().app_definitions.get_service_api_app(app_id, tenant_id=tenant_id) + except AppDefinitionUnavailableError as error: + raise http_errors.AppNotFoundError() from error + except AppAbnormalStatusError as error: + raise http_errors.AppAbnormalStatusError() from error + except AppApiDisabledError as error: + raise http_errors.AppApiDisabledError() from error + except WorkspaceNotFoundError as error: + raise http_errors.WorkspaceNotFoundError() from error + except WorkspaceArchivedError as error: + raise http_errors.WorkspaceArchivedError() from error diff --git a/api/controllers/web/message.py b/api/controllers/web/message.py index 7fa3c025874..c1162419e53 100644 --- a/api/controllers/web/message.py +++ b/api/controllers/web/message.py @@ -7,7 +7,7 @@ from flask import Response from pydantic import BaseModel, Field, TypeAdapter from controllers.common.controller_schemas import MessageFeedbackPayload, MessageListQuery -from controllers.common.errors import InternalServerError, NotFoundError +from controllers.common.errors import InternalServerError, MessageFeedbackRatingRequiredError, NotFoundError from controllers.common.fields import GeneratedAppResponse from controllers.common.schema import query_params_from_model, register_response_schema_models, register_schema_models from controllers.console.wraps import model_validate @@ -42,6 +42,7 @@ from services.errors.app import MoreLikeThisDisabledError from services.errors.app_model_config import AppModelConfigBrokenError from services.errors.conversation import ConversationNotExistsError from services.errors.message import ( + FeedbackRatingRequiredError, FirstMessageNotExistsError, MessageActorNotFoundError, MessageNotExistsError, @@ -117,6 +118,7 @@ class MessageListApi(WebApiResource): @web_ns.route("/messages//feedbacks") class MessageFeedbackApi(WebApiResource): + @error_responses(AppUnavailableError, MessageFeedbackRatingRequiredError) @web_ns.doc("Create Message Feedback") @web_ns.doc(description="Submit feedback (like/dislike) for a specific message.") @web_ns.doc(params={"message_id": {"description": "Message UUID", "type": "string", "required": True}}) @@ -133,32 +135,44 @@ class MessageFeedbackApi(WebApiResource): ) @web_ns.doc( responses={ - 200: "Feedback submitted successfully", - 400: "Bad Request", - 401: "Unauthorized", - 404: "Message Not Found", - 500: "Internal Server Error", + HTTPStatus.OK: "Feedback submitted successfully", + HTTPStatus.BAD_REQUEST: ( + "`message_feedback_rating_required`: Cannot revoke feedback that does not exist. " + "`app_unavailable`: App is no longer available." + ), + HTTPStatus.UNAUTHORIZED: "Unauthorized", + HTTPStatus.FORBIDDEN: "Forbidden", + HTTPStatus.NOT_FOUND: "Message Not Found", + HTTPStatus.INTERNAL_SERVER_ERROR: "Internal Server Error", } ) - @web_ns.response(200, "Feedback submitted successfully", web_ns.models[ResultResponse.__name__]) + @web_ns.response(HTTPStatus.OK, "Feedback submitted successfully", web_ns.models[ResultResponse.__name__]) @web_ns.expect(web_ns.models[MessageFeedbackPayload.__name__]) @model_validate(MessageFeedbackPayload) - def post(self, payload: MessageFeedbackPayload, app_model: App, end_user: EndUser, message_id: UUID): + def post( + self, payload: MessageFeedbackPayload, app_model: App, end_user: EndUser, message_id: UUID + ) -> dict[str, object]: message_id_str = str(message_id) try: - MessageService.create_feedback( - app_model=app_model, + application_services().message_feedbacks.set_feedback( + app_id=app_model.id, + app_owner_tenant_id=app_model.tenant_id, message_id=message_id_str, - user=end_user, + actor=MessageEndUser(end_user_id=end_user.id), rating=FeedbackRating(payload.rating) if payload.rating else None, content=payload.content, - session=db.session(), ) - except MessageNotExistsError as caught_error: - raise NotFoundError("Message Not Exists.") from caught_error + except AppDefinitionUnavailableError as error: + raise AppUnavailableError() from error + except MessageActorNotFoundError as error: + raise NotFoundError("End user not found") from error + except MessageNotExistsError as error: + raise NotFoundError("Message Not Exists.") from error + except FeedbackRatingRequiredError as error: + raise MessageFeedbackRatingRequiredError() from error - return ResultResponse(result="success").model_dump(mode="json") + return helper.dump_response(ResultResponse, {"result": "success"}) @web_ns.route("/messages//more-like-this") diff --git a/api/extensions/application_services/installed_app.py b/api/extensions/application_services/installed_app.py index 84f1e46d88a..30f6d66cfe8 100644 --- a/api/extensions/application_services/installed_app.py +++ b/api/extensions/application_services/installed_app.py @@ -13,7 +13,7 @@ from services.installed_app_access_service import InstalledAppAccessService from services.installed_app_conversation_service import InstalledAppConversationService from services.installed_app_generation_adapters import AppGenerateServiceRuntime from services.installed_app_generation_service import InstalledAppGenerationService -from services.installed_app_message_adapters import InstalledAppMessageRuntime, emit_installed_app_feedback +from services.installed_app_message_adapters import InstalledAppMessageRuntime from services.installed_app_message_service import InstalledAppMessageService from services.installed_app_service import InstalledAppService, WorkspaceRoleLookup from services.webapp_access_query_service import WebAppAccessQueryService @@ -74,6 +74,5 @@ def build_installed_app_services( messages=InstalledAppMessageService( messages=SQLAlchemyInstalledAppMessageRepository(session_factory=database_client), get_extra_contents=message_runtime.get_extra_contents, - emit_feedback=emit_installed_app_feedback, ), ) diff --git a/api/extensions/ext_application_services.py b/api/extensions/ext_application_services.py index 9b613251036..1325b6db99b 100644 --- a/api/extensions/ext_application_services.py +++ b/api/extensions/ext_application_services.py @@ -144,6 +144,7 @@ from services.inner_mail_service import InnerMailService from services.knowledge.api_key_service import DatasetApiKeyService from services.knowledge_fs.builtin_tool.knowledge_fs.knowledge_fs import KnowledgeFSProvider from services.knowledge_fs.workflow_runtime import build_knowledge_fs_node_dependencies +from services.message_feedback_service import MessageFeedbackService from services.message_file_preview_service import MessageFilePreviewService from services.message_more_like_this_generator import MessageMoreLikeThisGenerator from services.message_more_like_this_service import MessageMoreLikeThisService @@ -301,6 +302,7 @@ class ApplicationServices: file_grants: FileGrantService files: FileService human_input_file_uploads: HumanInputFileUploadService + message_feedbacks: MessageFeedbackService message_file_previews: MessageFilePreviewService message_suggested_questions: MessageSuggestedQuestions message_more_like_this: MessageMoreLikeThisService @@ -663,6 +665,7 @@ def build_application_services( files=file_service, remote_files=remote_file_service, ), + message_feedbacks=MessageFeedbackService(repository=messages), message_file_previews=MessageFilePreviewService( files=MessageFilePreviewQueryRepository(session_factory=database_client), storage=storage, diff --git a/api/openapi/markdown/console-openapi.md b/api/openapi/markdown/console-openapi.md index 4accd1cd5eb..004eae02bd1 100644 --- a/api/openapi/markdown/console-openapi.md +++ b/api/openapi/markdown/console-openapi.md @@ -1192,6 +1192,7 @@ Create or update Agent App message feedback | Code | Description | Schema | | ---- | ----------- | ------ | | 200 | Feedback updated successfully | **application/json**: [SimpleResultResponse](#simpleresultresponse)
| +| 400 | Bad Request | **application/json**: [AppUnavailableError](#appunavailableerror)
[MessageFeedbackRatingRequiredError](#messagefeedbackratingrequirederror)
[ErrorResponse](#errorresponse)
| | 404 | Agent or message not found | **application/json**: [AgentNotFoundError](#agentnotfounderror)
[ErrorResponse](#errorresponse)
| | default | Error | **application/json**: [ErrorResponse](#errorresponse)
| @@ -3219,6 +3220,8 @@ Create or update message feedback (like/dislike) | Code | Description | Schema | | ---- | ----------- | ------ | | 200 | Feedback updated successfully | **application/json**: [SimpleResultResponse](#simpleresultresponse)
| +| 400 | Bad Request | **application/json**: [AppUnavailableError](#appunavailableerror)
[MessageFeedbackRatingRequiredError](#messagefeedbackratingrequirederror)
[ErrorResponse](#errorresponse)
| +| 403 | Insufficient permissions | **application/json**: [ErrorResponse](#errorresponse)
| | 404 | Message not found | **application/json**: [AppNotFoundError](#appnotfounderror)
[ErrorResponse](#errorresponse)
| | default | Error | **application/json**: [ErrorResponse](#errorresponse)
| @@ -8011,7 +8014,7 @@ Request body: | Code | Description | Schema | | ---- | ----------- | ------ | | 200 | Feedback submitted successfully | **application/json**: [ResultResponse](#resultresponse)
| -| 400 | Bad Request | **application/json**: [MessageFeedbackRatingRequiredError](#messagefeedbackratingrequirederror)
[ErrorResponse](#errorresponse)
| +| 400 | Bad Request | **application/json**: [AppUnavailableError](#appunavailableerror)
[MessageFeedbackRatingRequiredError](#messagefeedbackratingrequirederror)
[ErrorResponse](#errorresponse)
| | 403 | Forbidden | **application/json**: [AccessDeniedError](#accessdeniederror)
[ErrorResponse](#errorresponse)
| | 404 | Not Found | **application/json**: [InstalledAppNotFoundError](#installedappnotfounderror)
[MessageNotFoundError](#messagenotfounderror)
[ErrorResponse](#errorresponse)
| | 503 | Service Unavailable | **application/json**: [WebAppAccessUnavailableError](#webappaccessunavailableerror)
[ErrorResponse](#errorresponse)
| diff --git a/api/openapi/markdown/service-openapi.md b/api/openapi/markdown/service-openapi.md index b505b6a575f..60d05cca434 100644 --- a/api/openapi/markdown/service-openapi.md +++ b/api/openapi/markdown/service-openapi.md @@ -1745,8 +1745,9 @@ Retrieve a paginated list of all feedback submitted for messages in this applica | Code | Description | Schema | | ---- | ----------- | ------ | | 200 | A list of application feedbacks. | **application/json**: [AppFeedbackListResponse](#appfeedbacklistresponse)
| +| 400 | `app_unavailable`: App is no longer available. | **application/json**: [AppUnavailableError](#appunavailableerror)
[ErrorResponse](#errorresponse)
| | 401 | Unauthorized - invalid API token | **application/json**: [ErrorResponse](#errorresponse)
| -| 403 | Forbidden - token scope, app, dataset, or workspace access denied | **application/json**: [ErrorResponse](#errorresponse)
| +| 403 | Forbidden - token scope, app, dataset, or workspace access denied | **application/json**: [AppAbnormalStatusError](#appabnormalstatuserror)
[AppApiDisabledError](#appapidisablederror)
[AppNotFoundError](#appnotfounderror)
[WorkspaceArchivedError](#workspacearchivederror)
[WorkspaceNotFoundError](#workspacenotfounderror)
[ErrorResponse](#errorresponse)
| | default | Error | **application/json**: [ErrorResponse](#errorresponse)
| ### [POST] /messages/{message_id}/feedbacks @@ -1771,9 +1772,9 @@ Submit feedback for a message. End users can rate messages as `like` or `dislike | Code | Description | Schema | | ---- | ----------- | ------ | | 200 | Feedback submitted successfully | **application/json**: [ResultResponse](#resultresponse)
| -| 400 | Bad request - invalid feedback payload | **application/json**: [ErrorResponse](#errorresponse)
| +| 400 | `message_feedback_rating_required`: Cannot revoke feedback that does not exist. `app_unavailable`: App is no longer available. | **application/json**: [AppUnavailableError](#appunavailableerror)
[MessageFeedbackRatingRequiredError](#messagefeedbackratingrequirederror)
[ErrorResponse](#errorresponse)
| | 401 | Unauthorized - invalid API token | **application/json**: [ErrorResponse](#errorresponse)
| -| 403 | Forbidden - token scope, app, dataset, or workspace access denied | **application/json**: [ErrorResponse](#errorresponse)
| +| 403 | Forbidden - token scope, app, dataset, or workspace access denied | **application/json**: [AppAbnormalStatusError](#appabnormalstatuserror)
[AppApiDisabledError](#appapidisablederror)
[AppNotFoundError](#appnotfounderror)
[WorkspaceArchivedError](#workspacearchivederror)
[WorkspaceNotFoundError](#workspacenotfounderror)
[ErrorResponse](#errorresponse)
| | 404 | `not_found` : Message does not exist. | **application/json**: [ErrorResponse](#errorresponse)
| | default | Error | **application/json**: [ErrorResponse](#errorresponse)
| @@ -7064,6 +7065,14 @@ Agent Knowledge Base is unavailable. | rating | string,
**Available values:** "dislike", "like" | Feedback rating. Set to `null` to revoke previously submitted feedback. | No | | user | string | End-user identifier, defined by your app and unique within it. Identifies the end user for this request. See [End User Identity](/api-reference/guides/end-user-identity) for endpoint-specific access rules. | Yes | +#### MessageFeedbackRatingRequiredError + +A rating is required when there is no existing feedback to remove. + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| MessageFeedbackRatingRequiredError | | A rating is required when there is no existing feedback to remove. | | + #### MessageFile | Name | Type | Description | Required | diff --git a/api/openapi/markdown/web-openapi.md b/api/openapi/markdown/web-openapi.md index 5d02fa7ffc4..69af62206a7 100644 --- a/api/openapi/markdown/web-openapi.md +++ b/api/openapi/markdown/web-openapi.md @@ -549,8 +549,9 @@ Submit feedback (like/dislike) for a specific message. | Code | Description | Schema | | ---- | ----------- | ------ | | 200 | Feedback submitted successfully | **application/json**: [ResultResponse](#resultresponse)
| -| 400 | Bad Request | **application/json**: [ErrorResponse](#errorresponse)
| +| 400 | `message_feedback_rating_required`: Cannot revoke feedback that does not exist. `app_unavailable`: App is no longer available. | **application/json**: [AppUnavailableError](#appunavailableerror)
[MessageFeedbackRatingRequiredError](#messagefeedbackratingrequirederror)
[ErrorResponse](#errorresponse)
| | 401 | Unauthorized | **application/json**: [ErrorResponse](#errorresponse)
| +| 403 | Forbidden | **application/json**: [ErrorResponse](#errorresponse)
| | 404 | Message Not Found | **application/json**: [ErrorResponse](#errorresponse)
| | 500 | Internal Server Error | **application/json**: [ErrorResponse](#errorresponse)
| | default | Error | **application/json**: [ErrorResponse](#errorresponse)
| @@ -1586,6 +1587,14 @@ The upload token is malformed. | content | string | Optional text feedback providing additional detail. | No | | rating | string,
**Available values:** "dislike", "like" | Feedback rating. Set to `null` to revoke previously submitted feedback. | No | +#### MessageFeedbackRatingRequiredError + +A rating is required when there is no existing feedback to remove. + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| MessageFeedbackRatingRequiredError | | A rating is required when there is no existing feedback to remove. | | + #### MessageFile | Name | Type | Description | Required | diff --git a/api/repositories/app_definition_query_repository.py b/api/repositories/app_definition_query_repository.py index 48c70d381a2..aa0582a53c4 100644 --- a/api/repositories/app_definition_query_repository.py +++ b/api/repositories/app_definition_query_repository.py @@ -10,7 +10,7 @@ from core.agent.publish_visibility import agent_has_workflow_callable_active_sna from core.app.apps.agent_app.app_feature_projection import merge_agent_app_features from core.app.apps.agent_app.app_variable_projection import agent_app_variables_to_user_input_form from core.app.apps.agent_app.errors import AgentAppGeneratorError, AgentAppNotPublishedError -from models.account import Tenant, TenantStatus +from models.account import Account, Tenant, TenantAccountJoin, TenantAccountRole, TenantStatus from models.agent import AgentConfigSnapshot from models.agent_config_entities import AgentSoulConfig from models.model import App, AppMode, AppModelConfig, Site, load_annotation_reply_config @@ -145,6 +145,25 @@ class AppDefinitionQueryRepository(AppDefinitionQuery): tenant_status=tenant_status.value if tenant_status is not None else None, ) + @override + def has_service_api_owner(self, tenant_id: str) -> bool: + with self._session_factory() as session: + return bool( + session.scalar( + select( + select(Account.id) + .join(TenantAccountJoin, TenantAccountJoin.account_id == Account.id) + .join(Tenant, Tenant.id == TenantAccountJoin.tenant_id) + .where( + Tenant.id == tenant_id, + Tenant.status == TenantStatus.NORMAL, + TenantAccountJoin.role == TenantAccountRole.OWNER, + ) + .exists() + ) + ) + ) + @override def get_mode(self, app_id: str) -> str | None: with self._session_factory() as session: diff --git a/api/repositories/installed_app_message_repository.py b/api/repositories/installed_app_message_repository.py index 47f6fe8e481..b5f39648f14 100644 --- a/api/repositories/installed_app_message_repository.py +++ b/api/repositories/installed_app_message_repository.py @@ -6,21 +6,18 @@ from pydantic import JsonValue from sqlalchemy import and_, or_, select from sqlalchemy.orm import Session, sessionmaker -from models.enums import ConversationFromSource, FeedbackFromSource, FeedbackRating -from models.model import App, Conversation, InstalledApp, Message, MessageAgentThought, MessageFeedback +from models.enums import ConversationFromSource +from models.model import App, Conversation, InstalledApp, Message, MessageAgentThought from services.errors.conversation import ConversationNotExistsError -from services.errors.message import FirstMessageNotExistsError, MessageNotExistsError +from services.errors.message import FirstMessageNotExistsError from services.installed_app_access_service import InstalledAppNotFoundError, InstalledAppRef from services.installed_app_message_service import ( - FeedbackRatingRequiredError, InstalledAppMessageStore, MessageAgentThoughtRecord, - MessageFeedbackEvent, MessageFeedbackRecord, MessageFileRecord, MessageInputValue, MessagePage, - MessageRating, MessageRecord, ) @@ -86,63 +83,6 @@ class SQLAlchemyInstalledAppMessageRepository(InstalledAppMessageStore): data=tuple(self._to_record(message=message, session=session) for message in messages), ) - @override - def set_feedback( - self, - *, - installed_app: InstalledAppRef, - account_id: str, - message_id: str, - rating: MessageRating | None, - content: str | None, - ) -> MessageFeedbackEvent | None: - with self._session_factory.begin() as session: - app = self._get_app(session=session, installed_app=installed_app) - message = session.scalar( - select(Message).where( - Message.id == message_id, - Message.app_id == app.id, - Message.from_source == ConversationFromSource.CONSOLE, - Message.from_account_id == account_id, - Message.from_end_user_id.is_(None), - ) - ) - if message is None: - raise MessageNotExistsError(f"Message {message_id} does not belong to this account and app.") - feedback = message.admin_feedback_with_session(session=session) - if rating is None: - if feedback is None: - raise FeedbackRatingRequiredError( - f"Message {message_id} has no admin feedback to remove; a rating is required." - ) - session.delete(feedback) - return None - if feedback is None: - session.add( - MessageFeedback( - app_id=app.id, - conversation_id=message.conversation_id, - message_id=message.id, - rating=FeedbackRating(rating), - content=content, - from_source=FeedbackFromSource.ADMIN, - from_end_user_id=None, - from_account_id=account_id, - ) - ) - else: - feedback.rating = FeedbackRating(rating) - feedback.content = content - return MessageFeedbackEvent( - tenant_id=app.tenant_id, - app_id=app.id, - conversation_id=message.conversation_id, - message_id=message.id, - account_id=account_id, - rating=rating, - content=content, - ) - @staticmethod def _get_app(*, session: Session, installed_app: InstalledAppRef) -> App: app = session.scalar( diff --git a/api/repositories/message_repository.py b/api/repositories/message_repository.py index 86bdc2abc42..ae3e28a333b 100644 --- a/api/repositories/message_repository.py +++ b/api/repositories/message_repository.py @@ -1,4 +1,4 @@ -"""Message ownership and generation reads shared by message capabilities. +"""Message ownership, feedback and generation reads shared by message capabilities. History and more-like-this reads own short sessions and return detached data. Only suggested-question configuration preparation borrows the caller's session. @@ -20,12 +20,15 @@ from sqlalchemy.orm import Session, sessionmaker from core.memory.token_buffer_memory import PreparedHistory, TokenBufferMemory from models import Account, App, AppModelConfig, Conversation, EndUser, Message, MessageFile -from models.model import AppMode, load_annotation_reply_config +from models.enums import FeedbackFromSource, FeedbackRating +from models.model import AppMode, InstalledApp, MessageFeedback, load_annotation_reply_config from services.app_definition_query_service import AppDefinitionUnavailableError from services.entities.message_entities import MessageAccount, MessageActor from services.errors.app_model_config import AppModelConfigBrokenError from services.errors.conversation import ConversationNotExistsError -from services.errors.message import MessageActorNotFoundError, MessageNotExistsError +from services.errors.message import FeedbackRatingRequiredError, MessageActorNotFoundError, MessageNotExistsError +from services.installed_app_access_service import InstalledAppNotFoundError, InstalledAppRef +from services.message_feedback_service import MessageFeedbackRecord from services.message_more_like_this_service import ( MoreLikeThisFile, MoreLikeThisNotCompletionError, @@ -43,6 +46,140 @@ class MessageRepository: def __init__(self, *, session_factory: sessionmaker[Session]) -> None: self._session_factory: sessionmaker[Session] = session_factory + def set_feedback( + self, + *, + app_id: str, + app_owner_tenant_id: str, + actor: MessageActor, + message_id: str, + rating: FeedbackRating | None, + content: str | None, + app_wide: bool, + installed_app: InstalledAppRef | None, + ) -> MessageFeedbackRecord | None: + """Validate ownership and write atomically; None rating revokes feedback. + + app_wide is reserved for an admitted Console admin. Installed requests + also recheck their installation in this transaction; others pass None. + The returned record is detached; revocation returns None. + """ + with self._session_factory.begin() as session: + if installed_app is None: + app = self._get_app(session, app_id=app_id, tenant_id=app_owner_tenant_id) + else: + app = session.scalar( + select(App) + .join(InstalledApp, InstalledApp.app_id == App.id) + .where( + App.id == app_id, + App.tenant_id == app_owner_tenant_id, + App.tenant_id == installed_app.app_owner_tenant_id, + InstalledApp.id == installed_app.id, + InstalledApp.tenant_id == installed_app.tenant_id, + InstalledApp.app_id == installed_app.app_id, + ) + ) + if app is None: + raise InstalledAppNotFoundError(f"Installed app {installed_app.id} no longer exists") + if app is None: + raise AppDefinitionUnavailableError(f"App {app_id} is unavailable in tenant {app_owner_tenant_id}") + + account_id = actor.account_id if isinstance(actor, MessageAccount) else None + end_user_id = None if isinstance(actor, MessageAccount) else actor.end_user_id + if not self._actor_exists( + session, + app_id=app_id, + tenant_id=app_owner_tenant_id, + account_id=account_id, + end_user_id=end_user_id, + ): + raise MessageActorNotFoundError( + f"Message actor {account_id or end_user_id} is unavailable for app {app_id}" + ) + + if app_wide: + if not isinstance(actor, MessageAccount): + raise ValueError("App-wide feedback requires an admitted Console account") + message = session.scalar(select(Message).where(Message.id == message_id, Message.app_id == app_id)) + else: + message = self._get_message( + session, + message_id=message_id, + app_id=app_id, + source="console" if isinstance(actor, MessageAccount) else "api", + account_id=account_id, + end_user_id=end_user_id, + ) + if message is None: + raise MessageNotExistsError(f"Message {message_id} is unavailable for this app and actor") + + source = FeedbackFromSource.ADMIN if isinstance(actor, MessageAccount) else FeedbackFromSource.USER + feedback = session.scalar( + select(MessageFeedback).where( + MessageFeedback.app_id == app_id, + MessageFeedback.message_id == message_id, + MessageFeedback.from_source == source, + ) + ) + if rating is None: + if feedback is None: + raise FeedbackRatingRequiredError( + f"Message {message_id} has no {source.value} feedback to remove; a rating is required." + ) + session.delete(feedback) + return None + if feedback is None: + feedback = MessageFeedback( + app_id=app_id, + conversation_id=message.conversation_id, + message_id=message_id, + rating=rating, + content=content, + from_source=source, + from_end_user_id=end_user_id, + from_account_id=account_id, + ) + session.add(feedback) + else: + # Admin feedback is shared per message; updating it preserves the + # original reviewer identity, as in the existing Console endpoints. + feedback.rating = rating + feedback.content = content + session.flush() + return self._feedback_record(feedback) + + def get_feedbacks( + self, *, app_id: str, app_owner_tenant_id: str, page: int, limit: int + ) -> list[MessageFeedbackRecord]: + with self._session_factory() as session: + if self._get_app(session, app_id=app_id, tenant_id=app_owner_tenant_id) is None: + raise AppDefinitionUnavailableError(f"App {app_id} is unavailable in tenant {app_owner_tenant_id}") + feedbacks = session.scalars( + select(MessageFeedback) + .where(MessageFeedback.app_id == app_id) + .order_by(MessageFeedback.created_at.desc(), MessageFeedback.id.desc()) + .limit(limit) + .offset((page - 1) * limit) + ) + return [self._feedback_record(feedback) for feedback in feedbacks] + + @staticmethod + def _feedback_record(feedback: MessageFeedback) -> MessageFeedbackRecord: + return MessageFeedbackRecord( + id=feedback.id, + app_id=feedback.app_id, + conversation_id=feedback.conversation_id, + message_id=feedback.message_id, + rating=feedback.rating, + content=feedback.content, + from_source=feedback.from_source, + from_end_user_id=feedback.from_end_user_id, + from_account_id=feedback.from_account_id, + created_at=feedback.created_at.isoformat(), + updated_at=feedback.updated_at.isoformat(), + ) + def get_suggested_questions_context( self, *, diff --git a/api/services/app_definition_query_service.py b/api/services/app_definition_query_service.py index e8d291b52a6..201ff21c63a 100644 --- a/api/services/app_definition_query_service.py +++ b/api/services/app_definition_query_service.py @@ -64,6 +64,8 @@ class AppDefinitionQuery(Protocol): def get_service_api_record(self, app_id: str, *, tenant_id: str | None = None) -> ServiceApiAppRecord | None: ... + def has_service_api_owner(self, tenant_id: str) -> bool: ... + def get_published_parameter_config( self, app_id: str, @@ -122,6 +124,10 @@ class AppDefinitionQueryService: raise WorkspaceArchivedError("The workspace's status is archived.") return app + def has_service_api_owner(self, tenant_id: str) -> bool: + """Whether an active API workspace still has its legacy owner identity.""" + return self._definitions.has_service_api_owner(tenant_id) + def get_mode(self, app_id: str) -> str: mode = self._definitions.get_mode(app_id) if mode is None: diff --git a/api/services/errors/message.py b/api/services/errors/message.py index 710b918633c..0e9624ed993 100644 --- a/api/services/errors/message.py +++ b/api/services/errors/message.py @@ -5,6 +5,10 @@ class MessageActorNotFoundError(LookupError): """The account is missing, or the end user is outside the admitted app and tenant scope.""" +class FeedbackRatingRequiredError(BaseServiceError): + """Feedback cannot be revoked because no feedback exists for this source.""" + + class FirstMessageNotExistsError(BaseServiceError): pass diff --git a/api/services/installed_app_message_adapters.py b/api/services/installed_app_message_adapters.py index 418445fa827..561ad3336c3 100644 --- a/api/services/installed_app_message_adapters.py +++ b/api/services/installed_app_message_adapters.py @@ -1,15 +1,11 @@ -"""Execution extra contents and feedback telemetry for installed-app messages.""" +"""Execution extra contents for installed-app messages.""" -import logging from collections.abc import Mapping, Sequence from pydantic import JsonValue from sqlalchemy.orm import Session, sessionmaker from repositories.sqlalchemy_execution_extra_content_repository import SQLAlchemyExecutionExtraContentRepository -from services.installed_app_message_service import MessageFeedbackEvent - -logger = logging.getLogger(__name__) class InstalledAppMessageRuntime: @@ -24,27 +20,3 @@ class InstalledAppMessageRuntime: message_id: [content.model_dump(mode="json", exclude_none=True) for content in items] for message_id, items in zip(message_ids, contents, strict=True) } - - -def emit_installed_app_feedback(*, feedback: MessageFeedbackEvent) -> None: - # Feedback has already committed. Telemetry failure must not reject it. - try: - from core.telemetry import FeedbackCreatedEvent, TelemetryContext, emit - - emit( - FeedbackCreatedEvent( - context=TelemetryContext(tenant_id=feedback.tenant_id), - payload={ - "message_id": feedback.message_id, - "app_id": feedback.app_id, - "conversation_id": feedback.conversation_id, - "from_end_user_id": None, - "from_account_id": feedback.account_id, - "rating": feedback.rating, - "from_source": "admin", - "content": feedback.content, - }, - ) - ) - except Exception: - logger.warning("Failed to emit feedback telemetry for message %s", feedback.message_id, exc_info=True) diff --git a/api/services/installed_app_message_service.py b/api/services/installed_app_message_service.py index 2abd664cda0..f799d4efd31 100644 --- a/api/services/installed_app_message_service.py +++ b/api/services/installed_app_message_service.py @@ -1,17 +1,16 @@ -"""Message reads and feedback for an admitted installation.""" +"""Message reads for an admitted installation.""" from collections.abc import Mapping, Sequence from dataclasses import dataclass, field, replace from datetime import datetime from decimal import Decimal -from typing import Literal, Protocol +from typing import Protocol from pydantic import JsonValue from graphon.file import File from services.installed_app_access_service import InstalledAppRef -type MessageRating = Literal["like", "dislike"] type MessageInputValue = JsonValue | File | list[File] @@ -19,10 +18,6 @@ class MessageNotChatAppError(Exception): pass -class FeedbackRatingRequiredError(Exception): - pass - - @dataclass(frozen=True, slots=True) class MessageFeedbackRecord: rating: str | None @@ -88,17 +83,6 @@ class MessagePage: data: tuple[MessageRecord, ...] -@dataclass(frozen=True, slots=True) -class MessageFeedbackEvent: - tenant_id: str - app_id: str - conversation_id: str - message_id: str - account_id: str - rating: str - content: str | None - - class InstalledAppMessageStore(Protocol): def get_page( self, @@ -110,36 +94,20 @@ class InstalledAppMessageStore(Protocol): limit: int, ) -> MessagePage: ... - def set_feedback( - self, - *, - installed_app: InstalledAppRef, - account_id: str, - message_id: str, - rating: MessageRating | None, - content: str | None, - ) -> MessageFeedbackEvent | None: ... - class MessageExtraContentsQuery(Protocol): def __call__(self, *, message_ids: Sequence[str]) -> Mapping[str, list[dict[str, JsonValue]]]: ... -class MessageFeedbackEmitter(Protocol): - def __call__(self, *, feedback: MessageFeedbackEvent) -> None: ... - - class InstalledAppMessageService: def __init__( self, *, messages: InstalledAppMessageStore, get_extra_contents: MessageExtraContentsQuery, - emit_feedback: MessageFeedbackEmitter, ) -> None: self._messages: InstalledAppMessageStore = messages self._get_extra_contents: MessageExtraContentsQuery = get_extra_contents - self._emit_feedback: MessageFeedbackEmitter = emit_feedback def get_page( self, @@ -166,25 +134,6 @@ class InstalledAppMessageService: data=tuple(replace(message, extra_contents=contents.get(message.id, [])) for message in page.data), ) - def set_feedback( - self, - *, - installed_app: InstalledAppRef, - account_id: str, - message_id: str, - rating: MessageRating | None, - content: str | None, - ) -> None: - feedback = self._messages.set_feedback( - installed_app=installed_app, - account_id=account_id, - message_id=message_id, - rating=rating, - content=content, - ) - if feedback is not None: - self._emit_feedback(feedback=feedback) - @staticmethod def _require_chat_app(installed_app: InstalledAppRef) -> None: if installed_app.app_mode not in {"chat", "agent-chat", "advanced-chat"}: diff --git a/api/services/message_feedback_service.py b/api/services/message_feedback_service.py new file mode 100644 index 00000000000..9c321b65f9c --- /dev/null +++ b/api/services/message_feedback_service.py @@ -0,0 +1,140 @@ +"""Message feedback shared by user-facing and Console management endpoints.""" + +import logging +from dataclasses import dataclass +from typing import Protocol + +from models.enums import FeedbackRating +from services.entities.message_entities import MessageAccount, MessageActor +from services.installed_app_access_service import InstalledAppRef + +logger = logging.getLogger(__name__) + + +@dataclass(frozen=True, slots=True) +class MessageFeedbackRecord: + id: str + app_id: str + conversation_id: str + message_id: str + rating: str + content: str | None + from_source: str + from_end_user_id: str | None + from_account_id: str | None + created_at: str + updated_at: str + + +class MessageFeedbackStore(Protocol): + def set_feedback( + self, + *, + app_id: str, + app_owner_tenant_id: str, + actor: MessageActor, + message_id: str, + rating: FeedbackRating | None, + content: str | None, + app_wide: bool, + installed_app: InstalledAppRef | None, + ) -> MessageFeedbackRecord | None: + """Commit a rating and return detached data, or revoke it and return None.""" + ... + + def get_feedbacks( + self, *, app_id: str, app_owner_tenant_id: str, page: int, limit: int + ) -> list[MessageFeedbackRecord]: ... + + +class MessageFeedbackService: + def __init__(self, *, repository: MessageFeedbackStore) -> None: + self._repository: MessageFeedbackStore = repository + + def set_feedback( + self, + *, + app_id: str, + app_owner_tenant_id: str, + actor: MessageActor, + message_id: str, + rating: FeedbackRating | None, + content: str | None, + installed_app: InstalledAppRef | None = None, + ) -> None: + """Rate the actor's own message; None revokes existing feedback. + + Explore supplies its installation to recheck admission in the write + transaction. Other entrypoints have no installation (None). + Content may be absent (None) or explicitly empty, and is saved as given. + """ + feedback = self._repository.set_feedback( + app_id=app_id, + app_owner_tenant_id=app_owner_tenant_id, + actor=actor, + message_id=message_id, + rating=rating, + content=content, + app_wide=False, + installed_app=installed_app, + ) + if feedback is None: + return + + # The repository has committed and closed its session. Telemetry failure + # must not turn a successful feedback write into a failed HTTP request. + try: + from core.telemetry import FeedbackCreatedEvent, TelemetryContext, emit + + emit( + FeedbackCreatedEvent( + context=TelemetryContext(tenant_id=app_owner_tenant_id), + payload={ + "message_id": feedback.message_id, + "app_id": feedback.app_id, + "conversation_id": feedback.conversation_id, + "from_end_user_id": None if isinstance(actor, MessageAccount) else actor.end_user_id, + "from_account_id": actor.account_id if isinstance(actor, MessageAccount) else None, + "rating": feedback.rating, + "from_source": feedback.from_source, + "content": feedback.content, + }, + ) + ) + except Exception: + logger.warning("Failed to emit feedback telemetry for message %s", message_id, exc_info=True) + + def set_admin_feedback( + self, + *, + app_id: str, + app_owner_tenant_id: str, + account_id: str, + message_id: str, + rating: FeedbackRating | None, + content: str | None, + ) -> None: + """Rate any message in an admitted Console app, without user telemetry. + + None rating revokes existing admin feedback. None content means no comment. + Console admission owns workspace/RBAC checks; being a MessageAccount alone + does not grant this broader message access. + """ + self._repository.set_feedback( + app_id=app_id, + app_owner_tenant_id=app_owner_tenant_id, + actor=MessageAccount(account_id=account_id), + message_id=message_id, + rating=rating, + content=content, + app_wide=True, + installed_app=None, + ) + + def get_feedbacks( + self, *, app_id: str, app_owner_tenant_id: str, page: int, limit: int + ) -> list[MessageFeedbackRecord]: + """List app feedback newest first, using one-based pages.""" + return self._repository.get_feedbacks( + app_id=app_id, app_owner_tenant_id=app_owner_tenant_id, page=page, limit=limit + ) diff --git a/api/services/message_service.py b/api/services/message_service.py index 40f529f5fb0..661f24d0e42 100644 --- a/api/services/message_service.py +++ b/api/services/message_service.py @@ -1,4 +1,3 @@ -import logging from collections.abc import Sequence from sqlalchemy import select @@ -7,12 +6,10 @@ from sqlalchemy.orm import Session, sessionmaker from extensions.ext_database import db from libs.infinite_scroll_pagination import InfiniteScrollPagination from models import Account -from models.enums import FeedbackFromSource, FeedbackRating from models.model import ( App, EndUser, Message, - MessageFeedback, ) from repositories.execution_extra_content_repository import ExecutionExtraContentRepository from repositories.sqlalchemy_execution_extra_content_repository import ( @@ -25,8 +22,6 @@ from services.errors.message import ( MessageNotExistsError, ) -logger = logging.getLogger(__name__) - def _create_execution_extra_content_repository() -> ExecutionExtraContentRepository: session_maker = sessionmaker(bind=db.engine, expire_on_commit=False) @@ -161,107 +156,6 @@ class MessageService: return InfiniteScrollPagination(data=history_messages, limit=limit, has_more=has_more) - @classmethod - def create_feedback( - cls, - *, - app_model: App, - message_id: str, - user: Account | EndUser | None, - rating: FeedbackRating | None, - content: str | None, - session: Session, - ): - if not user: - raise ValueError("user cannot be None") - - message = cls.get_message(app_model=app_model, user=user, message_id=message_id, session=session) - - feedback = ( - message.user_feedback_with_session(session=session) - if isinstance(user, EndUser) - else message.admin_feedback_with_session(session=session) - ) - - if not rating and feedback: - session.delete(feedback) - elif rating and feedback: - feedback.rating = rating - feedback.content = content - elif not rating and not feedback: - raise ValueError("rating cannot be None when feedback not exists") - else: - assert rating is not None - feedback = MessageFeedback( - app_id=app_model.id, - conversation_id=message.conversation_id, - message_id=message.id, - rating=rating, - content=content, - from_source=(FeedbackFromSource.USER if isinstance(user, EndUser) else FeedbackFromSource.ADMIN), - from_end_user_id=(user.id if isinstance(user, EndUser) else None), - from_account_id=(user.id if isinstance(user, Account) else None), - ) - session.add(feedback) - - session.commit() - if rating: - cls._emit_feedback_telemetry( - app_model=app_model, message=message, user=user, rating=rating, content=content - ) - - return feedback - - @classmethod - def _emit_feedback_telemetry( - cls, - *, - app_model: App, - message: Message, - user: Account | EndUser, - rating: FeedbackRating | None, - content: str | None, - ) -> None: - try: - from core.telemetry import FeedbackCreatedEvent, TelemetryContext, emit - - if message.id is None: - return - - emit( - FeedbackCreatedEvent( - context=TelemetryContext(tenant_id=app_model.tenant_id), - payload={ - "message_id": message.id, - "app_id": app_model.id, - "conversation_id": message.conversation_id, - "from_end_user_id": user.id if isinstance(user, EndUser) else None, - "from_account_id": user.id if isinstance(user, Account) else None, - "rating": rating.value if rating else None, - "from_source": ( - FeedbackFromSource.USER if isinstance(user, EndUser) else FeedbackFromSource.ADMIN - ).value, - "content": content, - }, - ) - ) - except Exception: - logger.warning("Failed to emit feedback_created telemetry", exc_info=True) - - @classmethod - def get_all_messages_feedbacks(cls, app_model: App, page: int, limit: int, *, session: Session): - """Get all feedbacks of an app""" - offset = (page - 1) * limit - feedbacks = session.scalars( - select(MessageFeedback) - .where(MessageFeedback.app_id == app_model.id) - .order_by(MessageFeedback.created_at.desc(), MessageFeedback.id.desc()) - .limit(limit) - .offset(offset) - ).all() - - return [record.to_dict() for record in feedbacks] - @classmethod def get_message(cls, app_model: App, user: Account | EndUser | None, message_id: str, *, session: Session): message = session.scalar( diff --git a/api/tests/e2e_tests/containers/services/test_message_service.py b/api/tests/e2e_tests/containers/services/test_message_service.py index 4aec0eead87..f107c9fbfd5 100644 --- a/api/tests/e2e_tests/containers/services/test_message_service.py +++ b/api/tests/e2e_tests/containers/services/test_message_service.py @@ -4,8 +4,7 @@ import pytest from faker import Faker from sqlalchemy.orm import Session -from models.enums import ConversationFromSource, FeedbackRating, InvokeFrom -from models.model import MessageFeedback +from models.enums import ConversationFromSource, InvokeFrom from services.app_service import AppService, CreateAppParams from services.errors.message import ( FirstMessageNotExistsError, @@ -367,244 +366,6 @@ class TestMessageService: session=db_session_with_containers, ) - def test_create_feedback_success(self, db_session_with_containers: Session, mock_external_service_dependencies): - """ - Test successful creation of feedback. - """ - fake = Faker() - app, account = self._create_test_app_and_account(db_session_with_containers, mock_external_service_dependencies) - - # Create a conversation and message - conversation = self._create_test_conversation(db_session_with_containers, app, account, fake) - message = self._create_test_message(db_session_with_containers, app, conversation, account, fake) - - # Create feedback - rating = FeedbackRating.LIKE - content = fake.text(max_nb_chars=100) - feedback = MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=rating, - content=content, - session=db_session_with_containers, - ) - - # Verify feedback was created correctly - assert feedback.app_id == app.id - assert feedback.conversation_id == conversation.id - assert feedback.message_id == message.id - assert feedback.rating == rating - assert feedback.content == content - assert feedback.from_source == "admin" - assert feedback.from_account_id == account.id - assert feedback.from_end_user_id is None - - def test_create_feedback_no_user(self, db_session_with_containers: Session, mock_external_service_dependencies): - """ - Test creating feedback when no user is provided. - """ - fake = Faker() - app, account = self._create_test_app_and_account(db_session_with_containers, mock_external_service_dependencies) - - # Create a conversation and message - conversation = self._create_test_conversation(db_session_with_containers, app, account, fake) - message = self._create_test_message(db_session_with_containers, app, conversation, account, fake) - - # Test creating feedback with no user - with pytest.raises(ValueError, match="user cannot be None"): - MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=None, - rating=FeedbackRating.LIKE, - content=fake.text(max_nb_chars=100), - session=db_session_with_containers, - ) - - def test_create_feedback_update_existing( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test updating existing feedback. - """ - fake = Faker() - app, account = self._create_test_app_and_account(db_session_with_containers, mock_external_service_dependencies) - - # Create a conversation and message - conversation = self._create_test_conversation(db_session_with_containers, app, account, fake) - message = self._create_test_message(db_session_with_containers, app, conversation, account, fake) - - # Create initial feedback - initial_rating = FeedbackRating.LIKE - initial_content = fake.text(max_nb_chars=100) - feedback = MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=initial_rating, - content=initial_content, - session=db_session_with_containers, - ) - - # Update feedback - updated_rating = FeedbackRating.DISLIKE - updated_content = fake.text(max_nb_chars=100) - updated_feedback = MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=updated_rating, - content=updated_content, - session=db_session_with_containers, - ) - - # Verify feedback was updated correctly - assert updated_feedback.id == feedback.id - assert updated_feedback.rating == updated_rating - assert updated_feedback.content == updated_content - assert updated_feedback.rating != initial_rating - assert updated_feedback.content != initial_content - - def test_create_feedback_delete_existing( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test deleting existing feedback by setting rating to None. - """ - fake = Faker() - app, account = self._create_test_app_and_account(db_session_with_containers, mock_external_service_dependencies) - - # Create a conversation and message - conversation = self._create_test_conversation(db_session_with_containers, app, account, fake) - message = self._create_test_message(db_session_with_containers, app, conversation, account, fake) - - # Create initial feedback - feedback = MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=FeedbackRating.LIKE, - content=fake.text(max_nb_chars=100), - session=db_session_with_containers, - ) - - # Delete feedback by setting rating to None, - MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=None, - content=None, - session=db_session_with_containers, - ) - - # Verify feedback was deleted - - deleted_feedback = ( - db_session_with_containers.query(MessageFeedback).where(MessageFeedback.id == feedback.id).first() - ) - assert deleted_feedback is None - - def test_create_feedback_no_rating_when_not_exists( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test creating feedback with no rating when feedback doesn't exist. - """ - fake = Faker() - app, account = self._create_test_app_and_account(db_session_with_containers, mock_external_service_dependencies) - - # Create a conversation and message - conversation = self._create_test_conversation(db_session_with_containers, app, account, fake) - message = self._create_test_message(db_session_with_containers, app, conversation, account, fake) - - # Test creating feedback with no rating when no feedback exists - with pytest.raises(ValueError, match="rating cannot be None when feedback not exists"): - MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=None, - content=None, - session=db_session_with_containers, - ) - - def test_get_all_messages_feedbacks_success( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test successful retrieval of all message feedbacks. - """ - fake = Faker() - app, account = self._create_test_app_and_account(db_session_with_containers, mock_external_service_dependencies) - - # Create multiple conversations and messages with feedbacks - feedbacks = [] - for i in range(3): - conversation = self._create_test_conversation(db_session_with_containers, app, account, fake) - message = self._create_test_message(db_session_with_containers, app, conversation, account, fake) - - feedback = MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=FeedbackRating.LIKE if i % 2 == 0 else FeedbackRating.DISLIKE, - content=f"Feedback {i}: {fake.text(max_nb_chars=50)}", - session=db_session_with_containers, - ) - feedbacks.append(feedback) - - # Get all feedbacks, - result = MessageService.get_all_messages_feedbacks(app, page=1, limit=10, session=db_session_with_containers) - - # Verify results - assert len(result) == 3 - - # Verify feedbacks are ordered by created_at desc - for i in range(len(result) - 1): - assert result[i]["created_at"] >= result[i + 1]["created_at"] - - def test_get_all_messages_feedbacks_pagination( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test pagination of message feedbacks. - """ - fake = Faker() - app, account = self._create_test_app_and_account(db_session_with_containers, mock_external_service_dependencies) - - # Create multiple conversations and messages with feedbacks - for i in range(5): - conversation = self._create_test_conversation(db_session_with_containers, app, account, fake) - message = self._create_test_message(db_session_with_containers, app, conversation, account, fake) - - MessageService.create_feedback( - app_model=app, - message_id=message.id, - user=account, - rating=FeedbackRating.LIKE, - content=f"Feedback {i}", - session=db_session_with_containers, - ) - - # Get feedbacks with pagination - result_page_1 = MessageService.get_all_messages_feedbacks( - app, page=1, limit=3, session=db_session_with_containers - ) - result_page_2 = MessageService.get_all_messages_feedbacks( - app, page=2, limit=3, session=db_session_with_containers - ) - - # Verify pagination results - assert len(result_page_1) == 3 - assert len(result_page_2) == 2 - - # Verify no overlap between pages - page_1_ids = {feedback["id"] for feedback in result_page_1} - page_2_ids = {feedback["id"] for feedback in result_page_2} - assert len(page_1_ids.intersection(page_2_ids)) == 0 - def test_get_message_success(self, db_session_with_containers: Session, mock_external_service_dependencies): """ Test successful retrieval of message. diff --git a/api/tests/e2e_tests/contracts/controllers/console/test_message_feedback.py b/api/tests/e2e_tests/contracts/controllers/console/test_message_feedback.py new file mode 100644 index 00000000000..0573d5ea570 --- /dev/null +++ b/api/tests/e2e_tests/contracts/controllers/console/test_message_feedback.py @@ -0,0 +1,311 @@ +"""Feedback HTTP writes through real Console admission, application services and SQLite.""" + +from collections.abc import Iterator +from http import HTTPStatus +from typing import Literal +from uuid import uuid4 + +import pytest +from sqlalchemy import delete, event, func, select, update +from sqlalchemy.orm import Session, SessionTransaction +from werkzeug.test import TestResponse + +from constants import COOKIE_NAME_CSRF_TOKEN, HEADER_NAME_CSRF_TOKEN +from controllers.console.app.message import AgentMessageFeedbackApi, MessageFeedbackApi +from controllers.console.explore.message import MessageFeedbackApi as InstalledMessageFeedbackApi +from libs.external_api import ExternalApi +from libs.passport import PassportService +from libs.token import generate_csrf_token +from models.agent import Agent, AgentScope, AgentSource, AgentStatus +from models.enums import ConversationFromSource, FeedbackFromSource, FeedbackRating +from models.model import App, AppMode, InstalledApp, Message, MessageFeedback +from tests.e2e_tests.contracts.controllers.console.explore.test_message_suggested_questions import _Harness +from tests.e2e_tests.contracts.controllers.console.explore.test_message_suggested_questions import ( + harness as installed_app_harness, +) + +__all__ = ["installed_app_harness"] + + +@pytest.fixture +def harness(installed_app_harness: _Harness) -> Iterator[_Harness]: + state = installed_app_harness + api = ExternalApi(state.app) + api.add_resource(MessageFeedbackApi, "/apps//feedbacks", endpoint="console.app_feedback") + api.add_resource(AgentMessageFeedbackApi, "/agent//feedbacks", endpoint="console.agent_feedback") + api.add_resource( + InstalledMessageFeedbackApi, + "/installed-apps//messages//feedbacks", + endpoint="console.installed_feedback", + ) + yield state + if state.sessions: + state.assert_closed() + with state.factory() as session: + installation = session.get(InstalledApp, state.installation.id) + if installation is not None: + assert installation.last_used_at is None + + +def _post( + state: _Harness, + *, + body: dict[str, object], + route: str | None = None, + installation_id: str | None = None, + message_id: str | None = None, + authenticated: bool = True, + csrf: bool = True, +) -> TestResponse: + client = state.app.test_client() + token = generate_csrf_token(state.account.id) + headers = {HEADER_NAME_CSRF_TOKEN: token} if csrf else {} + if authenticated: + headers["Authorization"] = f"Bearer {PassportService().issue({'user_id': state.account.id})}" + client.set_cookie(COOKIE_NAME_CSRF_TOKEN, token) + if route is None: + route = ( + f"/installed-apps/{installation_id or state.installation.id}" + f"/messages/{message_id or state.message.id}/feedbacks" + ) + else: + body = {"message_id": message_id or state.message.id, **body} + return client.post(route, headers=headers, json=body) + + +def _success(response: TestResponse) -> None: + assert response.status_code == HTTPStatus.OK + assert response.get_json() == {"result": "success"} + assert response.headers["Content-Type"] == "application/json" + assert int(response.headers["Content-Length"]) == len(response.data) + + +def _error(response: TestResponse, *, status: HTTPStatus, code: str) -> None: + assert response.status_code == status + assert response.headers["Content-Type"] == "application/json" + assert int(response.headers["Content-Length"]) == len(response.data) + payload = response.get_json() + assert payload["status"] == status + assert payload["code"] == code + assert payload["message"] + + +def _admin_route(state: _Harness, kind: Literal["app", "agent"], *, scope: AgentScope = AgentScope.ROSTER) -> str: + """Make the admitted account an app owner while retaining a distinct installation fixture.""" + with state.factory.begin() as session: + session.execute(update(App).where(App.id == state.target.id).values(tenant_id=state.installation.tenant_id)) + if kind == "agent": + agent_id = str(uuid4()) + session.execute(update(App).where(App.id == state.target.id).values(mode=AppMode.AGENT)) + session.add( + Agent( + id=agent_id, + tenant_id=state.installation.tenant_id, + name="Feedback agent", + scope=scope, + source=AgentSource.AGENT_APP if scope == AgentScope.ROSTER else AgentSource.WORKFLOW, + status=AgentStatus.ACTIVE, + app_id=state.target.id if scope == AgentScope.ROSTER else str(uuid4()), + backing_app_id=state.target.id if scope == AgentScope.WORKFLOW_ONLY else None, + workflow_id=str(uuid4()) if scope == AgentScope.WORKFLOW_ONLY else None, + workflow_node_id="node" if scope == AgentScope.WORKFLOW_ONLY else None, + ) + ) + return f"/agent/{agent_id}/feedbacks" + return f"/apps/{state.target.id}/feedbacks" + + +def test_explore_feedback_create_update_revoke_with_cross_workspace_app(harness: _Harness) -> None: + assert harness.target.tenant_id != harness.installation.tenant_id + feedback_id: str | None = None + for rating, content in (("like", ""), ("dislike", "Changed my mind")): + _success(_post(harness, body={"rating": rating, "content": content})) + with harness.factory() as session: + rows = session.scalars(select(MessageFeedback)).all() + assert len(rows) == 1 + feedback = rows[0] + assert feedback_id is None or feedback.id == feedback_id + feedback_id = feedback.id + assert feedback.app_id == harness.target.id + assert feedback.message_id == harness.message.id + assert feedback.conversation_id == harness.conversation.id + assert feedback.rating.value == rating + assert feedback.content == content + assert feedback.from_source == FeedbackFromSource.ADMIN + assert feedback.from_account_id == harness.account.id + assert feedback.from_end_user_id is None + _success(_post(harness, body={"rating": None})) + with harness.factory() as session: + assert session.scalars(select(MessageFeedback)).all() == [] + + +@pytest.mark.parametrize("owner", ["app_id", "from_account_id", "from_source", "from_end_user_id", "missing"]) +def test_explore_requires_complete_message_ownership(harness: _Harness, owner: str) -> None: + with harness.factory.begin() as session: + if owner == "missing": + session.execute(delete(Message).where(Message.id == harness.message.id)) + else: + value = ConversationFromSource.API if owner == "from_source" else str(uuid4()) + session.execute(update(Message).where(Message.id == harness.message.id).values({owner: value})) + _error(_post(harness, body={"rating": "like"}), status=HTTPStatus.NOT_FOUND, code="message_not_found") + with harness.factory() as session: + assert session.scalars(select(MessageFeedback)).all() == [] + + +@pytest.mark.parametrize("mode", list(AppMode)) +def test_explore_feedback_accepts_all_app_modes(harness: _Harness, mode: AppMode) -> None: + with harness.factory.begin() as session: + session.execute(update(App).where(App.id == harness.target.id).values(mode=mode)) + _success(_post(harness, body={"rating": "like"})) + + +@pytest.mark.parametrize("admission", ["missing", "workspace", "app"]) +def test_explore_rejects_unavailable_installation(harness: _Harness, admission: str) -> None: + with harness.factory.begin() as session: + if admission == "workspace": + session.execute( + update(InstalledApp).where(InstalledApp.id == harness.installation.id).values(tenant_id=str(uuid4())) + ) + elif admission == "app": + session.execute(delete(App).where(App.id == harness.target.id)) + _error( + _post(harness, body={"rating": "like"}, installation_id=str(uuid4()) if admission == "missing" else None), + status=HTTPStatus.NOT_FOUND, + code="installed_app_not_found", + ) + + +def test_installation_removed_after_admission_cannot_receive_feedback(harness: _Harness) -> None: + def remove_installation(_session: Session, _transaction: SessionTransaction) -> None: + with harness.factory.begin() as session: + session.execute(delete(InstalledApp).where(InstalledApp.id == harness.installation.id)) + + event.listen(harness.admission_factory, "after_transaction_end", remove_installation, once=True) + try: + response = _post(harness, body={"rating": "like"}) + finally: + event.remove(harness.admission_factory, "after_transaction_end", remove_installation) + _error(response, status=HTTPStatus.NOT_FOUND, code="installed_app_not_found") + with harness.factory() as session: + assert session.scalars(select(MessageFeedback)).all() == [] + + +@pytest.mark.parametrize("kind", ["app", "agent"]) +@pytest.mark.parametrize("owner", ["another_account", "end_user"]) +def test_admin_can_rate_other_actors_messages_in_the_admitted_app( + harness: _Harness, kind: Literal["app", "agent"], owner: str +) -> None: + route = _admin_route(harness, kind) + with harness.factory.begin() as session: + message = session.get(Message, harness.message.id) + assert message is not None + message.from_account_id = str(uuid4()) if owner == "another_account" else None + message.from_end_user_id = str(uuid4()) if owner == "end_user" else None + message.from_source = ConversationFromSource.API if owner == "end_user" else ConversationFromSource.CONSOLE + session.add( + MessageFeedback( + app_id=message.app_id, + conversation_id=message.conversation_id, + message_id=message.id, + rating=FeedbackRating.LIKE, + from_source=FeedbackFromSource.USER, + from_end_user_id=message.from_end_user_id, + content="User feedback stays separate", + ) + ) + for rating, content in (("like", ""), ("dislike", None)): + _success(_post(harness, route=route, body={"rating": rating, "content": content})) + with harness.factory() as session: + feedback = session.scalar( + select(MessageFeedback).where(MessageFeedback.from_source == FeedbackFromSource.ADMIN) + ) + assert feedback is not None + assert feedback.rating.value == rating + assert feedback.content == content + assert feedback.from_account_id == harness.account.id + assert feedback.from_end_user_id is None + _success(_post(harness, route=route, body={"rating": None})) + with harness.factory() as session: + rows = session.scalars(select(MessageFeedback)).all() + assert len(rows) == 1 + assert rows[0].from_source == FeedbackFromSource.USER + assert rows[0].content == "User feedback stays separate" + + +@pytest.mark.parametrize("scope", [AgentScope.ROSTER, AgentScope.WORKFLOW_ONLY]) +def test_agent_feedback_resolves_existing_runtime_app(harness: _Harness, scope: AgentScope) -> None: + route = _admin_route(harness, "agent", scope=scope) + _success(_post(harness, route=route, body={"rating": "like"})) + + +def test_agent_feedback_does_not_create_a_missing_hidden_runtime_app(harness: _Harness) -> None: + route = _admin_route(harness, "agent", scope=AgentScope.WORKFLOW_ONLY) + with harness.factory.begin() as session: + session.execute(update(Agent).values(backing_app_id=None)) + app_count = session.scalar(select(func.count()).select_from(App)) + _error( + _post(harness, route=route, body={"rating": "like"}), status=HTTPStatus.NOT_FOUND, code="agent_not_found_error" + ) + with harness.factory() as session: + assert session.scalar(select(func.count()).select_from(App)) == app_count + assert session.scalar(select(Agent.backing_app_id)) is None + + +def test_plain_app_feedback_cannot_address_an_agents_hidden_runtime_app(harness: _Harness) -> None: + _admin_route(harness, "agent", scope=AgentScope.WORKFLOW_ONLY) + _error( + _post(harness, route=f"/apps/{harness.target.id}/feedbacks", body={"rating": "like"}), + status=HTTPStatus.NOT_FOUND, + code="app_not_found", + ) + + +@pytest.mark.parametrize("kind", ["app", "agent"]) +@pytest.mark.parametrize("missing", ["message", "message_app", "app_workspace", "app"]) +def test_admin_feedback_still_requires_the_admitted_app( + harness: _Harness, kind: Literal["app", "agent"], missing: str +) -> None: + route = _admin_route(harness, kind) + with harness.factory.begin() as session: + if missing == "message": + session.execute(delete(Message).where(Message.id == harness.message.id)) + elif missing == "message_app": + session.execute(update(Message).where(Message.id == harness.message.id).values(app_id=str(uuid4()))) + elif missing == "app_workspace": + session.execute(update(App).where(App.id == harness.target.id).values(tenant_id=str(uuid4()))) + else: + session.execute(delete(App).where(App.id == harness.target.id)) + code = ( + "not_found" if missing.startswith("message") else "app_not_found" if kind == "app" else "agent_not_found_error" + ) + _error(_post(harness, route=route, body={"rating": "like"}), status=HTTPStatus.NOT_FOUND, code=code) + with harness.factory() as session: + assert session.scalars(select(MessageFeedback)).all() == [] + + +@pytest.mark.parametrize("kind", ["explore", "app", "agent"]) +@pytest.mark.parametrize("body", [{"rating": "invalid"}, {"content": 123}]) +def test_invalid_feedback_payload_is_rejected(harness: _Harness, kind: str, body: dict[str, object]) -> None: + route = None if kind == "explore" else _admin_route(harness, "app" if kind == "app" else "agent") + _error(_post(harness, route=route, body=body), status=HTTPStatus.UNPROCESSABLE_ENTITY, code="unprocessable_entity") + + +@pytest.mark.parametrize("kind", ["explore", "app", "agent"]) +def test_revoke_without_feedback_has_specific_error(harness: _Harness, kind: str) -> None: + route = None if kind == "explore" else _admin_route(harness, "app" if kind == "app" else "agent") + _error( + _post(harness, route=route, body={"rating": None}), + status=HTTPStatus.BAD_REQUEST, + code="message_feedback_rating_required", + ) + + +@pytest.mark.parametrize("kind", ["explore", "app", "agent"]) +@pytest.mark.parametrize("missing", ["login", "csrf"]) +def test_feedback_requires_real_login_and_csrf(harness: _Harness, kind: str, missing: str) -> None: + route = None if kind == "explore" else _admin_route(harness, "app" if kind == "app" else "agent") + response = _post( + harness, route=route, body={"rating": "like"}, authenticated=missing != "login", csrf=missing != "csrf" + ) + _error(response, status=HTTPStatus.UNAUTHORIZED, code="unauthorized") + assert response.headers["WWW-Authenticate"] == 'Bearer realm="api"' diff --git a/api/tests/e2e_tests/contracts/controllers/service_api/app/test_message_feedback.py b/api/tests/e2e_tests/contracts/controllers/service_api/app/test_message_feedback.py new file mode 100644 index 00000000000..79005331d4f --- /dev/null +++ b/api/tests/e2e_tests/contracts/controllers/service_api/app/test_message_feedback.py @@ -0,0 +1,484 @@ +"""Feedback endpoints through real API token admission, repositories, and SQLite.""" + +from collections.abc import Iterator +from dataclasses import dataclass +from datetime import datetime +from decimal import Decimal +from http import HTTPStatus +from uuid import uuid4 + +import pytest +from flask import Flask +from flask_login import LoginManager +from redis import Redis +from sqlalchemy import Connection, Engine, delete, event, select, update +from sqlalchemy.orm import Session, SessionTransaction, sessionmaker +from werkzeug.test import TestResponse + +from constants.resource_access_token import ResourceAccessTokenResourceType +from controllers.service_api.app.message import AppGetFeedbacksApi, MessageFeedbackApi +from enums import DeploymentEdition +from extensions.ext_application_services import build_application_services +from extensions.ext_database import db +from extensions.ext_redis import RedisClientWrapper +from libs.external_api import ExternalApi +from machinery.context import RequestContext +from models.account import Account, Tenant, TenantAccountJoin, TenantAccountRole, TenantStatus +from models.enums import ApiTokenType, ConversationFromSource, EndUserType, FeedbackFromSource, FeedbackRating +from models.model import ApiToken, App, EndUser, Message, MessageFeedback +from models.resource_access_token import ResourceAccessToken +from services.auth.resource_access_token_contracts import ResourceAccessTokenCreateResult, ResourceAccessTokenResource +from services.resource_access_token_service import ResourceAccessTokenService +from tests.helpers.unit.model_factories import make_app, make_conversation, make_end_user, make_message + + +@dataclass(frozen=True) +class _Harness: + app: Flask + target: App + tenant: Tenant + owner: Account + end_user: EndUser + message: Message + factory: sessionmaker[Session] + sessions: list[Session] + resource_access_tokens: ResourceAccessTokenService + end_user_queries: list[str] + + def post( + self, + payload: dict[str, object], + *, + user: str | None = "alice", + message_id: str | None = None, + authorization: str | None = "Bearer feedback-test-token", + ) -> TestResponse: + body = dict(payload) + if user is not None: + body["user"] = user + headers: dict[str, str] = {"Authorization": authorization} if authorization is not None else {} + return self.app.test_client().post( + f"/messages/{message_id or self.message.id}/feedbacks", json=body, headers=headers + ) + + def get( + self, + query: str = "", + *, + authorization: str = "Bearer feedback-test-token", + requested_app_id: str | None = None, + ) -> TestResponse: + headers = {"Authorization": authorization} + if requested_app_id is not None: + headers["X-Dify-App-ID"] = requested_app_id + return self.app.test_client().get(f"/app/feedbacks{query}", headers=headers) + + def create_resource_token(self, *app_ids: str) -> ResourceAccessTokenCreateResult: + return self.resource_access_tokens.create( + RequestContext( + request_id="feedback-resource-token-test", + trace_id=None, + account_id=self.owner.id, + active_workspace_id=self.tenant.id, + ), + name="Feedback integration", + resources=tuple( + ResourceAccessTokenResource(ResourceAccessTokenResourceType.APP, app_id) for app_id in app_ids + ), + ) + + def feedbacks(self) -> list[MessageFeedback]: + with self.factory() as session: + return list(session.scalars(select(MessageFeedback)).all()) + + def assert_closed(self) -> None: + assert self.sessions + assert all(not session.in_transaction() and not session.identity_map for session in self.sessions) + + +@pytest.fixture +def harness(sqlite_engine: Engine, sqlite_session_factory: sessionmaker[Session]) -> Iterator[_Harness]: + tenant = Tenant(name="API feedback workspace") + target = make_app(app_id=str(uuid4()), tenant_id=tenant.id) + owner = Account(name="Owner", email=f"owner-{tenant.id}@example.com") + membership = TenantAccountJoin(tenant_id=tenant.id, account_id=owner.id, role=TenantAccountRole.OWNER) + end_user = make_end_user( + end_user_id=str(uuid4()), + tenant_id=tenant.id, + app_id=target.id, + end_user_type=EndUserType.SERVICE_API, + session_id="alice", + external_user_id="alice", + ) + conversation = make_conversation( + conversation_id=str(uuid4()), + inputs={}, + app_id=target.id, + from_source=ConversationFromSource.API, + from_end_user_id=end_user.id, + ) + message = make_message( + message_id=str(uuid4()), + inputs={}, + message={}, + query="question", + answer="answer", + message_unit_price=Decimal(0), + answer_unit_price=Decimal(0), + currency="USD", + app_id=target.id, + conversation_id=conversation.id, + from_source=ConversationFromSource.API, + from_end_user_id=end_user.id, + ) + token = ApiToken(app_id=target.id, tenant_id=tenant.id, type=ApiTokenType.APP, token="feedback-test-token") + with sqlite_session_factory.begin() as session: + session.add_all([tenant, owner, membership, target, end_user, conversation, message, token]) + app = Flask(__name__) + app.config.update(TESTING=True, RESTX_ERROR_404_HELP=False, SQLALCHEMY_DATABASE_URI=str(sqlite_engine.url)) + db.init_app(app) + LoginManager(app) + factory = sessionmaker(bind=sqlite_engine, expire_on_commit=False) + sessions: list[Session] = [] + end_user_queries: list[str] = [] + + def track(session: Session, _transaction: SessionTransaction, _connection: Connection) -> None: + assert all(not previous.in_transaction() for previous in sessions if previous is not session) + sessions.append(session) + + def track_end_user_queries(_connection: Connection, _cursor: object, statement: str, *_args: object) -> None: + if "end_users" in statement: + end_user_queries.append(statement) + + event.listen(sqlite_session_factory, "after_begin", track) + event.listen(factory, "after_begin", track) + event.listen(db.session.session_factory, "after_begin", track) + event.listen(sqlite_engine, "before_cursor_execute", track_end_user_queries) + redis_client = Redis() + redis = RedisClientWrapper() + redis.initialize(redis_client) + services = build_application_services( + database_client=factory, deployment_edition=DeploymentEdition.COMMUNITY, initialization_password="", redis=redis + ) + app.extensions["application_services"] = services + api = ExternalApi(app) + api.add_resource(MessageFeedbackApi, "/messages//feedbacks") + api.add_resource(AppGetFeedbacksApi, "/app/feedbacks") + yield _Harness( + app, + target, + tenant, + owner, + end_user, + message, + sqlite_session_factory, + sessions, + services.resource_access_tokens, + end_user_queries, + ) + event.remove(sqlite_session_factory, "after_begin", track) + event.remove(factory, "after_begin", track) + event.remove(db.session.session_factory, "after_begin", track) + event.remove(sqlite_engine, "before_cursor_execute", track_end_user_queries) + redis_client.close() + with app.app_context(): + db.session.remove() + db.engine.dispose() + + +def test_feedback_create_update_and_revoke(harness: _Harness) -> None: + created = harness.post({"rating": "like", "content": "useful"}) + assert created.status_code == HTTPStatus.OK + assert created.get_json() == {"result": "success"} + assert created.headers["Content-Type"] == "application/json" + [feedback] = harness.feedbacks() + original_id = feedback.id + assert feedback.from_source == FeedbackFromSource.USER + assert feedback.from_end_user_id == harness.end_user.id + assert feedback.from_account_id is None + assert feedback.app_id == harness.target.id + assert feedback.message_id == harness.message.id + assert feedback.conversation_id == harness.message.conversation_id + assert feedback.rating == FeedbackRating.LIKE + assert feedback.content == "useful" + updated = harness.post({"rating": "dislike", "content": None}) + assert updated.status_code == HTTPStatus.OK + [feedback] = harness.feedbacks() + assert feedback.id == original_id + assert feedback.rating == FeedbackRating.DISLIKE + assert feedback.content is None + assert harness.post({"rating": None}).get_json() == {"result": "success"} + assert harness.feedbacks() == [] + harness.assert_closed() + + +@pytest.mark.parametrize("rating", [None, "like"]) +def test_other_end_user_cannot_change_the_message_feedback(harness: _Harness, rating: str | None) -> None: + assert harness.post({"rating": "like"}).status_code == HTTPStatus.OK + response = harness.post({"rating": rating}, user="other-user") + assert response.status_code == HTTPStatus.NOT_FOUND + assert response.get_json()["code"] == "not_found" + [feedback] = harness.feedbacks() + assert feedback.from_end_user_id == harness.end_user.id + assert feedback.rating == FeedbackRating.LIKE + harness.assert_closed() + + +@pytest.mark.parametrize("field", ["app_id", "from_source", "from_account_id"]) +def test_foreign_app_or_console_message_is_not_accessible(harness: _Harness, field: str) -> None: + value = ConversationFromSource.CONSOLE if field == "from_source" else str(uuid4()) + with harness.factory.begin() as session: + session.execute(update(Message).where(Message.id == harness.message.id).values({field: value})) + response = harness.post({"rating": "like"}) + assert response.status_code == HTTPStatus.NOT_FOUND + assert response.get_json()["code"] == "not_found" + assert harness.feedbacks() == [] + + +def test_revoke_without_existing_feedback_has_precise_error(harness: _Harness) -> None: + response = harness.post({"rating": None}) + assert response.status_code == HTTPStatus.BAD_REQUEST + assert response.get_json()["code"] == "message_feedback_rating_required" + assert harness.feedbacks() == [] + harness.assert_closed() + + +@pytest.mark.parametrize("user", [None, ""]) +def test_missing_user_does_not_write_feedback(harness: _Harness, user: str | None) -> None: + response = harness.post({"rating": "like"}, user=user) + assert response.status_code == HTTPStatus.BAD_REQUEST + assert response.get_json()["message"] == "Arg user must be provided." + assert harness.feedbacks() == [] + + +@pytest.mark.parametrize("authorization", [None, "Basic feedback-test-token"]) +def test_invalid_authorization_is_rejected(harness: _Harness, authorization: str | None) -> None: + response = harness.post({"rating": "like"}, authorization=authorization) + assert response.status_code == HTTPStatus.UNAUTHORIZED + assert response.get_json()["code"] == "unauthorized" + assert response.headers["WWW-Authenticate"] == 'Bearer realm="api"' + assert harness.sessions == [] + + +def test_invalid_rating_is_rejected(harness: _Harness) -> None: + response = harness.post({"rating": "neutral"}) + assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY + assert response.get_json()["code"] == "unprocessable_entity" + assert harness.feedbacks() == [] + + +def test_feedback_list_preserves_shape_nulls_timestamp_format_and_stable_order(harness: _Harness) -> None: + timestamp = datetime(2024, 1, 2, 3, 4, 5) + records = [ + MessageFeedback( + app_id=harness.target.id, + conversation_id=harness.message.conversation_id, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + from_source=FeedbackFromSource.USER, + from_end_user_id=harness.end_user.id, + ), + MessageFeedback( + app_id=harness.target.id, + conversation_id=harness.message.conversation_id, + message_id=harness.message.id, + rating=FeedbackRating.DISLIKE, + content="admin comment", + from_source=FeedbackFromSource.ADMIN, + from_account_id=harness.owner.id, + ), + MessageFeedback( + app_id=str(uuid4()), + conversation_id=str(uuid4()), + message_id=str(uuid4()), + rating=FeedbackRating.LIKE, + from_source=FeedbackFromSource.USER, + from_end_user_id=str(uuid4()), + ), + ] + for index, record in enumerate(records, start=1): + record.id = f"00000000-0000-0000-0000-{index:012d}" + record.created_at = timestamp + record.updated_at = timestamp + with harness.factory.begin() as session: + session.add_all(records) + response = harness.get() + assert response.status_code == HTTPStatus.OK + assert response.get_json() == {"data": [records[1].to_dict(), records[0].to_dict()]} + assert response.headers["Content-Type"] == "application/json" + assert harness.get("?page=2&limit=1").get_json() == {"data": [records[0].to_dict()]} + assert harness.get("?page=3&limit=1").get_json() == {"data": []} + harness.assert_closed() + + +@pytest.mark.parametrize("query", ["?page=0", "?limit=0", "?limit=102"]) +def test_list_rejects_invalid_page_or_limit(harness: _Harness, query: str) -> None: + response = harness.get(query) + assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY + assert response.get_json()["code"] == "unprocessable_entity" + harness.assert_closed() + + +@pytest.mark.parametrize("missing", ["membership", "account"]) +def test_app_only_list_still_requires_workspace_owner(harness: _Harness, missing: str) -> None: + with harness.factory.begin() as session: + if missing == "membership": + session.execute(delete(TenantAccountJoin).where(TenantAccountJoin.tenant_id == harness.tenant.id)) + else: + session.execute(delete(Account).where(Account.id == harness.owner.id)) + response = harness.get() + assert response.status_code == HTTPStatus.UNAUTHORIZED + assert response.get_json()["code"] == "unauthorized" + assert response.get_json()["message"] == "Tenant owner account not found or tenant is not active." + harness.assert_closed() + + +@pytest.mark.parametrize("role", [TenantAccountRole.ADMIN, TenantAccountRole.EDITOR]) +def test_app_only_list_rejects_non_owner_membership(harness: _Harness, role: TenantAccountRole) -> None: + with harness.factory.begin() as session: + membership = session.scalar( + select(TenantAccountJoin).where( + TenantAccountJoin.tenant_id == harness.tenant.id, + TenantAccountJoin.account_id == harness.owner.id, + ) + ) + assert membership is not None + membership.role = role + + response = harness.get() + assert response.status_code == HTTPStatus.UNAUTHORIZED + assert response.get_json()["code"] == "unauthorized" + assert response.get_json()["message"] == "Tenant owner account not found or tenant is not active." + harness.assert_closed() + + +@pytest.mark.parametrize("endpoint", ["write", "list"]) +def test_archived_workspace_is_rejected(harness: _Harness, endpoint: str) -> None: + with harness.factory.begin() as session: + session.execute(update(Tenant).where(Tenant.id == harness.tenant.id).values(status=TenantStatus.ARCHIVE)) + response = harness.post({"rating": "like"}) if endpoint == "write" else harness.get() + assert response.status_code == HTTPStatus.FORBIDDEN + assert response.get_json()["code"] == "workspace_archived" + assert harness.feedbacks() == [] + harness.assert_closed() + + +def test_empty_feedback_list_keeps_data_array(harness: _Harness) -> None: + response = harness.get("?limit=101") + assert response.status_code == HTTPStatus.OK + assert response.get_json() == {"data": []} + harness.assert_closed() + + +def test_resource_token_can_create_and_list_feedback_without_provisioning_on_get(harness: _Harness) -> None: + token = harness.create_resource_token(harness.target.id) + authorization = f"Bearer {token.token}" + + response = harness.post({"rating": "like", "content": "resource token feedback"}, authorization=authorization) + + assert response.status_code == HTTPStatus.OK + assert response.get_json() == {"result": "success"} + [feedback] = harness.feedbacks() + assert feedback.from_end_user_id == harness.end_user.id + assert feedback.content == "resource token feedback" + assert harness.end_user_queries + harness.end_user_queries.clear() + + response = harness.get("?user=must-not-be-provisioned", authorization=authorization) + + assert response.status_code == HTTPStatus.OK + assert response.get_json() == {"data": [feedback.to_dict()]} + assert harness.end_user_queries == [] + with harness.factory() as session: + assert session.scalars(select(EndUser.id)).all() == [harness.end_user.id] + stored_token = session.get(ResourceAccessToken, token.token_id) + assert stored_token is not None + assert stored_token.last_used_at is not None + harness.assert_closed() + + +@pytest.mark.parametrize("select_other_app", [False, True], ids=["first-app", "second-app"]) +def test_multiple_app_resource_token_lists_only_header_selected_app(harness: _Harness, select_other_app: bool) -> None: + other = make_app(app_id=str(uuid4()), tenant_id=harness.tenant.id) + records = [ + MessageFeedback( + app_id=app_id, + conversation_id=str(uuid4()), + message_id=str(uuid4()), + rating=FeedbackRating.LIKE, + from_source=FeedbackFromSource.ADMIN, + from_account_id=harness.owner.id, + content=app_id, + ) + for app_id in (harness.target.id, other.id) + ] + with harness.factory.begin() as session: + session.add_all([other, *records]) + token = harness.create_resource_token(harness.target.id, other.id) + selected = records[1 if select_other_app else 0] + + response = harness.get( + "?user=must-not-be-provisioned", + authorization=f"Bearer {token.token}", + requested_app_id=selected.app_id, + ) + + assert response.status_code == HTTPStatus.OK + assert response.get_json() == {"data": [selected.to_dict()]} + assert harness.end_user_queries == [] + with harness.factory() as session: + stored_token = session.get(ResourceAccessToken, token.token_id) + assert stored_token is not None + assert stored_token.last_used_at is not None + harness.assert_closed() + + +def test_multiple_app_resource_token_list_requires_app_header(harness: _Harness) -> None: + other = make_app(app_id=str(uuid4()), tenant_id=harness.tenant.id) + with harness.factory.begin() as session: + session.add(other) + token = harness.create_resource_token(harness.target.id, other.id) + + response = harness.get("?user=must-not-be-provisioned", authorization=f"Bearer {token.token}") + + assert response.status_code == HTTPStatus.BAD_REQUEST + assert response.get_json() == { + "code": "bad_request", + "message": "App ID is required when a resource access token is bound to multiple apps.", + "status": 400, + } + assert harness.end_user_queries == [] + with harness.factory() as session: + stored_token = session.get(ResourceAccessToken, token.token_id) + assert stored_token is not None + assert stored_token.last_used_at is None + harness.assert_closed() + + +@pytest.mark.parametrize("missing", ["membership", "account"]) +def test_resource_token_requires_owner_for_list_but_not_feedback_write(harness: _Harness, missing: str) -> None: + token = harness.create_resource_token(harness.target.id) + authorization = f"Bearer {token.token}" + with harness.factory.begin() as session: + if missing == "membership": + session.execute(delete(TenantAccountJoin).where(TenantAccountJoin.tenant_id == harness.tenant.id)) + else: + session.execute(delete(Account).where(Account.id == harness.owner.id)) + + response = harness.get("?user=must-not-be-provisioned", authorization=authorization) + + assert response.status_code == HTTPStatus.UNAUTHORIZED + assert response.get_json()["code"] == "unauthorized" + assert response.get_json()["message"] == "Tenant owner account not found or tenant is not active." + assert harness.end_user_queries == [] + assert harness.feedbacks() == [] + harness.assert_closed() + + response = harness.post({"rating": "like"}, authorization=authorization) + + assert response.status_code == HTTPStatus.OK + assert response.get_json() == {"result": "success"} + [feedback] = harness.feedbacks() + assert feedback.from_end_user_id == harness.end_user.id + assert harness.end_user_queries + harness.assert_closed() diff --git a/api/tests/e2e_tests/contracts/controllers/web/test_message_feedback.py b/api/tests/e2e_tests/contracts/controllers/web/test_message_feedback.py new file mode 100644 index 00000000000..1ebc304d691 --- /dev/null +++ b/api/tests/e2e_tests/contracts/controllers/web/test_message_feedback.py @@ -0,0 +1,224 @@ +"""Web feedback through real passport admission, services, and SQLite writes.""" + +from collections.abc import Callable, Iterator +from dataclasses import dataclass +from decimal import Decimal +from http import HTTPStatus +from uuid import uuid4 + +import pytest +from flask import Flask +from redis import Redis +from sqlalchemy import Connection, Engine, event, select, update +from sqlalchemy.orm import Session, SessionTransaction, sessionmaker +from werkzeug.test import TestResponse + +from constants import HEADER_NAME_APP_CODE, HEADER_NAME_PASSPORT +from controllers.web.message import MessageFeedbackApi +from enums import DeploymentEdition +from extensions.ext_application_services import build_application_services +from extensions.ext_database import db +from extensions.ext_redis import RedisClientWrapper +from libs.external_api import ExternalApi +from libs.passport import PassportService +from models.enums import ConversationFromSource, CustomizeTokenStrategy, FeedbackFromSource, FeedbackRating +from models.model import App, AppModelConfig, EndUser, Message, MessageFeedback, Site +from tests.helpers.unit.model_factories import make_app, make_conversation, make_end_user, make_message + + +@dataclass(frozen=True) +class _Harness: + app: Flask + target: App + end_user: EndUser + message: Message + factory: sessionmaker[Session] + sessions: list[Session] + headers: dict[str, str] + + def post(self, payload: dict[str, object], *, message_id: str | None = None) -> TestResponse: + return self.app.test_client().post( + f"/messages/{message_id or self.message.id}/feedbacks", json=payload, headers=self.headers + ) + + def feedbacks(self) -> list[MessageFeedback]: + with self.factory() as session: + return list(session.scalars(select(MessageFeedback)).all()) + + def assert_closed(self) -> None: + assert self.sessions + assert all(not session.in_transaction() and not session.identity_map for session in self.sessions) + + +@pytest.fixture +def harness( + config_overrides: Callable[..., None], + sqlite_engine: Engine, + sqlite_session_factory: sessionmaker[Session], +) -> Iterator[_Harness]: + config_overrides(DEPLOYMENT_EDITION=DeploymentEdition.COMMUNITY, SECRET_KEY="feedback-passport-test-key-32-bytes") + target = make_app(app_id=str(uuid4()), tenant_id=str(uuid4())) + end_user = make_end_user(end_user_id=str(uuid4()), tenant_id=target.tenant_id, app_id=target.id) + config = AppModelConfig(app_id=target.id) + target.app_model_config_id = config.id + conversation = make_conversation( + conversation_id=str(uuid4()), + inputs={}, + app_id=target.id, + from_source=ConversationFromSource.API, + from_end_user_id=end_user.id, + ) + message = make_message( + message_id=str(uuid4()), + inputs={}, + message={}, + query="question", + answer="answer", + message_unit_price=Decimal(0), + answer_unit_price=Decimal(0), + currency="USD", + app_id=target.id, + conversation_id=conversation.id, + from_source=ConversationFromSource.API, + from_end_user_id=end_user.id, + ) + site = Site( + app_id=target.id, + title="Feedback app", + default_language="en-US", + customize_token_strategy=CustomizeTokenStrategy.UUID, + code="feedback-app", + ) + with sqlite_session_factory.begin() as session: + session.add_all([target, config, end_user, conversation, message, site]) + app = Flask(__name__) + app.config.update(TESTING=True, RESTX_ERROR_404_HELP=False, SQLALCHEMY_DATABASE_URI=str(sqlite_engine.url)) + db.init_app(app) + factory = sessionmaker(bind=sqlite_engine, expire_on_commit=False) + sessions: list[Session] = [] + + def track(session: Session, _transaction: SessionTransaction, _connection: Connection) -> None: + assert all(not previous.in_transaction() for previous in sessions if previous is not session) + sessions.append(session) + + event.listen(sqlite_session_factory, "after_begin", track) + event.listen(factory, "after_begin", track) + event.listen(db.session.session_factory, "after_begin", track) + redis_client = Redis() + redis = RedisClientWrapper() + redis.initialize(redis_client) + app.extensions["application_services"] = build_application_services( + database_client=factory, deployment_edition=DeploymentEdition.COMMUNITY, initialization_password="", redis=redis + ) + api = ExternalApi(app) + api.add_resource(MessageFeedbackApi, "/messages//feedbacks") + passport = PassportService().issue({"app_code": site.code, "app_id": target.id, "end_user_id": end_user.id}) + yield _Harness( + app, + target, + end_user, + message, + sqlite_session_factory, + sessions, + {HEADER_NAME_APP_CODE: "feedback-app", HEADER_NAME_PASSPORT: passport}, + ) + event.remove(sqlite_session_factory, "after_begin", track) + event.remove(factory, "after_begin", track) + event.remove(db.session.session_factory, "after_begin", track) + redis_client.close() + with app.app_context(): + db.session.remove() + db.engine.dispose() + + +def test_create_update_and_revoke_user_feedback(harness: _Harness) -> None: + created = harness.post({"rating": "like", "content": "有帮助"}) + assert created.status_code == HTTPStatus.OK + assert created.get_json() == {"result": "success"} + assert created.headers["Content-Type"] == "application/json" + [feedback] = harness.feedbacks() + assert feedback.app_id == harness.target.id + assert feedback.conversation_id == harness.message.conversation_id + assert feedback.message_id == harness.message.id + assert feedback.from_end_user_id == harness.end_user.id + assert feedback.from_account_id is None + assert feedback.from_source == FeedbackFromSource.USER + assert feedback.rating == FeedbackRating.LIKE + assert feedback.content == "有帮助" + feedback_id = feedback.id + + updated = harness.post({"rating": "dislike", "content": ""}) + assert updated.status_code == HTTPStatus.OK + [feedback] = harness.feedbacks() + assert feedback.id == feedback_id + assert feedback.rating == FeedbackRating.DISLIKE + assert feedback.content == "" + + revoked = harness.post({"rating": None}) + assert revoked.status_code == HTTPStatus.OK + assert revoked.get_json() == {"result": "success"} + assert harness.feedbacks() == [] + harness.assert_closed() + + +@pytest.mark.parametrize("payload", [{}, {"rating": None, "content": "comment only"}]) +def test_revoke_without_feedback_has_specific_error(harness: _Harness, payload: dict[str, object]) -> None: + response = harness.post(payload) + assert response.status_code == HTTPStatus.BAD_REQUEST + assert response.get_json()["code"] == "message_feedback_rating_required" + assert harness.feedbacks() == [] + harness.assert_closed() + + +@pytest.mark.parametrize("field", ["app_id", "from_end_user_id", "from_account_id", "from_source"]) +def test_message_ownership_is_enforced(harness: _Harness, field: str) -> None: + value = ConversationFromSource.CONSOLE if field == "from_source" else str(uuid4()) + with harness.factory.begin() as session: + session.execute(update(Message).where(Message.id == harness.message.id).values({field: value})) + response = harness.post({"rating": "like"}) + assert response.status_code == HTTPStatus.NOT_FOUND + assert response.get_json()["code"] == "not_found" + assert harness.feedbacks() == [] + harness.assert_closed() + + +def test_missing_message_returns_not_found(harness: _Harness) -> None: + response = harness.post({"rating": "like"}, message_id=str(uuid4())) + assert response.status_code == HTTPStatus.NOT_FOUND + assert response.get_json()["message"] == "Message Not Exists." + harness.assert_closed() + + +@pytest.mark.parametrize("field", ["app_id", "tenant_id"]) +def test_corrupt_end_user_scope_cannot_write_feedback(harness: _Harness, field: str) -> None: + with harness.factory.begin() as session: + session.execute(update(EndUser).where(EndUser.id == harness.end_user.id).values({field: str(uuid4())})) + response = harness.post({"rating": "like"}) + assert response.status_code == HTTPStatus.NOT_FOUND + assert response.get_json()["code"] == "not_found" + assert harness.feedbacks() == [] + + +def test_invalid_rating_is_rejected_without_persistence(harness: _Harness) -> None: + response = harness.post({"rating": "neutral"}) + assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY + assert response.get_json()["code"] == "unprocessable_entity" + assert harness.feedbacks() == [] + + +def test_revoking_user_feedback_preserves_admin_feedback(harness: _Harness) -> None: + with harness.factory.begin() as session: + session.add( + MessageFeedback( + app_id=harness.target.id, + conversation_id=harness.message.conversation_id, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + from_source=FeedbackFromSource.ADMIN, + from_account_id=str(uuid4()), + ) + ) + assert harness.post({"rating": "dislike"}).status_code == HTTPStatus.OK + assert harness.post({"rating": None}).status_code == HTTPStatus.OK + [remaining] = harness.feedbacks() + assert remaining.from_source == FeedbackFromSource.ADMIN diff --git a/api/tests/unit_tests/controllers/console/agent/test_roster.py b/api/tests/unit_tests/controllers/console/agent/test_roster.py index fdf02f432e9..c8a2f2bbe89 100644 --- a/api/tests/unit_tests/controllers/console/agent/test_roster.py +++ b/api/tests/unit_tests/controllers/console/agent/test_roster.py @@ -59,7 +59,6 @@ from controllers.console.app.error import CompletionRequestError from controllers.console.app.message import ( AgentChatMessageListApi, AgentMessageApi, - AgentMessageFeedbackApi, ) from enums import CloudPlan, DeploymentEdition from libs.infinite_scroll_pagination import InfiniteScrollPagination @@ -2225,17 +2224,12 @@ def test_agent_chat_message_routes_resolve_app_from_agent_id( captured["list"] = kwargs return {"data": []} - def update_message_feedback(**kwargs: object) -> dict[str, object]: - captured["feedback"] = kwargs - return {"result": "success"} - def get_message_detail(**kwargs: object) -> dict[str, object]: captured["detail"] = kwargs return {"id": message_id} monkeypatch.setattr(message_controller, "resolve_agent_runtime_app_model", resolve_agent_app_model) monkeypatch.setattr(message_controller, "_list_chat_messages", list_chat_messages) - monkeypatch.setattr(message_controller, "_update_message_feedback", update_message_feedback) monkeypatch.setattr(message_controller, "_get_message_detail", get_message_detail) assert unwrap(AgentChatMessageListApi.get)( AgentChatMessageListApi(), @@ -2248,19 +2242,6 @@ def test_agent_chat_message_routes_resolve_app_from_agent_id( list_call = cast(dict[str, object], captured["list"]) assert list_call["session"] is unbound_session assert list_call["app_model"] is app_model - with app.test_request_context(json={"message_id": message_id, "rating": "like"}): - assert unwrap(AgentMessageFeedbackApi.post)( - AgentMessageFeedbackApi(), - message_controller.AppMessageFeedbackPayload(message_id=message_id, rating="like"), - unbound_session, - "tenant-1", - current_user, - agent_id, - ) == {"result": "success"} - feedback_call = cast(dict[str, object], captured["feedback"]) - assert feedback_call["session"] is unbound_session - assert feedback_call["app_model"] is app_model - assert feedback_call["current_user"] is current_user assert unwrap(AgentMessageApi.get)(AgentMessageApi(), unbound_session, "tenant-1", agent_id, message_id) == { "id": message_id } @@ -2269,7 +2250,6 @@ def test_agent_chat_message_routes_resolve_app_from_agent_id( assert resolver_calls == [ {"session": unbound_session, "tenant_id": "tenant-1", "agent_id": agent_id}, {"session": unbound_session, "tenant_id": "tenant-1", "agent_id": agent_id}, - {"session": unbound_session, "tenant_id": "tenant-1", "agent_id": agent_id}, ] @@ -2396,30 +2376,6 @@ def test_list_agent_chat_messages_rejects_foreign_conversation( ) -def test_update_message_feedback_rejects_empty_rating_without_existing_feedback( - app: Flask, sqlite_session: Session -) -> None: - app_id = "00000000-0000-0000-0000-000000000001" - message_id = "00000000-0000-0000-0000-000000000002" - _, message = _persist_conversation_message( - sqlite_session, - app_id=app_id, - conversation_id="00000000-0000-0000-0000-000000000010", - message_id=message_id, - created_at=datetime(2025, 1, 1), - ) - with app.test_request_context(json={"message_id": message_id, "rating": None}): - with pytest.raises(ValueError, match="rating cannot be None"): - message_controller._update_message_feedback( - args=message_controller.AppMessageFeedbackPayload(message_id=message_id, rating=None), - session=sqlite_session, - current_user=_account(), - app_model=_app_detail_obj(id=app_id), - ) - - assert message.admin_feedback_with_session(session=sqlite_session) is None - - def test_dify_tool_candidate_response_keeps_granularity_fields() -> None: """Both selection granularities must survive the fields-layer model — the frontend needs granularity/tools_count to render the Tools menu.""" diff --git a/api/tests/unit_tests/controllers/console/app/test_message.py b/api/tests/unit_tests/controllers/console/app/test_message.py index b0700224381..5b8b7a84b8d 100644 --- a/api/tests/unit_tests/controllers/console/app/test_message.py +++ b/api/tests/unit_tests/controllers/console/app/test_message.py @@ -6,7 +6,6 @@ from unittest.mock import MagicMock import pytest from flask import Flask -from sqlalchemy import event from sqlalchemy.orm import Session from controllers.console.app import message as message_module @@ -102,10 +101,8 @@ class TestMessageApiCases: app_model = _app() message_id = "550e8400-e29b-41d4-a716-446655440000" list_messages = MagicMock(return_value={"data": []}) - update_feedback = MagicMock(return_value={"result": "success"}) get_message_detail = MagicMock(return_value={"id": message_id}) monkeypatch.setattr(message_module, "_list_chat_messages", list_messages) - monkeypatch.setattr(message_module, "_update_message_feedback", update_feedback) monkeypatch.setattr(message_module, "_get_message_detail", get_message_detail) assert unwrap(message_module.ChatMessageListApi.get)( @@ -115,58 +112,13 @@ class TestMessageApiCases: current_user, app_model, ) == {"data": []} - assert unwrap(message_module.MessageFeedbackApi.post)( - message_module.MessageFeedbackApi(), - message_module.AppMessageFeedbackPayload(message_id=message_id, rating="like"), - session, - current_user, - app_model, - ) == {"result": "success"} assert unwrap(message_module.MessageApi.get)(message_module.MessageApi(), session, app_model, message_id) == { "id": message_id } assert list_messages.call_args.kwargs["session"] is session - assert update_feedback.call_args.kwargs["session"] is session assert get_message_detail.call_args.kwargs["session"] is session - @staticmethod - def test_update_message_feedback_commits_injected_session(app: Flask, sqlite_session: Session) -> None: - message_id = "550e8400-e29b-41d4-a716-446655440000" - message = _persist_message(sqlite_session, message_id=message_id) - feedback = MessageFeedback( - app_id=message.app_id, - conversation_id=message.conversation_id, - message_id=message.id, - rating=FeedbackRating.DISLIKE, - from_source=FeedbackFromSource.ADMIN, - from_account_id="account-1", - ) - sqlite_session.add(feedback) - sqlite_session.commit() - session = sqlite_session - commits: list[str] = [] - event.listen(session, "after_commit", lambda _session: commits.append("commit")) - - with app.test_request_context(json={"message_id": message_id, "rating": "like", "content": "helpful"}): - result = message_module._update_message_feedback( - args=message_module.AppMessageFeedbackPayload( - message_id=message_id, - rating="like", - content="helpful", - ), - session=session, - current_user=_account(), - app_model=_app(), - ) - - assert result == {"result": "success"} - updated_feedback = sqlite_session.get(MessageFeedback, feedback.id) - assert updated_feedback is not None - assert updated_feedback.rating == FeedbackRating.LIKE - assert updated_feedback.content == "helpful" - assert commits == ["commit"] - @staticmethod def test_get_message_detail_uses_injected_session(monkeypatch: pytest.MonkeyPatch, sqlite_session: Session) -> None: message_id = "550e8400-e29b-41d4-a716-446655440000" diff --git a/api/tests/unit_tests/controllers/console/explore/test_message.py b/api/tests/unit_tests/controllers/console/explore/test_message.py index c8be961dea6..b26c760fbf7 100644 --- a/api/tests/unit_tests/controllers/console/explore/test_message.py +++ b/api/tests/unit_tests/controllers/console/explore/test_message.py @@ -5,12 +5,11 @@ from collections.abc import Mapping, Sequence from dataclasses import dataclass, field from datetime import datetime, timedelta from decimal import Decimal -from typing import Literal from uuid import uuid4 import pytest from pydantic import JsonValue -from sqlalchemy import Connection, event, select, update +from sqlalchemy import Connection, event, update from sqlalchemy.orm import Session, SessionTransaction, sessionmaker from werkzeug.test import TestResponse @@ -20,7 +19,7 @@ from models import App, AppMode, InstalledApp from models.enums import ConversationFromSource, ConversationStatus, FeedbackFromSource, FeedbackRating from models.model import Conversation, Message, MessageFeedback from repositories.installed_app_message_repository import SQLAlchemyInstalledAppMessageRepository -from services.installed_app_message_service import InstalledAppMessageService, MessageFeedbackEvent +from services.installed_app_message_service import InstalledAppMessageService from tests.unit_tests.controllers.console.explore.test_installed_app_admission import ( _assert_json_response, _Harness, @@ -30,8 +29,6 @@ from tests.unit_tests.controllers.console.explore.test_installed_app_admission i __all__ = ["harness"] _CREATED_AT = datetime(2024, 1, 1) -type _Operation = Literal["list", "feedback"] -_OPERATIONS: tuple[_Operation, ...] = ("list", "feedback") @dataclass(frozen=True) @@ -52,7 +49,6 @@ class _Messages: sessions: list[Session] = field(default_factory=list) extras: dict[str, list[dict[str, JsonValue]]] = field(default_factory=dict) extra_calls: list[list[str]] = field(default_factory=list) - feedback_events: list[MessageFeedbackEvent] = field(default_factory=list) def assert_sessions_closed(self) -> None: assert all(not session.in_transaction() and not session.identity_map for session in self.sessions) @@ -62,33 +58,18 @@ class _Messages: self.extra_calls.append(list(message_ids)) return self.extras - def emit_feedback(self, *, feedback: MessageFeedbackEvent) -> None: - self.assert_sessions_closed() - with self.factory() as session: - persisted = session.scalar(select(MessageFeedback).where(MessageFeedback.message_id == feedback.message_id)) - assert persisted is not None - assert persisted.rating.value == feedback.rating - assert persisted.content == feedback.content - self.feedback_events.append(feedback) - def request( self, - operation: _Operation, *, conversation_id: str = "", - message_id: str | None = None, installed_app_id: str | None = None, query: str | None = None, - body: dict[str, object] | None = None, ) -> TestResponse: url = f"/installed-apps/{installed_app_id or self.harness.installed_app.id}/messages" - if operation == "list": - url += f"?conversation_id={conversation_id}" - if query: - url += f"&{query}" - else: - url += f"/{message_id or uuid4()}/feedbacks" - return self.harness.app.test_client().open(url, method="POST" if operation == "feedback" else "GET", json=body) + url += f"?conversation_id={conversation_id}" + if query: + url += f"&{query}" + return self.harness.app.test_client().get(url) @pytest.fixture @@ -108,16 +89,11 @@ def messages( messages=InstalledAppMessageService( messages=SQLAlchemyInstalledAppMessageRepository(session_factory=factory), get_extra_contents=state.get_extra_contents, - emit_feedback=state.emit_feedback, ), ), ) monkeypatch.setattr(module, "application_services", lambda: state.services) - for resource, suffix in ( - (module.MessageListApi, ""), - (module.MessageFeedbackApi, "//feedbacks"), - ): - harness.api.add_resource(resource, f"/installed-apps//messages{suffix}") + harness.api.add_resource(module.MessageListApi, "/installed-apps//messages") return state @@ -225,7 +201,7 @@ def test_list_preserves_response_and_fetches_older_messages_in_display_order(mes ) ] ) - response = messages.request("list", conversation_id=conversation.id, query="limit=2") + response = messages.request(conversation_id=conversation.id, query="limit=2") body = response.get_json() assert response.status_code == 200 assert body["limit"] == 2 @@ -264,7 +240,7 @@ def test_list_preserves_response_and_fetches_older_messages_in_display_order(mes "metadata": {"retriever_resources": [], "zero": 0}, } _assert_json_response(response, status=200, body=body) - response = messages.request("list", conversation_id=conversation.id, query=f"limit=2&first_id={middle.id}") + response = messages.request(conversation_id=conversation.id, query=f"limit=2&first_id={middle.id}") assert response.get_json()["has_more"] is False assert [item["id"] for item in response.get_json()["data"]] == [oldest.id] assert messages.extra_calls == [[middle.id, newest.id], [oldest.id]] @@ -277,7 +253,7 @@ def test_empty_list_keeps_defaults_and_avoids_extra_content_io( ) -> None: conversation = _conversation(messages) _assert_json_response( - messages.request("list", conversation_id="" if empty_conversation_id else conversation.id), + messages.request(conversation_id="" if empty_conversation_id else conversation.id), status=200, body={"limit": 20, "has_more": False, "data": []}, ) @@ -286,7 +262,7 @@ def test_empty_list_keeps_defaults_and_avoids_extra_content_io( @pytest.mark.parametrize("query", ["limit=0", "limit=101", "limit=invalid", "first_id=invalid"]) def test_invalid_list_query_uses_shared_422(messages: _Messages, query: str) -> None: - _error(messages.request("list", conversation_id=str(uuid4()), query=query), status=422, code="unprocessable_entity") + _error(messages.request(conversation_id=str(uuid4()), query=query), status=422, code="unprocessable_entity") assert messages.extra_calls == [] @@ -301,7 +277,7 @@ def test_list_requires_conversation_ownership(messages: _Messages, owner: str) - is_deleted=owner == "deleted", ) _error( - messages.request("list", conversation_id=str(uuid4()) if owner == "missing" else conversation.id), + messages.request(conversation_id=str(uuid4()) if owner == "missing" else conversation.id), status=404, code="conversation_not_found", ) @@ -312,101 +288,15 @@ def test_list_rejects_cursor_from_another_conversation(messages: _Messages) -> N conversation = _conversation(messages) foreign = _message(messages, _conversation(messages)) _error( - messages.request("list", conversation_id=conversation.id, query=f"first_id={foreign.id}"), + messages.request(conversation_id=conversation.id, query=f"first_id={foreign.id}"), status=404, code="message_cursor_not_found", ) assert messages.extra_calls == [] -def test_feedback_create_update_revoke_persists_before_telemetry(messages: _Messages) -> None: - message = _message(messages, _conversation(messages)) - for rating, content in (("like", ""), ("dislike", "Changed my mind")): - _assert_json_response( - messages.request("feedback", message_id=message.id, body={"rating": rating, "content": content}), - status=200, - body={"result": "success"}, - ) - with messages.factory() as session: - rows = session.scalars(select(MessageFeedback)).all() - assert len(rows) == 1 - assert rows[0].rating.value == rating - assert rows[0].content == content - assert rows[0].from_source == FeedbackFromSource.ADMIN - assert rows[0].from_account_id == messages.harness.account.id - assert rows[0].from_end_user_id is None - _assert_json_response( - messages.request("feedback", message_id=message.id, body={"rating": None}), - status=200, - body={"result": "success"}, - ) - with messages.factory() as session: - assert session.scalars(select(MessageFeedback)).all() == [] - assert [feedback.rating for feedback in messages.feedback_events] == ["like", "dislike"] - assert all( - feedback.tenant_id == messages.harness.target_app.tenant_id - and feedback.app_id == message.app_id - and feedback.conversation_id == message.conversation_id - and feedback.message_id == message.id - and feedback.account_id == messages.harness.account.id - for feedback in messages.feedback_events - ) - - -def test_revoke_without_feedback_has_specific_400(messages: _Messages) -> None: - message = _message(messages, _conversation(messages)) - _error( - messages.request("feedback", message_id=message.id, body={"rating": None}), - status=400, - code="message_feedback_rating_required", - ) - assert messages.feedback_events == [] - - -@pytest.mark.parametrize("owner", ["app", "account", "source", "end_user", "missing"]) -def test_feedback_requires_complete_message_ownership(messages: _Messages, owner: str) -> None: - message = _message( - messages, - _conversation(messages), - app_id=str(uuid4()) if owner == "app" else None, - account_id=str(uuid4()) if owner == "account" else None, - source=ConversationFromSource.API if owner == "source" else ConversationFromSource.CONSOLE, - end_user_id=str(uuid4()) if owner == "end_user" else None, - ) - _error( - messages.request( - "feedback", message_id=str(uuid4()) if owner == "missing" else message.id, body={"rating": "like"} - ), - status=404, - code="message_not_found", - ) - assert messages.feedback_events == [] - with messages.factory() as session: - assert session.scalars(select(MessageFeedback)).all() == [] - - -@pytest.mark.parametrize("mode", list(AppMode)) -def test_feedback_accepts_every_app_mode(messages: _Messages, mode: AppMode) -> None: - _set_mode(messages, mode) - message = _message(messages, _conversation(messages)) - _assert_json_response( - messages.request("feedback", message_id=message.id, body={"rating": "like"}), - status=200, - body={"result": "success"}, - ) - - -@pytest.mark.parametrize("body", [{"rating": "invalid"}, {"content": 123}]) -def test_invalid_feedback_uses_shared_422(messages: _Messages, body: dict[str, object]) -> None: - _error(messages.request("feedback", body=body), status=422, code="unprocessable_entity") - assert messages.feedback_events == [] - - -@pytest.mark.parametrize("operation", _OPERATIONS) @pytest.mark.parametrize("admission", ["missing", "denied", "wrong-workspace"]) -def test_all_message_handlers_apply_installed_app_admission( - messages: _Messages, operation: _Operation, admission: str -) -> None: +def test_list_applies_installed_app_admission(messages: _Messages, admission: str) -> None: if admission == "denied": messages.harness.state.allowed = False elif admission == "wrong-workspace": @@ -417,14 +307,14 @@ def test_all_message_handlers_apply_installed_app_admission( .values(tenant_id=str(uuid4())) ) _error( - messages.request(operation, installed_app_id=str(uuid4()) if admission == "missing" else None), + messages.request(installed_app_id=str(uuid4()) if admission == "missing" else None), status=403 if admission == "denied" else 404, code="access_denied" if admission == "denied" else "installed_app_not_found", ) - assert messages.extra_calls == messages.feedback_events == [] + assert messages.extra_calls == [] def test_list_mode_rejection_precedes_message_queries(messages: _Messages) -> None: _set_mode(messages, AppMode.COMPLETION) - _error(messages.request("list"), status=400, code="not_chat_app") + _error(messages.request(), status=400, code="not_chat_app") assert messages.extra_calls == [] diff --git a/api/tests/unit_tests/controllers/service_api/app/test_message.py b/api/tests/unit_tests/controllers/service_api/app/test_message.py index 24a366d32bb..31efc405bba 100644 --- a/api/tests/unit_tests/controllers/service_api/app/test_message.py +++ b/api/tests/unit_tests/controllers/service_api/app/test_message.py @@ -27,14 +27,12 @@ from sqlalchemy.orm import Session from controllers.common.errors import NotFoundError from controllers.service_api.app.error import NotChatAppError from controllers.service_api.app.message import ( - AppGetFeedbacksApi, FeedbackListQuery, - MessageFeedbackApi, MessageFeedbackPayload, MessageListApi, MessageListQuery, ) -from models.enums import EndUserType, FeedbackRating +from models.enums import EndUserType from models.model import App, AppMode, EndUser from services.errors.conversation import ConversationNotExistsError from services.errors.message import ( @@ -271,16 +269,6 @@ class TestMessageService: assert hasattr(MessageService, "pagination_by_first_id") assert callable(MessageService.pagination_by_first_id) - def test_create_feedback_method_exists(self): - """Test MessageService.create_feedback exists.""" - assert hasattr(MessageService, "create_feedback") - assert callable(MessageService.create_feedback) - - def test_get_all_messages_feedbacks_method_exists(self): - """Test MessageService.get_all_messages_feedbacks exists.""" - assert hasattr(MessageService, "get_all_messages_feedbacks") - assert callable(MessageService.get_all_messages_feedbacks) - @patch.object(MessageService, "pagination_by_first_id") def test_pagination_by_first_id_returns_pagination_result(self, mock_pagination, orm_session: Session): """Test pagination_by_first_id returns expected format.""" @@ -335,51 +323,6 @@ class TestMessageService: session=orm_session, ) - @patch.object(MessageService, "create_feedback") - def test_create_feedback_with_rating_and_content(self, mock_create_feedback, orm_session: Session): - """Test create_feedback with rating and content.""" - mock_create_feedback.return_value = None - - MessageService.create_feedback( - app_model=_app(), - message_id=str(uuid.uuid4()), - user=_end_user(), - rating=FeedbackRating.LIKE, - content="Great response!", - session=orm_session, - ) - - mock_create_feedback.assert_called_once() - - @patch.object(MessageService, "create_feedback") - def test_create_feedback_raises_message_not_exists_error(self, mock_create_feedback, orm_session: Session): - """Test create_feedback raises MessageNotExistsError.""" - mock_create_feedback.side_effect = MessageNotExistsError() - - with pytest.raises(MessageNotExistsError): - MessageService.create_feedback( - app_model=_app(), - message_id="invalid_message_id", - user=_end_user(), - rating=FeedbackRating.LIKE, - content=None, - session=orm_session, - ) - - @patch.object(MessageService, "get_all_messages_feedbacks") - def test_get_all_messages_feedbacks_returns_list(self, mock_get_feedbacks, orm_session: Session): - """Test get_all_messages_feedbacks returns list of feedbacks.""" - mock_feedbacks = [ - {"message_id": str(uuid.uuid4()), "rating": "like"}, - {"message_id": str(uuid.uuid4()), "rating": "dislike"}, - ] - mock_get_feedbacks.return_value = mock_feedbacks - - result = MessageService.get_all_messages_feedbacks(app_model=_app(), page=1, limit=20, session=orm_session) - - assert len(result) == 2 - assert result[0]["rating"] == "like" - class TestMessageListApi: def test_not_chat_app(self, app: Flask) -> None: @@ -446,55 +389,3 @@ class TestMessageListApi: app_model=app_model, end_user=end_user, ) - - -class TestMessageFeedbackApi: - def test_not_found(self, app: Flask, monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr( - MessageService, - "create_feedback", - lambda *_args, **_kwargs: (_ for _ in ()).throw(MessageNotExistsError()), - ) - - api = MessageFeedbackApi() - handler = unwrap(api.post) - app_model = _app() - end_user = _end_user() - - with app.test_request_context( - "/messages/m1/feedbacks", - method="POST", - json={"rating": "like", "content": "ok"}, - ): - payload = MessageFeedbackPayload.model_validate(request.get_json() or {}) - with pytest.raises(NotFoundError): - handler(api, payload, app_model=app_model, end_user=end_user, message_id="m1") - - -class TestAppGetFeedbacksApi: - def test_success(self, app: Flask, monkeypatch: pytest.MonkeyPatch) -> None: - feedback = { - "id": "feedback-1", - "app_id": "app-1", - "conversation_id": "conversation-1", - "message_id": "message-1", - "rating": "like", - "content": "helpful answer", - "from_source": "user", - "from_end_user_id": "end-user-1", - "from_account_id": None, - "created_at": "2024-01-02T03:04:05", - "updated_at": "2024-01-02T03:04:06", - } - monkeypatch.setattr(MessageService, "get_all_messages_feedbacks", lambda *_args, **_kwargs: [feedback]) - - api = AppGetFeedbacksApi() - handler = unwrap(api.get) - app_model = _app() - - with app.test_request_context("/app/feedbacks?page=1&limit=20", method="GET"): - response = handler( - api, FeedbackListQuery.model_validate(request.args.to_dict(flat=True)), app_model=app_model - ) - - assert response == {"data": [feedback]} diff --git a/api/tests/unit_tests/controllers/web/test_message.py b/api/tests/unit_tests/controllers/web/test_message.py index c1036e6c67a..09362e8b178 100644 --- a/api/tests/unit_tests/controllers/web/test_message.py +++ b/api/tests/unit_tests/controllers/web/test_message.py @@ -7,18 +7,14 @@ import uuid from datetime import datetime from pathlib import Path from types import ModuleType, SimpleNamespace -from unittest.mock import ANY, MagicMock, patch +from unittest.mock import ANY, patch from uuid import uuid4 import pytest from flask import Flask from flask.views import MethodView -from controllers.common.controller_schemas import MessageFeedbackPayload, MessageListQuery -from controllers.common.errors import NotFoundError -from controllers.web.message import ( - MessageFeedbackApi, -) +from controllers.common.controller_schemas import MessageListQuery from core.app.entities.app_invoke_entities import InvokeFrom from core.entities.execution_extra_content import HumanInputContent from models.enums import ConversationFromSource, EndUserType, FeedbackFromSource, FeedbackRating @@ -31,72 +27,6 @@ from models.model import ( MessageAgentThought, MessageFeedback, ) -from services.errors.message import MessageNotExistsError -from tests.helpers.unit.model_factories import make_end_user - -"""Unit tests for controllers.web.message — feedback, more-like-this, suggested questions.""" - - -def _chat_app() -> App: - return App(id="app-1", tenant_id="tenant-1", mode=AppMode.CHAT) - - -def _end_user() -> EndUser: - return make_end_user(end_user_id="eu-1") - - -# The @model_validate and @with_session decorators wrap the handlers; tests -# call the undecorated function so they can pass a pydantic payload / a fake -# session directly. -_feedback_post = inspect.unwrap(MessageFeedbackApi.post) - - -class TestMessageEndpointsCases: - # --------------------------------------------------------------------------- - # MessageFeedbackApi - # --------------------------------------------------------------------------- - class TestMessageFeedbackApi: - @patch("controllers.web.message.MessageService.create_feedback") - def test_feedback_success(self, mock_create: MagicMock, app: Flask) -> None: - payload = MessageFeedbackPayload.model_validate({"rating": "like", "content": "great"}) - msg_id = uuid4() - - with app.test_request_context(f"/messages/{msg_id}/feedbacks", method="POST"): - result = _feedback_post(MessageFeedbackApi(), payload, _chat_app(), _end_user(), msg_id) - - assert result == {"result": "success"} - mock_create.assert_called_once() - - @patch("controllers.web.message.MessageService.create_feedback") - def test_feedback_null_rating(self, mock_create: MagicMock, app: Flask) -> None: - payload = MessageFeedbackPayload.model_validate({"rating": None}) - msg_id = uuid4() - - with app.test_request_context(f"/messages/{msg_id}/feedbacks", method="POST"): - result = _feedback_post(MessageFeedbackApi(), payload, _chat_app(), _end_user(), msg_id) - - assert result == {"result": "success"} - - @patch( - "controllers.web.message.MessageService.create_feedback", - side_effect=MessageNotExistsError(), - ) - def test_feedback_message_not_found(self, mock_create: MagicMock, app: Flask) -> None: - payload = MessageFeedbackPayload.model_validate({"rating": "dislike"}) - msg_id = uuid4() - - with app.test_request_context(f"/messages/{msg_id}/feedbacks", method="POST"): - with pytest.raises(NotFoundError, match="Message Not Exists"): - _feedback_post(MessageFeedbackApi(), payload, _chat_app(), _end_user(), msg_id) - - # --------------------------------------------------------------------------- - # MessageMoreLikeThisApi - # --------------------------------------------------------------------------- - - # --------------------------------------------------------------------------- - # MessageSuggestedQuestionApi - # --------------------------------------------------------------------------- - """Unit tests for controllers.web.message message list mapping.""" diff --git a/api/tests/unit_tests/repositories/test_installed_app_message_repository.py b/api/tests/unit_tests/repositories/test_installed_app_message_repository.py index a82611889df..972c8d0f4e4 100644 --- a/api/tests/unit_tests/repositories/test_installed_app_message_repository.py +++ b/api/tests/unit_tests/repositories/test_installed_app_message_repository.py @@ -2,20 +2,19 @@ import json from dataclasses import replace from datetime import datetime, timedelta from decimal import Decimal -from typing import Literal from uuid import uuid4 import pytest -from sqlalchemy import Engine, event, inspect, select, update +from sqlalchemy import inspect, update from sqlalchemy.orm import Session, sessionmaker from models.enums import ConversationFromSource, CreatorUserRole, FeedbackFromSource, FeedbackRating from models.model import App, AppMode, Conversation, InstalledApp, Message, MessageAgentThought, MessageFeedback from repositories.installed_app_message_repository import SQLAlchemyInstalledAppMessageRepository from services.errors.conversation import ConversationNotExistsError -from services.errors.message import FirstMessageNotExistsError, MessageNotExistsError +from services.errors.message import FirstMessageNotExistsError from services.installed_app_access_service import InstalledAppNotFoundError, InstalledAppRef -from services.installed_app_message_service import FeedbackRatingRequiredError, MessagePage, MessageRating +from services.installed_app_message_service import MessagePage _ACCOUNT_ID = "11111111-1111-4111-8111-111111111111" _OWNER_TENANT_ID = "22222222-2222-4222-8222-222222222222" @@ -246,12 +245,12 @@ def test_cursor_must_belong_to_the_requested_conversation( @pytest.mark.parametrize("mismatch", ["id", "tenant_id", "app_id", "deleted_app", "deleted_installation"]) -def test_read_and_feedback_revalidate_installation_after_admission( +def test_read_revalidates_installation_after_admission( sqlite_session_factory: sessionmaker[Session], installation: InstalledAppRef, mismatch: str ) -> None: with sqlite_session_factory.begin() as session: conversation = _conversation(session, installation) - message = _message(session, conversation) + _message(session, conversation) if mismatch == "deleted_app": app = session.get(App, installation.app_id) assert app is not None @@ -272,122 +271,3 @@ def test_read_and_feedback_revalidate_installation_after_admission( repository.get_page( installed_app=ref, account_id=_ACCOUNT_ID, conversation_id=conversation.id, first_id=None, limit=20 ) - with pytest.raises(InstalledAppNotFoundError): - repository.set_feedback( - installed_app=ref, account_id=_ACCOUNT_ID, message_id=message.id, rating="like", content=None - ) - - -@pytest.mark.parametrize("mismatch", ["app_id", "from_account_id", "from_source", "from_end_user_id"]) -def test_feedback_checks_each_message_ownership_predicate( - sqlite_session_factory: sessionmaker[Session], installation: InstalledAppRef, mismatch: str -) -> None: - with sqlite_session_factory.begin() as session: - message = _message(session, _conversation(session, installation)) - value = ConversationFromSource.API if mismatch == "from_source" else str(uuid4()) - session.execute(update(Message).where(Message.id == message.id).values({mismatch: value})) - repository = SQLAlchemyInstalledAppMessageRepository(session_factory=sqlite_session_factory) - with pytest.raises(MessageNotExistsError): - repository.set_feedback( - installed_app=installation, account_id=_ACCOUNT_ID, message_id=message.id, rating="like", content=None - ) - with sqlite_session_factory() as session: - assert session.scalars(select(MessageFeedback)).all() == [] - - -def test_feedback_create_update_and_delete_preserve_admin_scope_and_owner_tenant( - sqlite_session_factory: sessionmaker[Session], installation: InstalledAppRef -) -> None: - with sqlite_session_factory.begin() as session: - message = _message(session, _conversation(session, installation)) - user_feedback = _feedback(session, message, source=FeedbackFromSource.USER) - repository = SQLAlchemyInstalledAppMessageRepository(session_factory=sqlite_session_factory) - with pytest.raises(FeedbackRatingRequiredError, match=message.id): - repository.set_feedback( - installed_app=installation, account_id=_ACCOUNT_ID, message_id=message.id, rating=None, content=None - ) - created = repository.set_feedback( - installed_app=installation, account_id=_ACCOUNT_ID, message_id=message.id, rating="like", content="" - ) - assert created is not None - assert ( - created.tenant_id, - created.app_id, - created.conversation_id, - created.message_id, - created.account_id, - created.rating, - created.content, - ) == (_OWNER_TENANT_ID, installation.app_id, message.conversation_id, message.id, _ACCOUNT_ID, "like", "") - with sqlite_session_factory.begin() as session: - feedback = message.admin_feedback_with_session(session=session) - assert feedback is not None - assert (feedback.from_source, feedback.from_account_id, feedback.from_end_user_id, feedback.content) == ( - FeedbackFromSource.ADMIN, - _ACCOUNT_ID, - None, - "", - ) - feedback.from_account_id = original_owner = str(uuid4()) - feedback_id = feedback.id - updated = repository.set_feedback( - installed_app=installation, account_id=_ACCOUNT_ID, message_id=message.id, rating="dislike", content=None - ) - assert updated is not None - assert (updated.rating, updated.content) == ("dislike", None) - with sqlite_session_factory() as session: - feedback = session.get(MessageFeedback, feedback_id) - assert feedback is not None - assert (feedback.rating, feedback.content, feedback.from_account_id) == ( - FeedbackRating.DISLIKE, - None, - original_owner, - ) - assert ( - repository.set_feedback( - installed_app=installation, account_id=_ACCOUNT_ID, message_id=message.id, rating=None, content=None - ) - is None - ) - with sqlite_session_factory() as session: - remaining = session.scalars(select(MessageFeedback)).all() - assert [feedback.id for feedback in remaining] == [user_feedback.id] - assert (remaining[0].rating, remaining[0].content) == (FeedbackRating.LIKE, "Original") - - -@pytest.mark.parametrize("operation", ["create", "update", "delete"]) -def test_failed_feedback_commit_rolls_back_and_preserves_original_error( - sqlite_session_factory: sessionmaker[Session], - sqlite_engine: Engine, - installation: InstalledAppRef, - operation: Literal["create", "update", "delete"], -) -> None: - with sqlite_session_factory.begin() as session: - message = _message(session, _conversation(session, installation)) - if operation != "create": - _feedback(session, message, source=FeedbackFromSource.ADMIN) - failing_factory = sessionmaker(bind=sqlite_engine) - failure = RuntimeError("database commit unavailable") - - @event.listens_for(failing_factory, "before_commit") - def fail_commit(session: Session) -> None: - session.flush() - raise failure - - rating: MessageRating | None = None if operation == "delete" else "dislike" - with pytest.raises(RuntimeError) as error: - SQLAlchemyInstalledAppMessageRepository(session_factory=failing_factory).set_feedback( - installed_app=installation, - account_id=_ACCOUNT_ID, - message_id=message.id, - rating=rating, - content="Discarded", - ) - assert error.value is failure - with sqlite_session_factory() as session: - feedback = message.admin_feedback_with_session(session=session) - if operation == "create": - assert feedback is None - else: - assert feedback is not None - assert (feedback.rating, feedback.content) == (FeedbackRating.LIKE, "Original") diff --git a/api/tests/unit_tests/services/test_installed_app_message_adapters.py b/api/tests/unit_tests/services/test_installed_app_message_adapters.py index 76060330e2c..25b2e191b87 100644 --- a/api/tests/unit_tests/services/test_installed_app_message_adapters.py +++ b/api/tests/unit_tests/services/test_installed_app_message_adapters.py @@ -3,17 +3,13 @@ from datetime import datetime, timedelta from uuid import uuid4 -import pytest from sqlalchemy import Connection, Engine, event from sqlalchemy.orm import Session, SessionTransaction, sessionmaker -from core import telemetry -from core.telemetry import FeedbackCreatedEvent from core.workflow.nodes.human_input.entities import FormDefinition, UserActionConfig from models.execution_extra_content import HumanInputContent from models.human_input import HumanInputForm -from services.installed_app_message_adapters import InstalledAppMessageRuntime, emit_installed_app_feedback -from services.installed_app_message_service import MessageFeedbackEvent +from services.installed_app_message_adapters import InstalledAppMessageRuntime def test_extra_contents_use_real_repository_preserve_message_order_and_omit_none( @@ -76,42 +72,3 @@ def test_extra_contents_use_real_repository_preserve_message_order_and_omit_none assert len(read_sessions) == 1 assert not read_sessions[0].in_transaction() assert not read_sessions[0].identity_map - - -@pytest.mark.parametrize("fail", [False, True]) -def test_feedback_telemetry_preserves_event_fields_and_suppresses_sink_failure( - monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, fail: bool -) -> None: - events: list[FeedbackCreatedEvent] = [] - - def emit(event: FeedbackCreatedEvent) -> None: - events.append(event) - if fail: - raise RuntimeError("Telemetry unavailable") - - monkeypatch.setattr(telemetry, "emit", emit) - emit_installed_app_feedback( - feedback=MessageFeedbackEvent( - tenant_id="owner-tenant", - app_id="app", - conversation_id="conversation", - message_id="message", - account_id="account", - rating="dislike", - content="", - ) - ) - assert len(events) == 1 - assert events[0].context.tenant_id == "owner-tenant" - assert events[0].payload == { - "message_id": "message", - "app_id": "app", - "conversation_id": "conversation", - "from_end_user_id": None, - "from_account_id": "account", - "rating": "dislike", - "from_source": "admin", - "content": "", - } - if fail: - assert "Failed to emit feedback telemetry for message message" in caplog.text diff --git a/api/tests/unit_tests/services/test_installed_app_message_service.py b/api/tests/unit_tests/services/test_installed_app_message_service.py index 4db73c982c7..7dd09c0099b 100644 --- a/api/tests/unit_tests/services/test_installed_app_message_service.py +++ b/api/tests/unit_tests/services/test_installed_app_message_service.py @@ -6,14 +6,11 @@ from decimal import Decimal import pytest from pydantic import JsonValue -from services.errors.message import MessageNotExistsError from services.installed_app_access_service import InstalledAppRef from services.installed_app_message_service import ( InstalledAppMessageService, - MessageFeedbackEvent, MessageNotChatAppError, MessagePage, - MessageRating, MessageRecord, ) @@ -52,8 +49,6 @@ class _Store: default_factory=lambda: MessagePage(limit=2, has_more=True, data=(_message("first"), _message("second"))) ) events: list[str] = field(default_factory=list) - write_error: Exception | None = None - rating: MessageRating | None = None def get_page( self, @@ -74,37 +69,11 @@ class _Store: self.events.append("read closed") return self.page - def set_feedback( - self, - *, - installed_app: InstalledAppRef, - account_id: str, - message_id: str, - rating: MessageRating | None, - content: str | None, - ) -> MessageFeedbackEvent | None: - if self.write_error is not None: - raise self.write_error - self.rating = rating - self.events.append("write committed and closed") - if rating is None: - return None - return MessageFeedbackEvent( - tenant_id="owner-workspace", - app_id=installed_app.app_id, - conversation_id="conversation", - message_id=message_id, - account_id=account_id, - rating=rating, - content=content, - ) - @dataclass class _Callbacks: store: _Store queried_ids: list[tuple[str, ...]] = field(default_factory=list) - feedback: list[MessageFeedbackEvent] = field(default_factory=list) def get_extra_contents(self, *, message_ids: Sequence[str]) -> Mapping[str, list[dict[str, JsonValue]]]: assert self.store.events == ["read closed"] @@ -112,17 +81,10 @@ class _Callbacks: self.queried_ids.append(tuple(message_ids)) return {"first": [{"type": "workflow", "workflow_run_id": "run"}], "not-in-page": [{"type": "ignored"}]} - def emit_feedback(self, *, feedback: MessageFeedbackEvent) -> None: - assert self.store.events[-1] == "write committed and closed" - assert self.store.rating == feedback.rating - self.store.events.append("emit feedback") - self.feedback.append(feedback) - def service(self) -> InstalledAppMessageService: return InstalledAppMessageService( messages=self.store, get_extra_contents=self.get_extra_contents, - emit_feedback=self.emit_feedback, ) @@ -151,46 +113,6 @@ def test_empty_page_skips_extra_content_query() -> None: assert state.queried_ids == [] -@pytest.mark.parametrize("rating", ["like", "dislike", None]) -def test_feedback_emits_only_for_committed_rating_and_allows_completion_apps(rating: MessageRating | None) -> None: - state = _Callbacks(store=_Store()) - state.service().set_feedback( - installed_app=replace(_REF, app_mode="completion"), - account_id="account", - message_id="message", - rating=rating, - content="", - ) - if rating is None: - assert state.feedback == [] - assert state.store.events == ["write committed and closed"] - else: - assert state.feedback == [ - MessageFeedbackEvent( - tenant_id="owner-workspace", - app_id="app", - conversation_id="conversation", - message_id="message", - account_id="account", - rating=rating, - content="", - ) - ] - assert state.store.events == ["write committed and closed", "emit feedback"] - - -def test_feedback_write_error_propagates_without_emitting() -> None: - failure = MessageNotExistsError("Message was deleted") - state = _Callbacks(store=_Store(write_error=failure)) - with pytest.raises(MessageNotExistsError) as error: - state.service().set_feedback( - installed_app=_REF, account_id="account", message_id="message", rating="like", content=None - ) - assert error.value is failure - assert state.store.rating is None - assert state.feedback == [] - - @pytest.mark.parametrize("mode", ["completion", "workflow"]) def test_non_chat_admission_rejects_reads_before_database(mode: str) -> None: state = _Callbacks(store=_Store()) diff --git a/api/tests/unit_tests/services/test_message_feedback_service.py b/api/tests/unit_tests/services/test_message_feedback_service.py new file mode 100644 index 00000000000..e2e9af4942b --- /dev/null +++ b/api/tests/unit_tests/services/test_message_feedback_service.py @@ -0,0 +1,575 @@ +"""Exercise feedback policy, persistence, and telemetry through real dependencies.""" + +import json +from collections.abc import Callable, Iterator +from dataclasses import asdict, dataclass, replace +from datetime import datetime, timedelta +from decimal import Decimal +from uuid import uuid4 + +import pytest +from celery import Celery +from celery import current_app as current_celery_app +from celery.signals import before_task_publish +from sqlalchemy import Connection, Engine, event, func, inspect, select, text, update +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session, SessionTransaction, sessionmaker + +from enums import DeploymentEdition +from models import Account, App, AppMode, Conversation, EndUser, Message +from models.enums import ConversationFromSource, EndUserType, FeedbackFromSource, FeedbackRating +from models.model import InstalledApp, MessageFeedback +from repositories.message_repository import MessageRepository +from services.app_definition_query_service import AppDefinitionUnavailableError +from services.entities.message_entities import MessageAccount, MessageActor, MessageEndUser +from services.errors.message import FeedbackRatingRequiredError, MessageActorNotFoundError, MessageNotExistsError +from services.installed_app_access_service import InstalledAppNotFoundError, InstalledAppRef +from services.message_feedback_service import MessageFeedbackService + + +@dataclass +class _Harness: + service: MessageFeedbackService + app: App + account: Account + end_user: EndUser + actor: MessageActor + message: Message + factory: sessionmaker[Session] + sessions: list[Session] + committed_sessions: list[Session] + + def set_feedback(self, *, rating: FeedbackRating | None, content: str | None = None) -> None: + self.service.set_feedback( + app_id=self.app.id, + app_owner_tenant_id=self.app.tenant_id, + actor=self.actor, + message_id=self.message.id, + rating=rating, + content=content, + ) + + def assert_closed(self) -> None: + assert self.sessions + assert all(not session.in_transaction() and not session.identity_map for session in self.sessions) + + +def _message(session: Session, *, app: App, actor: MessageActor) -> Message: + account_id = actor.account_id if isinstance(actor, MessageAccount) else None + end_user_id = actor.end_user_id if isinstance(actor, MessageEndUser) else None + source = ConversationFromSource.CONSOLE if isinstance(actor, MessageAccount) else ConversationFromSource.API + conversation = Conversation( + app_id=app.id, + mode=AppMode.CHAT, + name="Feedback conversation", + _inputs={}, + from_source=source, + from_account_id=account_id, + from_end_user_id=end_user_id, + ) + session.add(conversation) + session.flush() + message = Message( + app_id=app.id, + conversation_id=conversation.id, + _inputs={}, + query="Question", + answer="Answer", + message={}, + message_unit_price=Decimal(0), + answer_unit_price=Decimal(0), + currency="USD", + from_source=source, + from_account_id=account_id, + from_end_user_id=end_user_id, + ) + session.add(message) + session.flush() + return message + + +@pytest.fixture(params=["account", "end_user"]) +def harness( + request: pytest.FixtureRequest, + sqlite_engine: Engine, + sqlite_session_factory: sessionmaker[Session], + config_overrides: Callable[..., None], +) -> _Harness: + config_overrides(DEPLOYMENT_EDITION=DeploymentEdition.COMMUNITY, ENTERPRISE_TELEMETRY_ENABLED=False) + with sqlite_session_factory.begin() as session: + app = App(tenant_id=str(uuid4()), name="Chat", mode=AppMode.CHAT, enable_site=True, enable_api=True) + account = Account(name="Reviewer", email="reviewer@example.com") + session.add_all([app, account]) + session.flush() + end_user = EndUser( + tenant_id=app.tenant_id, app_id=app.id, type=EndUserType.SERVICE_API, session_id=str(uuid4()) + ) + session.add(end_user) + session.flush() + actor: MessageActor = ( + MessageAccount(account_id=account.id) + if request.param == "account" + else MessageEndUser(end_user_id=end_user.id) + ) + message = _message(session, app=app, actor=actor) + + sessions: list[Session] = [] + committed_sessions: list[Session] = [] + factory = sessionmaker(bind=sqlite_engine, expire_on_commit=False) + + @event.listens_for(factory, "after_begin") + def record_session(session: Session, _transaction: SessionTransaction, _connection: Connection) -> None: + sessions.append(session) + + @event.listens_for(factory, "after_commit") + def record_commit(session: Session) -> None: + committed_sessions.append(session) + + return _Harness( + service=MessageFeedbackService(repository=MessageRepository(session_factory=factory)), + app=app, + account=account, + end_user=end_user, + actor=actor, + message=message, + factory=sqlite_session_factory, + sessions=sessions, + committed_sessions=committed_sessions, + ) + + +def _feedback(harness: _Harness) -> MessageFeedback | None: + with harness.factory() as session: + return session.scalar(select(MessageFeedback).where(MessageFeedback.message_id == harness.message.id)) + + +def test_create_update_and_revoke_feedback_preserve_identity_and_optional_content(harness: _Harness) -> None: + harness.set_feedback(rating=FeedbackRating.LIKE, content="") + first = _feedback(harness) + assert first is not None + assert first.app_id == harness.app.id + assert first.conversation_id == harness.message.conversation_id + assert first.rating == FeedbackRating.LIKE + assert first.content == "" + assert first.from_source == ( + FeedbackFromSource.ADMIN if isinstance(harness.actor, MessageAccount) else FeedbackFromSource.USER + ) + assert first.from_account_id == (harness.account.id if isinstance(harness.actor, MessageAccount) else None) + assert first.from_end_user_id == (harness.end_user.id if isinstance(harness.actor, MessageEndUser) else None) + + harness.set_feedback(rating=FeedbackRating.DISLIKE, content=None) + updated = _feedback(harness) + assert updated is not None + assert (updated.id, updated.rating, updated.content) == (first.id, FeedbackRating.DISLIKE, None) + assert updated.created_at == first.created_at + + harness.set_feedback(rating=None, content="Ignored when revoking") + assert _feedback(harness) is None + assert len(harness.committed_sessions) == 3 + harness.assert_closed() + + +def test_revocation_without_feedback_has_specific_error_and_does_not_commit(harness: _Harness) -> None: + with pytest.raises(FeedbackRatingRequiredError): + harness.set_feedback(rating=None) + assert _feedback(harness) is None + assert harness.committed_sessions == [] + harness.assert_closed() + + +@pytest.mark.parametrize("field", ["app_id", "from_account_id", "from_end_user_id", "from_source"]) +def test_owned_feedback_checks_every_message_owner_predicate(harness: _Harness, field: str) -> None: + value = str(uuid4()) + if field == "from_source": + value = ( + ConversationFromSource.API if isinstance(harness.actor, MessageAccount) else ConversationFromSource.CONSOLE + ) + with harness.factory.begin() as session: + session.execute(update(Message).where(Message.id == harness.message.id).values({field: value})) + with pytest.raises(MessageNotExistsError): + harness.set_feedback(rating=FeedbackRating.LIKE) + assert _feedback(harness) is None + assert harness.committed_sessions == [] + harness.assert_closed() + + +@pytest.mark.parametrize("missing", ["app", "actor", "message"]) +def test_feedback_revalidates_resources_after_admission(harness: _Harness, missing: str) -> None: + with harness.factory.begin() as session: + if missing == "app": + record = session.get(App, harness.app.id) + elif missing == "message": + record = session.get(Message, harness.message.id) + elif isinstance(harness.actor, MessageAccount): + record = session.get(Account, harness.account.id) + else: + record = session.get(EndUser, harness.end_user.id) + assert record is not None + session.delete(record) + error = { + "app": AppDefinitionUnavailableError, + "actor": MessageActorNotFoundError, + "message": MessageNotExistsError, + }[missing] + with pytest.raises(error): + harness.set_feedback(rating=FeedbackRating.LIKE) + assert harness.committed_sessions == [] + harness.assert_closed() + + +def test_feedback_rejects_an_app_from_another_tenant(harness: _Harness) -> None: + with pytest.raises(AppDefinitionUnavailableError): + harness.service.set_feedback( + app_id=harness.app.id, + app_owner_tenant_id=str(uuid4()), + actor=harness.actor, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + content=None, + ) + assert _feedback(harness) is None + assert harness.committed_sessions == [] + harness.assert_closed() + + +@pytest.mark.parametrize("field", ["app_id", "tenant_id"]) +def test_end_user_must_belong_to_both_app_and_owner_tenant(harness: _Harness, field: str) -> None: + with harness.factory.begin() as session: + session.execute(update(EndUser).where(EndUser.id == harness.end_user.id).values({field: str(uuid4())})) + with pytest.raises(MessageActorNotFoundError): + harness.service.set_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + actor=MessageEndUser(end_user_id=harness.end_user.id), + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + content=None, + ) + assert harness.committed_sessions == [] + + +def test_admin_feedback_is_app_wide_and_preserves_the_original_reviewer(harness: _Harness) -> None: + with harness.factory.begin() as session: + second_reviewer = Account(name="Another reviewer", email="second-reviewer@example.com") + session.add(second_reviewer) + session.flush() + for account_id, rating in ( + (harness.account.id, FeedbackRating.LIKE), + (second_reviewer.id, FeedbackRating.DISLIKE), + ): + harness.service.set_admin_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + account_id=account_id, + message_id=harness.message.id, + rating=rating, + content="Admin feedback", + ) + feedback = _feedback(harness) + assert feedback is not None + assert feedback.rating == FeedbackRating.DISLIKE + assert feedback.from_source == FeedbackFromSource.ADMIN + assert feedback.from_account_id == harness.account.id + assert feedback.from_end_user_id is None + with harness.factory() as session: + assert session.scalar(select(func.count()).select_from(MessageFeedback)) == 1 + harness.assert_closed() + + +def test_admin_scope_still_checks_the_message_app(harness: _Harness) -> None: + with harness.factory.begin() as session: + session.execute(update(Message).where(Message.id == harness.message.id).values(app_id=str(uuid4()))) + with pytest.raises(MessageNotExistsError): + harness.service.set_admin_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + account_id=harness.account.id, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + content=None, + ) + assert _feedback(harness) is None + assert harness.committed_sessions == [] + + +def test_user_and_admin_feedback_coexist_and_are_revoked_independently(harness: _Harness) -> None: + with harness.factory.begin() as session: + message = _message(session, app=harness.app, actor=MessageEndUser(end_user_id=harness.end_user.id)) + for actor in (MessageEndUser(end_user_id=harness.end_user.id), MessageAccount(account_id=harness.account.id)): + if isinstance(actor, MessageEndUser): + harness.service.set_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + actor=actor, + message_id=message.id, + rating=FeedbackRating.LIKE, + content="End-user feedback", + ) + else: + harness.service.set_admin_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + account_id=actor.account_id, + message_id=message.id, + rating=FeedbackRating.DISLIKE, + content="Admin feedback", + ) + harness.service.set_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + actor=MessageEndUser(end_user_id=harness.end_user.id), + message_id=message.id, + rating=None, + content=None, + ) + with harness.factory() as session: + remaining = session.scalars(select(MessageFeedback).where(MessageFeedback.message_id == message.id)).all() + assert len(remaining) == 1 + assert (remaining[0].from_source, remaining[0].rating) == (FeedbackFromSource.ADMIN, FeedbackRating.DISLIKE) + + +def _installation(harness: _Harness) -> InstalledAppRef: + with harness.factory.begin() as session: + installed = InstalledApp( + tenant_id=str(uuid4()), + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + position=0, + is_pinned=False, + ) + session.add(installed) + session.flush() + return InstalledAppRef( + id=installed.id, + tenant_id=installed.tenant_id, + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + app_mode=harness.app.mode, + ) + + +def test_cross_workspace_installation_uses_the_app_owner_and_does_not_change_usage(harness: _Harness) -> None: + installation = _installation(harness) + with harness.factory() as session: + last_used = session.scalar(select(InstalledApp.last_used_at).where(InstalledApp.id == installation.id)) + harness.service.set_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + actor=harness.actor, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + content="Shared app", + installed_app=installation, + ) + assert _feedback(harness) is not None + with harness.factory() as session: + assert session.scalar(select(InstalledApp.last_used_at).where(InstalledApp.id == installation.id)) == last_used + harness.assert_closed() + + +@pytest.mark.parametrize("mismatch", ["id", "tenant_id", "app_id", "app_owner_tenant_id", "deleted"]) +def test_installed_feedback_revalidates_installation_in_the_write_transaction(harness: _Harness, mismatch: str) -> None: + installation = _installation(harness) + if mismatch == "deleted": + with harness.factory.begin() as session: + installed = session.get(InstalledApp, installation.id) + assert installed is not None + session.delete(installed) + else: + installation = replace(installation, **{mismatch: str(uuid4())}) + with pytest.raises(InstalledAppNotFoundError): + harness.service.set_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + actor=harness.actor, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + content=None, + installed_app=installation, + ) + assert _feedback(harness) is None + assert harness.committed_sessions == [] + harness.assert_closed() + + +def test_listing_preserves_detached_fields_tied_timestamp_order_and_pagination(harness: _Harness) -> None: + timestamp = datetime(2026, 10, 9, 10, 30, 1, 123456) + with harness.factory.begin() as session: + records: list[MessageFeedback] = [] + for index in range(3): + feedback = MessageFeedback( + app_id=harness.app.id, + conversation_id=harness.message.conversation_id, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + content="" if index == 0 else None, + from_source=FeedbackFromSource.ADMIN, + from_account_id=harness.account.id, + ) + feedback.created_at = timestamp if index < 2 else timestamp - timedelta(hours=1) + feedback.updated_at = timestamp + session.add(feedback) + records.append(feedback) + # A valid foreign-app row must not change pagination or appear in results. + other_app = App( + tenant_id=harness.app.tenant_id, name="Other", mode=AppMode.CHAT, enable_site=True, enable_api=True + ) + session.add(other_app) + session.flush() + decoy_message = _message(session, app=other_app, actor=harness.actor) + decoy = MessageFeedback( + app_id=other_app.id, + conversation_id=decoy_message.conversation_id, + message_id=decoy_message.id, + rating=FeedbackRating.DISLIKE, + from_source=FeedbackFromSource.ADMIN, + from_account_id=harness.account.id, + ) + decoy.created_at = timestamp + timedelta(hours=1) + session.add(decoy) + session.flush() + expected = [ + record.to_dict() for record in sorted(records, key=lambda row: (row.created_at, row.id), reverse=True) + ] + + first = harness.service.get_feedbacks( + app_id=harness.app.id, app_owner_tenant_id=harness.app.tenant_id, page=1, limit=2 + ) + last = harness.service.get_feedbacks( + app_id=harness.app.id, app_owner_tenant_id=harness.app.tenant_id, page=2, limit=2 + ) + empty = harness.service.get_feedbacks( + app_id=harness.app.id, app_owner_tenant_id=harness.app.tenant_id, page=3, limit=2 + ) + assert [asdict(record) for record in first + last] == expected + assert len(first) == 2 + assert len(last) == 1 + assert empty == [] + assert first[0].created_at == "2026-10-09T10:30:01.123456" + assert all(inspect(record, raiseerr=False) is None for record in first + last) + assert harness.committed_sessions == [] + harness.assert_closed() + + +def test_listing_rejects_a_foreign_tenant_even_when_feedback_exists(harness: _Harness) -> None: + harness.set_feedback(rating=FeedbackRating.LIKE) + with pytest.raises(AppDefinitionUnavailableError): + harness.service.get_feedbacks(app_id=harness.app.id, app_owner_tenant_id=str(uuid4()), page=1, limit=20) + harness.assert_closed() + + +@pytest.fixture +def telemetry_broker(config_overrides: Callable[..., None]) -> Iterator[Celery]: + """Use Celery's real in-memory transport without workers or network access.""" + config_overrides(DEPLOYMENT_EDITION=DeploymentEdition.ENTERPRISE, ENTERPRISE_TELEMETRY_ENABLED=True) + restore_app = current_celery_app.set_current + broker = Celery("message-feedback-tests", broker="memory://") + broker.conf.update(task_always_eager=False, task_ignore_result=True) + try: + yield broker + finally: + broker.close() + restore_app() + + +def test_telemetry_is_published_after_commit_and_close_for_create_and_update_only( + harness: _Harness, telemetry_broker: Celery +) -> None: + assert telemetry_broker.conf.broker_url == "memory://" + published: list[tuple[object, bool, int]] = [] + + def record_publish(sender: object, body: object, **_kwargs: object) -> None: + if sender == "tasks.enterprise_telemetry_task.process_enterprise_telemetry": + closed = all(not session.in_transaction() and not session.identity_map for session in harness.sessions) + published.append((body, closed, len(harness.committed_sessions))) + + before_task_publish.connect(record_publish, weak=False) + try: + harness.set_feedback(rating=FeedbackRating.LIKE, content="First") + harness.set_feedback(rating=FeedbackRating.DISLIKE, content="Updated") + harness.set_feedback(rating=None) + harness.service.set_admin_feedback( + app_id=harness.app.id, + app_owner_tenant_id=harness.app.tenant_id, + account_id=harness.account.id, + message_id=harness.message.id, + rating=FeedbackRating.LIKE, + content="Admin route remains silent", + ) + finally: + before_task_publish.disconnect(record_publish) # type: ignore[attr-defined] # Missing from celery-types. + + assert len(published) == 2 + for index, (body, closed, commits) in enumerate(published): + assert closed + assert commits == index + 1 + assert isinstance(body, tuple) + args = body[0] + assert isinstance(args, tuple) + envelope = json.loads(args[0]) + assert envelope["tenant_id"] == harness.app.tenant_id + assert envelope["payload"] == { + "app_id": harness.app.id, + "conversation_id": harness.message.conversation_id, + "message_id": harness.message.id, + "rating": "like" if index == 0 else "dislike", + "from_source": "admin" if isinstance(harness.actor, MessageAccount) else "user", + "from_account_id": harness.account.id if isinstance(harness.actor, MessageAccount) else None, + "from_end_user_id": harness.end_user.id if isinstance(harness.actor, MessageEndUser) else None, + "content": "First" if index == 0 else "Updated", + } + harness.assert_closed() + + +def test_telemetry_transport_failure_does_not_rollback_feedback( + harness: _Harness, telemetry_broker: Celery, caplog: pytest.LogCaptureFixture +) -> None: + telemetry_broker.conf.broker_url = "nonexistent-feedback-test-transport://" + harness.set_feedback(rating=FeedbackRating.LIKE, content="Persist before telemetry") + feedback = _feedback(harness) + assert feedback is not None + assert feedback.content == "Persist before telemetry" + assert len(harness.committed_sessions) == 1 + assert "telemetry" in caplog.text.lower() + harness.assert_closed() + + +@pytest.mark.parametrize("operation", ["INSERT", "UPDATE", "DELETE"]) +def test_failed_database_write_rolls_back_without_publishing_telemetry( + harness: _Harness, telemetry_broker: Celery, operation: str +) -> None: + assert telemetry_broker.conf.broker_url == "memory://" + if operation != "INSERT": + harness.set_feedback(rating=FeedbackRating.LIKE, content="Original") + before = _feedback(harness) + harness.committed_sessions.clear() + with harness.factory.begin() as session: + # A real database error exercises flush/rollback without replacing the + # transaction or repository with a failing test implementation. + session.execute( + text( + f"CREATE TRIGGER reject_feedback_write BEFORE {operation} ON message_feedbacks " + "BEGIN SELECT RAISE(ABORT, 'feedback write rejected'); END" + ) + ) + published: list[object] = [] + + def record_publish(sender: object, **_kwargs: object) -> None: + if sender == "tasks.enterprise_telemetry_task.process_enterprise_telemetry": + published.append(sender) + + before_task_publish.connect(record_publish, weak=False) + try: + with pytest.raises(IntegrityError, match="feedback write rejected"): + harness.set_feedback(rating=None if operation == "DELETE" else FeedbackRating.DISLIKE, content="Changed") + finally: + before_task_publish.disconnect(record_publish) # type: ignore[attr-defined] # Missing from celery-types. + after = _feedback(harness) + if before is None: + assert after is None + else: + assert after is not None + assert after.to_dict() == before.to_dict() + assert harness.committed_sessions == [] + assert published == [] + harness.assert_closed() diff --git a/api/tests/unit_tests/services/test_message_service.py b/api/tests/unit_tests/services/test_message_service.py index 028087fe67d..6e24a21d128 100644 --- a/api/tests/unit_tests/services/test_message_service.py +++ b/api/tests/unit_tests/services/test_message_service.py @@ -14,8 +14,6 @@ from models.account import Account, AccountStatus from models.enums import ( ConversationFromSource, EndUserType, - FeedbackFromSource, - FeedbackRating, ) from models.model import ( App, @@ -23,7 +21,6 @@ from models.model import ( Conversation, EndUser, Message, - MessageFeedback, ) from repositories.sqlalchemy_execution_extra_content_repository import SQLAlchemyExecutionExtraContentRepository from services.errors.message import ( @@ -131,26 +128,6 @@ class MessageServiceTestDataFactory: message.updated_at = timestamp return message - @staticmethod - def create_feedback( - feedback_id: str, - message: Message, - *, - source: FeedbackFromSource, - rating: FeedbackRating = FeedbackRating.LIKE, - ) -> MessageFeedback: - feedback = MessageFeedback( - app_id=message.app_id, - conversation_id=message.conversation_id, - message_id=message.id, - rating=rating, - from_source=source, - from_end_user_id="user-456" if source == FeedbackFromSource.USER else None, - from_account_id="account-123" if source == FeedbackFromSource.ADMIN else None, - ) - feedback.id = feedback_id - return feedback - @pytest.fixture def factory() -> MessageServiceTestDataFactory: @@ -551,107 +528,3 @@ class TestMessageServiceGetMessage: message_id="missing", session=sqlite_session, ) - - -class TestMessageServiceFeedback: - def test_create_new_end_user_feedback( - self, - factory: MessageServiceTestDataFactory, - sqlite_session: Session, - sqlite_engine: Engine, - ) -> None: - user = factory.create_end_user() - message = factory.create_message("msg-123") - _persist(sqlite_session, message) - - feedback = MessageService.create_feedback( - app_model=factory.create_app(), - message_id=message.id, - user=user, - rating=FeedbackRating.LIKE, - content="Good answer", - session=sqlite_session, - ) - - with Session(sqlite_engine) as verification_session: - persisted = verification_session.get(MessageFeedback, feedback.id) - assert persisted is not None - assert persisted.rating == FeedbackRating.LIKE - assert persisted.content == "Good answer" - assert persisted.from_source == FeedbackFromSource.USER - - def test_update_account_feedback( - self, - factory: MessageServiceTestDataFactory, - sqlite_session: Session, - sqlite_engine: Engine, - ) -> None: - user = factory.create_account() - message = factory.create_message( - "msg-123", - from_source=ConversationFromSource.CONSOLE, - from_end_user_id=None, - from_account_id=user.id, - ) - feedback = factory.create_feedback("feedback-1", message, source=FeedbackFromSource.ADMIN) - _persist(sqlite_session, message, feedback) - - result = MessageService.create_feedback( - app_model=factory.create_app(), - message_id=message.id, - user=user, - rating=FeedbackRating.DISLIKE, - content="Bad answer", - session=sqlite_session, - ) - - assert result.id == feedback.id - with Session(sqlite_engine) as verification_session: - persisted = verification_session.get(MessageFeedback, feedback.id) - assert persisted is not None - assert persisted.rating == FeedbackRating.DISLIKE - assert persisted.content == "Bad answer" - - def test_delete_feedback( - self, - factory: MessageServiceTestDataFactory, - sqlite_session: Session, - sqlite_engine: Engine, - ) -> None: - user = factory.create_end_user() - message = factory.create_message("msg-123") - feedback = factory.create_feedback("feedback-1", message, source=FeedbackFromSource.USER) - _persist(sqlite_session, message, feedback) - - MessageService.create_feedback( - app_model=factory.create_app(), - message_id=message.id, - user=user, - rating=None, - content=None, - session=sqlite_session, - ) - - with Session(sqlite_engine) as verification_session: - assert verification_session.get(MessageFeedback, feedback.id) is None - - def test_get_all_feedbacks_is_app_scoped_and_paginated( - self, - factory: MessageServiceTestDataFactory, - sqlite_session: Session, - ) -> None: - message = factory.create_message("msg-123") - newest = factory.create_feedback("feedback-new", message, source=FeedbackFromSource.USER) - oldest = factory.create_feedback("feedback-old", message, source=FeedbackFromSource.USER) - other_message = factory.create_message("other-msg", app_id="app-456") - other_app = factory.create_feedback("feedback-other", other_message, source=FeedbackFromSource.USER) - newest.created_at = datetime(2024, 1, 2) - oldest.created_at = datetime(2024, 1, 1) - other_app.created_at = datetime(2024, 1, 3) - _persist(sqlite_session, newest, oldest, other_app) - - result = MessageService.get_all_messages_feedbacks( - app_model=factory.create_app(), page=1, limit=1, session=sqlite_session - ) - - assert [record["id"] for record in result] == [newest.id] diff --git a/packages/contracts/generated/api/console/agent/errors.gen.ts b/packages/contracts/generated/api/console/agent/errors.gen.ts index 430ab71899a..e4ab8194790 100644 --- a/packages/contracts/generated/api/console/agent/errors.gen.ts +++ b/packages/contracts/generated/api/console/agent/errors.gen.ts @@ -194,6 +194,14 @@ const zAppSuggestedQuestionsAfterAnswerDisabledError = /* @__PURE__ */ (() => }), ))() +const zMessageFeedbackRatingRequiredError = /* @__PURE__ */ (() => + zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), + ))() + const zAgentModelNotConfiguredError = /* @__PURE__ */ (() => zErrorResponse.and( z.object({ @@ -514,7 +522,11 @@ export const post12Errors = { export const post13Errors = { agent_not_found_error: { status: 404, data: zAgentNotFoundError } } -export const post14Errors = { agent_not_found_error: { status: 404, data: zAgentNotFoundError } } +export const post14Errors = { + agent_not_found_error: { status: 404, data: zAgentNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, +} export const get19Errors = { agent_not_found_error: { status: 404, data: zAgentNotFoundError } } diff --git a/packages/contracts/generated/api/console/agent/orpc.gen.ts b/packages/contracts/generated/api/console/agent/orpc.gen.ts index 5ae49bd98bd..3c0eb62c2e6 100644 --- a/packages/contracts/generated/api/console/agent/orpc.gen.ts +++ b/packages/contracts/generated/api/console/agent/orpc.gen.ts @@ -23,6 +23,7 @@ import { zInvalidComposerConfigError, zInvalidTracingConfigError, zMaxKeysExceededError, + zMessageFeedbackRatingRequiredError, zModelCurrentlyNotSupportError, zNoActiveBindingError, zNoAudioUploadedError, @@ -1070,7 +1071,11 @@ export const post14 = oc z.object({ body: zPostAgentByAgentIdFeedbacksBody, params: zPostAgentByAgentIdFeedbacksPath }), ) .output(zPostAgentByAgentIdFeedbacksResponse) - .errors({ agent_not_found_error: { status: 404, data: zAgentNotFoundError } }) + .errors({ + agent_not_found_error: { status: 404, data: zAgentNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, + }) export const feedbacks = { post: post14, diff --git a/packages/contracts/generated/api/console/agent/types.gen.ts b/packages/contracts/generated/api/console/agent/types.gen.ts index ec9f4e4cdce..17dc43a42ec 100644 --- a/packages/contracts/generated/api/console/agent/types.gen.ts +++ b/packages/contracts/generated/api/console/agent/types.gen.ts @@ -434,6 +434,11 @@ export type AppMessageFeedbackPayload = { rating?: 'dislike' | 'like' | null } +export type MessageFeedbackRatingRequiredError = ErrorResponse & { + code: 'message_feedback_rating_required' + status: 400 +} + export type AgentLogSourceListResponse = { data: Array groups: Array @@ -3793,6 +3798,7 @@ export type PostAgentByAgentIdFeedbacksData = { } export type PostAgentByAgentIdFeedbacksErrors = { + 400: AppUnavailableError | MessageFeedbackRatingRequiredError | ErrorResponse 404: AgentNotFoundError | ErrorResponse default: ErrorResponse } diff --git a/packages/contracts/generated/api/console/agent/zod.gen.ts b/packages/contracts/generated/api/console/agent/zod.gen.ts index 576127e7804..467920194c2 100644 --- a/packages/contracts/generated/api/console/agent/zod.gen.ts +++ b/packages/contracts/generated/api/console/agent/zod.gen.ts @@ -447,6 +447,18 @@ export const zAppMessageFeedbackPayload = z.object({ rating: z.enum(['dislike', 'like']).nullish(), }) +/** + * MessageFeedbackRatingRequiredError + * + * A rating is required when there is no existing feedback to remove. + */ +export const zMessageFeedbackRatingRequiredError = zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), +) + /** * AgentPublishPayload */ diff --git a/packages/contracts/generated/api/console/apps/errors.gen.ts b/packages/contracts/generated/api/console/apps/errors.gen.ts index bddd4453114..ff8103364bb 100644 --- a/packages/contracts/generated/api/console/apps/errors.gen.ts +++ b/packages/contracts/generated/api/console/apps/errors.gen.ts @@ -210,6 +210,14 @@ const zRosterAgentPackageExportFailedError = /* @__PURE__ */ (() => }), ))() +const zMessageFeedbackRatingRequiredError = /* @__PURE__ */ (() => + zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), + ))() + const zProviderNotSupportTextToSpeechError = /* @__PURE__ */ (() => zErrorResponse.and( z.object({ @@ -662,7 +670,11 @@ export const get31Errors = { roster_agent_package_export_failed: { status: 500, data: zRosterAgentPackageExportFailedError }, } -export const post23Errors = { app_not_found: { status: 404, data: zAppNotFoundError } } +export const post23Errors = { + app_not_found: { status: 404, data: zAppNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, +} export const get32Errors = { app_not_found: { status: 404, data: zAppNotFoundError } } diff --git a/packages/contracts/generated/api/console/apps/orpc.gen.ts b/packages/contracts/generated/api/console/apps/orpc.gen.ts index ee740b62c22..756b0a92fac 100644 --- a/packages/contracts/generated/api/console/apps/orpc.gen.ts +++ b/packages/contracts/generated/api/console/apps/orpc.gen.ts @@ -26,6 +26,7 @@ import { zInvalidRosterAgentPackageError, zInvalidTracingConfigError, zMaxKeysExceededError, + zMessageFeedbackRatingRequiredError, zModelCurrentlyNotSupportError, zNoActiveBindingError, zNoAudioUploadedError, @@ -2068,7 +2069,11 @@ export const post23 = oc }) .input(z.object({ body: zPostAppsByAppIdFeedbacksBody, params: zPostAppsByAppIdFeedbacksPath })) .output(zPostAppsByAppIdFeedbacksResponse) - .errors({ app_not_found: { status: 404, data: zAppNotFoundError } }) + .errors({ + app_not_found: { status: 404, data: zAppNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, + }) export const feedbacks = { post: post23, diff --git a/packages/contracts/generated/api/console/apps/types.gen.ts b/packages/contracts/generated/api/console/apps/types.gen.ts index b95f3668601..b436c94748e 100644 --- a/packages/contracts/generated/api/console/apps/types.gen.ts +++ b/packages/contracts/generated/api/console/apps/types.gen.ts @@ -634,6 +634,11 @@ export type AppMessageFeedbackPayload = { rating?: 'dislike' | 'like' | null } +export type MessageFeedbackRatingRequiredError = ErrorResponse & { + code: 'message_feedback_rating_required' + status: 400 +} + export type AppIconPayload = { icon?: string | null icon_background?: string | null @@ -6467,6 +6472,8 @@ export type PostAppsByAppIdFeedbacksData = { } export type PostAppsByAppIdFeedbacksErrors = { + 400: AppUnavailableError | MessageFeedbackRatingRequiredError | ErrorResponse + 403: ErrorResponse 404: AppNotFoundError | ErrorResponse default: ErrorResponse } diff --git a/packages/contracts/generated/api/console/apps/zod.gen.ts b/packages/contracts/generated/api/console/apps/zod.gen.ts index 11876aa3b98..f148fcb1f80 100644 --- a/packages/contracts/generated/api/console/apps/zod.gen.ts +++ b/packages/contracts/generated/api/console/apps/zod.gen.ts @@ -619,6 +619,18 @@ export const zAppMessageFeedbackPayload = z.object({ rating: z.enum(['dislike', 'like']).nullish(), }) +/** + * MessageFeedbackRatingRequiredError + * + * A rating is required when there is no existing feedback to remove. + */ +export const zMessageFeedbackRatingRequiredError = zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), +) + /** * AppNamePayload */ diff --git a/packages/contracts/generated/api/console/installed-apps/errors.gen.ts b/packages/contracts/generated/api/console/installed-apps/errors.gen.ts index 50020e98a49..15ef763a0f3 100644 --- a/packages/contracts/generated/api/console/installed-apps/errors.gen.ts +++ b/packages/contracts/generated/api/console/installed-apps/errors.gen.ts @@ -353,6 +353,7 @@ export const getErrors = { export const post7Errors = { access_denied: { status: 403, data: zAccessDeniedError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, installed_app_not_found: { status: 404, data: zInstalledAppNotFoundError }, message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, message_not_found: { status: 404, data: zMessageNotFoundError }, diff --git a/packages/contracts/generated/api/console/installed-apps/orpc.gen.ts b/packages/contracts/generated/api/console/installed-apps/orpc.gen.ts index 1ee7bfb5e8e..394fb30b7fe 100644 --- a/packages/contracts/generated/api/console/installed-apps/orpc.gen.ts +++ b/packages/contracts/generated/api/console/installed-apps/orpc.gen.ts @@ -395,6 +395,7 @@ export const post7 = oc .output(zPostInstalledAppsByInstalledAppIdMessagesByMessageIdFeedbacksResponse) .errors({ access_denied: { status: 403, data: zAccessDeniedError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, installed_app_not_found: { status: 404, data: zInstalledAppNotFoundError }, message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, message_not_found: { status: 404, data: zMessageNotFoundError }, diff --git a/packages/contracts/generated/api/console/installed-apps/types.gen.ts b/packages/contracts/generated/api/console/installed-apps/types.gen.ts index 647705eb0b9..b799a754a63 100644 --- a/packages/contracts/generated/api/console/installed-apps/types.gen.ts +++ b/packages/contracts/generated/api/console/installed-apps/types.gen.ts @@ -1415,7 +1415,7 @@ export type PostInstalledAppsByInstalledAppIdMessagesByMessageIdFeedbacksData = } export type PostInstalledAppsByInstalledAppIdMessagesByMessageIdFeedbacksErrors = { - 400: MessageFeedbackRatingRequiredError | ErrorResponse + 400: AppUnavailableError | MessageFeedbackRatingRequiredError | ErrorResponse 403: AccessDeniedError | ErrorResponse 404: InstalledAppNotFoundError | MessageNotFoundError | ErrorResponse 503: WebAppAccessUnavailableError | ErrorResponse diff --git a/packages/contracts/generated/api/service/errors.gen.ts b/packages/contracts/generated/api/service/errors.gen.ts index 7e91269a1b1..e60f394d895 100644 --- a/packages/contracts/generated/api/service/errors.gen.ts +++ b/packages/contracts/generated/api/service/errors.gen.ts @@ -266,6 +266,14 @@ const zKnowledgeFsUpstreamUnavailableError = /* @__PURE__ */ (() => }), ))() +const zMessageFeedbackRatingRequiredError = /* @__PURE__ */ (() => + zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), + ))() + const zModelCurrentlyNotSupportError = /* @__PURE__ */ (() => zErrorResponse.and( z.object({ @@ -410,6 +418,15 @@ const zWorkspaceNotFoundError = /* @__PURE__ */ (() => }), ))() +export const get2Errors = { + app_abnormal_status: { status: 403, data: zAppAbnormalStatusError }, + app_api_disabled: { status: 403, data: zAppApiDisabledError }, + app_not_found: { status: 403, data: zAppNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + workspace_archived: { status: 403, data: zWorkspaceArchivedError }, + workspace_not_found: { status: 403, data: zWorkspaceNotFoundError }, +} + export const post3Errors = { app_unavailable: { status: 400, data: zAppUnavailableError }, audio_too_large: { status: 413, data: zAudioTooLargeError }, @@ -1241,6 +1258,16 @@ export const get60Errors = { knowledge_fs_upstream_unavailable: { status: 502, data: zKnowledgeFsUpstreamUnavailableError }, } +export const post58Errors = { + app_abnormal_status: { status: 403, data: zAppAbnormalStatusError }, + app_api_disabled: { status: 403, data: zAppApiDisabledError }, + app_not_found: { status: 403, data: zAppNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, + workspace_archived: { status: 403, data: zWorkspaceArchivedError }, + workspace_not_found: { status: 403, data: zWorkspaceNotFoundError }, +} + export const get61Errors = { agent_version_not_found_error: { status: 404, data: zAgentVersionNotFoundError }, app_abnormal_status: { status: 403, data: zAppAbnormalStatusError }, diff --git a/packages/contracts/generated/api/service/orpc.gen.ts b/packages/contracts/generated/api/service/orpc.gen.ts index 710791dd301..8b45b974fba 100644 --- a/packages/contracts/generated/api/service/orpc.gen.ts +++ b/packages/contracts/generated/api/service/orpc.gen.ts @@ -35,6 +35,7 @@ import { zKnowledgeFsRequestTooLargeError, zKnowledgeFsResourceNotFoundError, zKnowledgeFsUpstreamUnavailableError, + zMessageFeedbackRatingRequiredError, zModelCurrentlyNotSupportError, zNoAudioUploadedError, zNoFileUploadedError, @@ -497,6 +498,14 @@ export const get2 = oc }) .input(z.object({ query: zGetAppFeedbacksQuery.optional() }).optional()) .output(zGetAppFeedbacksResponse) + .errors({ + app_abnormal_status: { status: 403, data: zAppAbnormalStatusError }, + app_api_disabled: { status: 403, data: zAppApiDisabledError }, + app_not_found: { status: 403, data: zAppNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + workspace_archived: { status: 403, data: zWorkspaceArchivedError }, + workspace_not_found: { status: 403, data: zWorkspaceNotFoundError }, + }) export const feedbacks = { get: get2, @@ -4728,6 +4737,15 @@ export const post58 = oc }), ) .output(zPostMessagesByMessageIdFeedbacksResponse) + .errors({ + app_abnormal_status: { status: 403, data: zAppAbnormalStatusError }, + app_api_disabled: { status: 403, data: zAppApiDisabledError }, + app_not_found: { status: 403, data: zAppNotFoundError }, + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, + workspace_archived: { status: 403, data: zWorkspaceArchivedError }, + workspace_not_found: { status: 403, data: zWorkspaceNotFoundError }, + }) export const feedbacks2 = { post: post58, diff --git a/packages/contracts/generated/api/service/types.gen.ts b/packages/contracts/generated/api/service/types.gen.ts index b81cccf632d..8d71ef18dbb 100644 --- a/packages/contracts/generated/api/service/types.gen.ts +++ b/packages/contracts/generated/api/service/types.gen.ts @@ -3269,6 +3269,11 @@ export type MessageFeedbackPayloadWithUser = { user: string } +export type MessageFeedbackRatingRequiredError = ErrorResponse & { + code: 'message_feedback_rating_required' + status: 400 +} + export type MessageFile = { belongs_to?: string | null filename: string @@ -4566,8 +4571,15 @@ export type GetAppFeedbacksData = { } export type GetAppFeedbacksErrors = { + 400: AppUnavailableError | ErrorResponse 401: ErrorResponse - 403: ErrorResponse + 403: + | AppAbnormalStatusError + | AppApiDisabledError + | AppNotFoundError + | WorkspaceArchivedError + | WorkspaceNotFoundError + | ErrorResponse default: ErrorResponse } @@ -8827,9 +8839,15 @@ export type PostMessagesByMessageIdFeedbacksData = { } export type PostMessagesByMessageIdFeedbacksErrors = { - 400: ErrorResponse + 400: AppUnavailableError | MessageFeedbackRatingRequiredError | ErrorResponse 401: ErrorResponse - 403: ErrorResponse + 403: + | AppAbnormalStatusError + | AppApiDisabledError + | AppNotFoundError + | WorkspaceArchivedError + | WorkspaceNotFoundError + | ErrorResponse 404: ErrorResponse default: ErrorResponse } diff --git a/packages/contracts/generated/api/service/zod.gen.ts b/packages/contracts/generated/api/service/zod.gen.ts index 83d99840adf..436c200486a 100644 --- a/packages/contracts/generated/api/service/zod.gen.ts +++ b/packages/contracts/generated/api/service/zod.gen.ts @@ -3961,6 +3961,18 @@ export const zMessageFeedbackPayloadWithUser = z.object({ user: z.string(), }) +/** + * MessageFeedbackRatingRequiredError + * + * A rating is required when there is no existing feedback to remove. + */ +export const zMessageFeedbackRatingRequiredError = zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), +) + /** * MessageFile */ diff --git a/packages/contracts/generated/api/web/errors.gen.ts b/packages/contracts/generated/api/web/errors.gen.ts index f8f8be7a441..0dcf2bf9e56 100644 --- a/packages/contracts/generated/api/web/errors.gen.ts +++ b/packages/contracts/generated/api/web/errors.gen.ts @@ -194,6 +194,14 @@ const zMalformedUploadTokenError = /* @__PURE__ */ (() => }), ))() +const zMessageFeedbackRatingRequiredError = /* @__PURE__ */ (() => + zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), + ))() + const zModelCurrentlyNotSupportError = /* @__PURE__ */ (() => zErrorResponse.and( z.object({ @@ -524,6 +532,11 @@ export const post16Errors = { authentication_failed: { status: 401, data: zAuthenticationFailedError }, } +export const post18Errors = { + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, +} + export const get4Errors = { app_more_like_this_disabled: { status: 403, data: zAppMoreLikeThisDisabledError }, app_unavailable: { status: 400, data: zAppUnavailableError }, diff --git a/packages/contracts/generated/api/web/orpc.gen.ts b/packages/contracts/generated/api/web/orpc.gen.ts index c8a28383efd..e1dfa5214b9 100644 --- a/packages/contracts/generated/api/web/orpc.gen.ts +++ b/packages/contracts/generated/api/web/orpc.gen.ts @@ -26,6 +26,7 @@ import { zInvalidParamError, zInvalidUploadTokenError, zMalformedUploadTokenError, + zMessageFeedbackRatingRequiredError, zModelCurrentlyNotSupportError, zNoAudioUploadedError, zNoFileUploadedError, @@ -804,6 +805,10 @@ export const post18 = oc }), ) .output(zPostMessagesByMessageIdFeedbacksResponse) + .errors({ + app_unavailable: { status: 400, data: zAppUnavailableError }, + message_feedback_rating_required: { status: 400, data: zMessageFeedbackRatingRequiredError }, + }) export const feedbacks = { post: post18, diff --git a/packages/contracts/generated/api/web/types.gen.ts b/packages/contracts/generated/api/web/types.gen.ts index 349c98900a0..f25aba7e230 100644 --- a/packages/contracts/generated/api/web/types.gen.ts +++ b/packages/contracts/generated/api/web/types.gen.ts @@ -466,6 +466,11 @@ export type MessageFeedbackPayload = { rating?: 'dislike' | 'like' | null } +export type MessageFeedbackRatingRequiredError = ErrorResponse & { + code: 'message_feedback_rating_required' + status: 400 +} + export type MessageFile = { belongs_to?: string | null filename: string @@ -1973,8 +1978,9 @@ export type PostMessagesByMessageIdFeedbacksData = { } export type PostMessagesByMessageIdFeedbacksErrors = { - 400: ErrorResponse + 400: AppUnavailableError | MessageFeedbackRatingRequiredError | ErrorResponse 401: ErrorResponse + 403: ErrorResponse 404: ErrorResponse 500: ErrorResponse default: ErrorResponse diff --git a/packages/contracts/generated/api/web/zod.gen.ts b/packages/contracts/generated/api/web/zod.gen.ts index 3812870c630..5ea48cd0f0e 100644 --- a/packages/contracts/generated/api/web/zod.gen.ts +++ b/packages/contracts/generated/api/web/zod.gen.ts @@ -696,6 +696,18 @@ export const zMessageFeedbackPayload = z.object({ rating: z.enum(['dislike', 'like']).nullish(), }) +/** + * MessageFeedbackRatingRequiredError + * + * A rating is required when there is no existing feedback to remove. + */ +export const zMessageFeedbackRatingRequiredError = zErrorResponse.and( + z.object({ + code: z.literal('message_feedback_rating_required'), + status: z.literal(400), + }), +) + /** * MessageFile */