Copilot commented on code in PR #358: URL: https://github.com/apache/hugegraph-ai/pull/358#discussion_r3346504063
########## hugegraph-python-client/src/pyhugegraph/utils/util.py: ########## @@ -1,143 +1,211 @@ -# 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. - - -import json -import traceback - -import requests - -from pyhugegraph.utils.exceptions import ( - NotAuthorizedError, - NotFoundError, - ServiceUnavailableError, -) -from pyhugegraph.utils.log import log - - -def create_exception(response_content): - try: - data = json.loads(response_content) - if "ServiceUnavailableException" in data.get("exception", ""): - raise ServiceUnavailableError( - f'ServiceUnavailableException, "message": "{data["message"]}", "cause": "{data["cause"]}"' - ) - except (json.JSONDecodeError, KeyError) as e: - raise Exception(f"Error parsing response content: {response_content}") from e - raise Exception(response_content) - - -def check_if_authorized(response): - if response.status_code == 401: - raise NotAuthorizedError(f"Please check your username and password. {response.content!s}") - return True - - -def check_if_success(response, error=None): - if (not str(response.status_code).startswith("20")) and check_if_authorized(response): - if error is None: - error = NotFoundError(response.content) - - req = response.request - req_body = req.body if req.body else "Empty body" - response_body = response.text if response.text else "Empty body" - log.error( - "Error-Client: Request URL: %s, Request Body: %s, Response Body: %s", - req.url, - req_body, - response_body, - ) - raise error - return True - - -class ResponseValidation: - def __init__(self, content_type: str = "json", strict: bool = True) -> None: - super().__init__() - self._content_type = content_type - self._strict = strict - - def __call__(self, response: requests.Response, method: str, path: str): - """ - Validate the HTTP response according to the provided content type and strictness. - - :param response: HTTP response object - :param method: HTTP method used (e.g., 'GET', 'POST') - :param path: URL path of the request - :return: Parsed response content or empty dict if none applicable - """ - result = {} - - try: - response.raise_for_status() - if response.status_code == 204: - log.debug("No content returned (204) for %s: %s", method, path) - else: - if self._content_type == "raw": - result = response - elif self._content_type == "json": - result = response.json() - elif self._content_type == "text": - result = response.text - else: - raise ValueError(f"Unknown content type: {self._content_type}") - - except requests.exceptions.HTTPError as e: - if not self._strict and response.status_code == 404: - log.info("Resource %s not found (404)", path) - else: - if response.status_code == 401: - check_if_authorized(response) - - try: - body = response.json() - if isinstance(body, dict): - status = body.get("status") - status_message = status.get("message") if isinstance(status, dict) else None - details = ( - body.get("message") - or body.get("exception") - or status_message - or response.text - or "unknown error" - ) - else: - details = response.text or "unknown error" - except (ValueError, KeyError, AttributeError, TypeError): - details = response.text or "unknown error" - - req_body = response.request.body if response.request.body else "Empty body" - req_body = req_body.encode("utf-8").decode("unicode_escape") - log.error( - "%s: %s\n[Body]: %s\n[Server Exception]: %s", - method, - str(e).encode("utf-8").decode("unicode_escape"), - req_body, - details, - ) - - if response.status_code == 404: - raise NotFoundError(response.content) from e - raise Exception(f"Server Exception: {details}") from e - - except Exception: # pylint: disable=broad-exception-caught - log.error("Unhandled exception occurred: %s", traceback.format_exc()) - - return result - - def __repr__(self) -> str: - return f"ResponseValidation(content_type={self._content_type}, strict={self._strict})" +# 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. + + +import json +import re +import traceback + +import requests + +from pyhugegraph.utils.exceptions import ( + NotAuthorizedError, + NotFoundError, + ResponseParseError, + ServerError, + ServiceUnavailableError, +) +from pyhugegraph.utils.log import log + +REDACTED_VALUE = "***REDACTED***" +SENSITIVE_KEY_PARTS = ( + "api_key", + "authorization", + "password", + "passwd", + "pwd", + "secret", + "token", +) +ESCAPE_MARKERS = ("\\u", "\\U", "\\x", "\\n", "\\r", "\\t") +RESPONSE_CONTENT_TYPES = {"raw", "json", "text"} + + +def _is_sensitive_key(key) -> bool: + key_lower = str(key).lower() + return any(part in key_lower for part in SENSITIVE_KEY_PARTS) + + +def _may_contain_sensitive_key(value: str) -> bool: + value_lower = value.lower() + return any(part in value_lower for part in SENSITIVE_KEY_PARTS) + + +def _decode_escaped_text(value): + if not isinstance(value, str) or "\\" not in value: + return value + if not any(marker in value for marker in ESCAPE_MARKERS): + return value + return value.encode("utf-8", errors="replace").decode("unicode_escape", errors="replace") + + +def redact_sensitive_data(value): + if isinstance(value, dict): + return { + key: REDACTED_VALUE if _is_sensitive_key(key) else redact_sensitive_data(item) + for key, item in value.items() + } + if isinstance(value, list): + return [redact_sensitive_data(item) for item in value] + if isinstance(value, tuple): + return tuple(redact_sensitive_data(item) for item in value) + if isinstance(value, bytes): + value = value.decode("utf-8", errors="replace") + if isinstance(value, str): + if not _may_contain_sensitive_key(value): + return value + try: + parsed = json.loads(value) + except json.JSONDecodeError: + redacted = re.sub( + r'(?i)("?[a-z0-9_-]*(?:api_key|authorization|password|passwd|pwd|secret|token)[a-z0-9_-]*"?\s*[:=]\s*)"[^"]*"', + rf"\1\"{REDACTED_VALUE}\"", + value, + ) + return re.sub( + r"(?i)(api_key|authorization|password|passwd|pwd|secret|token)=([^&\s]+)", + rf"\1={REDACTED_VALUE}", + redacted, + ) + return json.dumps(redact_sensitive_data(parsed), ensure_ascii=False) + return value + + +def create_exception(response_content): + try: + data = json.loads(response_content) + if "ServiceUnavailableException" in data.get("exception", ""): + raise ServiceUnavailableError( + f'ServiceUnavailableException, "message": "{data["message"]}", "cause": "{data["cause"]}"' + ) + except (json.JSONDecodeError, KeyError) as e: + raise Exception(f"Error parsing response content: {response_content}") from e + raise Exception(response_content) + + +def check_if_authorized(response): + if response.status_code == 401: + raise NotAuthorizedError(f"Please check your username and password. {response.content!s}") + return True + + +def check_if_success(response, error=None): + if (not str(response.status_code).startswith("20")) and check_if_authorized(response): + if error is None: + error = NotFoundError(response.content) + + req = response.request + req_body = redact_sensitive_data(req.body) if req.body else "Empty body" + response_body = response.text if response.text else "Empty body" Review Comment: `check_if_success()` now redacts the request body but still logs the raw `response.text`. Error responses (especially from auth/config endpoints) can echo credentials/tokens back, so the response body should be passed through the same redaction helper before logging. ########## hugegraph-llm/src/hugegraph_llm/api/rag_api.py: ########## @@ -45,103 +66,110 @@ def rag_http_api( apply_reranker_conf, gremlin_generate_selective_func, ): - @router.post("/rag", status_code=status.HTTP_200_OK) - def rag_answer_api(req: RAGRequest): - set_graph_config(req) - - # Basic parameter validation: empty query => 400 - if not req.query or not str(req.query).strip(): - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail="Query must not be empty.", - ) - - result = rag_answer_func( - text=req.query, - raw_answer=req.raw_answer, - vector_only_answer=req.vector_only, - graph_only_answer=req.graph_only, - graph_vector_answer=req.graph_vector_answer, - graph_ratio=req.graph_ratio, - rerank_method=req.rerank_method, - near_neighbor_first=req.near_neighbor_first, - gremlin_tmpl_num=req.gremlin_tmpl_num, - max_graph_items=req.max_graph_items, - topk_return_results=req.topk_return_results, - vector_dis_threshold=req.vector_dis_threshold, - topk_per_keyword=req.topk_per_keyword, - # Keep prompt params in the end - custom_related_information=req.custom_priority_info, - answer_prompt=req.answer_prompt or prompt.answer_prompt, - keywords_extract_prompt=req.keywords_extract_prompt or prompt.keywords_extract_prompt, - gremlin_prompt=req.gremlin_prompt or prompt.gremlin_generate_prompt, - ) - # TODO: we need more info in the response for users to understand the query logic - return { - "query": req.query, - **{ - key: value - for key, value in zip( - ["raw_answer", "vector_only", "graph_only", "graph_vector_answer"], - result, - ) - if getattr(req, key) - }, - } - - def set_graph_config(req): - if req.client_config: - huge_settings.graph_url = req.client_config.url - huge_settings.graph_name = req.client_config.graph - huge_settings.graph_user = req.client_config.user - huge_settings.graph_pwd = req.client_config.pwd - huge_settings.graph_space = req.client_config.gs - - @router.post("/rag/graph", status_code=status.HTTP_200_OK) - def graph_rag_recall_api(req: GraphRAGRequest): + @contextmanager + def request_graph_config(req): + # TODO: pass graph config through request-scoped flow/operator context + # instead of temporarily mutating process-global huge_settings. + original_values = _snapshot_settings(huge_settings, _GRAPH_CONFIG_FIELD_MAP.values()) Review Comment: `request_graph_config()` still mutates the process-global `huge_settings` without any synchronization. Under concurrent requests (FastAPI threadpool), one request can overwrite another request’s graph settings mid-flight, and the `finally` restore can clobber the other request’s active config. If request-scoped settings aren’t available yet, guard the mutation with a lock to prevent cross-request leakage. ########## hugegraph-llm/src/hugegraph_llm/operators/llm_op/property_graph_extract.py: ########## @@ -67,15 +67,11 @@ def filter_item(schema, items) -> List[Dict[str, Any]]: item_type = item["type"] if item_type == "vertex": label = item["label"] - non_nullable_keys = set(properties_map[item_type][label]["properties"]).difference( - set(properties_map[item_type][label]["nullable_keys"]) - ) - for key in non_nullable_keys: - if key not in item["properties"]: - item["properties"][key] = "NULL" - for key, value in item["properties"].items(): - if not isinstance(value, str): - item["properties"][key] = str(value) + item["properties"] = { Review Comment: `filter_item()` claims to “filter vertex and edge with invalid properties”, but the updated logic only filters vertex properties. Edge properties (and any unexpected keys) now pass through unfiltered, which can cause schema validation/import failures downstream and also diverges from the function’s stated intent. -- 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]
