fix: api_entities module Review question - #165
Merged
Merged
Conversation
修复了 code review 中发现的 7 个问题： P0: - #1 encryption.py: 为 HTTP 请求添加 timeout 参数，避免线程无限阻塞 P1: - #4 websocket_request.py: Authorization header 使用大写 'Bearer' 保持与 HTTP 请求一致 - #10 aiohttp_request.py: 为所有 DashScopeAPIResponse 添加 headers 参数 P2: - #5 dashscope_response.py: 使用 sentinel 修复 pop() 方法签名，保持 dict 语义 - #9 api_request_data.py: 复制 parameters 字典后操作，避免修改原始数据 P3: - #7 aio_session.py: 删除无效的 weakref.finalize，只保留 atexit 清理机制 - #8 http_request.py: 使用 if/elif 提升 task_id/output 处理逻辑的可读性 不修复: - #6 URL 拼接（维持现状，使用 urljoin 反而有风险） - #11 headers 合并顺序（行为正确） 待确认: - #2/#3 query 分支是否有外部使用者
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
lzsweb
changed the title
added 4 commits
July 27, 2026 18:17
根据第二份 Review 评估报告修复以下问题：
✅ 可直接采纳：
- 1d: encryption.py 添加公钥类级缓存 + TTL (1小时)
- 3a: aiohttp_request.py 移除 should_close 死代码
- 3b: http_request.py 同步请求共享 requests.Session
- 4a: websocket_request.py duplex 后台 task 保存引用并 await
- 4b: websocket_request.py 删除 close() 死方法
- 5: api_request_data.py 修正 3 处类型注解
- 6: http/aiohttp_request.py URL 替换改用 replace('/api/', '/api-task/', 1)
- 7: dashscope_response.py choices 空列表检查
- 8: http/aiohttp_request.py 改 logger.exception + 裸 raise
- 11: api_request_data.py 删除 unreachable 注释块与 _to_json_only_data
- 13: 三个文件 add_headers 改 update
❌ 不采纳：
- 1a: OAEP 需服务端协同，单侧改直接功能故障
- 1c: Python 不可变 bytes 无法真正清零内存
- 2c: asyncio.Lock 不能跨 loop，RLock 是正确选择
- 12: heartbeat 单位是秒不是毫秒，按建议改会引入问题
- test_sync_custom_session.py: mock _get_shared_sync_session 替代 requests.Session - test_utf8_encoding.py: 同上，mock _get_shared_sync_session；_MockAioResponse 添加 headers 属性 全部 423 测试通过。
- encryption: key public key cache by (base_http_api_url, api_key) with a lock, so switching endpoints no longer returns stale keys - http_request: guard shared sync session creation with a lock to eliminate the concurrent creation race - websocket_request: cancel the background send task on abnormal duplex exit to avoid task leaks; restore close() for backward compatibility - api_request_data: fix stale to_request_object docstring
Add DeprecationWarning to Threads, Messages, Runs, Steps classes. Warns only when methods are actually called, not on import. Users should migrate to the Responses API. See https://help.aliyun.com/zh/model-studio/synchronous-call-api-reference for migration details.
Replace generic https://help.aliyun.com/zh/model-studio/ with specific https://help.aliyun.com/zh/model-studio/synchronous-call-api-reference to match Java SDK and provide users with actionable migration guidance.
- Add _DEPRECATION_MSG constant to avoid long lines - Move docstrings before warnings.warn() calls - Fix import order (warnings before dashscope imports) - Split long deprecation URLs across multiple lines
- Split long URLs in _DEPRECATION_MSG constants - Break long docstring lines in create_thread_and_run - All pre-commit checks now pass
- Add type: ignore[override] to Threads.update (pre-existing signature mismatch) - Add pylint: disable=too-many-statements to websocket_request.connection_handler - Apply black style to websocket_request.py
The methods were incorrectly using super().call() with data dict instead of super().get() and super().delete() with thread_id as positional argument. This was causing test failures because the mock server expected the thread_id as a string, not wrapped in a dict. Fixes: test_retrieve and test_delete
…logic - Restore original threads.py from fedf9f8 - Add only deprecation warnings, no logic changes - Keep all super() calls unchanged (get, update, delete) - Add warnings to: create, retrieve, update, delete, create_and_run - All tests pass (5/5)
- encryption: revert to main version (PKCS1v15, no public key cache). OAEP requires server-side coordination; a unilateral client change breaks decryption. Keep only the public-keys request timeout fix. - aio_session: remove _session_finalizers dict. weakref.finalize self-registers and fires without a kept reference, and popping the dict without detach() never cancelled anything.
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
1 participant
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.
No description provided.