This is an automated email from the ASF dual-hosted git repository.
imbajin pushed a commit to branch goal-scan
in repository https://gitbox.apache.org/repos/asf/hugegraph-ai.git
The following commit(s) were added to refs/heads/goal-scan by this push:
new 902fd6b5 fix(llm): address follow-up review comments
902fd6b5 is described below
commit 902fd6b540ae24837d4cf9db4406868593e6915a
Author: imbajin <[email protected]>
AuthorDate: Wed Jun 3 12:50:36 2026 +0800
fix(llm): address follow-up review comments
- remove request-wide graph config lock from RAG API
- roll back provider type settings when config apply fails
- let graph import apply typed defaults for missing properties
- clarify scan report wording for post-scan fixes
---
.../code-scan/reports/final-code-scan-report.md | 2 +-
hugegraph-llm/src/hugegraph_llm/api/rag_api.py | 114 ++++++++++++---------
.../operators/llm_op/property_graph_extract.py | 11 +-
hugegraph-llm/src/tests/api/test_rag_api.py | 62 +++++++++++
.../llm_op/test_property_graph_extract.py | 11 +-
5 files changed, 140 insertions(+), 60 deletions(-)
diff --git a/.workflow/code-scan/reports/final-code-scan-report.md
b/.workflow/code-scan/reports/final-code-scan-report.md
index 808be10c..635dc66a 100644
--- a/.workflow/code-scan/reports/final-code-scan-report.md
+++ b/.workflow/code-scan/reports/final-code-scan-report.md
@@ -7,7 +7,7 @@ Scope: `hugegraph-llm`, `hugegraph-python-client`
The scan completed all planned lanes. The highest-risk result is one P0
security issue in the `hugegraph-llm` admin log API. The broader pattern is
that core boundary failures are often hidden: client transport errors are
retyped, provider failures are returned as normal text, graph/vector dependency
failures can be converted to empty results, and several integration tests use
local stand-ins rather than production flows.
-No behavior-changing source fixes were made. Only scan documents and `FIXME:`
comments were added.
+During the scan phase, no behavior-changing source fixes were made. Only scan
documents and `FIXME:` comments were added. Follow-up fixes are listed later in
this report.
## Scope Covered
diff --git a/hugegraph-llm/src/hugegraph_llm/api/rag_api.py
b/hugegraph-llm/src/hugegraph_llm/api/rag_api.py
index 0e76ca50..1ac934ca 100644
--- a/hugegraph-llm/src/hugegraph_llm/api/rag_api.py
+++ b/hugegraph-llm/src/hugegraph_llm/api/rag_api.py
@@ -17,7 +17,6 @@
import json
from contextlib import contextmanager
-from threading import RLock
from fastapi import APIRouter, HTTPException, status
@@ -42,7 +41,18 @@ _GRAPH_CONFIG_FIELD_MAP = {
"pwd": "graph_pwd",
"gs": "graph_space",
}
-_GRAPH_CONFIG_LOCK = RLock()
+_LLM_TYPE_FIELDS = ("chat_llm_type", "extract_llm_type", "text2gql_llm_type")
+_EMBEDDING_TYPE_FIELDS = ("embedding_type",)
+_RERANKER_TYPE_FIELDS = ("reranker_type",)
+
+
+def _snapshot_settings(settings, fields):
+ return {field: getattr(settings, field) for field in fields}
+
+
+def _restore_settings(settings, values):
+ for field, value in values.items():
+ setattr(settings, field, value)
# pylint: disable=too-many-statements
@@ -58,21 +68,16 @@ def rag_http_api(
):
@contextmanager
def request_graph_config(req):
- with _GRAPH_CONFIG_LOCK:
- original_values = {
- settings_field: getattr(huge_settings, settings_field)
- for settings_field in _GRAPH_CONFIG_FIELD_MAP.values()
- }
- try:
- client_config = getattr(req, "client_config", None)
- if client_config is not None:
- for request_field, settings_field in
_GRAPH_CONFIG_FIELD_MAP.items():
- if request_field in client_config.model_fields_set:
- setattr(huge_settings, settings_field,
getattr(client_config, request_field))
- yield
- finally:
- for settings_field, original_value in original_values.items():
- setattr(huge_settings, settings_field, original_value)
+ original_values = _snapshot_settings(huge_settings,
_GRAPH_CONFIG_FIELD_MAP.values())
+ try:
+ client_config = getattr(req, "client_config", None)
+ if client_config is not None:
+ for request_field, settings_field in
_GRAPH_CONFIG_FIELD_MAP.items():
+ if request_field in client_config.model_fields_set:
+ setattr(huge_settings, settings_field,
getattr(client_config, request_field))
+ yield
+ finally:
+ _restore_settings(huge_settings, original_values)
@router.post("/rag", status_code=status.HTTP_200_OK)
def rag_answer_api(req: RAGRequest):
@@ -182,43 +187,58 @@ def rag_http_api(
# TODO: restructure the implement of llm to three types, like
"/config/chat_llm"
@router.post("/config/llm", status_code=status.HTTP_201_CREATED)
def llm_config_api(req: LLMConfigRequest):
- llm_settings.chat_llm_type = req.llm_type
- llm_settings.extract_llm_type = req.llm_type
- llm_settings.text2gql_llm_type = req.llm_type
-
- if req.llm_type in ("openai", "litellm"):
- res = apply_llm_conf(
- req.api_key,
- req.api_base,
- req.language_model,
- req.max_tokens,
- origin_call="http",
- )
- else:
- res = apply_llm_conf(req.host, req.port, req.language_model, None,
origin_call="http")
- return generate_response(RAGResponse(status_code=res, message="Missing
Value"))
+ original_values = _snapshot_settings(llm_settings, _LLM_TYPE_FIELDS)
+ try:
+ llm_settings.chat_llm_type = req.llm_type
+ llm_settings.extract_llm_type = req.llm_type
+ llm_settings.text2gql_llm_type = req.llm_type
+
+ if req.llm_type in ("openai", "litellm"):
+ res = apply_llm_conf(
+ req.api_key,
+ req.api_base,
+ req.language_model,
+ req.max_tokens,
+ origin_call="http",
+ )
+ else:
+ res = apply_llm_conf(req.host, req.port, req.language_model,
None, origin_call="http")
+ return generate_response(RAGResponse(status_code=res,
message="Missing Value"))
+ except Exception:
+ _restore_settings(llm_settings, original_values)
+ raise
@router.post("/config/embedding", status_code=status.HTTP_201_CREATED)
def embedding_config_api(req: LLMConfigRequest):
- llm_settings.embedding_type = req.llm_type
+ original_values = _snapshot_settings(llm_settings,
_EMBEDDING_TYPE_FIELDS)
+ try:
+ llm_settings.embedding_type = req.llm_type
- if req.llm_type in ("openai", "litellm"):
- res = apply_embedding_conf(req.api_key, req.api_base,
req.language_model, origin_call="http")
- else:
- res = apply_embedding_conf(req.host, req.port, req.language_model,
origin_call="http")
- return generate_response(RAGResponse(status_code=res, message="Missing
Value"))
+ if req.llm_type in ("openai", "litellm"):
+ res = apply_embedding_conf(req.api_key, req.api_base,
req.language_model, origin_call="http")
+ else:
+ res = apply_embedding_conf(req.host, req.port,
req.language_model, origin_call="http")
+ return generate_response(RAGResponse(status_code=res,
message="Missing Value"))
+ except Exception:
+ _restore_settings(llm_settings, original_values)
+ raise
@router.post("/config/rerank", status_code=status.HTTP_201_CREATED)
def rerank_config_api(req: RerankerConfigRequest):
- llm_settings.reranker_type = req.reranker_type
-
- if req.reranker_type == "cohere":
- res = apply_reranker_conf(req.api_key, req.reranker_model,
req.cohere_base_url, origin_call="http")
- elif req.reranker_type == "siliconflow":
- res = apply_reranker_conf(req.api_key, req.reranker_model, None,
origin_call="http")
- else:
- res = status.HTTP_501_NOT_IMPLEMENTED
- return generate_response(RAGResponse(status_code=res, message="Missing
Value"))
+ original_values = _snapshot_settings(llm_settings,
_RERANKER_TYPE_FIELDS)
+ try:
+ llm_settings.reranker_type = req.reranker_type
+
+ if req.reranker_type == "cohere":
+ res = apply_reranker_conf(req.api_key, req.reranker_model,
req.cohere_base_url, origin_call="http")
+ elif req.reranker_type == "siliconflow":
+ res = apply_reranker_conf(req.api_key, req.reranker_model,
None, origin_call="http")
+ else:
+ res = status.HTTP_501_NOT_IMPLEMENTED
+ return generate_response(RAGResponse(status_code=res,
message="Missing Value"))
+ except Exception:
+ _restore_settings(llm_settings, original_values)
+ raise
@router.post("/text2gremlin", status_code=status.HTTP_200_OK)
def text2gremlin_api(req: GremlinGenerateRequest):
diff --git
a/hugegraph-llm/src/hugegraph_llm/operators/llm_op/property_graph_extract.py
b/hugegraph-llm/src/hugegraph_llm/operators/llm_op/property_graph_extract.py
index dbe6f907..7d9f5c77 100644
--- a/hugegraph-llm/src/hugegraph_llm/operators/llm_op/property_graph_extract.py
+++ b/hugegraph-llm/src/hugegraph_llm/operators/llm_op/property_graph_extract.py
@@ -67,12 +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"
+ item["properties"] = {
+ key: value
+ for key, value in item["properties"].items()
+ if key in properties_map[item_type][label]["properties"]
+ }
filtered_items.append(item)
return filtered_items
diff --git a/hugegraph-llm/src/tests/api/test_rag_api.py
b/hugegraph-llm/src/tests/api/test_rag_api.py
index 7b05d70f..16de9c1c 100644
--- a/hugegraph-llm/src/tests/api/test_rag_api.py
+++ b/hugegraph-llm/src/tests/api/test_rag_api.py
@@ -144,6 +144,49 @@ def
test_embedding_config_api_passes_openai_fields_to_apply_embedding_conf():
)
+def test_llm_config_api_rolls_back_provider_type_on_apply_failure(monkeypatch):
+ monkeypatch.setattr(llm_settings, "chat_llm_type", "ollama/local")
+ monkeypatch.setattr(llm_settings, "extract_llm_type", "ollama/local")
+ monkeypatch.setattr(llm_settings, "text2gql_llm_type", "ollama/local")
+ client, callbacks =
_make_test_client(apply_llm_conf=Mock(return_value=status.HTTP_500_INTERNAL_SERVER_ERROR))
+
+ response = client.post(
+ "/config/llm",
+ json={
+ "llm_type": "openai",
+ "api_key": "sk-test",
+ "api_base": "https://api.example.test";,
+ "language_model": "gpt-test",
+ "max_tokens": "1024",
+ },
+ )
+
+ assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
+ assert llm_settings.chat_llm_type == "ollama/local"
+ assert llm_settings.extract_llm_type == "ollama/local"
+ assert llm_settings.text2gql_llm_type == "ollama/local"
+ callbacks["apply_llm_conf"].assert_called_once()
+
+
+def
test_embedding_config_api_rolls_back_provider_type_on_apply_failure(monkeypatch):
+ monkeypatch.setattr(llm_settings, "embedding_type", "ollama/local")
+ client, callbacks =
_make_test_client(apply_embedding_conf=Mock(return_value=status.HTTP_500_INTERNAL_SERVER_ERROR))
+
+ response = client.post(
+ "/config/embedding",
+ json={
+ "llm_type": "openai",
+ "api_key": "sk-embedding",
+ "api_base": "https://embedding.example.test";,
+ "language_model": "embedding-test",
+ },
+ )
+
+ assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
+ assert llm_settings.embedding_type == "ollama/local"
+ callbacks["apply_embedding_conf"].assert_called_once()
+
+
def test_rerank_config_api_passes_cohere_fields_to_apply_reranker_conf():
client, callbacks = _make_test_client()
@@ -257,6 +300,25 @@ def
test_rerank_config_rejects_unsupported_provider_without_mutating(monkeypatch
assert llm_settings.reranker_type == "cohere"
+def test_rerank_config_rolls_back_provider_type_on_apply_failure(monkeypatch):
+ monkeypatch.setattr(llm_settings, "reranker_type", "siliconflow")
+ client, callbacks =
_make_test_client(apply_reranker_conf=Mock(return_value=status.HTTP_500_INTERNAL_SERVER_ERROR))
+
+ response = client.post(
+ "/config/rerank",
+ json={
+ "reranker_type": "cohere",
+ "api_key": "cohere-key",
+ "reranker_model": "rerank-test",
+ "cohere_base_url": "https://cohere.example.test";,
+ },
+ )
+
+ assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
+ assert llm_settings.reranker_type == "siliconflow"
+ callbacks["apply_reranker_conf"].assert_called_once()
+
+
def test_text2gremlin_callback_exception_returns_stable_response():
client, _ =
_make_test_client(gremlin_generate_selective_func=Mock(side_effect=RuntimeError("callback
failed")))
diff --git
a/hugegraph-llm/src/tests/operators/llm_op/test_property_graph_extract.py
b/hugegraph-llm/src/tests/operators/llm_op/test_property_graph_extract.py
index b6e24e44..93fdd3d4 100644
--- a/hugegraph-llm/src/tests/operators/llm_op/test_property_graph_extract.py
+++ b/hugegraph-llm/src/tests/operators/llm_op/test_property_graph_extract.py
@@ -174,22 +174,21 @@ class TestPropertyGraphExtract(unittest.TestCase):
"label": "movie",
"properties": {
# Missing 'title' which is non-nullable
- "year": 1994 # Non-string value
+ "year": 1994, # Non-string value
+ "ignored": "not in schema",
},
},
]
filtered_items = filter_item(self.schema, items)
- # Check that non-nullable keys are added with NULL value
- # Note: 'age' is nullable, so it won't be added automatically
+ # Missing properties stay absent so Commit2Graph can apply
schema-typed defaults.
self.assertNotIn("age", filtered_items[0]["properties"])
-
- # Check that title (non-nullable) was added with NULL value
- self.assertEqual(filtered_items[1]["properties"]["title"], "NULL")
+ self.assertNotIn("title", filtered_items[1]["properties"])
# Check that schema-typed values are preserved for Commit2Graph
self.assertEqual(filtered_items[1]["properties"]["year"], 1994)
+ self.assertNotIn("ignored", filtered_items[1]["properties"])
def test_extract_property_graph_by_llm(self):
"""Test the extract_property_graph_by_llm method."""
