Copilot commented on code in PR #335: URL: https://github.com/apache/hugegraph-ai/pull/335#discussion_r3271028652
########## hugegraph-python-client/src/tests/api/test_response_validation.py: ########## @@ -0,0 +1,38 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from unittest.mock import Mock + +import pytest +import requests +from pyhugegraph.utils.util import ResponseValidation + + +def test_response_validation_raises_http_error_with_numeric_status_body(): + response = Mock(spec=requests.Response) + response.status_code = 400 + response.text = '{"status":400,"message":"bad gremlin"}' + response.content = response.text.encode("utf-8") + response.json.return_value = {"status": 400, "message": "bad gremlin"} + response.request = Mock() + response.request.body = "g.V2()" + response.raise_for_status.side_effect = requests.exceptions.HTTPError("400 Client Error") + + validator = ResponseValidation() + + with pytest.raises(Exception, match="bad gremlin"): + validator(response, "POST", "/gremlin") Review Comment: Test name says it “raises http error”, but `ResponseValidation` wraps 400 responses into a generic `Exception` ("Server Exception: ..."). Rename the test to reflect the actual contract (or assert the specific exception type/message you expect) to avoid misleading future readers. ########## hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/schema_manager.py: ########## @@ -57,9 +58,12 @@ def simple_schema(self, schema: Dict[str, Any]) -> Dict[str, Any]: def run(self, context: Optional[Dict[str, Any]]) -> Dict[str, Any]: if context is None: context = {} - schema = self.schema.getSchema() + try: + schema = self.schema.getSchema() + except RequestException as e: + raise ValueError(f"Failed to connect HugeGraph to get schema '{self.graph_name}': {e}") from e if not schema["vertexlabels"] and not schema["edgelabels"]: - raise Exception(f"Can not get {self.graph_name}'s schema from HugeGraph!") + raise ValueError(f"Can not get {self.graph_name}'s schema from HugeGraph!") Review Comment: The new error messages have grammar/spelling issues that reduce clarity: “Failed to connect HugeGraph…” should be “Failed to connect to HugeGraph…”, and “Can not get …” should be “Cannot get …”. Please update the messages (and keep wording consistent across similar errors). ########## hugegraph-llm/src/hugegraph_llm/models/llms/litellm.py: ########## @@ -104,7 +104,7 @@ async def agenerate( return response.choices[0].message.content except (RateLimitError, BudgetExceededError, APIError) as e: log.error("Error in async LiteLLM call: %s", e) - return f"Error: {str(e)}" + raise Review Comment: `generate`/`agenerate` now re-raise LiteLLM exceptions, but `generate_streaming`/`agenerate_streaming` still convert similar exceptions into "Error: …" strings/yields. This makes the client’s error contract inconsistent across methods and can lead to silent failures in streaming paths. Consider standardizing on either raising exceptions or returning error strings across all methods. ########## hugegraph-python-client/src/pyhugegraph/utils/util.py: ########## @@ -101,9 +101,18 @@ def __call__(self, response: requests.Response, method: str, path: str): log.info("Resource %s not found (404)", path) else: try: - details = response.json().get("exception", "key 'exception' not found") + body = response.json() + status = body.get("status") + status_message = status.get("message") if isinstance(status, dict) else None + details = ( + body.get("exception") + or status_message + or body.get("message") + or response.text + or "unknown error" + ) except (ValueError, KeyError): Review Comment: `response.json()` can legally return a non-dict (e.g., list/string/number). In that case `body.get(...)` raises `AttributeError`, which will bypass the intended fallback and mask the original HTTP error. Guard with `isinstance(body, dict)` (or catch `AttributeError`/`TypeError`) and fall back to `response.text`/`unknown error` when the JSON payload isn’t an object. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
