Dev #55
Merged
Dev #55
Conversation
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
Reviewer's GuideRefactors network and timeout handling in the napcat adapter utilities, adjusts forward-message return semantics and error handling, and updates the README to document dev/classical branch stability and usage guidance. Sequence diagram for updated get_image_base64 network handlingsequenceDiagram
participant Caller
participant Utils as NapcatUtils
participant Aiohttp as aiohttp_ClientSession
participant Remote as RemoteServer
Caller->>Utils: get_image_base64(url)
activate Utils
Utils->>Aiohttp: create ClientSession()
activate Aiohttp
Utils->>Aiohttp: session.get(url, timeout=10)
Aiohttp->>Remote: HTTP GET url
Remote-->>Aiohttp: HTTP response
Aiohttp-->>Utils: response
alt response.status != 200
Utils-->>Caller: raise Exception HTTP_Error_status
else response.status == 200
Utils->>Aiohttp: response.read()
Aiohttp-->>Utils: image_bytes
Utils-->>Caller: base64_b64encode(image_bytes)
end
deactivate Aiohttp
deactivate Utils
opt any_exception
Utils->>Utils: log error 图片下载失败
Utils-->>Caller: re_raise_exception
end
Sequence diagram for updated get_forward_message error handling and return typesequenceDiagram
participant Caller
participant Utils as NapcatUtils
participant Adapter as NapcatAdapter
Caller->>Utils: get_forward_message(raw_message, adapter)
activate Utils
Utils->>Utils: extract forward_message_data from raw_message
alt forward_message_data empty
Utils->>Utils: log warning 转发消息内容为空
Utils-->>Caller: None
else forward_message_data present
Utils->>Adapter: get_msg(forward_id)
activate Adapter
Adapter-->>Utils: response or timeout or error
deactivate Adapter
alt response is None
Utils->>Utils: log error 获取转发消息失败，返回值为空
Utils-->>Caller: [{sender:{nickname:系统}, message:[{type:text, data:[获取转发消息失败]}]}]
else adapter timeout
Utils->>Utils: log error 获取转发消息超时
Utils-->>Caller: [{sender:{nickname:系统}, message:[{type:text, data:[获取转发消息超时]}]}]
else adapter exception
Utils->>Utils: log error 获取转发消息失败
Utils-->>Caller: [{sender:{nickname:系统}, message:[{type:text, data:[获取转发消息失败_with_reason]}]}]
else response ok
Utils->>Utils: response_data = (response.data or {})
alt response_data empty
Utils->>Utils: log warning 转发消息内容为空或获取失败
Utils-->>Caller: None
else response_data present
Utils->>Utils: log debug 转发消息原始格式
Utils-->>Caller: response_data as list_of_message_nodes
end
end
deactivate Utils
end
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 - I've found 3 issues, and left some high level feedback:
- In
get_image_base64, consider reusing anaiohttp.ClientSessioninstead of creating a new session on every call to avoid unnecessary connection overhead and improve performance under load. get_forward_messagenow usually returns alist[dict](including for error cases) but still returnsNonewhenresponse_datais empty; aligning all error/empty cases to a consistent list-based return shape would simplify caller handling and reduce the need for extraNonechecks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_image_base64`, consider reusing an `aiohttp.ClientSession` instead of creating a new session on every call to avoid unnecessary connection overhead and improve performance under load.
- `get_forward_message` now usually returns a `list[dict]` (including for error cases) but still returns `None` when `response_data` is empty; aligning all error/empty cases to a consistent list-based return shape would simplify caller handling and reduce the need for extra `None` checks.
## Individual Comments
### Comment 1
<location> `src/plugins/built_in/napcat_adapter/src/handlers/utils.py:238-239` </location>
<code_context>
- raise Exception(f"HTTP Error: {response.status}")
- image_bytes = response.data
- return base64.b64encode(image_bytes).decode("utf-8")
+ async with aiohttp.ClientSession() as session:
+ async with session.get(url, timeout=10) as response:
+ if response.status != 200:
+ raise Exception(f"HTTP Error: {response.status}")
</code_context>
<issue_to_address>
**suggestion:** Consider reusing an aiohttp.ClientSession and configuring SSL/timeouts explicitly instead of creating a new session per call.
Creating a new `ClientSession` on every `get_image_base64` call adds unnecessary overhead and can trigger aiohttp warnings. Prefer a shared or injected session, configured once with `ClientTimeout` and a `TCPConnector` (including SSL options), so you can preserve any required relaxed SSL/cipher/TLS settings while keeping the async API.
Suggested implementation:
```python
import aiohttp
# Shared HTTP client session for outbound requests from this module.
# NOTE: This avoids creating a new ClientSession per call, which is inefficient
# and may trigger aiohttp warnings. Configure timeout/SSL/connector here.
_HTTP_SESSION: aiohttp.ClientSession | None = None
async def get_http_session() -> aiohttp.ClientSession:
"""Get (or lazily create) a shared aiohttp ClientSession.
Centralizes timeout/SSL/connector configuration so callers do not need to
create their own sessions and we can preserve any custom TLS settings.
"""
global _HTTP_SESSION
if _HTTP_SESSION is None or _HTTP_SESSION.closed:
timeout = aiohttp.ClientTimeout(total=10)
connector = aiohttp.TCPConnector(
# Set SSL options explicitly; adjust as needed for your environment.
# ssl=None keeps default verification; set ssl=False to disable it.
ssl=None,
# You can tune connection limits here if needed.
limit=100,
)
_HTTP_SESSION = aiohttp.ClientSession(
timeout=timeout,
connector=connector,
)
return _HTTP_SESSION
```
```python
# sourcery skip: raise-specific-error
"""下载图片/视频并返回Base64"""
logger.debug(f"下载图片: {url}")
try:
session = await get_http_session()
async with session.get(url) as response:
if response.status != 200:
raise Exception(f"HTTP Error: {response.status}")
image_bytes = await response.read()
return base64.b64encode(image_bytes).decode("utf-8")
except Exception as e:
logger.error(f"图片下载失败: {e!s}")
```
If this function is not already declared as `async`, it must be updated to `async def ...` to allow `await get_http_session()`.
If your project has a centralized lifecycle management (startup/shutdown hooks), you may also want to add logic there to gracefully close `_HTTP_SESSION` (e.g. `await _HTTP_SESSION.close()` on shutdown) rather than relying on process exit.
</issue_to_address>
### Comment 2
<location> `src/plugins/built_in/napcat_adapter/src/handlers/utils.py:375-381` </location>
<code_context>
async def get_forward_message(
raw_message: dict, *, adapter: "NapcatAdapter | None" = None
-) -> dict[str, Any] | None:
</code_context>
<issue_to_address>
**question (bug_risk):** The return type of get_forward_message is now a list, but some branches still return None; consider making the result shape more consistent.
The function now sometimes returns a synthetic single-element list on error/timeout, but still uses `None` when `response_data` is falsy ("转发消息内容为空或获取失败"). This forces callers to handle three shapes: a normal list from the API, a synthetic error list, and `None`. Consider standardizing on either a list for all non-success cases (e.g., a system message element) or consistently using `None` for all failures, and ensure this matches existing caller expectations to avoid subtle logic errors.
</issue_to_address>
### Comment 3
<location> `README.md:36` </location>
<code_context>
+> **分支状态说明**
+>
+> - `dev` 分支当前用于架构重构与大规模改动，功能不稳定，无法保证可用性。
+> - 现行可用的稳定版本已迁移至 `classical` 分支，并在该分支持续维护。
+> - 如需稳定使用，请切换到 `classical` 分支：
+>
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding “上” in “并在该分支持续维护” for more natural written Chinese.
This matches standard written usage and improves readability.
```suggestion
> - 现行可用的稳定版本已迁移至 `classical` 分支，并在该分支上持续维护。
```
</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
+238
to
+239
| async with aiohttp.ClientSession() as session: | ||
| async with session.get(url, timeout=10) as response: |
There was a problem hiding this comment.
suggestion: Consider reusing an aiohttp.ClientSession and configuring SSL/timeouts explicitly instead of creating a new session per call.
Creating a new ClientSession on every get_image_base64 call adds unnecessary overhead and can trigger aiohttp warnings. Prefer a shared or injected session, configured once with ClientTimeout and a TCPConnector (including SSL options), so you can preserve any required relaxed SSL/cipher/TLS settings while keeping the async API.
Suggested implementation:
import aiohttp
# Shared HTTP client session for outbound requests from this module.
# NOTE: This avoids creating a new ClientSession per call, which is inefficient
# and may trigger aiohttp warnings. Configure timeout/SSL/connector here.
_HTTP_SESSION: aiohttp.ClientSession | None = None
async def get_http_session() -> aiohttp.ClientSession:
"""Get (or lazily create) a shared aiohttp ClientSession.
Centralizes timeout/SSL/connector configuration so callers do not need to
create their own sessions and we can preserve any custom TLS settings.
"""
global _HTTP_SESSION
if _HTTP_SESSION is None or _HTTP_SESSION.closed:
timeout = aiohttp.ClientTimeout(total=10)
connector = aiohttp.TCPConnector(
# Set SSL options explicitly; adjust as needed for your environment.
# ssl=None keeps default verification; set ssl=False to disable it.
ssl=None,
# You can tune connection limits here if needed.
limit=100,
)
_HTTP_SESSION = aiohttp.ClientSession(
timeout=timeout,
connector=connector,
)
return _HTTP_SESSION # sourcery skip: raise-specific-error
"""下载图片/视频并返回Base64"""
logger.debug(f"下载图片: {url}")
try:
session = await get_http_session()
async with session.get(url) as response:
if response.status != 200:
raise Exception(f"HTTP Error: {response.status}")
image_bytes = await response.read()
return base64.b64encode(image_bytes).decode("utf-8")
except Exception as e:
logger.error(f"图片下载失败: {e!s}")If this function is not already declared as async, it must be updated to async def ... to allow await get_http_session().
If your project has a centralized lifecycle management (startup/shutdown hooks), you may also want to add logic there to gracefully close _HTTP_SESSION (e.g. await _HTTP_SESSION.close() on shutdown) rather than relying on process exit.
Comment on lines
375
to
381
|
|
||
| async def get_forward_message( | ||
| raw_message: dict, *, adapter: "NapcatAdapter | None" = None | ||
| ) -> dict[str, Any] | None: | ||
| ) -> list[dict[str, Any]] | None: | ||
| forward_message_data: dict = raw_message.get("data", {}) | ||
| if not forward_message_data: | ||
| logger.warning("转发消息内容为空") |
There was a problem hiding this comment.
question (bug_risk): The return type of get_forward_message is now a list, but some branches still return None; consider making the result shape more consistent.
The function now sometimes returns a synthetic single-element list on error/timeout, but still uses None when response_data is falsy ("转发消息内容为空或获取失败"). This forces callers to handle three shapes: a normal list from the API, a synthetic error list, and None. Consider standardizing on either a list for all non-success cases (e.g., a system message element) or consistently using None for all failures, and ensure this matches existing caller expectations to avoid subtle logic errors.
| > **分支状态说明** | ||
| > | ||
| > - `dev` 分支当前用于架构重构与大规模改动，功能不稳定，无法保证可用性。 | ||
| > - 现行可用的稳定版本已迁移至 `classical` 分支，并在该分支持续维护。 |
There was a problem hiding this comment.
nitpick (typo): Consider adding “上” in “并在该分支持续维护” for more natural written Chinese.
This matches standard written usage and improves readability.
Suggested change
| > - 现行可用的稳定版本已迁移至 `classical` 分支，并在该分支持续维护。 | |
| > - 现行可用的稳定版本已迁移至 `classical` 分支，并在该分支上持续维护。 |
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
Update network handling for media and forwarded messages and clarify branch stability in the README.
Bug Fixes:
Enhancements:
Documentation: