Conversation
审阅者指南（在小型 PR 上默认折叠）审阅者指南为管理员通知端点新增了健壮的 JSON 解析/错误处理，并修改了项目列表逻辑：返回所有项目，但在排序时优先显示 type_id 与请求匹配的项目。 notify_admin 的 JSON 解析和错误处理时序图sequenceDiagram
actor Client
participant APIRouter as APIRouter_notify_admin
participant Endpoint as notify_admin
participant Request as Request
participant Logger as logger
participant Settings as settings
participant HTTPException as HTTPException
Client->>APIRouter: POST /notify_admin
APIRouter->>Endpoint: notify_admin(request)
Endpoint->>Request: json()
alt valid_json
Request-->>Endpoint: body
Endpoint->>Logger: info(str(body))
Endpoint->>Settings: read notify_admin_url
alt notify_admin_url_not_configured
Endpoint-->>Client: HTTP 200 { ec: 200, msg: disabled }
else notify_admin_url_configured
Endpoint->>ExternalService: POST notify_admin_url with body
ExternalService-->>Endpoint: response
Endpoint-->>Client: HTTP 200 { ec: 200 }
end
else invalid_json
Request-->>Endpoint: raise Exception
Endpoint->>Logger: error(notify_admin invalid json)
Endpoint->>HTTPException: construct 400 Invalid JSON
HTTPException-->>Client: HTTP 400 { detail: Invalid JSON }
end
query_project 返回所有项目并按 type_id 匹配情况排序的流程图flowchart TD
A_start["Client calls GET /project with type_id"] --> B_loadCache["Load project_cache[0]"]
B_loadCache --> C_iterate["Iterate over all projects p in project_cache[0]"]
C_iterate --> D_buildItem["For each p build dict { name, github, desc, tags, download }"]
D_buildItem --> E_collect["Collect list data of all project dicts"]
E_collect --> F_sort["Sort data with key: p.type_id != type_id (matches first)"]
F_sort --> G_response["Return { ec: 200, data } with all projects ordered"]
文件级变更
技巧与命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 以：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideAdds robust JSON parsing/error handling to the admin notification endpoint and changes project listing to include all projects while ordering those matching the requested type_id first. Sequence diagram for notify_admin JSON parsing and error handlingsequenceDiagram
actor Client
participant APIRouter as APIRouter_notify_admin
participant Endpoint as notify_admin
participant Request as Request
participant Logger as logger
participant Settings as settings
participant HTTPException as HTTPException
Client->>APIRouter: POST /notify_admin
APIRouter->>Endpoint: notify_admin(request)
Endpoint->>Request: json()
alt valid_json
Request-->>Endpoint: body
Endpoint->>Logger: info(str(body))
Endpoint->>Settings: read notify_admin_url
alt notify_admin_url_not_configured
Endpoint-->>Client: HTTP 200 { ec: 200, msg: disabled }
else notify_admin_url_configured
Endpoint->>ExternalService: POST notify_admin_url with body
ExternalService-->>Endpoint: response
Endpoint-->>Client: HTTP 200 { ec: 200 }
end
else invalid_json
Request-->>Endpoint: raise Exception
Endpoint->>Logger: error(notify_admin invalid json)
Endpoint->>HTTPException: construct 400 Invalid JSON
HTTPException-->>Client: HTTP 400 { detail: Invalid JSON }
end
Flow diagram for query_project returning all projects ordered by type_id matchflowchart TD
A_start["Client calls GET /project with type_id"] --> B_loadCache["Load project_cache[0]"]
B_loadCache --> C_iterate["Iterate over all projects p in project_cache[0]"]
C_iterate --> D_buildItem["For each p build dict { name, github, desc, tags, download }"]
D_buildItem --> E_collect["Collect list data of all project dicts"]
E_collect --> F_sort["Sort data with key: p.type_id != type_id (matches first)"]
F_sort --> G_response["Return { ec: 200, data } with all projects ordered"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - 我已经审查了你的更改，这里是一些反馈：
- 在
notify_admin中，建议捕获更具体的 JSON 解析异常（例如JSONDecodeError或ValueError），而不是捕获Exception，以避免掩盖无关错误并让调试更容易。 query_project中新的排序逻辑（sorted(project_cache[0], key=lambda p: p.type_id != type_id)）只能保证匹配type_id的条目排在前面；如果你需要在分组内或跨分组保持稳定或更有意义的顺序，建议添加一个次级排序键（例如名称、id 或创建时间）。
给 AI 代理的提示
请根据本次代码审查中的评论进行修改：
## 总体评论
- 在 `notify_admin` 中，建议捕获更具体的 JSON 解析异常（例如 `JSONDecodeError` 或 `ValueError`），而不是捕获 `Exception`，以避免掩盖无关错误并让调试更容易。
- `query_project` 中新的排序逻辑（`sorted(project_cache[0], key=lambda p: p.type_id != type_id)`）只能保证匹配 `type_id` 的条目排在前面；如果你需要在分组内或跨分组保持稳定或更有意义的顺序，建议添加一个次级排序键（例如名称、id 或创建时间）。
## 单独评论
### 评论 1
<location> `src/notify_admin/__init__.py:11-15` </location>
<code_context>
@router.post("/notify_admin")
async def notify_admin(request: Request):
- body = await request.json()
+ try:
+ body = await request.json()
+ except Exception as e:
+ logger.error(f"notify_admin invalid json: {e}")
+ raise HTTPException(status_code=400, detail="Invalid JSON")
+
logger.info(str(body))
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 在解析 JSON 时收窄异常类型，而不是捕获所有 Exception。
在这里捕获 `Exception` 也会把无关的错误（例如连接问题、内部故障）一起吞掉，并统一标记为 "Invalid JSON"，从而掩盖真正的问题。更好的做法是只捕获 `request.json()` 可能抛出的特定 JSON 解析异常（例如 `JSONDecodeError` / `ValueError`，或者框架自己的异常类型），让其他异常继续向上传播为 500，这样只有真正的解析错误才会返回 400。
建议实现方式：
```python
from json import JSONDecodeError
from fastapi import Request, APIRouter, HTTPException
from aiohttp import ClientSession
from loguru import logger
```
```python
@router.post("/notify_admin")
async def notify_admin(request: Request):
try:
body = await request.json()
except JSONDecodeError as e:
logger.error(f"notify_admin invalid json: {e}")
raise HTTPException(status_code=400, detail="Invalid JSON")
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey there - I've reviewed your changes - here's some feedback:
- In
notify_admin, consider catching the specific JSON parse exception (e.g.JSONDecodeErrororValueError) instead ofExceptionto avoid masking unrelated errors and make debugging easier. - The new sorting in
query_project(sorted(project_cache[0], key=lambda p: p.type_id != type_id)) only guarantees that matchingtype_iditems come first; if a stable or more meaningful order within and across groups is important, consider adding a secondary sort key (e.g. name, id, or created time).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `notify_admin`, consider catching the specific JSON parse exception (e.g. `JSONDecodeError` or `ValueError`) instead of `Exception` to avoid masking unrelated errors and make debugging easier.
- The new sorting in `query_project` (`sorted(project_cache[0], key=lambda p: p.type_id != type_id)`) only guarantees that matching `type_id` items come first; if a stable or more meaningful order within and across groups is important, consider adding a secondary sort key (e.g. name, id, or created time).
## Individual Comments
### Comment 1
<location> `src/notify_admin/__init__.py:11-15` </location>
<code_context>
@router.post("/notify_admin")
async def notify_admin(request: Request):
- body = await request.json()
+ try:
+ body = await request.json()
+ except Exception as e:
+ logger.error(f"notify_admin invalid json: {e}")
+ raise HTTPException(status_code=400, detail="Invalid JSON")
+
logger.info(str(body))
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Narrow the exception type when parsing JSON instead of catching all Exceptions.
Catching `Exception` here will also swallow unrelated errors (e.g., connection issues, internal failures) and mislabel them as "Invalid JSON", which obscures real problems. Instead, catch only the specific JSON parsing exception(s) that `request.json()` can raise (e.g., `JSONDecodeError` / `ValueError` or the framework’s own type) and let other exceptions propagate as 500s so only true parse errors return a 400.
Suggested implementation:
```python
from json import JSONDecodeError
from fastapi import Request, APIRouter, HTTPException
from aiohttp import ClientSession
from loguru import logger
```
```python
@router.post("/notify_admin")
async def notify_admin(request: Request):
try:
body = await request.json()
except JSONDecodeError as e:
logger.error(f"notify_admin invalid json: {e}")
raise HTTPException(status_code=400, detail="Invalid JSON")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+11
to
+15
| try: | ||
| body = await request.json() | ||
| except Exception as e: | ||
| logger.error(f"notify_admin invalid json: {e}") | ||
| raise HTTPException(status_code=400, detail="Invalid JSON") |
There was a problem hiding this comment.
suggestion (bug_risk): 在解析 JSON 时收窄异常类型，而不是捕获所有 Exception。
在这里捕获 Exception 也会把无关的错误（例如连接问题、内部故障）一起吞掉，并统一标记为 "Invalid JSON"，从而掩盖真正的问题。更好的做法是只捕获 request.json() 可能抛出的特定 JSON 解析异常（例如 JSONDecodeError / ValueError，或者框架自己的异常类型），让其他异常继续向上传播为 500，这样只有真正的解析错误才会返回 400。
建议实现方式：
from json import JSONDecodeError
from fastapi import Request, APIRouter, HTTPException
from aiohttp import ClientSession
from loguru import logger@router.post("/notify_admin")
async def notify_admin(request: Request):
try:
body = await request.json()
except JSONDecodeError as e:
logger.error(f"notify_admin invalid json: {e}")
raise HTTPException(status_code=400, detail="Invalid JSON")Original comment in English
suggestion (bug_risk): Narrow the exception type when parsing JSON instead of catching all Exceptions.
Catching Exception here will also swallow unrelated errors (e.g., connection issues, internal failures) and mislabel them as "Invalid JSON", which obscures real problems. Instead, catch only the specific JSON parsing exception(s) that request.json() can raise (e.g., JSONDecodeError / ValueError or the framework’s own type) and let other exceptions propagate as 500s so only true parse errors return a 400.
Suggested implementation:
from json import JSONDecodeError
from fastapi import Request, APIRouter, HTTPException
from aiohttp import ClientSession
from loguru import logger@router.post("/notify_admin")
async def notify_admin(request: Request):
try:
body = await request.json()
except JSONDecodeError as e:
logger.error(f"notify_admin invalid json: {e}")
raise HTTPException(status_code=400, detail="Invalid JSON")
Contributor
There was a problem hiding this comment.
Pull request overview
This release updates the /project endpoint to return all projects with sorted results and adds JSON parsing error handling to the /notify_admin endpoint.
- Changed
/projectendpoint from filtering by type_id to returning all projects sorted by type_id match - Added error handling for invalid JSON requests in
/notify_adminendpoint
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/project/init.py | Modified query logic to return all projects sorted by type_id match instead of filtering |
| src/notify_admin/init.py | Added try-catch block and HTTPException for invalid JSON request handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| for p in project_cache[0] | ||
| if p.type_id == type_id | ||
| for p in sorted(project_cache[0], key=lambda p: p.type_id != type_id) |
There was a problem hiding this comment.
The sorting logic change has altered the behavior of the endpoint. Previously, the code filtered projects to only return those matching the specified type_id. Now it returns ALL projects but sorts them so matching projects appear first. This changes the API response to include projects of all types, which may be a breaking change for API consumers expecting only filtered results. Consider whether this behavior change is intentional and documented.
Suggested change
| for p in sorted(project_cache[0], key=lambda p: p.type_id != type_id) | |
| for p in project_cache[0] if p.type_id == type_id |
| body = await request.json() | ||
| try: | ||
| body = await request.json() | ||
| except Exception as e: |
There was a problem hiding this comment.
Using a broad Exception handler can mask specific errors and make debugging difficult. Consider catching more specific exceptions like JSONDecodeError from the json module, which is the actual exception raised when JSON parsing fails.
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
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.
Summary by Sourcery
在管理员通知端点中处理无效的 JSON 负载，并调整项目查询结果的排序以优先显示特定的
type_id。Bug Fixes:
/notify_admin端点接收到无效的 JSON 请求体时，返回 400 错误。Enhancements:
/project查询中返回所有缓存的项目，并通过排序使具有请求type_id的条目优先显示。Original summary in English
Summary by Sourcery
Handle invalid JSON payloads in the admin notification endpoint and adjust project query results ordering to prioritize a specific type_id.
Bug Fixes:
Enhancements: