Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The exception handling in the _send_and_close method is using a bare except statement, which can suppress important errors and make debugging difficult. It's recommended to log the exception or handle specific exceptions to ensure that important issues are not missed.
Recommended Change:
try:
await websocket.close()
except Exception as e:
print(f"Error closing WebSocket for client {client_id}: {str(e)}")| if zip_file_path.exists(): | ||
| # 启动延迟删除线程 (0.5 * 60 * 60 秒 = 30 分钟) | ||
| threading.Thread(target=delayed_delete, | ||
| args=(zip_file_path, int(0.5 * 60 * 60)), | ||
| daemon=True).start() | ||
|
|
||
| # 任务成功，使用 asyncio.run() 在当前线程中运行异步通知任务 | ||
| asyncio.run(manager.send_personal_message(client_id, { | ||
| message = { | ||
| "status": "download_ready", | ||
| "file_name": file_title, | ||
| "message": f"文件 '{file_title}' 已完成处理，可以下载。" | ||
| })) | ||
|
|
||
| } | ||
| else: | ||
| # 任务失败，发送 WebSocket 错误通知 | ||
| asyncio.run(manager.send_personal_message(client_id, { | ||
| message = { | ||
| "status": "error", | ||
| "file_name": file_title, | ||
| "message": f"文件 '{file_title}' 未找到或处理失败。" | ||
| })) | ||
|
|
||
| } |
There was a problem hiding this comment.
The logic for checking the existence of the zip file and preparing the message to send to the client does not account for the scenario where the file does not exist but the message indicates it's ready. This could mislead the client into thinking a file is ready for download when it isn't.
Recommended Change:
if zip_file_path.exists():
message = {
"status": "download_ready",
"file_name": file_title,
"message": f"文件 '{file_title}' 已完成处理，可以下载。"
}
else:
message = {
"status": "error",
"file_name": file_title,
"message": f"文件 '{file_title}' 未找到或处理失败。"
}| response = client.get("/v1/{0}".format(nowtimestamp)) | ||
| timedelta = int(time.time() * 1000) - nowtimestamp | ||
| ms = int(timedelta) | ||
| assert response.status_code == 200 | ||
| assert response.json().get("status") == "ok" | ||
| assert response.json().get("app") == "jmcomic_server_api" | ||
| assert int(response.json().get("latency")) <= ms | ||
| assert int(response.json().get("latency")) > 0 | ||
|
|
||
|
|
||
| def test_search_album(): | ||
| client = TestClient(app) | ||
| tag = "全彩" | ||
| num = 1 | ||
| response = client.get("/v1/search/{0}/{1}".format(tag, num)) | ||
| assert response.status_code == 200 | ||
| assert isinstance(response.json(), list) | ||
| assert len(response.json()) > 0 | ||
| first_album = response.json()[0] | ||
| assert "album_id" in first_album | ||
| assert "title" in first_album | ||
|
|
||
|
|
||
| def test_get_cover_and_info(): | ||
| client = TestClient(app) | ||
| aid = 1225432 | ||
| response = client.get("/v1/info/{0}".format(aid)) | ||
| assert response.status_code == 200 | ||
| info_json = response.json() | ||
| assert info_json.get("status") == "success" | ||
| assert "全彩" in info_json.get("tag", []) | ||
| assert int(info_json.get("view_count")) > 0 | ||
| assert int(info_json.get("like_count")) > 0 | ||
| assert info_json.get("page_count") == "0" | ||
| response = client.get("/v1/get/cover/{0}".format(aid)) | ||
| assert response.status_code == 200 | ||
| assert response.headers["content-type"] == "image/jpeg" | ||
| file_path = FILE_PATH / f"cover-{aid}.jpg" | ||
| assert file_path.exists() | ||
| file_path.unlink() | ||
| assert file_path.exists() == False | ||
|
|
||
|
|
||
| def test_download_album(): | ||
| client = TestClient(app) | ||
| aid = 1225432 | ||
| client_id = "1145141919810" | ||
| with client.websocket_connect(f"/ws/notifications/{client_id}") as websocket: | ||
| response = client.post(f"/v1/download/album/{aid}", json={"client_id": client_id}) | ||
| assert response.status_code == 202 | ||
| assert response.json() == { | ||
| "status": "processing", | ||
| "message": "下载任务已在后台启动，请通过 WebSocket 监听 'download_ready' 通知。" | ||
| } | ||
| data = websocket.receive_json() | ||
| assert data == { | ||
| "status": "download_ready", | ||
| "file_name": "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分", | ||
| "message": f"文件 '［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分' 已完成处理，可以下载。" | ||
| } | ||
| client.get("/v1/download/album/{0}".format(aid)) |
There was a problem hiding this comment.
The use of .format() for URL construction is outdated and less readable compared to f-strings, which are available in Python 3.6 and later. Consider refactoring these to use f-strings for better readability and maintainability.
Recommended Change:
response = client.get(f"/v1/{nowtimestamp}")
# Similarly for other lines| assert file_path.exists() | ||
| file_path.unlink() | ||
| assert file_path.exists() == False | ||
|
|
||
|
|
||
| def test_download_album(): | ||
| client = TestClient(app) | ||
| aid = 1225432 | ||
| client_id = "1145141919810" | ||
| with client.websocket_connect(f"/ws/notifications/{client_id}") as websocket: | ||
| response = client.post(f"/v1/download/album/{aid}", json={"client_id": client_id}) | ||
| assert response.status_code == 202 | ||
| assert response.json() == { | ||
| "status": "processing", | ||
| "message": "下载任务已在后台启动，请通过 WebSocket 监听 'download_ready' 通知。" | ||
| } | ||
| data = websocket.receive_json() | ||
| assert data == { | ||
| "status": "download_ready", | ||
| "file_name": "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分", | ||
| "message": f"文件 '［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分' 已完成处理，可以下载。" | ||
| } | ||
| client.get("/v1/download/album/{0}".format(aid)) | ||
| file_title = "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分" | ||
| zip_file_name = f"{file_title}.zip" | ||
| zip_file_path = FILE_PATH / zip_file_name | ||
| assert zip_file_path.exists() == True | ||
| zip_file_path.unlink() | ||
| assert zip_file_path.exists() == False |
There was a problem hiding this comment.
The pattern of checking if a file exists and then deleting it can lead to race conditions, especially in a multi-threaded or multi-process environment. This could make the tests flaky. Consider using a try-except block to handle potential FileNotFoundError which would be more robust.
Recommended Change:
try:
file_path.unlink()
except FileNotFoundError:
pass| def test_get_comic_info(): | ||
| testClient = jmcomic.JmOption.default().new_jm_client() | ||
| page = testClient.search_site(search_query="1225432") | ||
| album: jmcomic.JmAlbumDetail = page.single_album | ||
| assert album.title == "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分" | ||
| assert album.tags == ["全彩","贫乳","调教","中文"] | ||
| assert album.tags == ["全彩", "贫乳", "调教", "中文"] | ||
| assert album.views is not None | ||
| assert album.likes is not None | ||
|
|
||
|
|
||
| def test_rank_comic(): | ||
| client = jmcomic.JmOption.default().new_jm_client() | ||
| page1: jmcomic.JmCategoryPage = client.month_ranking(1) |
There was a problem hiding this comment.
Issue: Test Dependence on External State
The tests test_get_comic_info and test_rank_comic are heavily dependent on the external state of a comic database. This dependence can lead to flaky tests if the external data changes.
Recommendation:
- Mock the responses from
jmcomic.JmOption.default().new_jm_client()to ensure that the tests are not affected by changes in external data. This will make the tests more reliable and predictable.
| FILE_PATH = Path(f"{current_dir}/temp") | ||
| os.makedirs(FILE_PATH, exist_ok=True) |
There was a problem hiding this comment.
Issue: Lack of Cleanup for Temporary Directory
The directory created for temporary files in the test setup is not cleaned up after the tests, which can lead to residual data accumulation.
Recommendation:
- Implement a teardown mechanism or use a Python context manager to ensure that the directory is removed after the tests are completed. This will help in maintaining a clean test environment and prevent issues related to leftover data.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the WebSocket implementation for album download notifications and adds comprehensive API test coverage. The main changes focus on improving thread-safe communication between background tasks and WebSocket clients, along with code formatting improvements.
Key changes:
- Refactored WebSocket connection management to use
asyncio.run_coroutine_threadsafe()for thread-safe message delivery from background tasks - Added comprehensive API tests covering health checks, search, info retrieval, and full download workflows with WebSocket integration
- Updated formatting in existing tests and removed
--no-cache-dirflag from Dockerfile
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| main.py | Refactored ConnectionManager to track the main event loop and use thread-safe coroutine execution; removed client_id validation and WebSocket receive loop; removed delayed file deletion after download |
| tests/test_api.py | Added comprehensive API tests including WebSocket-based download workflow testing |
| tests/test_jmcomic_lib.py | Minor formatting improvements (spacing in list literals and blank lines) |
| requirements.txt | Added httpx dependency for test client |
| README.md | Removed --no-cache-dir flag from pip install command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
The WebSocket endpoint no longer maintains an active connection or handles disconnections. After connect() is called, the function immediately returns, causing the WebSocket to close. This breaks the expected behavior where clients should maintain a persistent connection to receive notifications. The removed try-except block with the receive loop was necessary to keep the connection alive until the task completes and notification is sent.
| try: | |
| while True: | |
| # Keep the connection alive by waiting for messages from the client | |
| await websocket.receive_text() | |
| except WebSocketDisconnect: | |
| pass | |
| finally: | |
| await manager.disconnect(client_id) |
| if zip_file_path.exists(): | ||
| # 启动延迟删除线程 (0.5 * 60 * 60 秒 = 30 分钟) | ||
| threading.Thread(target=delayed_delete, | ||
| args=(zip_file_path, int(0.5 * 60 * 60)), | ||
| daemon=True).start() | ||
|
|
||
| # 任务成功，使用 asyncio.run() 在当前线程中运行异步通知任务 | ||
| asyncio.run(manager.send_personal_message(client_id, { | ||
| message = { | ||
| "status": "download_ready", | ||
| "file_name": file_title, | ||
| "message": f"文件 '{file_title}' 已完成处理，可以下载。" | ||
| })) | ||
|
|
||
| } |
There was a problem hiding this comment.
The delayed deletion thread that was previously started for downloaded zip files has been removed. This means downloaded zip files will never be cleaned up, leading to unbounded disk space usage over time. The previous implementation started a 30-minute delayed deletion thread, which should be preserved or replaced with an alternative cleanup mechanism.
| file_path = FILE_PATH / f"cover-{aid}.jpg" | ||
| assert file_path.exists() | ||
| file_path.unlink() | ||
| assert file_path.exists() == False |
There was a problem hiding this comment.
Redundant comparison to boolean literal. Should use assert not file_path.exists() instead of assert file_path.exists() == False for better readability and following Python best practices.
| assert file_path.exists() == False | |
| assert not file_path.exists() |
| file_title = "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分" | ||
| zip_file_name = f"{file_title}.zip" | ||
| zip_file_path = FILE_PATH / zip_file_name | ||
| assert zip_file_path.exists() == True |
There was a problem hiding this comment.
Redundant comparison to boolean literal. Should use assert zip_file_path.exists() instead of assert zip_file_path.exists() == True for better readability and following Python best practices.
| assert zip_file_path.exists() == True | |
| assert zip_file_path.exists() |
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as e: | |
| print(f"[WebSocket Error] 关闭客户端 {client_id} 的连接时发生异常: {e}") |
| assert response.json().get("status") == "ok" | ||
| assert response.json().get("app") == "jmcomic_server_api" | ||
| assert int(response.json().get("latency")) <= ms | ||
| assert int(response.json().get("latency")) > 0 |
There was a problem hiding this comment.
The response.json() method is called multiple times within the same test function, which results in parsing the JSON response multiple times. This is inefficient as it unnecessarily repeats the parsing process.
Recommended Change:
Store the result of response.json() in a variable and reuse it:
json_response = response.json()
assert json_response.get("status") == "ok"
assert json_response.get("app") == "jmcomic_server_api"
assert int(json_response.get("latency")) <= ms
assert int(json_response.get("latency")) > 0| with client.websocket_connect(f"/ws/notifications/{client_id}") as websocket: | ||
| response = client.post(f"/v1/download/album/{aid}", json={"client_id": client_id}) | ||
| assert response.status_code == 202 | ||
| assert response.json() == { | ||
| "status": "processing", | ||
| "message": "下载任务已在后台启动，请通过 WebSocket 监听 'download_ready' 通知。" | ||
| } | ||
| data = websocket.receive_json() | ||
| assert data == { | ||
| "status": "download_ready", | ||
| "file_name": "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分", | ||
| "message": f"文件 '［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分' 已完成处理，可以下载。" | ||
| } |
There was a problem hiding this comment.
The WebSocket communication assumes successful data reception without handling potential exceptions or errors. This can lead to unhandled exceptions if the WebSocket fails to receive data or receives unexpected data.
Recommended Change:
Implement error handling for WebSocket operations to ensure robustness. Use a try-except block to catch exceptions such as WebSocketDisconnect and handle unexpected data formats:
try:
data = websocket.receive_json()
assert data == {
"status": "download_ready",
"file_name": "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分",
"message": f"文件 '［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分' 已完成处理，可以下载。"
}
except WebSocketDisconnect:
# Handle disconnection appropriately
except json.JSONDecodeError:
# Handle JSON parsing errors
| assert response.status_code == 200 | ||
| assert response.json().get("status") == "ok" | ||
| assert response.json().get("app") == "jmcomic_server_api" | ||
| assert int(response.json().get("latency")) <= ms | ||
| assert int(response.json().get("latency")) > 0 |
There was a problem hiding this comment.
The response.json() method is called multiple times within the same test function, which results in parsing the JSON response multiple times. This is inefficient as it unnecessarily repeats the parsing process.
Recommended Change:
Store the result of response.json() in a variable and reuse it:
json_response = response.json()
assert json_response.get("status") == "ok"
assert json_response.get("app") == "jmcomic_server_api"
assert int(json_response.get("latency")) \<= ms
assert int(json_response.get("latency")) \> 0| with client.websocket_connect(f"/ws/notifications/{client_id}") as websocket: | ||
| response = client.post(f"/v1/download/album/{aid}", json={"client_id": client_id}) | ||
| assert response.status_code == 202 | ||
| assert response.json() == { | ||
| "status": "processing", | ||
| "message": "下载任务已在后台启动，请通过 WebSocket 监听 'download_ready' 通知。" | ||
| } | ||
| data = websocket.receive_json() | ||
| assert data == { | ||
| "status": "download_ready", | ||
| "file_name": "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分", | ||
| "message": f"文件 '［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分' 已完成处理，可以下载。" | ||
| } |
There was a problem hiding this comment.
The WebSocket communication assumes successful data reception without handling potential exceptions or errors. This can lead to unhandled exceptions if the WebSocket fails to receive data or receives unexpected data.
Recommended Change:
Implement error handling for WebSocket operations to ensure robustness. Use a try-except block to catch exceptions such as WebSocketDisconnect and handle unexpected data formats:
try:
data = websocket.receive_json()
assert data == {
"status": "download_ready",
"file_name": "［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分",
"message": f"文件 '［酸菜鱼ゅ°］ヒルチャールに败北した胡桃 表情、台词差分' 已完成处理，可以下载。"
}
except WebSocketDisconnect:
# Handle disconnection appropriately
except json.JSONDecodeError:
# Handle JSON parsing errors
No description provided.