Skip to content

BACKEND_CODE_REVIEW.md

Latest commit

 

History

History
216 lines (138 loc) · 10.6 KB

BACKEND_CODE_REVIEW.md

File metadata and controls

216 lines (138 loc) · 10.6 KB

Code Review Summary

基于 anything-extract/.agents/skills/backend-code-review 对 anything-extract/backend 的后端代码进行了审查，范围包括：db schema、architecture、data access、SQLAlchemy 使用及通用安全/性能/质量规则。

Found 4 critical issues that need to be fixed:

🔴 Critical (Must Fix)

1. 迁移/DDL 中表名与列名拼接导致潜在 SQL 注入

FilePath: backend/core/database.py line 396-402

def _ensure_column(db, inspector, table_name: str, column_name: str, column_sql: str) -> None:
    columns = {col["name"] for col in inspector.get_columns(table_name)}
    if column_name in columns:
        return
    db.execute(text(f"ALTER TABLE {table_name} ADD COLUMN {column_name} {column_sql}"))
    db.commit()

Explanation

  • 规则要求：迁移逻辑使用原始 SQL 时须避免字符串拼接表名/列名导致注入（references/db-schema-rule.md）。
  • 当前 table_name、column_name、column_sql 均直接拼入 SQL，若将来从配置或外部传入则存在注入风险；且不符合“列名/表名若来自变量，需白名单校验”的实践。

Suggested Fix

  1. 对 table_name 和 column_name 做白名单校验（只允许已知表名、已知列名集合）。
  2. 例如：定义 ALLOWED_TABLES = {"knowledge_bases", "documents", ...} 和每表允许的列名集合，校验后再拼接；或仅在 init_db 内用字面量调用，并加注释说明“禁止将此处参数改为外部输入”。

2. 自建 Session 在异常时未 rollback，可能导致连接/事务状态异常

FilePath: backend/workers/ingest_worker.py line 34-43

db = SessionLocal()
try:
    processed = await self.queue_service.process_next_job(db, self.worker_id)
    ...
except Exception as exc:
    document_logger.error(...)
    processed = False
finally:
    db.close()

Explanation

  • 规则要求：在 worker/后台任务内使用 SessionLocal() 时，必须在异常时 rollback()，并在 finally 中 close()（references/sqlalchemy-rule.md）。
  • 当前在 except 中未调用 db.rollback()。若 process_next_job 或其内部在 commit 之后、或在不一致状态下抛出，仅 close() 可能留下未回滚的事务，增加连接/状态异常风险。

Suggested Fix

  1. 在 except 中调用 db.rollback()，再 re-raise 或继续。
  2. 示例：
except Exception as exc:
    document_logger.error("Ingest worker loop error: %s", exc, exc_info=True)
    try:
        db.rollback()
    except Exception:
        pass
    processed = False
finally:
    db.close()

3. utils 层依赖 service，违反“utils 与业务解耦”

FilePath: backend/utils/document_parser.py line 8-9

from services.qanything_parser_bridge import QAnythingParserBridge
from services.runtime_config_service import runtime_config_service

Explanation

  • 规则要求：utils 应为可复用通用工具，不包含产品/领域规则、工作流编排或业务决策；禁止在 utils 中引用 service、core 业务逻辑（references/architecture-rule.md）。
  • DocumentParser 依赖 QAnythingParserBridge 与 runtime_config_service，将解析桥接与运行时配置耦合在 utils 中，不利于复用与单测，且依赖方向不符合“utils 保持与业务解耦”。

Suggested Fix

  1. 将“解析 + 桥接 + 配置”的编排放入 service 或 core（例如 services/document_parser_service.py 或 core/parsing/），内部依赖 QAnythingParserBridge 与 runtime_config_service。
  2. 将 utils/document_parser.py 收缩为纯解析工具（仅接受 file_path、file_type、以及由调用方传入的配置/模式参数），不直接 import services。

4. init_db 中 UPDATE 使用字符串拼接而非参数化

FilePath: backend/core/database.py line 352-358

db.execute(
    text(
        f"UPDATE documents SET knowledge_base_id = '{default_kb.id}' "
        "WHERE knowledge_base_id IS NULL"
    )
)
db.commit()

Explanation

  • 规则要求：迁移逻辑使用原始 SQL 时需避免字符串拼接；默认值需与当前数据库语法兼容（references/db-schema-rule.md）。
  • default_kb.id 虽来自当前查询结果（非用户输入），但用 f-string 拼入 SQL 不符合“参数化”的最佳实践，且若将来对值做转义疏漏可能引入风险。

Suggested Fix

  1. 使用 SQLAlchemy 参数化：text("UPDATE documents SET knowledge_base_id = :kb_id WHERE knowledge_base_id IS NULL").bindparams(kb_id=default_kb.id)，然后 db.execute(...)。
  2. 或在同一事务内用 ORM：db.query(Document).filter(Document.knowledge_base_id.is_(None)).update({Document.knowledge_base_id: default_kb.id}, synchronize_session=False)。

Found 5 suggestions for improvement:

🟡 Suggestions (Should Consider)

1. 知识库相关路由中复杂数据访问与多次 commit 应收敛到 service

FilePath: backend/app/api/knowledge_bases.py

  • 多处路由内直接进行多步 db.query、db.commit、db.refresh（如 rename_knowledge_base_document、patch_document_settings、create_document_segment、delete_document_segments、patch_document_segment、patch_documents_status_batch、reindex_document、get_document_indexing_status、get_batch_indexing_status 等）。
  • 规则建议：业务逻辑与写操作、事务边界应由 service 统一负责；router 保持薄层（references/architecture-rule.md、data-access-rule.md）。

Suggested Fix

  • 将“重命名文档”“更新文档设置”“创建/删除/更新分段”“批量状态变更”“重新入队”“索引/批次状态查询”等封装为 KnowledgeBaseService 或 DocumentService 的方法，router 只做参数校验、调用 service、映射异常到 HTTP。

2. 全局文档接口 GET/DELETE by document_id 未按知识库隔离

FilePath: backend/app/api/documents.py

  • get_document(document_id)、get_document_status(document_id)、delete_document(document_id)、retry_document_ingest(document_id) 等仅按 document_id 查询，未带 knowledge_base_id。
  • 规则说明：若接口设计为「全局 document」、单用户场景可不强制带 kb_id；若未来做多知识库/权限隔离，应在 service 层增加文档归属或权限校验（references/sqlalchemy-rule.md）。

Suggested Fix

  • 在 DocumentService 中为“按 document_id 访问”提供显式方法（如 get_document_by_id），并在注释或后续迭代中约定：多租户/多知识库时在此处增加 knowledge_base_id 或权限校验。

3. 文档列表与详情中重复的“文档 + ingest_job + hit_count/segment_count”逻辑

FilePath: backend/app/api/knowledge_bases.py（如 rename_knowledge_base_document、patch_document_settings 等）

  • 多处“查 document → 再查 job、hit_count/segment_count → 序列化”的重复模式。
  • 规则建议：复杂或重复的查询应收敛到 service 的单一方法（references/data-access-rule.md）。

Suggested Fix

  • 已有 kb_service.get_document_detail 可复用；对“仅需 document + job + hit_count”的返回，可增加类似 get_document_with_summary(db, kb_id, doc_id)，避免在路由中重复拼装。

4. DocumentIngestJob 未冗余 knowledge_base_id

FilePath: backend/core/database.py（模型 DocumentIngestJob）

  • 与知识库绑定的实体建议包含 knowledge_base_id，便于隔离查询与扩展（references/db-schema-rule.md）。
  • 当前通过 document_id → Document → knowledge_base_id 间接关联；若需按知识库筛任务或做清理，可考虑在 DocumentIngestJob 上冗余 knowledge_base_id 并参与索引。

Suggested Fix

  • 在迁移中为 document_ingest_jobs 增加 knowledge_base_id（可从 Document 回填），并在创建/更新 job 时维护；查询按 kb 过滤时可走该字段。

5. _ensure_column 的表名/列名建议显式白名单

FilePath: backend/core/database.py _ensure_column 及 init_db 调用处

  • 即便当前调用均为字面量，为满足“列名/表名若来自变量，需白名单校验”的规范，建议对 table_name、column_name 做显式白名单校验，防止日后误传变量。

Suggested Fix

  • 定义常量集合（如 INIT_DB_TABLES、每表允许的 INIT_DB_COLUMNS[table_name]），在 _ensure_column 开头校验 table_name in INIT_DB_TABLES 且 column_name in INIT_DB_COLUMNS.get(table_name, set())，不通过则 raise ValueError。

Found 2 optional nits:

🟢 Nits (Optional)

1. 处理规则创建逻辑重复

  • backend/app/api/knowledge_bases.py 中 _ensure_kb_process_rule 与 backend/services/knowledge_base_service.py 中 _ensure_kb_process_rule 逻辑重复。
  • 可只保留 service 内实现，API 层通过 kb_service 或单独 helper 获取/创建规则，减少重复与分歧。

2. 部分路由多次 db.refresh

  • 部分路由在 commit 后对多个实体分别 db.refresh(...)。
  • 若 service 返回已组装的 DTO，可减少在 router 内对 ORM 的 refresh 次数，由 service 在 commit 后一次性 refresh 并映射为响应结构。

✅ What's Good

  • 分层与依赖方向：IndexingRunner.run(db, document) 通过参数接收 db 与 document，不依赖 Web/Request，符合“core 不依赖 app/api”的架构规则。
  • 知识库删除：删除知识库已委托给 KnowledgeBaseService.delete_knowledge_base，router 仅做异常到 HTTP 的映射，符合“业务逻辑在 service、router 薄层”。
  • 文档创建与入队：create_document_for_knowledge_base、文档上传后的入队均通过 service/ingest_queue_service 完成，职责清晰。
  • 按知识库隔离的查询：get_knowledge_base_document、get_document_segments、reindex_document 等均带 kb_id + doc_id，且查询条件包含 knowledge_base_id，符合“涉及知识库资源的查询必须带 knowledge_base_id”。
  • 模型 @property：Document.data_source_info_dict、retrieval_model_dict、rules_dict、keywords_list 等仅基于当前行已加载字段做 JSON 解析，未在 property 内查询其他表或新建 Session，符合 db-schema 规则。
  • 写路径 commit：主要写路径（knowledge_base_service、document_service、ingest_queue_service、init_db）均在适当位置显式 commit()，事务边界清晰。
  • Worker 内 Session 使用：ingest_worker 在 finally 中统一 db.close()，仅缺异常时 rollback（已列为 Critical 修复项）。

如需我按上述建议直接改代码，可以指定优先项（例如先做 Critical 1、2、4 和 Suggestion 1）。