From b6bab6361479f1b24f7adef00bf9b27c07ec3975 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 13:41:59 +0000 Subject: [PATCH] test(web): cover the error and response builders, and stop dropping empty values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit errors.py and error_handler.py's response builders had no direct tests, though every API response passes through them. Two bugs surfaced. WebInterfaceError set suggested_fixes with `or`, so a caller passing [] to mean "I have no suggestions for this one" got the default list instead. Only None should fall back. create_success_response gated `data` on `is not None` but `message` and `metadata` on truthiness, so an explicitly-passed "" or {} vanished from the response while 0 and False survived — the response shape depended on the value. api_helpers.success_response() then re-gated metadata the same way, which is the path every api_v3 endpoint actually calls, so fixing only the inner function would have changed nothing observable. Both now use `is not None`. That wrapper also merged request timing into the caller's own metadata dict in place. A caller reusing a dict across requests would accumulate previous responses' timings; it now copies before adding. 79 tests: category inference for every error code, mapped vs fallback suggestions, the JSON shape including which keys are omitted when empty, exception-to-code inference, and the success/error builders end to end. Two behaviours are pinned as deliberate rather than fixed: an empty context stays out of the response body, and from_exception's `message` is the fixed per-code string, never the raw exception text. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --- src/web_interface/api_helpers.py | 20 +-- src/web_interface/error_handler.py | 13 +- src/web_interface/errors.py | 6 +- test/web_interface/test_error_handler.py | 149 ++++++++++++++++ test/web_interface/test_errors.py | 208 +++++++++++++++++++++++ 5 files changed, 379 insertions(+), 17 deletions(-) create mode 100644 test/web_interface/test_error_handler.py create mode 100644 test/web_interface/test_errors.py diff --git a/src/web_interface/api_helpers.py b/src/web_interface/api_helpers.py index 7ba6567a..3cff5293 100644 --- a/src/web_interface/api_helpers.py +++ b/src/web_interface/api_helpers.py @@ -29,18 +29,16 @@ def success_response( Flask jsonify response """ response_data = create_success_response(data, message, metadata) - - # Add request metadata if available - if metadata is None: - metadata = {} - - # Add timing if request start time is available + + # Timing is merged into whatever the caller passed, without inventing a + # metadata block for responses that have neither. + enriched = dict(metadata) if metadata is not None else {} if hasattr(request, 'start_time'): - metadata['response_time_ms'] = int((time.time() - request.start_time) * 1000) - - if metadata: - response_data['metadata'] = metadata - + enriched['response_time_ms'] = int((time.time() - request.start_time) * 1000) + + if metadata is not None or enriched: + response_data['metadata'] = enriched + return jsonify(response_data) diff --git a/src/web_interface/error_handler.py b/src/web_interface/error_handler.py index ea6423a4..f5acbc29 100644 --- a/src/web_interface/error_handler.py +++ b/src/web_interface/error_handler.py @@ -142,14 +142,17 @@ def create_success_response( "status": "success" } + # All three use `is not None` rather than truthiness: "" and {} are + # values a caller chose to send, and dropping them silently would make + # the response shape depend on the data. if data is not None: response["data"] = data - - if message: + + if message is not None: response["message"] = message - - if metadata: + + if metadata is not None: response["metadata"] = metadata - + return response diff --git a/src/web_interface/errors.py b/src/web_interface/errors.py index 11397892..bb7c6e07 100644 --- a/src/web_interface/errors.py +++ b/src/web_interface/errors.py @@ -89,7 +89,11 @@ class WebInterfaceError: self.category = category or self._infer_category(error_code) self.details = details self.context = context or {} - self.suggested_fixes = suggested_fixes or self._get_default_suggestions(error_code) + # `is None`, not truthiness: an explicit [] means "this caller has + # no suggestions to offer", which the default list would override. + self.suggested_fixes = ( + suggested_fixes if suggested_fixes is not None + else self._get_default_suggestions(error_code)) self.original_error = original_error def _infer_category(self, error_code: ErrorCode) -> ErrorCategory: diff --git a/test/web_interface/test_error_handler.py b/test/web_interface/test_error_handler.py new file mode 100644 index 00000000..ccbae7b6 --- /dev/null +++ b/test/web_interface/test_error_handler.py @@ -0,0 +1,149 @@ +""" +Tests for the response builders in src/web_interface/error_handler.py and +the success path in src/web_interface/api_helpers.py. + +describe_exception() in the same module is already covered by +test/test_web_error_detail.py and is not duplicated here. + +Regression coverage for one fixed bug: create_success_response used +truthiness for `message` and `metadata` while using `is not None` for +`data`, so an explicitly-passed "" or {} was silently dropped — +api_helpers.success_response() repeated the same gate, which is the path +every api_v3 endpoint actually calls. +""" + +import pytest +from flask import Flask + +from src.web_interface.api_helpers import success_response +from src.web_interface.error_handler import ( + create_error_response, + create_success_response, +) +from src.web_interface.errors import ErrorCode, WebInterfaceError + + +@pytest.fixture +def app(): + return Flask(__name__) + + +class TestCreateErrorResponse: + def test_returns_response_and_status_tuple(self, app): + with app.test_request_context(): + response, status = create_error_response( + ErrorCode.CONFIG_SAVE_FAILED, "could not save") + assert status == 500 + assert response.get_json()["message"] == "could not save" + + def test_status_code_passthrough(self, app): + with app.test_request_context(): + _, status = create_error_response( + ErrorCode.INVALID_INPUT, "bad", status_code=400) + assert status == 400 + + def test_body_matches_the_error_dataclass(self, app): + with app.test_request_context(): + response, _ = create_error_response( + ErrorCode.NETWORK_ERROR, "offline", + details="connection refused", context={"url": "http://x"}) + expected = WebInterfaceError( + error_code=ErrorCode.NETWORK_ERROR, message="offline", + details="connection refused", context={"url": "http://x"}).to_dict() + assert response.get_json() == expected + + def test_none_context_produces_no_context_key(self, app): + with app.test_request_context(): + response, _ = create_error_response(ErrorCode.SYSTEM_ERROR, "boom") + assert "context" not in response.get_json() + + def test_suggested_fixes_passed_through(self, app): + with app.test_request_context(): + response, _ = create_error_response( + ErrorCode.SYSTEM_ERROR, "boom", suggested_fixes=["Try again"]) + assert response.get_json()["suggested_fixes"] == ["Try again"] + + +class TestCreateSuccessResponse: + def test_bare_success(self): + assert create_success_response() == {"status": "success"} + + def test_data_included(self): + assert create_success_response(data={"a": 1})["data"] == {"a": 1} + + @pytest.mark.parametrize("falsy", [0, "", False, {}, []]) + def test_falsy_data_is_still_included(self, falsy): + assert create_success_response(data=falsy)["data"] == falsy + + def test_none_data_omitted(self): + assert "data" not in create_success_response(data=None) + + def test_message_included(self): + assert create_success_response(message="done")["message"] == "done" + + def test_empty_message_is_still_included(self): + # Regression: `if message:` dropped an explicitly-passed "". + assert create_success_response(message="")["message"] == "" + + def test_none_message_omitted(self): + assert "message" not in create_success_response(message=None) + + def test_metadata_included(self): + assert create_success_response(metadata={"v": 1})["metadata"] == {"v": 1} + + def test_empty_metadata_is_still_included(self): + # Regression: `if metadata:` dropped an explicitly-passed {}. + assert create_success_response(metadata={})["metadata"] == {} + + def test_none_metadata_omitted(self): + assert "metadata" not in create_success_response(metadata=None) + + +class TestSuccessResponseHelper: + """api_helpers.success_response — the wrapper every endpoint calls.""" + + def test_plain_response_has_no_metadata_block(self, app): + with app.test_request_context(): + body = success_response(data={"a": 1}).get_json() + assert body == {"status": "success", "data": {"a": 1}} + + def test_explicit_empty_metadata_survives_the_wrapper(self, app): + # Regression: the wrapper re-gated metadata on truthiness after + # create_success_response had already included it, so {} was + # dropped again on the way out. + with app.test_request_context(): + body = success_response(data=None, metadata={}).get_json() + assert body["metadata"] == {} + + def test_caller_metadata_preserved(self, app): + with app.test_request_context(): + body = success_response(metadata={"version": "1.2"}).get_json() + assert body["metadata"]["version"] == "1.2" + + def test_timing_added_when_request_has_start_time(self, app): + with app.test_request_context() as ctx: + ctx.request.start_time = 0.0 + body = success_response(data={"a": 1}).get_json() + assert "response_time_ms" in body["metadata"] + + def test_timing_merges_with_caller_metadata(self, app): + with app.test_request_context() as ctx: + ctx.request.start_time = 0.0 + body = success_response(metadata={"version": "1.2"}).get_json() + assert body["metadata"]["version"] == "1.2" + assert "response_time_ms" in body["metadata"] + + def test_caller_metadata_dict_is_not_mutated(self, app): + # The helper used to add response_time_ms straight into the dict the + # caller passed, so a module-level or reused metadata dict would + # accumulate timings from previous requests. + caller_metadata = {"version": "1.2"} + with app.test_request_context() as ctx: + ctx.request.start_time = 0.0 + success_response(metadata=caller_metadata) + assert caller_metadata == {"version": "1.2"} + + def test_message_passed_through(self, app): + with app.test_request_context(): + body = success_response(message="saved").get_json() + assert body["message"] == "saved" diff --git a/test/web_interface/test_errors.py b/test/web_interface/test_errors.py new file mode 100644 index 00000000..d707f080 --- /dev/null +++ b/test/web_interface/test_errors.py @@ -0,0 +1,208 @@ +""" +Tests for src/web_interface/errors.py — the structured error type behind +every API error response (category inference, default suggestions, the +JSON shape, and exception conversion). + +Pure logic; no Flask context needed. + +Regression coverage for one fixed bug: suggested_fixes used `or`, so a +caller passing [] to mean "no suggestions" silently got the default list. +""" + +import pytest + +from src.web_interface.errors import ErrorCategory, ErrorCode, WebInterfaceError + + +class TestCategoryInference: + @pytest.mark.parametrize("code,expected", [ + (ErrorCode.CONFIG_SAVE_FAILED, ErrorCategory.CONFIGURATION), + (ErrorCode.CONFIG_ROLLBACK_FAILED, ErrorCategory.CONFIGURATION), + (ErrorCode.PLUGIN_NOT_FOUND, ErrorCategory.PLUGIN), + (ErrorCode.PLUGIN_OPERATION_CONFLICT, ErrorCategory.PLUGIN), + (ErrorCode.VALIDATION_ERROR, ErrorCategory.VALIDATION), + (ErrorCode.SCHEMA_VALIDATION_FAILED, ErrorCategory.VALIDATION), + (ErrorCode.INVALID_INPUT, ErrorCategory.VALIDATION), + (ErrorCode.NETWORK_ERROR, ErrorCategory.NETWORK), + (ErrorCode.API_ERROR, ErrorCategory.NETWORK), + (ErrorCode.TIMEOUT, ErrorCategory.NETWORK), + (ErrorCode.PERMISSION_DENIED, ErrorCategory.PERMISSION), + (ErrorCode.FILE_PERMISSION_ERROR, ErrorCategory.PERMISSION), + (ErrorCode.SYSTEM_ERROR, ErrorCategory.SYSTEM), + (ErrorCode.SERVICE_UNAVAILABLE, ErrorCategory.SYSTEM), + (ErrorCode.UNKNOWN_ERROR, ErrorCategory.UNKNOWN), + ]) + def test_every_code_prefix_maps_to_its_category(self, code, expected): + assert WebInterfaceError(code, "msg").category is expected + + def test_explicit_category_overrides_inference(self): + error = WebInterfaceError( + ErrorCode.CONFIG_SAVE_FAILED, "msg", category=ErrorCategory.SYSTEM) + assert error.category is ErrorCategory.SYSTEM + + def test_every_error_code_gets_a_category(self): + # No code may fall through uncategorized as the enum grows. + for code in ErrorCode: + assert isinstance(WebInterfaceError(code, "msg").category, ErrorCategory) + + +class TestDefaultSuggestions: + def test_mapped_code_gets_specific_suggestions(self): + fixes = WebInterfaceError(ErrorCode.CONFIG_SAVE_FAILED, "msg").suggested_fixes + assert "Check available disk space" in fixes + + def test_unmapped_code_gets_generic_fallback(self): + # PLUGIN_UPDATE_FAILED has no entry in suggestions_map. + fixes = WebInterfaceError(ErrorCode.PLUGIN_UPDATE_FAILED, "msg").suggested_fixes + assert fixes == ["Review error details and try again"] + + def test_explicit_suggestions_win(self): + error = WebInterfaceError( + ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=["Do the thing"]) + assert error.suggested_fixes == ["Do the thing"] + + def test_explicit_empty_list_is_respected(self): + # Regression: `suggested_fixes or default` treated [] as "unset", + # so a caller could not express "I have no suggestions". + error = WebInterfaceError( + ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=[]) + assert error.suggested_fixes == [] + + def test_none_still_gets_defaults(self): + error = WebInterfaceError( + ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=None) + assert len(error.suggested_fixes) > 0 + + +class TestToDict: + def test_base_keys_always_present(self): + result = WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom").to_dict() + assert result["status"] == "error" + assert result["error_code"] == "SYSTEM_ERROR" + assert result["error_category"] == "system" + assert result["message"] == "boom" + + def test_details_included_when_set(self): + result = WebInterfaceError( + ErrorCode.SYSTEM_ERROR, "boom", details="disk full").to_dict() + assert result["details"] == "disk full" + + def test_details_omitted_when_absent(self): + assert "details" not in WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom").to_dict() + + def test_context_included_when_non_empty(self): + result = WebInterfaceError( + ErrorCode.SYSTEM_ERROR, "boom", context={"path": "/tmp/x"}).to_dict() + assert result["context"] == {"path": "/tmp/x"} + + def test_empty_context_is_omitted(self): + # Pinned as intentional, not a bug: __init__ normalizes context to + # {}, and an empty context carries no information, so it is left out + # rather than padding every error body with "context": {}. + result = WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom", context={}).to_dict() + assert "context" not in result + + def test_empty_suggestions_omitted(self): + result = WebInterfaceError( + ErrorCode.SYSTEM_ERROR, "boom", suggested_fixes=[]).to_dict() + assert "suggested_fixes" not in result + + def test_is_json_serializable(self): + import json + error = WebInterfaceError( + ErrorCode.NETWORK_ERROR, "boom", + details="timeout", context={"url": "http://x"}) + assert json.loads(json.dumps(error.to_dict()))["error_code"] == "NETWORK_ERROR" + + +class TestFromException: + @pytest.mark.parametrize("exc_name,expected", [ + ("ConfigError", ErrorCode.CONFIG_LOAD_FAILED), + ("PluginError", ErrorCode.PLUGIN_LOAD_FAILED), + ("PermissionError", ErrorCode.PERMISSION_DENIED), + ("AccessDenied", ErrorCode.PERMISSION_DENIED), + ("ValidationError", ErrorCode.VALIDATION_ERROR), + ("SchemaError", ErrorCode.VALIDATION_ERROR), + ("NetworkError", ErrorCode.NETWORK_ERROR), + ("ConnectionError", ErrorCode.NETWORK_ERROR), + ("TimeoutError", ErrorCode.TIMEOUT), + ("SomethingElse", ErrorCode.UNKNOWN_ERROR), + ]) + def test_code_inferred_from_exception_class_name(self, exc_name, expected): + exc = type(exc_name, (Exception,), {})("boom") + assert WebInterfaceError.from_exception(exc).error_code is expected + + def test_explicit_code_skips_inference(self): + error = WebInterfaceError.from_exception( + ValueError("boom"), error_code=ErrorCode.PLUGIN_NOT_FOUND) + assert error.error_code is ErrorCode.PLUGIN_NOT_FOUND + + def test_message_is_the_safe_one_not_the_exception_text(self): + # The raw exception text is not echoed into `message`; that field is + # a fixed, user-facing string per code. + error = WebInterfaceError.from_exception(ValueError("secret-ish detail")) + assert error.message == "An unexpected error occurred" + assert "secret-ish" not in error.message + + def test_exception_type_recorded_in_context(self): + error = WebInterfaceError.from_exception(ValueError("boom")) + assert error.context["exception_type"] == "ValueError" + + def test_caller_context_is_preserved_alongside_type(self): + error = WebInterfaceError.from_exception( + ValueError("boom"), context={"plugin_id": "clock"}) + assert error.context["plugin_id"] == "clock" + assert error.context["exception_type"] == "ValueError" + + def test_caller_supplied_exception_type_is_overwritten(self): + error = WebInterfaceError.from_exception( + ValueError("boom"), context={"exception_type": "Fake"}) + assert error.context["exception_type"] == "ValueError" + + def test_original_error_retained(self): + exc = ValueError("boom") + assert WebInterfaceError.from_exception(exc).original_error is exc + + def test_every_code_has_a_safe_message(self): + for code in ErrorCode: + assert WebInterfaceError._safe_message(code) + + +class TestExceptionDetails: + def test_context_dict_is_flattened(self): + exc = ValueError("boom") + exc.context = {"config_path": "/etc/x.json", "line": 4} + details = WebInterfaceError._get_exception_details(exc) + assert "config_path: /etc/x.json" in details + assert "line: 4" in details + assert "; " in details + + def test_exception_type_key_excluded(self): + exc = ValueError("boom") + exc.context = {"exception_type": "ValueError", "path": "/tmp/x"} + details = WebInterfaceError._get_exception_details(exc) + assert "exception_type" not in details + assert details == "path: /tmp/x" + + def test_context_with_only_exception_type_gives_none(self): + exc = ValueError("boom") + exc.context = {"exception_type": "ValueError"} + assert WebInterfaceError._get_exception_details(exc) is None + + def test_no_context_attribute_gives_none(self): + assert WebInterfaceError._get_exception_details(ValueError("boom")) is None + + def test_non_dict_context_gives_none(self): + exc = ValueError("boom") + exc.context = "not a dict" + assert WebInterfaceError._get_exception_details(exc) is None + + def test_empty_context_gives_none(self): + exc = ValueError("boom") + exc.context = {} + assert WebInterfaceError._get_exception_details(exc) is None + + def test_details_flow_into_from_exception(self): + exc = ValueError("boom") + exc.context = {"config_path": "/etc/x.json"} + assert "config_path" in WebInterfaceError.from_exception(exc).details