diff --git a/api/tests/test_containers_integration_tests/.ruff.toml b/api/tests/test_containers_integration_tests/.ruff.toml index 11088d4867..250cf103ab 100644 --- a/api/tests/test_containers_integration_tests/.ruff.toml +++ b/api/tests/test_containers_integration_tests/.ruff.toml @@ -10,7 +10,6 @@ extend-select = ["ANN401", "ARG", "TID251"] "services/test_app_dsl_service.py" = ["ANN401", "TID251", "ARG"] "services/test_file_service_zip_and_lookup.py" = ["ANN401", "TID251", "ARG"] "services/test_hit_testing_service.py" = ["ANN401", "TID251"] -"services/test_recommended_app_service.py" = ["ANN401", "TID251", "ARG"] "trigger/conftest.py" = ["ANN401", "TID251"] "trigger/test_trigger_e2e.py" = ["ANN401", "TID251", "ARG"] "controllers/console/app/test_app_apis.py" = ["ARG"] diff --git a/api/tests/test_containers_integration_tests/pyrefly.toml b/api/tests/test_containers_integration_tests/pyrefly.toml index 36d83da43e..06ea10036f 100644 --- a/api/tests/test_containers_integration_tests/pyrefly.toml +++ b/api/tests/test_containers_integration_tests/pyrefly.toml @@ -114,7 +114,6 @@ project-excludes = [ "services/test_model_provider_service.py", "services/test_oauth_server_service.py", "services/test_ops_service.py", - "services/test_recommended_app_service.py", "services/test_restore_archived_workflow_run.py", "services/test_saved_message_service.py", "services/test_schedule_service.py", diff --git a/api/tests/test_containers_integration_tests/services/test_recommended_app_service.py b/api/tests/test_containers_integration_tests/services/test_recommended_app_service.py index ac556a6c79..3c7ea311da 100644 --- a/api/tests/test_containers_integration_tests/services/test_recommended_app_service.py +++ b/api/tests/test_containers_integration_tests/services/test_recommended_app_service.py @@ -2,7 +2,7 @@ from __future__ import annotations import uuid from types import SimpleNamespace -from typing import Any, cast +from typing import TypedDict, Unpack, cast from unittest.mock import MagicMock, patch import pytest @@ -13,13 +13,40 @@ from models.model import AccountTrialAppRecord, TrialApp from services import recommended_app_service as service_module from services.recommended_app_service import RecommendedAppService + +class RecommendedAppPayload(TypedDict, total=False): + id: str + app_id: str + name: str + description: str + category: str + icon: str + model_config: object + workflows: list[str] + tools: list[str] + can_trial: bool + + +class AppsResponse(TypedDict): + recommended_apps: list[RecommendedAppPayload] | None + categories: list[str] + + +class AppDetailKwargs(TypedDict, total=False): + category: str + icon: str + model_config: object + workflows: list[str] + tools: list[str] + + # ── Helpers ──────────────────────────────────────────────────────────── def _apps_response( - recommended_apps: list[dict] | None = None, + recommended_apps: list[RecommendedAppPayload] | None = None, categories: list[str] | None = None, -) -> dict: +) -> AppsResponse: if recommended_apps is None: recommended_apps = [ {"id": "app-1", "name": "Test App 1", "description": "d1", "category": "productivity"}, @@ -34,30 +61,26 @@ def _app_detail( app_id: str = "app-123", name: str = "Test App", description: str = "Test description", - **kwargs: Any, -) -> dict: - detail: dict[str, Any] = { - "id": app_id, - "name": name, - "description": description, - "category": kwargs.get("category", "productivity"), - "icon": kwargs.get("icon", "🚀"), - "model_config": kwargs.get("model_config", {}), - } - detail.update(kwargs) + **kwargs: Unpack[AppDetailKwargs], +) -> RecommendedAppPayload: + detail = RecommendedAppPayload( + id=app_id, + name=name, + description=description, + category=kwargs.get("category", "productivity"), + icon=kwargs.get("icon", "🚀"), + model_config=kwargs.get("model_config", {}), + ) + detail.update(**kwargs) return detail -def _recommendation_detail(result: dict[str, Any] | None) -> dict[str, Any] | None: - return cast("dict[str, Any] | None", result) - - def _mock_factory_for_apps( monkeypatch: pytest.MonkeyPatch, *, mode: str, - result: dict[str, Any], - fallback_result: dict[str, Any] | None = None, + result: AppsResponse, + fallback_result: AppsResponse | None = None, ) -> tuple[MagicMock, MagicMock]: retrieval_instance = MagicMock() retrieval_instance.get_recommended_apps_and_categories.return_value = result @@ -85,7 +108,7 @@ def _mock_factory_for_apps( class TestRecommendedAppServiceGetApps: @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) @patch("services.recommended_app_service.dify_config") - def test_success_with_apps(self, mock_config, mock_factory_class): + def test_success_with_apps(self, mock_config: MagicMock, mock_factory_class: MagicMock) -> None: mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" expected = _apps_response() @@ -104,9 +127,9 @@ class TestRecommendedAppServiceGetApps: @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) @patch("services.recommended_app_service.dify_config") - def test_fallback_to_builtin_when_empty(self, mock_config, mock_factory_class): + def test_fallback_to_builtin_when_empty(self, mock_config: MagicMock, mock_factory_class: MagicMock) -> None: mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" - empty_response = {"recommended_apps": [], "categories": []} + empty_response = AppsResponse(recommended_apps=[], categories=[]) builtin_response = _apps_response( recommended_apps=[{"id": "builtin-1", "name": "Builtin App", "category": "default"}] ) @@ -127,9 +150,9 @@ class TestRecommendedAppServiceGetApps: @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) @patch("services.recommended_app_service.dify_config") - def test_fallback_when_none_recommended_apps(self, mock_config, mock_factory_class): + def test_fallback_when_none_recommended_apps(self, mock_config: MagicMock, mock_factory_class: MagicMock) -> None: mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "db" - none_response = {"recommended_apps": None, "categories": ["test"]} + none_response = AppsResponse(recommended_apps=None, categories=["test"]) builtin_response = _apps_response() mock_db_instance = MagicMock() @@ -147,7 +170,7 @@ class TestRecommendedAppServiceGetApps: @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) @patch("services.recommended_app_service.dify_config") - def test_different_languages(self, mock_config, mock_factory_class): + def test_different_languages(self, mock_config: MagicMock, mock_factory_class: MagicMock) -> None: mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "builtin" for language in ["en-US", "zh-CN", "ja-JP", "fr-FR"]: @@ -165,7 +188,7 @@ class TestRecommendedAppServiceGetApps: @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) @patch("services.recommended_app_service.dify_config") - def test_uses_correct_factory_mode(self, mock_config, mock_factory_class): + def test_uses_correct_factory_mode(self, mock_config: MagicMock, mock_factory_class: MagicMock) -> None: for mode in ["remote", "builtin", "db"]: mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = mode response = _apps_response() @@ -182,25 +205,49 @@ class TestRecommendedAppServiceGetApps: class TestRecommendedAppServiceGetDetail: + @patch("services.recommended_app_service.FeatureService", autospec=True) @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) @patch("services.recommended_app_service.dify_config") - def test_success(self, mock_config, mock_factory_class): + def test_returns_retrieval_detail_when_trial_disabled( + self, mock_config: MagicMock, mock_factory_class: MagicMock, mock_feature_service: MagicMock + ) -> None: mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" - expected = _app_detail(app_id="app-123", name="Productivity App", description="A great app") + mock_feature_service.get_system_features.return_value = SimpleNamespace(enable_trial_app=False) + cases: list[tuple[str, RecommendedAppPayload]] = [ + ( + "complex-app", + _app_detail( + app_id="complex-app", + name="Complex App", + model_config={ + "provider": "openai", + "model": "gpt-4", + "parameters": {"temperature": 0.7, "max_tokens": 2000, "top_p": 1.0}, + }, + workflows=["workflow-1", "workflow-2"], + tools=["tool-1", "tool-2", "tool-3"], + ), + ), + ("app-empty", RecommendedAppPayload()), + ] - mock_instance = MagicMock() - mock_instance.get_recommend_app_detail.return_value = expected - mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) + for app_id, expected in cases: + mock_instance = MagicMock() + mock_instance.get_recommend_app_detail.return_value = expected + mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) - result = _recommendation_detail(RecommendedAppService.get_recommend_app_detail("app-123")) + result = RecommendedAppService.get_recommend_app_detail(app_id) - assert result == expected - assert result["id"] == "app-123" - mock_instance.get_recommend_app_detail.assert_called_once_with("app-123") + assert result == expected + mock_instance.get_recommend_app_detail.assert_called_once_with(app_id) + @patch("services.recommended_app_service.FeatureService", autospec=True) @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) @patch("services.recommended_app_service.dify_config") - def test_different_modes(self, mock_config, mock_factory_class): + def test_different_modes( + self, mock_config: MagicMock, mock_factory_class: MagicMock, mock_feature_service: MagicMock + ) -> None: + mock_feature_service.get_system_features.return_value = SimpleNamespace(enable_trial_app=False) for mode in ["remote", "builtin", "db"]: mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = mode detail = _app_detail(app_id="test-app", name=f"App from {mode}") @@ -208,71 +255,19 @@ class TestRecommendedAppServiceGetDetail: mock_instance.get_recommend_app_detail.return_value = detail mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) - result = _recommendation_detail(RecommendedAppService.get_recommend_app_detail("test-app")) + result = RecommendedAppService.get_recommend_app_detail("test-app") - assert result["name"] == f"App from {mode}" + assert result is not None + mock_instance.get_recommend_app_detail.assert_called_with("test-app") mock_factory_class.get_recommend_app_factory.assert_called_with(mode) - @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) - @patch("services.recommended_app_service.dify_config") - def test_returns_none_when_not_found(self, mock_config, mock_factory_class): - mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" - mock_instance = MagicMock() - mock_instance.get_recommend_app_detail.return_value = None - mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) - - result = _recommendation_detail(RecommendedAppService.get_recommend_app_detail("nonexistent")) - - assert result is None - mock_instance.get_recommend_app_detail.assert_called_once_with("nonexistent") - - @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) - @patch("services.recommended_app_service.dify_config") - def test_returns_empty_dict(self, mock_config, mock_factory_class): - mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "builtin" - mock_instance = MagicMock() - mock_instance.get_recommend_app_detail.return_value = {} - mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) - - result = _recommendation_detail(RecommendedAppService.get_recommend_app_detail("app-empty")) - - assert result == {} - - @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) - @patch("services.recommended_app_service.dify_config") - def test_complex_model_config(self, mock_config, mock_factory_class): - mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" - complex_config = { - "provider": "openai", - "model": "gpt-4", - "parameters": {"temperature": 0.7, "max_tokens": 2000, "top_p": 1.0}, - } - expected = _app_detail( - app_id="complex-app", - name="Complex App", - model_config=complex_config, - workflows=["workflow-1", "workflow-2"], - tools=["tool-1", "tool-2", "tool-3"], - ) - mock_instance = MagicMock() - mock_instance.get_recommend_app_detail.return_value = expected - mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) - - result = _recommendation_detail(RecommendedAppService.get_recommend_app_detail("complex-app")) - - assert result["model_config"] == complex_config - assert len(result["workflows"]) == 2 - assert len(result["tools"]) == 3 - # ── Integration tests: trial app features (real DB) ──────────────────── class TestRecommendedAppServiceTrialFeatures: - def test_get_apps_should_not_query_trial_table_when_disabled( - self, db_session_with_containers: Session, monkeypatch: pytest.MonkeyPatch - ): - expected = {"recommended_apps": [{"app_id": "app-1"}], "categories": ["all"]} + def test_get_apps_should_not_query_trial_table_when_disabled(self, monkeypatch: pytest.MonkeyPatch) -> None: + expected = AppsResponse(recommended_apps=[RecommendedAppPayload(app_id="app-1")], categories=["all"]) retrieval_instance, builtin_instance = _mock_factory_for_apps(monkeypatch, mode="remote", result=expected) monkeypatch.setattr( service_module.FeatureService, @@ -288,7 +283,7 @@ class TestRecommendedAppServiceTrialFeatures: def test_get_apps_should_enrich_can_trial_when_enabled( self, db_session_with_containers: Session, monkeypatch: pytest.MonkeyPatch - ): + ) -> None: app_id_1 = str(uuid.uuid4()) app_id_2 = str(uuid.uuid4()) tenant_id = str(uuid.uuid4()) @@ -297,11 +292,11 @@ class TestRecommendedAppServiceTrialFeatures: db_session_with_containers.add(TrialApp(app_id=app_id_1, tenant_id=tenant_id)) db_session_with_containers.commit() - remote_result = {"recommended_apps": [], "categories": []} - fallback_result = { - "recommended_apps": [{"app_id": app_id_1}, {"app_id": app_id_2}], - "categories": ["all"], - } + remote_result = AppsResponse(recommended_apps=[], categories=[]) + fallback_result = AppsResponse( + recommended_apps=[RecommendedAppPayload(app_id=app_id_1), RecommendedAppPayload(app_id=app_id_2)], + categories=["all"], + ) _, builtin_instance = _mock_factory_for_apps( monkeypatch, mode="remote", result=remote_result, fallback_result=fallback_result ) @@ -323,7 +318,7 @@ class TestRecommendedAppServiceTrialFeatures: db_session_with_containers: Session, monkeypatch: pytest.MonkeyPatch, has_trial_app: bool, - ): + ) -> None: app_id = str(uuid.uuid4()) tenant_id = str(uuid.uuid4()) @@ -331,7 +326,7 @@ class TestRecommendedAppServiceTrialFeatures: db_session_with_containers.add(TrialApp(app_id=app_id, tenant_id=tenant_id)) db_session_with_containers.commit() - detail = {"id": app_id, "name": "Test App"} + detail = RecommendedAppPayload(id=app_id, name="Test App") retrieval_instance = MagicMock() retrieval_instance.get_recommend_app_detail.return_value = detail retrieval_factory = MagicMock(return_value=retrieval_instance) @@ -347,38 +342,34 @@ class TestRecommendedAppServiceTrialFeatures: MagicMock(return_value=SimpleNamespace(enable_trial_app=True)), ) - result = cast(dict[str, Any], RecommendedAppService.get_recommend_app_detail(app_id)) + result = RecommendedAppService.get_recommend_app_detail(app_id) + assert result is not None + detail_result = cast(RecommendedAppPayload, result) - assert result["id"] == app_id - assert result["can_trial"] is has_trial_app + assert detail_result["id"] == app_id + assert detail_result["can_trial"] is has_trial_app - def test_get_detail_returns_none_when_not_found_and_trial_enabled( + @patch("services.recommended_app_service.FeatureService", autospec=True) + @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) + @patch("services.recommended_app_service.dify_config") + def test_get_detail_returns_none_before_reading_trial_flag( self, - db_session_with_containers: Session, - monkeypatch: pytest.MonkeyPatch, - ): - """Regression: accessing result['id'] when result is None must not crash.""" - retrieval_instance = MagicMock() - retrieval_instance.get_recommend_app_detail.return_value = None - retrieval_factory = MagicMock(return_value=retrieval_instance) - monkeypatch.setattr(service_module.dify_config, "HOSTED_FETCH_APP_TEMPLATES_MODE", "remote", raising=False) - monkeypatch.setattr( - service_module.RecommendAppRetrievalFactory, - "get_recommend_app_factory", - MagicMock(return_value=retrieval_factory), - ) - monkeypatch.setattr( - service_module.FeatureService, - "get_system_features", - MagicMock(return_value=SimpleNamespace(enable_trial_app=True)), - ) + mock_config: MagicMock, + mock_factory_class: MagicMock, + mock_feature_service: MagicMock, + ) -> None: + mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" + mock_instance = MagicMock() + mock_instance.get_recommend_app_detail.return_value = None + mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) result = RecommendedAppService.get_recommend_app_detail("nonexistent") assert result is None - retrieval_instance.get_recommend_app_detail.assert_called_once_with("nonexistent") + mock_instance.get_recommend_app_detail.assert_called_once_with("nonexistent") + mock_feature_service.get_system_features.assert_not_called() - def test_add_trial_app_record_increments_count_for_existing(self, db_session_with_containers: Session): + def test_add_trial_app_record_increments_count_for_existing(self, db_session_with_containers: Session) -> None: app_id = str(uuid.uuid4()) account_id = str(uuid.uuid4()) @@ -396,7 +387,7 @@ class TestRecommendedAppServiceTrialFeatures: assert record is not None assert record.count == 4 - def test_add_trial_app_record_creates_new_record(self, db_session_with_containers: Session): + def test_add_trial_app_record_creates_new_record(self, db_session_with_containers: Session) -> None: app_id = str(uuid.uuid4()) account_id = str(uuid.uuid4()) diff --git a/api/tests/unit_tests/services/test_recommended_app_service.py b/api/tests/unit_tests/services/test_recommended_app_service.py deleted file mode 100644 index 980b8291e2..0000000000 --- a/api/tests/unit_tests/services/test_recommended_app_service.py +++ /dev/null @@ -1,46 +0,0 @@ -"""Unit tests for RecommendedAppService.get_recommend_app_detail null handling. - -Regression tests for #36096: accessing result['id'] when the retrieval -returns None causes a TypeError / KeyError in self-hosted mode. -""" - -from types import SimpleNamespace -from unittest.mock import MagicMock, patch - -from services.recommended_app_service import RecommendedAppService - - -class TestGetRecommendAppDetailNullCheck: - @patch("services.recommended_app_service.FeatureService", autospec=True) - @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) - @patch("services.recommended_app_service.dify_config") - def test_returns_none_when_retrieval_returns_none_and_trial_disabled( - self, mock_config, mock_factory_class, mock_feature_service - ): - mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" - mock_instance = MagicMock() - mock_instance.get_recommend_app_detail.return_value = None - mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) - mock_feature_service.get_system_features.return_value = SimpleNamespace(enable_trial_app=False) - - result = RecommendedAppService.get_recommend_app_detail("nonexistent") - - assert result is None - - @patch("services.recommended_app_service.FeatureService", autospec=True) - @patch("services.recommended_app_service.RecommendAppRetrievalFactory", autospec=True) - @patch("services.recommended_app_service.dify_config") - def test_returns_none_when_retrieval_returns_none_and_trial_enabled( - self, mock_config, mock_factory_class, mock_feature_service - ): - """Regression for #36096: must not crash when result is None and enable_trial_app is True.""" - mock_config.HOSTED_FETCH_APP_TEMPLATES_MODE = "remote" - mock_instance = MagicMock() - mock_instance.get_recommend_app_detail.return_value = None - mock_factory_class.get_recommend_app_factory.return_value = MagicMock(return_value=mock_instance) - mock_feature_service.get_system_features.return_value = SimpleNamespace(enable_trial_app=True) - - result = RecommendedAppService.get_recommend_app_detail("nonexistent") - - assert result is None - mock_instance.get_recommend_app_detail.assert_called_once_with("nonexistent")