<fix>[testlib]: rewrite async poll URL to use client-configured hostname#3666
Open
zstack-robot-2 wants to merge 1 commit into5.5.12from
Open
<fix>[testlib]: rewrite async poll URL to use client-configured hostname#3666zstack-robot-2 wants to merge 1 commit into5.5.12from
zstack-robot-2 wants to merge 1 commit into5.5.12from
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough在 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Action as Action/_json_http/_poll_result
participant Server as Server
participant Config as Config
Client->>Action: 发起请求/触发动作
Action->>Server: 发送初始请求
Server-->>Action: 返回 202 + Location (服务器 URL)
Action->>Config: 读取 hostname/port
Config-->>Action: 返回 hostname:port
Action->>Action: 使用 urlparse/urlunparse 重写 Location 的 netloc 为 hostname:port
Action->>Server: 对重写后的 URL 进行轮询请求
Server-->>Action: 返回轮询响应（bytes 或 已解码）
Action-->>Client: 返回轻量封装的 {status, data}
评估代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 minutes 诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/jin.ma/fix/pysdk-poll-url-rewrite
branch
2 times, most recently
from
1ba62e8 to
ab463a4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testlib/src/main/resources/zssdk.py`:
- Around line 79-83: The _error function currently sets err.description from
desc which breaks callers expecting result.error.desc; update _error to write
both properties for a compatibility period: set err.desc = desc as before and
also set err.description = desc (or vice‑versa) so both fields contain the same
text; keep err.code and err.details assignments unchanged and ensure the
dual-write is applied inside the _error function to preserve backward
compatibility with existing consumers (refer to function name _error and fields
err.desc / err.description).
- Around line 373-376: When constructing location via parsed._replace you must
handle IPv6 hosts by wrapping them in square brackets; change the netloc
construction around configured_host and configured_port so that if
configured_host is an IPv6 address it becomes "[host]:port" otherwise
"host:port". Use a reliable check (e.g. via the ipaddress module or validating
presence of ':' but prefer ipaddress.ip_address(configured_host).version==6) and
update the netloc assignment used in parsed._replace (referenced variables:
configured_host, configured_port, parsed, location) to emit bracketed IPv6
addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: dba16009-bec5-400d-870e-d358935e61db
📒 Files selected for processing (1)
testlib/src/main/resources/zssdk.py
MatheMatrix
force-pushed
the
sync/jin.ma/fix/pysdk-poll-url-rewrite
branch
from
ab463a4 to
74e51e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
testlib/src/main/resources/zssdk.py (1)
516-526: 响应处理改进合理。字节到 UTF-8 的解码确保了 Python 3 兼容性，轻量级
_Rsp封装提供了干净的接口。可选：使用 namedtuple 简化
from collections import namedtuple _Rsp = namedtuple('_Rsp', ['status', 'data']) # 然后直接 return _Rsp(status=rsp.status, data=data)不过当前实现也是可行的，这只是风格偏好。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testlib/src/main/resources/zssdk.py` around lines 516 - 526, Replace the lightweight _Rsp class with a namedtuple to simplify the response wrapper: import namedtuple from collections, define _Rsp = namedtuple('_Rsp', ['status', 'data']) and return _Rsp(status=rsp.status, data=data) instead of instantiating the custom class; update any references to _Rsp accordingly (symbols: _Rsp, rsp, data).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testlib/src/main/resources/zssdk.py`:
- Around line 364-365: The error message raised by SdkError in the block that
checks "if not location:" is missing the opening "[" and also fails to
interpolate the "%s" placeholder; update the message to include the leading "["
and format it with the appropriate API identifier (e.g., self.PATH or the
correct variable used to identify the API) so the "%s" is replaced, keeping the
SdkError raise in the same "if not location:" block and referencing SdkError and
the API identifier (self.PATH) for locating the change.
---
Nitpick comments:
In `@testlib/src/main/resources/zssdk.py`:
- Around line 516-526: Replace the lightweight _Rsp class with a namedtuple to
simplify the response wrapper: import namedtuple from collections, define _Rsp =
namedtuple('_Rsp', ['status', 'data']) and return _Rsp(status=rsp.status,
data=data) instead of instantiating the custom class; update any references to
_Rsp accordingly (symbols: _Rsp, rsp, data).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 13abd97e-ad6c-48cb-97a5-f4eabf0cb97b
📒 Files selected for processing (1)
testlib/src/main/resources/zssdk.py
1. Rewrite async poll URL to use client-configured hostname
2. Fix _error() field: dual-write err.description + err.desc
3. Fix mutable default arg headers={} in _json_http
4. Fix re.findall regex missing escape for path param validation
5. Fix min_length validation using > instead of <
6. Fix path param check to 'if u not in params'
7. Fix bare except to except Exception
8. Remove duplicate import time
9. Decode rsp.data bytes to str for readable error messages
10. Handle IPv6 addresses in poll URL with bracket notation
11. Fix missing bracket and format arg in poll error message
Resolves: ZSTAC-83925
Change-Id: I7a7062676b676a6f67646c7862706365666a6b6e
MatheMatrix
force-pushed
the
sync/jin.ma/fix/pysdk-poll-url-rewrite
branch
from
74e51e8 to
17a0ec8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testlib/src/main/resources/zssdk.py (1)
55-63:⚠️ Potential issue | 🟠 Major异步异常被吞掉会让调用方拿不到失败结果
Line 60-61 只打印异常栈但不重抛/不返回，
_async_poll中一旦异常发生，cb可能永远收不到结果，排障也困难。建议至少保留返回值并重抛，避免静默失败。建议修改
def _exception_safe(func): `@functools.wraps`(func) def wrap(*args, **kwargs): try: - func(*args, **kwargs) + return func(*args, **kwargs) except Exception: print(traceback.format_exc()) + raise return wrap🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testlib/src/main/resources/zssdk.py` around lines 55 - 63, _exception_safe currently swallows exceptions (wrap in the decorator prints the traceback but neither returns nor re-raises), causing callers like _async_poll and cb to never receive failure results; change the wrap inside _exception_safe to call and return the wrapped function's result, and on exception log the traceback and re-raise (e.g. capture the exception as e, print/log traceback, then raise) so callers receive the error instead of silent failure; ensure functools.wraps(func) is preserved and the wrapper still returns the original function's return value when no error occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testlib/src/main/resources/zssdk.py`:
- Around line 373-379: 当前对 `location` 只替换了 `netloc`（使用 parsed/_replace 和
urlunparse），导致当服务端返回相对 Location（如 "/v1/..."）时会生成以 "//host:port/..." 的无 scheme
URL；请在处理前检查 parsed.scheme（由 urlparse 返回的 parsed 对象），如果为空则设置默认 scheme 为 "http"（与
SDK 其它构造一致），然后再用 parsed._replace 来构造新的
URL；参考标识符：parsed、urlparse、parsed._replace、urlunparse、location、__config__[CONFIG_HOSTNAME]、__config__[CONFIG_PORT]、configured_host、configured_port。
---
Outside diff comments:
In `@testlib/src/main/resources/zssdk.py`:
- Around line 55-63: _exception_safe currently swallows exceptions (wrap in the
decorator prints the traceback but neither returns nor re-raises), causing
callers like _async_poll and cb to never receive failure results; change the
wrap inside _exception_safe to call and return the wrapped function's result,
and on exception log the traceback and re-raise (e.g. capture the exception as
e, print/log traceback, then raise) so callers receive the error instead of
silent failure; ensure functools.wraps(func) is preserved and the wrapper still
returns the original function's return value when no error occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: b80257b6-d0c4-4fe9-989d-670760a1aa32
📒 Files selected for processing (1)
testlib/src/main/resources/zssdk.py
Comment on lines
+373
to
+379
| parsed = urlparse(location) | ||
| configured_host = __config__[CONFIG_HOSTNAME] | ||
| configured_port = str(__config__[CONFIG_PORT]) | ||
| if ':' in configured_host and not configured_host.startswith('['): | ||
| configured_host = '[%s]' % configured_host | ||
| location = urlunparse(parsed._replace( | ||
| netloc='%s:%s' % (configured_host, configured_port))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
try:
from urllib.parse import urlparse, urlunparse
except ImportError:
from urlparse import urlparse, urlunparse
location = '/v1/api-jobs/uuid'
parsed = urlparse(location)
rewritten = urlunparse(parsed._replace(netloc='127.0.0.1:8080'))
print('input =', location)
print('rewritten=', rewritten)
print('scheme =', urlparse(rewritten).scheme or '<empty>')
PYRepository: MatheMatrix/zstack
Length of output: 154
🏁 Script executed:
find . -name "zssdk.py" -type fRepository: MatheMatrix/zstack
Length of output: 99
🏁 Script executed:
sed -n '370,385p' ./testlib/src/main/resources/zssdk.py | cat -nRepository: MatheMatrix/zstack
Length of output: 829
🏁 Script executed:
# 检查更大的上下文，找到函数定义
sed -n '360,390p' ./testlib/src/main/resources/zssdk.py | cat -nRepository: MatheMatrix/zstack
Length of output: 1621
🏁 Script executed:
# 检查整个文件是否有中文字符
grep -n '[^\x00-\x7F]' ./testlib/src/main/resources/zssdk.py | head -20Repository: MatheMatrix/zstack
Length of output: 472
🏁 Script executed:
python3 << 'EOF'
import re
with open('./testlib/src/main/resources/zssdk.py', 'r', encoding='utf-8', errors='ignore') as f:
lines = f.readlines()
for i, line in enumerate(lines, 1):
if any(ord(c) > 127 for c in line):
print(f"Line {i}: {line.rstrip()}")
EOFRepository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
python3 << 'EOF'
try:
from urllib.parse import urlparse, urlunparse
except ImportError:
from urlparse import urlparse, urlunparse
# Test case 1: relative path
location = '/v1/api-jobs/uuid'
parsed = urlparse(location)
# Current behavior (wrong)
rewritten_old = urlunparse(parsed._replace(netloc='127.0.0.1:8080'))
print("Test 1: Relative path /v1/api-jobs/uuid")
print(f" Current (wrong): {rewritten_old}")
print(f" Scheme: {urlparse(rewritten_old).scheme or '<empty>'}")
# Proposed fix
scheme = parsed.scheme or 'http'
rewritten_new = urlunparse(parsed._replace(
scheme=scheme,
netloc='127.0.0.1:8080'))
print(f" Proposed (fixed): {rewritten_new}")
print(f" Scheme: {urlparse(rewritten_new).scheme or '<empty>'}")
# Test case 2: absolute URL with scheme
print("\nTest 2: Absolute URL with scheme")
location2 = 'http://internal-server:8080/v1/api-jobs/uuid'
parsed2 = urlparse(location2)
scheme2 = parsed2.scheme or 'http'
rewritten2 = urlunparse(parsed2._replace(
scheme=scheme2,
netloc='127.0.0.1:8080'))
print(f" Input: {location2}")
print(f" Output: {rewritten2}")
# Test case 3: absolute URL with https
print("\nTest 3: Absolute URL with https")
location3 = 'https://internal-server:8080/v1/api-jobs/uuid'
parsed3 = urlparse(location3)
scheme3 = parsed3.scheme or 'http'
rewritten3 = urlunparse(parsed3._replace(
scheme=scheme3,
netloc='127.0.0.1:8080'))
print(f" Input: {location3}")
print(f" Output: {rewritten3}")
EOFRepository: MatheMatrix/zstack
Length of output: 552
相对 Location 会被重写成无 scheme 的 URL
当前只替换 netloc。当服务端返回相对路径（如 /v1/api-jobs/...）时，重写结果可能是 //host:port/...，后续请求在部分客户端会失败。建议在缺少 scheme 时补默认 scheme（与 SDK 当前构造 URL 的 http 一致）。
建议修改
parsed = urlparse(location)
configured_host = __config__[CONFIG_HOSTNAME]
configured_port = str(__config__[CONFIG_PORT])
if ':' in configured_host and not configured_host.startswith('['):
configured_host = '[%s]' % configured_host
+ scheme = parsed.scheme or 'http'
location = urlunparse(parsed._replace(
+ scheme=scheme,
netloc='%s:%s' % (configured_host, configured_port)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testlib/src/main/resources/zssdk.py` around lines 373 - 379, 当前对 `location`
只替换了 `netloc`（使用 parsed/_replace 和 urlunparse），导致当服务端返回相对 Location（如
"/v1/..."）时会生成以 "//host:port/..." 的无 scheme URL；请在处理前检查 parsed.scheme（由 urlparse
返回的 parsed 对象），如果为空则设置默认 scheme 为 "http"（与 SDK 其它构造一致），然后再用 parsed._replace
来构造新的
URL；参考标识符：parsed、urlparse、parsed._replace、urlunparse、location、__config__[CONFIG_HOSTNAME]、__config__[CONFIG_PORT]、configured_host、configured_port。
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.
变更说明
Python SDK 异步 API 返回 202 后，轮询地址使用服务端返回的 Location URL，可能包含内网 IP（如 10.x.x.x），导致客户端通过 127.0.0.1/VPN/NAT 连接时轮询不可达。
修改
_poll_result方法，将轮询 URL 的 host:port 替换为客户端配置的 hostname:port。修改前
修改后
影响范围
仅影响
./runMavenProfile pysdk生成的zssdk.py中的异步 API 轮询路径。测试
sync from gitlab !9526