Skip to content

feat: add a GraphRAG query length config#11

Open
imbajin wants to merge 4 commits into
mainfrom
fix-graphrag-query-maxlength-name
Open

feat: add a GraphRAG query length config#11
imbajin wants to merge 4 commits into
mainfrom
fix-graphrag-query-maxlength-name

Conversation

@imbajin
Copy link
Copy Markdown

@imbajin imbajin commented May 28, 2025

This commit renames the configuration parameter for GraphRAG's maximum query length, as per your feedback, to make it shorter and more concise.

Changes:

  • Renamed graph_rag_max_query_length to rag_query_max_length in LLMConfig.
  • Corresponding environment variable changed from GRAPH_RAG_MAX_QUERY_LENGTH to RAG_QUERY_MAX_LENGTH.
  • Updated GraphRAGQuery and its unit tests to use the new names.
  • Updated hugegraph-llm/README.md to reflect the new environment variable name.

Summary by CodeRabbit

  • 新功能
    • 增加了对RAG相关查询输入长度的限制,默认最大字符数为50,超出将被拒绝处理。
    • 支持通过环境变量自定义最大查询长度,可在.env文件或Web界面进行配置。
  • 文档
    • README文档新增了环境变量配置说明,详细介绍了RAG查询长度限制的设置方法和相关变量。
  • Bug修复
    • 对RAG API及相关演示界面增加了输入长度校验,防止过长查询导致异常。
  • 测试
    • 新增了针对RAG查询长度校验的单元测试,覆盖API接口和演示功能的正常与异常场景。

This commit renames the configuration parameter for GraphRAG's
maximum query length, as per your feedback, to make it shorter
and more concise.

Changes:
- Renamed `graph_rag_max_query_length` to `rag_query_max_length`
  in `LLMConfig`.
- Corresponding environment variable changed from
  `GRAPH_RAG_MAX_QUERY_LENGTH` to `RAG_QUERY_MAX_LENGTH`.
- Updated `GraphRAGQuery` and its unit tests to use the new names.
- Updated `hugegraph-llm/README.md` to reflect the new environment
  variable name.
@imbajin imbajin requested a review from Copilot May 28, 2025 06:47
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2025

Walkthrough

本次更新在Retrieval-Augmented Generation(RAG)相关功能中引入了查询长度限制。通过配置项RAG_QUERY_MAX_LENGTH(默认50),对RAG相关API、Gradio演示和内部流程的用户查询长度进行统一校验。相关配置说明已补充至README,并增加了对应的单元测试。

Changes

文件/路径分组 变更概要
hugegraph-llm/README.md 增加.env配置文件说明,详细介绍RAG_QUERY_MAX_LENGTH环境变量及其作用。
hugegraph-llm/src/hugegraph_llm/config/llm_config.py LLMConfig类新增rag_query_max_length属性,默认50,可通过环境变量配置。
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py rag_answerrag_answer_streaming函数中增加查询长度校验,超长时返回警告和空结果。
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py graph_rag_recall函数开头增加查询长度校验,超长时返回错误信息并终止处理。
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py RAGRequestGraphRAGRequest模型新增check_query_length校验方法,自动校验查询长度。
hugegraph-llm/src/tests/api/test_rag_api.py 新增RAG API查询长度校验相关单元测试,覆盖正常与异常场景。
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py 新增rag_block查询长度校验及功能单元测试,覆盖同步与异步流程。

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant API/Gradio
    participant LLMConfig
    participant RAG Pipeline

    User->>API/Gradio: 提交query
    API/Gradio->>LLMConfig: 获取rag_query_max_length
    API/Gradio->>API/Gradio: 校验query长度
    alt 长度超限
        API/Gradio-->>User: 返回警告/错误信息
    else 长度合规
        API/Gradio->>RAG Pipeline: 正常处理query
        RAG Pipeline-->>API/Gradio: 返回结果
        API/Gradio-->>User: 返回RAG结果
    end
Loading

Poem

兔子翻阅.env,
查询长度有新规。
超过五十要警告,
API与界面同心齐。
单元测试来护航,
RAG世界更整齐!
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR renames the configuration parameter for GraphRAG's maximum query length to a shorter, more concise name and updates its usage throughout the codebase.

  • Renames configuration parameter in LLMConfig and environment variable.
  • Updates GraphRAGQuery to enforce query length based on the new parameter.
  • Adjusts unit tests and documentation to reflect the new name.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py Updates unit tests to use the renamed configuration parameter and verifies behavior for different query lengths.
hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py Modifies the run method to read the new configuration parameter and enforce query length accordingly.
hugegraph-llm/src/hugegraph_llm/config/llm_config.py Renames config property and environment variable for GraphRAG query length.
hugegraph-llm/README.md Updates documentation to reflect the new environment variable name and its usage.

Comment thread hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py Outdated
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license-eye has checked 283 files.

Valid Invalid Ignored Fixed
236 1 46 0
Click to see the invalid file list
  • hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py
Use this command to fix any missing license headers
```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

Comment thread hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py (1)

112-121: 移除冗余的类型转换并优化配置实例化

实现逻辑正确,但存在两个改进点:

  1. int(llm_config.rag_query_max_length) 中的类型转换是冗余的,因为配置属性已经是 int 类型
  2. 在每次调用 run 方法时创建 LLMConfig 实例可能影响性能

建议的优化:

-        llm_config = LLMConfig()
-        query = context["query"]
-        max_len = int(llm_config.rag_query_max_length)
+        query = context["query"]
+        max_len = LLMConfig().rag_query_max_length

或者在类初始化时缓存配置:

     def __init__(self, ...):
         # ... existing code ...
+        self._llm_config = LLMConfig()
         
     def run(self, context: Dict[str, Any]) -> Dict[str, Any]:
-        llm_config = LLMConfig()
         query = context["query"]
-        max_len = int(llm_config.rag_query_max_length)
+        max_len = self._llm_config.rag_query_max_length
hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py (3)

5-5: 移除未使用的导入

静态分析工具检测到 LLMConfig 导入但未使用。测试中通过 mock 处理了 LLMConfig,不需要直接导入。

 from hugegraph_llm.operators.hugegraph_op.graph_rag_query import GraphRAGQuery
-from hugegraph_llm.config.llm_config import LLMConfig
🧰 Tools
🪛 Ruff (0.11.9)

5-5: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)


70-72: 修复 f-string 格式问题

静态分析工具检测到 f-string 中没有占位符。应该移除多余的 f 前缀。

-        expected_error_message = f"Error: Query is too long. Maximum allowed length is 10 characters."
+        expected_error_message = "Error: Query is too long. Maximum allowed length is 10 characters."
         self.assertEqual(str(cm.exception), expected_error_message)
-        mock_log.error.assert_called_once_with(f"Query exceeds maximum length of 10 characters.")
+        mock_log.error.assert_called_once_with("Query exceeds maximum length of 10 characters.")
🧰 Tools
🪛 Ruff (0.11.9)

70-70: f-string without any placeholders

Remove extraneous f prefix

(F541)


72-72: f-string without any placeholders

Remove extraneous f prefix

(F541)


76-81: 简化异常处理模式

静态分析工具建议使用 contextlib.suppress 替换 try-except-pass 模式,代码更加简洁。

+        from contextlib import suppress
         # Ensure init_client was not called because the error should be raised before
         graph_rag_query_instance.init_client = MagicMock() # Assign a mock to check if it's called
-        try:
+        with suppress(ValueError):
             graph_rag_query_instance.run(context) 
-        except ValueError:
-            pass # Expected
         graph_rag_query_instance.init_client.assert_not_called()
🧰 Tools
🪛 Ruff (0.11.9)

76-79: Use contextlib.suppress(ValueError) instead of try-except-pass

(SIM105)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 434d691 and 8571116.

📒 Files selected for processing (4)
  • hugegraph-llm/README.md (1 hunks)
  • hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1 hunks)
  • hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py (2 hunks)
  • hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py (1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py

5-5: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)


70-70: f-string without any placeholders

Remove extraneous f prefix

(F541)


72-72: f-string without any placeholders

Remove extraneous f prefix

(F541)


76-79: Use contextlib.suppress(ValueError) instead of try-except-pass

(SIM105)

🪛 GitHub Actions: License header & 3rd-party check
hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py

[error] 1-1: File does not have a valid license header.

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build (3.11)
  • GitHub Check: build (3.10)
🔇 Additional comments (2)
hugegraph-llm/README.md (1)

61-62: 文档更新准确清晰

新增的环境变量文档说明了 RAG_QUERY_MAX_LENGTH 的用途和默认值,为用户提供了清晰的配置指导。

hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py (1)

8-119: 测试覆盖率完整且实现良好

测试用例很好地覆盖了查询长度验证的三种主要场景:

  • 查询长度在限制内
  • 查询长度超出限制
  • 查询长度等于限制

使用 mock 来隔离被测功能是恰当的做法,确保了测试的准确性和可靠性。

🧰 Tools
🪛 Ruff (0.11.9)

70-70: f-string without any placeholders

Remove extraneous f prefix

(F541)


72-72: f-string without any placeholders

Remove extraneous f prefix

(F541)


76-79: Use contextlib.suppress(ValueError) instead of try-except-pass

(SIM105)

Comment thread hugegraph-llm/src/hugegraph_llm/config/llm_config.py
Comment thread hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py Outdated
…eedback.

Here's what I did:
- I moved the query length checks to happen earlier, in the Gradio UI and the API.
- I removed the query length validation from `GraphRAGQuery.run()`.
- Some obsolete unit tests for `GraphRAGQuery` validation were also deleted.
- In the Gradio interfaces (`rag_block.py`), if a query is too long, I'll now show a warning and return an empty response in `rag_answer` and `rag_answer_streaming`. For `text2gremlin_block.py` (`graph_rag_recall`), I'll return an error dictionary.
- For the HTTP API endpoints (`rag_api.py`), the `/rag` and `/rag/graph` endpoints will now check the query length and will signal an error (400) if it's too long.
- I also reorganized the `hugegraph-llm/README.md` by creating a new "Environment Variables for Configuration" section to make the `.env` settings clearer.
- I added new unit tests to cover the Gradio validation in `test_rag_block.py` and the API endpoint validation in `test_rag_api.py`.
- The configuration `RAG_QUERY_MAX_LENGTH` (which defaults to 50) is used by these new validation points.
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license-eye has checked 284 files.

Valid Invalid Ignored Fixed
236 2 46 0
Click to see the invalid file list
  • hugegraph-llm/src/tests/api/test_rag_api.py
  • hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
Use this command to fix any missing license headers
```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

Comment thread hugegraph-llm/src/tests/api/test_rag_api.py
Comment thread hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (7)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py (1)

197-203: 查询长度验证实现正确,建议性能优化

验证逻辑实现正确,错误处理适当。不过建议考虑以下优化:

  1. 性能优化:每次调用都创建新的 LLMConfig() 实例可能影响性能,可以考虑在模块级别缓存配置或使用单例模式。

  2. 日志记录良好:截断查询到100字符的做法很好,既提供了调试信息又保护了用户隐私。

考虑这样优化性能:

+# 在模块顶部
+_llm_config = None
+
+def get_llm_config():
+    global _llm_config
+    if _llm_config is None:
+        _llm_config = LLMConfig()
+    return _llm_config

 def graph_rag_recall(...):
-    llm_config = LLMConfig()
+    llm_config = get_llm_config()
     max_len = int(llm_config.rag_query_max_length)
hugegraph-llm/README.md (1)

78-90: 环境变量文档详尽,建议修复格式问题

新增的环境变量配置部分提供了详细且有用的信息。不过根据静态分析工具的提示,需要修复一些markdown格式问题。

修复markdown列表缩进问题:

-    -   **Description**: Sets the maximum character length for user queries submitted to the RAG (Retrieval Augmented Generation) functionalities, including the interactive demo and APIs. Queries exceeding this length will be rejected by the system.
-    -   **Default Value**: `50`
-    -   **Example**: If set to `100`, queries longer than 100 characters will be rejected.
+  - **Description**: Sets the maximum character length for user queries submitted to the RAG (Retrieval Augmented Generation) functionalities, including the interactive demo and APIs. Queries exceeding this length will be rejected by the system.
+  - **Default Value**: `50`
+  - **Example**: If set to `100`, queries longer than 100 characters will be rejected.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~84-~84: Loose punctuation mark.
Context: ...figured: - RAG_QUERY_MAX_LENGTH: - Description: Sets the maxim...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 markdownlint-cli2 (0.17.2)

85-85: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


86-86: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


87-87: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

hugegraph-llm/src/hugegraph_llm/api/rag_api.py (2)

47-56: API查询长度验证实现正确

/rag 端点正确实现了查询长度验证,使用了适当的HTTP状态码(400 Bad Request)和错误消息。日志记录也包含了适当的查询预览。

与其他文件类似,建议避免重复创建 LLMConfig 实例以提高性能。可以考虑在模块级别缓存配置实例。


100-109: 一致的验证逻辑实现

/rag/graph 端点实现了与 /rag 端点相同的查询长度验证逻辑,保持了代码的一致性。错误处理和日志记录都很适当。

建议考虑将查询长度验证逻辑提取为一个共享的装饰器或工具函数,以减少代码重复:

def validate_query_length(query: str, endpoint_name: str):
    llm_config = LLMConfig()  # 或使用缓存实例
    max_len = int(llm_config.rag_query_max_length)
    if len(query) > max_len:
        log.warning(
            f"API query for {endpoint_name} exceeds maximum length of {max_len} characters. Query: '{query[:100]}...'"
        )
        raise HTTPException(
            status_code=status.HTTP_400_BAD_REQUEST,
            detail=f"Query is too long. Maximum allowed length is {max_len} characters.",
        )
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py (2)

52-60: 同步函数查询长度验证实现正确

rag_answer 函数中正确实现了查询长度验证。使用 gr.Warning() 为Gradio界面提供用户友好的错误提示,返回空字符串元组符合函数签名要求。

建议考虑提取验证逻辑为共享函数,减少重复代码:

def validate_query_length_for_demo(query: str) -> bool:
    """验证查询长度,返回是否有效。如果无效,会记录日志并显示Gradio警告。"""
    llm_config = LLMConfig()
    max_len = int(llm_config.rag_query_max_length)
    if len(query) > max_len:
        log.warning(f"Input query exceeds maximum length of {max_len} characters. Query: '{query[:100]}...'")
        gr.Warning(f"Query is too long! Maximum allowed length is {max_len} characters.")
        return False
    return True

173-182: 异步生成器查询长度验证实现正确

rag_answer_streaming 函数中正确实现了查询长度验证,适当地使用了 yieldreturn 来处理异步生成器的早期退出。验证逻辑与同步版本保持一致。

建议与同步版本使用相同的共享验证函数,并为异步生成器版本做适当的调整:

async def handle_invalid_query_streaming():
    """处理无效查询的异步生成器版本"""
    yield "", "", "", ""
    return

# 在函数中使用:
if not validate_query_length_for_demo(text):
    async for result in handle_invalid_query_streaming():
        yield result
    return
hugegraph-llm/src/tests/api/test_rag_api.py (1)

69-72: RAG 请求配置需要改进

当前的 RAGRequest 只设置了 raw_answer=True,但根据代码注释,这是为了确保至少请求一种答案类型。建议明确所有字段以提高测试的清晰度。

 req = RAGRequest(
     query="Short query.",
-    raw_answer=True # ensure at least one answer type is requested
+    raw_answer=True,
+    vector_only_answer=False,
+    graph_only_answer=False,
+    graph_vector_answer=False
 )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8571116 and d63808a.

📒 Files selected for processing (6)
  • hugegraph-llm/README.md (2 hunks)
  • hugegraph-llm/src/hugegraph_llm/api/rag_api.py (3 hunks)
  • hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py (3 hunks)
  • hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py (2 hunks)
  • hugegraph-llm/src/tests/api/test_rag_api.py (1 hunks)
  • hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py (1)
hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1)
  • LLMConfig (25-104)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py (2)
hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1)
  • LLMConfig (25-104)
hugegraph-ml/src/hugegraph_ml/models/seal.py (1)
  • warning (799-800)
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py

2-2: unittest.mock.AsyncMock imported but unused

Remove unused import: unittest.mock.AsyncMock

(F401)


4-4: gradio imported but unused

Remove unused import: gradio

(F401)


6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

hugegraph-llm/src/tests/api/test_rag_api.py

6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

🪛 GitHub Actions: License header & 3rd-party check
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py

[error] 1-1: File does not have a valid license header.

hugegraph-llm/src/tests/api/test_rag_api.py

[error] 1-1: File does not have a valid license header.

🪛 LanguageTool
hugegraph-llm/README.md

[uncategorized] ~84-~84: Loose punctuation mark.
Context: ...figured: - RAG_QUERY_MAX_LENGTH: - Description: Sets the maxim...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 markdownlint-cli2 (0.17.2)
hugegraph-llm/README.md

85-85: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


86-86: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


87-87: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build (3.11)
  • GitHub Check: build (3.10)
🔇 Additional comments (17)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py (1)

25-25: 导入语句正确添加

正确导入了 LLMConfig,符合新增的查询长度验证功能需求。

hugegraph-llm/README.md (1)

61-61: 文档更新清晰且有用

更新了 .env 文件的描述,为用户提供了清晰的配置指导,并添加了对新环境变量配置部分的引用。

hugegraph-llm/src/hugegraph_llm/api/rag_api.py (1)

30-30: 导入添加正确

正确导入了 LLMConfig 以支持查询长度验证功能。

hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py (1)

27-27: 导入正确添加

正确导入了 LLMConfig 以支持查询长度验证功能。

hugegraph-llm/src/tests/api/test_rag_api.py (6)

4-8: 导入声明组织良好且必要

尽管静态分析工具提示 LLMConfig 导入未使用,但实际上它在测试中通过 @patch 装饰器被使用。所有导入都是测试所必需的。

🧰 Tools
🪛 Ruff (0.11.9)

6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)


11-31: 测试设置结构合理

setUp 方法正确地初始化了所有必要的模拟对象,并且调用 rag_http_api 函数来设置路由。这种方法确保了测试的隔离性和可重复性。


32-36: 助手方法实现简洁有效

get_endpoint_function 方法提供了一种干净的方式来获取特定路径的端点函数,错误处理也很合适。


38-58: 查询长度验证测试覆盖全面

测试正确验证了当查询超过最大长度限制时:

  1. 抛出 HTTP 400 错误
  2. 错误消息准确反映限制
  3. 记录警告日志
  4. 不调用实际的 RAG 函数

测试逻辑完整且断言充分。


89-108: 图RAG召回API测试实现正确

测试覆盖了 /rag/graph 端点的查询长度验证,断言逻辑与RAG答案API测试保持一致,确保了行为的统一性。


120-121: 模拟返回值结构合理

为图RAG召回功能设置的模拟返回值包含了预期的 keywordsmatch_vids 字段,符合API的响应结构。

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (7)

1-7: 导入声明使用正确

尽管静态分析工具提示某些导入未使用,但它们实际上都是必要的:

  • AsyncMock 虽然未直接使用,但为异步测试提供了类型支持
  • gradio 通过 gr.Warning 在模拟中使用
  • LLMConfig 通过 @patch 装饰器使用

建议忽略这些静态分析警告。

🧰 Tools
🪛 Ruff (0.11.9)

2-2: unittest.mock.AsyncMock imported but unused

Remove unused import: unittest.mock.AsyncMock

(F401)


4-4: gradio imported but unused

Remove unused import: gradio

(F401)


6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

🪛 GitHub Actions: License header & 3rd-party check

[error] 1-1: File does not have a valid license header.


12-26: 通用RAG参数助手方法设计优秀

_get_common_rag_args 方法提供了一套完整的RAG函数参数,确保测试的一致性和可维护性。参数配置覆盖了各种RAG选项。


28-44: 同步查询长度验证测试实现正确

测试正确验证了当查询超长时:

  1. 触发 Gradio 警告
  2. 记录警告日志
  3. 返回空元组结果

模拟和断言都很到位。


56-61: RAG管道模拟配置合理

模拟的 RAGPipeline 返回值结构包含了所有预期的答案类型,与实际函数的返回格式保持一致。


81-97: 异步查询长度验证测试实现正确

异步测试正确使用了 async for 来收集流式结果,并验证了相同的错误处理逻辑。测试结构与同步版本保持一致。


117-124: 异步生成器模拟实现优雅

使用本地异步生成器函数来模拟 run_streaming 方法是一种很好的做法,正确模拟了异步流式响应的行为。


129-140: 异步测试断言完整

测试正确验证了:

  1. 没有触发警告
  2. 各个组件被正确调用
  3. 收集到预期数量的结果
  4. 结果内容符合预期

异步测试的实现很扎实。

Comment thread hugegraph-llm/src/tests/api/test_rag_api.py
Comment thread hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@imbajin
Copy link
Copy Markdown
Author

imbajin commented May 28, 2025

@codecov-ai-reviewer review

@ghost
Copy link
Copy Markdown

ghost commented May 28, 2025

On it! We are reviewing the PR and will provide feedback shortly.

@ghost
Copy link
Copy Markdown

ghost commented May 28, 2025

PR Description

This pull request introduces a query length limitation feature to the RAG (Retrieval Augmented Generation) functionality. The goal is to prevent resource exhaustion, maintain performance, and mitigate potential security risks associated with excessively long queries. This limitation is applied consistently across both the web UI demo and the API endpoints.

Click to see more

Key Technical Changes

The key technical changes include:

  1. Introduction of a new environment variable RAG_QUERY_MAX_LENGTH to configure the maximum allowed length for user queries. The default value is set to 50 characters.
  2. Implementation of query length validation logic in the API endpoints (/rag and /rag/graph) and the Gradio-based UI components (rag_answer, rag_answer_streaming, and graph_rag_recall).
  3. Addition of unit tests to verify the correct behavior of the query length validation logic, including tests for exceeding the limit, staying within the limit, and handling edge cases.
  4. Updates to the documentation to explain the new configuration option and its purpose.

Architecture Decisions

The primary architectural decision was to implement the query length limitation as a configurable parameter via an environment variable. This allows administrators to easily adjust the limit based on their specific resource constraints and security requirements. The validation logic is implemented directly within the API endpoints and UI components to provide immediate feedback to the user and prevent unnecessary processing of long queries. The LLMConfig class is used to centralize access to the configuration parameter.

Dependencies and Interactions

This pull request introduces a dependency on the LLMConfig class for accessing the RAG_QUERY_MAX_LENGTH configuration parameter. It interacts with the FastAPI framework for defining API endpoints and handling HTTP exceptions. It also interacts with the Gradio library for displaying warnings in the UI. The changes affect the RAG pipeline by potentially preventing the execution of long queries.

Risk Considerations

Potential risks and considerations include:

  1. The chosen default value of 50 characters may be too restrictive for some use cases. It's important to monitor user feedback and adjust the default value if necessary.
  2. The query length limitation may negatively impact the user experience if users are unable to submit complex queries. It's important to provide clear and informative error messages to guide users in formulating shorter queries.
  3. The implementation relies on the correct parsing of the RAG_QUERY_MAX_LENGTH environment variable. It's important to handle potential parsing errors gracefully and provide a reasonable default value in case of failure.

Notable Implementation Details

Notable implementation details include:

  1. The use of HTTPException in the API endpoints to return a 400 Bad Request error when a query exceeds the length limit.
  2. The use of gr.Warning in the Gradio UI components to display a warning message to the user.
  3. The truncation of long queries in the log messages to prevent excessively long log entries.
  4. The use of mocking in the unit tests to isolate the query length validation logic and verify its behavior.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
hugegraph-llm/src/tests/api/test_rag_api.py (1)

19-19: 清理未使用的导入

静态分析工具指出 LLMConfig 导入但未直接使用。虽然在 patch 装饰器中引用了它,但可以考虑移除这个导入,因为 patch 可以使用字符串路径。

-from hugegraph_llm.config.llm_config import LLMConfig

然后在 patch 装饰器中使用字符串路径即可正常工作。

🧰 Tools
🪛 Ruff (0.11.9)

19-19: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)

15-19: 清理未使用的导入

静态分析工具标识了几个未使用的导入:

  1. AsyncMock - 在代码中未直接使用
  2. gradio - 虽然注释说用于类型提示,但实际只在 patch 中使用
  3. LLMConfig - 类似于第一个文件,只在 patch 装饰器中使用
-from unittest.mock import patch, MagicMock, AsyncMock
+from unittest.mock import patch, MagicMock

-import gradio as gr # Import for type hinting, will be mocked

-from hugegraph_llm.config.llm_config import LLMConfig

在 patch 装饰器中可以直接使用字符串路径,无需导入这些模块。

🧰 Tools
🪛 Ruff (0.11.9)

15-15: unittest.mock.AsyncMock imported but unused

Remove unused import: unittest.mock.AsyncMock

(F401)


17-17: gradio imported but unused

Remove unused import: gradio

(F401)


19-19: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d63808a and abb1aa1.

📒 Files selected for processing (3)
  • hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1 hunks)
  • hugegraph-llm/src/tests/api/test_rag_api.py (1 hunks)
  • hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • hugegraph-llm/src/hugegraph_llm/config/llm_config.py
🧰 Additional context used
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/api/test_rag_api.py

19-19: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py

15-15: unittest.mock.AsyncMock imported but unused

Remove unused import: unittest.mock.AsyncMock

(F401)


17-17: gradio imported but unused

Remove unused import: gradio

(F401)


19-19: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build (3.10)
  • GitHub Check: build (3.11)
🔇 Additional comments (2)
hugegraph-llm/src/tests/api/test_rag_api.py (1)

24-149: 测试覆盖度全面且实现正确

这个测试类很好地验证了新增的查询长度限制功能:

  1. 正确测试了查询过长时返回 HTTP 400 错误
  2. 验证了有效查询能正常调用相应函数并返回预期响应
  3. 同时覆盖了 /rag/rag/graph 两个端点
  4. 使用了适当的模拟策略来隔离测试逻辑

测试逻辑准确,模拟配置合理,能够有效验证 API 层面的查询长度验证功能。

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)

23-154: 测试设计优秀且实现全面

这个测试类展现了以下优秀特点:

  1. 全面的测试覆盖:同时测试了同步和异步函数的查询长度验证
  2. 正确的异步测试实现:使用 async for 正确测试异步生成器函数
  3. 完善的模拟策略:广泛模拟了 RAGPipeline、AnswerSynthesize、Gradio 警告和日志记录
  4. 清晰的测试逻辑:分别验证查询过长和有效查询的不同行为
  5. 良好的测试组织:使用辅助方法 _get_common_rag_args() 减少代码重复

特别值得称赞的是异步流式函数的测试实现,正确地收集了所有生成的结果并进行验证。

This commit refactors query length validation to use Pydantic's
built-in validation mechanisms within the request models,
as per your feedback for a cleaner API design.

Key changes:
- Removed manual query length validation logic from the API endpoint
  functions in `hugegraph-llm/src/hugegraph_llm/api/rag_api.py`.
- Added `@validator` methods to `RAGRequest` and `GraphRAGRequest`
  in `hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py`.
  These validators check the `query` field length against
  `rag_query_max_length` from `LLMConfig` and raise a `ValueError`
  if the query is too long. FastAPI converts these to HTTP 422 responses.
- Updated API unit tests in `hugegraph-llm/src/tests/api/test_rag_api.py`
  to expect HTTP 422 Unprocessable Entity errors for query length
  violations and adjusted mocking strategies accordingly.
- Gradio UI continues to use its own explicit checks for immediate
  user feedback (`gr.Warning`).
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
hugegraph-llm/src/tests/api/test_rag_api.py (1)

1-1: ⚠️ Potential issue

缺少许可证头部信息

根据管道失败日志和之前的审查评论,此文件缺少有效的许可证头部信息。

请在文件开头添加标准的 Apache 许可证头部:

+# 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 unittest
🧰 Tools
🪛 GitHub Actions: License header & 3rd-party check

[error] 1-1: File does not have a valid license header.

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)

1-1: ⚠️ Potential issue

缺少许可证头部信息

根据管道失败日志和之前的审查评论,此文件缺少有效的许可证头部信息。

请在文件开头添加标准的 Apache 许可证头部:

+# 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 unittest
🧰 Tools
🪛 GitHub Actions: License header & 3rd-party check

[error] 1-1: File does not have a valid license header.

🧹 Nitpick comments (4)
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py (1)

65-71: 优化验证器性能并减少代码重复

查询长度验证逻辑是正确的,但可以进一步优化:

  1. 性能问题:每次验证都创建新的 LLMConfig 实例可能影响性能
  2. 不必要的类型转换rag_query_max_length 已经是 int 类型,无需再次转换
  3. 代码重复:该验证器在 GraphRAGRequest 中完全重复
+# 在文件顶部添加全局配置实例
+_llm_config = LLMConfig()
+
 @validator('query')
 def check_query_length(cls, value):
-    llm_config = LLMConfig()
-    max_len = int(llm_config.rag_query_max_length)
+    max_len = _llm_config.rag_query_max_length
     if len(value) > max_len:
         raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
     return value

或者考虑将验证逻辑提取为共享函数以减少重复。

hugegraph-llm/src/tests/api/test_rag_api.py (2)

6-6: 移除未使用的导入

根据静态分析,LLMConfig 导入未被直接使用,应当移除以保持代码整洁。

-from hugegraph_llm.config.llm_config import LLMConfig
🧰 Tools
🪛 Ruff (0.11.9)

6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)


40-40: 移除未使用的变量

根据静态分析,这些本地变量被赋值但从未使用。

-rag_answer_api_endpoint = self.get_endpoint_function("/rag")
+self.get_endpoint_function("/rag")  # 验证端点存在

-graph_rag_recall_api_endpoint = self.get_endpoint_function("/rag/graph")
+self.get_endpoint_function("/rag/graph")  # 验证端点存在

Also applies to: 124-124

🧰 Tools
🪛 Ruff (0.11.9)

40-40: Local variable rag_answer_api_endpoint is assigned to but never used

Remove assignment to unused variable rag_answer_api_endpoint

(F841)

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)

2-2: 清理未使用的导入

根据静态分析,存在多个未使用的导入,应当移除以保持代码整洁。

-from unittest.mock import patch, MagicMock, AsyncMock
+from unittest.mock import patch, MagicMock

-import gradio as gr # Import for type hinting, will be mocked

-from hugegraph_llm.config.llm_config import LLMConfig

AsyncMockgradioLLMConfig 在测试中都被模拟,不需要直接导入。

Also applies to: 4-4, 6-6

🧰 Tools
🪛 Ruff (0.11.9)

2-2: unittest.mock.AsyncMock imported but unused

Remove unused import: unittest.mock.AsyncMock

(F401)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between abb1aa1 and 5ea06a3.

📒 Files selected for processing (4)
  • hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py (3 hunks)
  • hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1 hunks)
  • hugegraph-llm/src/tests/api/test_rag_api.py (1 hunks)
  • hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • hugegraph-llm/src/hugegraph_llm/config/llm_config.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py (1)
hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1)
  • LLMConfig (25-104)
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/api/test_rag_api.py

6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)


40-40: Local variable rag_answer_api_endpoint is assigned to but never used

Remove assignment to unused variable rag_answer_api_endpoint

(F841)


85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


124-124: Local variable graph_rag_recall_api_endpoint is assigned to but never used

Remove assignment to unused variable graph_rag_recall_api_endpoint

(F841)


134-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py

2-2: unittest.mock.AsyncMock imported but unused

Remove unused import: unittest.mock.AsyncMock

(F401)


4-4: gradio imported but unused

Remove unused import: gradio

(F401)


6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused

Remove unused import: hugegraph_llm.config.llm_config.LLMConfig

(F401)

🪛 GitHub Actions: License header & 3rd-party check
hugegraph-llm/src/tests/api/test_rag_api.py

[error] 1-1: File does not have a valid license header.

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py

[error] 1-1: File does not have a valid license header.

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build (3.11)
  • GitHub Check: build (3.10)
🔇 Additional comments (3)
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py (1)

21-21: 导入语句正确

新增的 validatorLLMConfig 导入是正确的,支持新的查询长度验证功能。

Also applies to: 23-23

hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (2)

31-44: 测试逻辑正确且全面

查询长度超限的测试逻辑实现得很好:

  • 正确模拟了 LLMConfig 和相关依赖
  • 验证了 Gradio 警告和日志记录的调用
  • 确认返回了正确的空结果

同步和异步版本的测试都覆盖了相同的场景,确保了一致性。

Also applies to: 81-97


50-76: 测试覆盖了正常流程

正常查询长度的测试很好地验证了:

  • RAG 管道的正确调用
  • 模拟结果的正确返回
  • 没有触发警告或错误

异步流式测试特别好地处理了异步生成器的测试,确保流式响应的正确性。

Also applies to: 104-141

Comment on lines +99 to +105
@validator('query')
def check_query_length(cls, value):
llm_config = LLMConfig()
max_len = int(llm_config.rag_query_max_length)
if len(value) > max_len:
raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
return value
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

代码重复:与 RAGRequest 中的验证器相同

此验证器与 RAGRequest 中的验证器完全相同,存在代码重复问题。

建议将验证逻辑提取为共享函数:

+def validate_query_length(value: str) -> str:
+    """共享的查询长度验证函数"""
+    llm_config = LLMConfig()
+    max_len = llm_config.rag_query_max_length
+    if len(value) > max_len:
+        raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
+    return value
+
 @validator('query')
 def check_query_length(cls, value):
-    llm_config = LLMConfig()
-    max_len = int(llm_config.rag_query_max_length)
-    if len(value) > max_len:
-        raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
-    return value
+    return validate_query_length(value)

然后在 RAGRequest 中使用相同的共享函数。

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@validator('query')
def check_query_length(cls, value):
llm_config = LLMConfig()
max_len = int(llm_config.rag_query_max_length)
if len(value) > max_len:
raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
return value
# Shared helper for query‐length validation
def validate_query_length(value: str) -> str:
"""共享的查询长度验证函数"""
llm_config = LLMConfig()
max_len = llm_config.rag_query_max_length
if len(value) > max_len:
raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
return value
# In your Pydantic model:
@validator('query')
def check_query_length(cls, value):
return validate_query_length(value)
🤖 Prompt for AI Agents
In hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py around lines 99 to
105, the query length validator duplicates the same logic found in RAGRequest,
causing code repetition. Extract the query length validation logic into a shared
helper function and then call this function from both the current validator and
the one in RAGRequest to eliminate duplication.

Comment on lines +46 to +86
with self.assertRaises(HTTPException) as cm:
# Directly instantiating RAGRequest with invalid data won't raise HTTPException here,
# FastAPI does this when processing the request.
# To simulate FastAPI's behavior, we assume the endpoint is called with data
# that *would* cause Pydantic to fail during request body parsing.
# The actual RAGRequest instantiation happens inside FastAPI's request handling.
# For a unit test, we are directly calling the endpoint function.
# Pydantic validation for path/query/body parameters is typically handled by
# FastAPI's request parsing layer *before* the endpoint function is called.
# However, if the endpoint function itself receives the raw request model and
# Pydantic validation happens upon model instantiation *within* the endpoint,
# then the test structure is fine. Given the current structure of FastAPI,
# the validation for RAGRequest happens *before* rag_answer_api is called.
# This test simulates the state *after* FastAPI has parsed and validated,
# and if validation failed, it would have raised HTTPException(422).
# Since we are calling the function directly, we must ensure the Pydantic model
# itself raises an error that FastAPI would catch and convert to 422.
# Let's assume the endpoint *receives* an already validated model or the validation
# is part of the endpoint for this test to make sense as written.
# The instructions imply Pydantic validation in the model will lead to a 422.
# This means FastAPI's handling of Pydantic's ValueError.
# We will construct the request, and the endpoint call will internally trigger validation
# if the model is instantiated there, or FastAPI handles it if passed as type hint.
# For this test to be accurate to FastAPI behavior for request body validation:
# We should not expect to catch HTTPException directly from RAGRequest instantiation here.
# Instead, the endpoint call should be the one raising it due to FastAPI's processing.
# The current test structure where `rag_answer_api_endpoint(req)` is called is correct
# if we assume FastAPI passes a validated model or the model instantiation happens inside.
# Given Pydantic validator raises ValueError, FastAPI converts this to HTTP 422.

# Simulate calling the endpoint which would trigger Pydantic validation via FastAPI
# For the purpose of this unit test, we'll assume the Pydantic model validation
# error (ValueError) is caught by FastAPI and results in an HTTPException(422).
# Since we call the endpoint function directly, we need to simulate this.
# The most direct way to test the Pydantic validator itself is to instantiate the model.
try:
RAGRequest(query="This is a very long query that exceeds the limit.")
except ValueError as e: # Pydantic validator raises ValueError
# FastAPI would catch this and convert it to HTTPException 422
raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

简化测试逻辑并改进异常处理

当前的测试逻辑过于复杂,试图模拟 FastAPI 的内部行为。同时,异常处理可以改进。

简化测试逻辑:

 with self.assertRaises(HTTPException) as cm:
-    # 移除复杂的注释和说明
-    try:
-        RAGRequest(query="This is a very long query that exceeds the limit.")
-    except ValueError as e: # Pydantic validator raises ValueError
-         # FastAPI would catch this and convert it to HTTPException 422
-        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e))
+    try:
+        RAGRequest(query="This is a very long query that exceeds the limit.")
+    except ValueError as e:
+        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) from e

类似地修复第二个测试中的异常处理:

     try:
         GraphRAGRequest(query="This is a very long query for graph recall that exceeds limit.")
     except ValueError as e:
-        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e))
+        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) from e

Also applies to: 129-135

🧰 Tools
🪛 Ruff (0.11.9)

85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🤖 Prompt for AI Agents
In hugegraph-llm/src/tests/api/test_rag_api.py around lines 46 to 86 and also
lines 129 to 135, the test logic is overly complex trying to simulate FastAPI's
internal validation and exception handling. Simplify the test by directly
instantiating the RAGRequest model with invalid data and asserting that it
raises a ValueError, which is the actual Pydantic validation error. Then
separately test that the endpoint raises HTTPException with status 422 when
given invalid input, avoiding nested try-except blocks and unnecessary comments
about FastAPI internals. This will make the tests clearer and more maintainable.

@imbajin imbajin force-pushed the main branch 4 times, most recently from 6a76d6c to fe94881 Compare July 28, 2025 07:34
text2gql_llm_type: Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"] = "openai"
embedding_type: Optional[Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"]] = "openai"
reranker_type: Optional[Literal["cohere", "siliconflow"]] = None
rag_query_max_length: int = os.environ.get("RAG_QUERY_MAX_LENGTH", 50)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 [Critical] Missing proper type conversion for environment variable

The environment variable is retrieved without proper conversion to int. When RAG_QUERY_MAX_LENGTH is set in the environment, it will be a string, but the default value is an integer (50), causing type inconsistency.

Suggested change
rag_query_max_length: int = os.environ.get("RAG_QUERY_MAX_LENGTH", 50)
rag_query_max_length: int = int(os.environ.get("RAG_QUERY_MAX_LENGTH", "50"))

This ensures consistent integer type regardless of whether the value comes from environment or default.

llm_config = LLMConfig()
max_len = int(llm_config.rag_query_max_length)
if len(value) > max_len:
raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [Medium] Duplicated validation logic between RAGRequest and GraphRAGRequest

Both classes have identical check_query_length validators. Consider extracting to a base class or mixin to reduce duplication and ensure consistency:

class QueryValidationMixin:
    @validator('query')
    def check_query_length(cls, value):
        llm_config = LLMConfig()
        max_len = int(llm_config.rag_query_max_length)
        if len(value) > max_len:
            raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
        return value

class RAGRequest(BaseModel, QueryValidationMixin):
    # ... existing fields

class GraphRAGRequest(BaseModel, QueryValidationMixin):
    # ... existing fields

This would make future maintenance easier and ensure both validators stay in sync.

text2gql_llm_type: Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"] = "openai"
embedding_type: Optional[Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"]] = "openai"
reranker_type: Optional[Literal["cohere", "siliconflow"]] = None
rag_query_max_length: int = os.environ.get("RAG_QUERY_MAX_LENGTH", 50)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 [Critical] Type conversion error in llm_config.py line 33

os.environ.get() returns string but assigned to int field without conversion. Will cause TypeError at runtime.

Fix: rag_query_max_length: int = int(os.environ.get('RAG_QUERY_MAX_LENGTH', '50'))

description="Prompt for the Text2Gremlin query.",
)

@validator('query')
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [Medium] Duplicated validation logic violates DRY principle

The same query length validation is duplicated here and in rag_requests.py lines 99-105. Consider extracting to a shared validation utility function to improve maintainability.

Suggested approach: Create a validate_query_length() utility that can be reused across all validation points.

log.warning(
f"Input query for graph_rag_recall exceeds maximum length of {max_len} characters. Query: '{query[:100]}...'"
)
return {"error": f"Query is too long! Maximum allowed length is {max_len} characters.", "graph_result": []}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [Medium] Inconsistent error handling breaks API contract

This function returns a dict with error key instead of raising an exception like other validation points. This inconsistency makes error handling unpredictable for API consumers.

Should either raise a proper exception or document this different behavior clearly.

# if we assume FastAPI passes a validated model or the model instantiation happens inside.
# Given Pydantic validator raises ValueError, FastAPI converts this to HTTP 422.

# Simulate calling the endpoint which would trigger Pydantic validation via FastAPI
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [Medium] Test doesn't properly validate FastAPI behavior

This test manually creates HTTPException to simulate FastAPI's validation, but doesn't actually test the real request flow. Should use FastAPI's TestClient for proper integration testing to ensure validation works correctly in production.

Example:

from fastapi.testclient import TestClient
client = TestClient(app)
response = client.post('/rag', json={'query': long_query})
assert response.status_code == 422

def check_query_length(cls, value):
llm_config = LLMConfig()
max_len = int(llm_config.rag_query_max_length)
if len(value) > max_len:
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 [Minor] Missing edge case validation

No validation for negative or zero values. Consider adding:

if not value or len(value.strip()) == 0:
    raise ValueError('Query cannot be empty')
if max_len <= 0:
    raise ValueError('Invalid max length configuration')

@imbajin
Copy link
Copy Markdown
Author

imbajin commented Sep 29, 2025

Code Review Summary

Overall Assessment: Changes Required

Key Findings

  • 🚨 Critical: 1 issue (type conversion runtime error)
  • ⚠️ Medium: 4 issues (code duplication, inconsistent errors, test structure)
  • 🧹 Minor: 2 issues (magic numbers, edge cases)

Critical Issue

llm_config.py line 33: os.environ.get returns string but assigned to int field. Will cause TypeError. Must add int() conversion.

Main Improvements Needed

  1. Fix type conversion bug (blocker)
  2. Extract duplicated validation to shared utility
  3. Make error handling consistent
  4. Use TestClient for proper test coverage
  5. Define constants for magic numbers

See line-level comments for specific fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants