fix: align knowledge base CRUD API contract with kb_name, canonical payload fields, and matching OpenAPI types#9000
Merged
Conversation
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The legacy
name→kb_namenormalization now exists both inKnowledgeBaseService._canonical_kb_payloadandKnowledgeBaseRequest.canonical_payload; consider centralizing this logic in one place (e.g., only the schema helper) to avoid future drift between the two paths. - Instead of manually handling the legacy
namefield viagetattr(self, "name", None)andpayload.get("name"), you could model it explicitly in the Pydantic schema with aField(alias="name")or similar, which would make the legacy compatibility more self-documenting and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The legacy `name` → `kb_name` normalization now exists both in `KnowledgeBaseService._canonical_kb_payload` and `KnowledgeBaseRequest.canonical_payload`; consider centralizing this logic in one place (e.g., only the schema helper) to avoid future drift between the two paths.
- Instead of manually handling the legacy `name` field via `getattr(self, "name", None)` and `payload.get("name")`, you could model it explicitly in the Pydantic schema with a `Field(alias="name")` or similar, which would make the legacy compatibility more self-documenting and less error-prone.
## Individual Comments
### Comment 1
<location path="tests/unit/test_knowledge_base_service_contract.py" line_range="187" />
<code_context>
+@pytest.mark.asyncio
+async def test_create_kb_accepts_legacy_name_field():
</code_context>
<issue_to_address>
**suggestion (testing):** Extend create_kb tests to cover validation errors (missing names / embedding provider).
To round this out, please also add tests for validation failures:
- When both `kb_name` and `name` are missing/empty, `create_kb` should raise `KnowledgeBaseServiceError("知识库名称不能为空")`.
- When `embedding_provider_id` is missing or invalid (provider manager returns `None`), the service should raise the current error used in that case.
These will ensure the service enforces the new create semantics and catches regressions in validation behavior.
</issue_to_address>
### Comment 2
<location path="astrbot/dashboard/services/knowledge_base_service.py" line_range="32" />
<code_context>
def _payload(data: object) -> dict[str, Any]:
return data if isinstance(data, dict) else {}
+ @staticmethod
+ def _canonical_kb_payload(data: object) -> dict[str, Any]:
+ """Normalize knowledge base create/update payloads.
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing request canonicalization in the schema and making `update_kb`’s update fields explicit to avoid indirection and duplicated normalization logic.
You can simplify both of the flagged areas without losing any of the new behavior.
### 1. Avoid duplicating canonicalization logic
Instead of maintaining `_canonical_kb_payload` and a separate `canonical_payload` on the schema, centralize the logic in the schema and call it here.
For example, if you already have `KnowledgeBaseRequest.canonical_payload()`:
```python
# service.py
from .schemas import KnowledgeBaseRequest # adjust import
@staticmethod
def _canonical_kb_payload(data: object) -> dict[str, Any]:
raw = KnowledgeBaseService._payload(data)
req = KnowledgeBaseRequest(**raw)
return req.canonical_payload()
```
Or, if you can call the schema directly where you currently use `_canonical_kb_payload`, remove the helper entirely and keep the service dealing only with canonical dicts:
```python
async def create_kb(self, data: object) -> tuple[dict[str, Any], str]:
kb_manager = self.get_kb_manager()
raw = self._payload(data)
payload = KnowledgeBaseRequest(**raw).canonical_payload()
...
```
This way the `name ➜ kb_name` migration and any future normalization lives in one place.
---
### 2. Simplify `update_kb` without losing partial-update semantics
You can keep the “at least one field” validation and the “default to current values” behavior, but make the update fields explicit instead of flowing through `update_keys` / `update_data` / `**update_data`.
Current logic (simplified):
```python
update_keys = [...]
provided_updates = {key: payload[key] for key in update_keys if key in payload}
...
current = current_kb.kb
update_data = {key: getattr(current, key) for key in update_keys}
update_data.update(provided_updates)
kb_helper = await self.get_kb_manager().update_kb(
kb_id=kb_id,
**update_data,
)
```
You can replace it with explicit parameters:
```python
async def update_kb(self, data: object) -> tuple[dict[str, Any], str]:
payload = self._canonical_kb_payload(data)
kb_id = payload.get("kb_id")
if not kb_id:
raise KnowledgeBaseServiceError("缺少参数 kb_id")
# Allowed update fields
fields = (
"kb_name",
"description",
"emoji",
"embedding_provider_id",
"rerank_provider_id",
"chunk_size",
"chunk_overlap",
"top_k_dense",
"top_k_sparse",
"top_m_final",
)
if not any(field in payload for field in fields):
raise KnowledgeBaseServiceError("至少需要提供一个更新字段")
current_kb = await self.get_kb_manager().get_kb(kb_id)
if not current_kb:
raise KnowledgeBaseServiceError("知识库不存在")
current = current_kb.kb
kb_helper = await self.get_kb_manager().update_kb(
kb_id=kb_id,
kb_name=payload.get("kb_name", current.kb_name),
description=payload.get("description", current.description),
emoji=payload.get("emoji", current.emoji),
embedding_provider_id=payload.get("embedding_provider_id", current.embedding_provider_id),
rerank_provider_id=payload.get("rerank_provider_id", current.rerank_provider_id),
chunk_size=payload.get("chunk_size", current.chunk_size),
chunk_overlap=payload.get("chunk_overlap", current.chunk_overlap),
top_k_dense=payload.get("top_k_dense", current.top_k_dense),
top_k_sparse=payload.get("top_k_sparse", current.top_k_sparse),
top_m_final=payload.get("top_m_final", current.top_m_final),
)
if not kb_helper:
raise KnowledgeBaseServiceError("知识库不存在")
return kb_helper.kb.model_dump(), "更新知识库成功"
```
This keeps:
- Partial updates (only provided fields overwrite).
- Defaults from current KB values for missing fields.
- The “at least one update field” requirement.
But it removes the extra dict-building and makes the updatable fields obvious at the call site.
</issue_to_address>
### Comment 3
<location path="astrbot/dashboard/schemas.py" line_range="207" />
<code_context>
class KnowledgeBaseRequest(OpenModel):
- kb_id: str | None = None
- name: str | None = None
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing knowledge base payload normalization in `KnowledgeBaseRequest` using Pydantic field aliases for the legacy `name` field to avoid custom migration logic and duplicated canonicalization.
You can reduce complexity here by (1) making `KnowledgeBaseRequest` the single source of truth for normalization, and (2) using Pydantic aliases to handle the legacy `name` field instead of custom logic.
### 1. Remove duplicate canonicalization in the service
Instead of having both `canonical_payload()` on the model and `_canonical_kb_payload()` in the service, keep only the model-level one and call it from the service:
```python
# in the service, replace `_canonical_kb_payload(...)` usage with:
payload = kb_request.canonical_payload()
# ...use `payload` directly
```
Then delete `_canonical_kb_payload` entirely. This way, any future changes to the KB payload shape happen in one place.
### 2. Use a field alias for the legacy `name` input
You can avoid manual `getattr(self, "name", None)` and keep the migration logic declarative:
```python
from pydantic import Field
class KnowledgeBaseRequest(OpenModel):
kb_name: str | None = Field(None, alias="name") # accept legacy "name"
description: str | None = None
emoji: str | None = None
embedding_provider_id: str | None = None
rerank_provider_id: str | None = None
chunk_size: int | None = None
chunk_overlap: int | None = None
top_k_dense: int | None = None
top_k_sparse: int | None = None
top_m_final: int | None = None
def canonical_payload(self) -> dict[str, Any]:
return self.model_dump(
exclude_unset=True,
include={
"kb_name",
"description",
"emoji",
"embedding_provider_id",
"rerank_provider_id",
"chunk_size",
"chunk_overlap",
"top_k_dense",
"top_k_sparse",
"top_m_final",
},
by_alias=False, # ensure we output `kb_name`, not `name`
)
```
This keeps all existing behavior (including support for legacy `name`) while:
- Eliminating duplicated normalization logic between layers.
- Removing the manual migration code path that can drift from the service logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the knowledge base management system by standardizing payload schemas, introducing pagination to the listing service, updating OpenAPI specifications, and adding unit tests. However, several critical issues were identified during the review: removing _model_dict will cause NameError exceptions in other endpoints that still reference it; the KnowledgeBaseCreateRequest schema is missing from schemas.py despite being expected by the frontend and OpenAPI spec; combining allOf with additionalProperties: false in the OpenAPI spec will cause validation failures; and using getattr without a default value when updating knowledge bases could lead to an AttributeError.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
lxfight
force-pushed
the
fix/kb-contract-crud
branch
from
85f98f2 to
69de462
Compare
Member
Author
|
@gemini-code-assist review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the knowledge base creation and update schemas, introducing a distinct KnowledgeBaseCreateRequest and migrating the legacy name field to kb_name. It updates the backend services, OpenAPI specifications, frontend API clients, and UI components to align with these schema changes, and adds comprehensive unit tests. The reviewer suggested avoiding in-place modification of the input dictionary in _canonical_kb_payload to prevent unexpected side-effects.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ``kb_name`` migration while preserving operational fields | ||
| like ``kb_id``. | ||
| """ | ||
| raw = KnowledgeBaseService._payload(data) |
Contributor
There was a problem hiding this comment.
Modifying the input data dictionary in-place via raw.update(canonical) can lead to unexpected side-effects in callers that reuse the dictionary. It is safer to create a copy of the dictionary first.
Suggested change
| raw = KnowledgeBaseService._payload(data) | |
| raw = dict(KnowledgeBaseService._payload(data)) |
Unify knowledge base create/update/list with kb_name, canonical_payload, optional list pagination with total, and matching OpenAPI plus dashboard types.
…fix OpenAPI allOf validation - Replace 3 remaining _model_dict() calls with payload.model_dump(exclude_none=True) - Add KnowledgeBaseCreateRequest schema with required kb_name + embedding_provider_id - Remove additionalProperties: false from OpenAPI allOf sub-schema to avoid validation failures - Use Pydantic Field(alias="name") + populate_by_name=True for legacy name migration - Delegate _canonical_kb_payload to KnowledgeBaseRequest to eliminate duplicated logic - Add getattr default None for update_kb to prevent AttributeError - Add validation error tests: missing kb_name, missing embedding_provider_id
lxfight
force-pushed
the
fix/kb-contract-crud
branch
from
7a12034 to
de70eaa
Compare
Soulter
approved these changes
Jul 3, 2026
Soulter
left a comment
Soulter
left a comment
Member
There was a problem hiding this comment.
looks good. Potential breaking changes in /kb api.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unify knowledge base create/update/list with kb_name, canonical_payload, optional list pagination with total, and matching OpenAPI plus dashboard types.
The knowledge base CRUD (create/update/list) API contract is inconsistent between frontend and backend. The backend schema uses a
namefield while the generated frontend types expectkb_name. Both create and update share the sameKnowledgeBaseRequestschema, but create requireskb_nameandembedding_provider_id, while update does not — this shared schema leads to confusing validation semantics. Additionally, the knowledge base payload is missing canonical fields such asemoji,chunk_size,chunk_overlap,top_k_dense,top_k_sparse, andtop_m_final.This change unifies the frontend-backend contract: renames
nametokb_name, introducesKnowledgeBaseCreateRequestto separate create and update semantics, promotes optional configuration parameters into canonicalKnowledgeBaseRequestfields, and syncs the OpenAPI spec, backend schemas, and generated frontend types.Modifications / 改动点
openspec/openapi-v1.yaml):KnowledgeBaseRequest:name→kb_name; addedemoji,chunk_size,chunk_overlap,top_k_dense,top_k_sparse,top_m_finalfields;embedding_provider_id/rerank_provider_idnow acceptnullKnowledgeBaseCreateRequestinheritingKnowledgeBaseRequestviaallOf, markingkb_nameandembedding_provider_idas requiredastrbot/dashboard/schemas.py): syncedKnowledgeBaseRequestandKnowledgeBaseCreateRequestPydantic modelsastrbot/dashboard/api/knowledge_bases.py):create_knowledge_baseusesKnowledgeBaseCreateRequest;update_knowledge_baseandlist_knowledge_basesadapted tokb_nameastrbot/dashboard/services/knowledge_base_service.py): adapted to new schema field names and typesdashboard/src/api/generated/openapi-v1/types.gen.ts): regenerateddashboard/src/api/v1.ts): adapted to new typesdashboard/src/views/knowledge-base/KBList.vue):name→kb_nametests/unit/test_knowledge_base_service_contract.py): added 266 lines of end-to-end contract testsScreenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Align knowledge base CRUD contracts across backend, OpenAPI spec, and frontend, including canonical field names, pagination behavior, and configuration parameters.
New Features:
Bug Fixes:
Enhancements:
Tests: