<fix>[core]: support configure reply-location URL#3874
Closed
ZStack-Robot wants to merge 1 commit into
Closed
Conversation
Walkthrough新增三个全局配置（回复位置的协议、IP、端口）；RESTFacade 接口扩展并由 RESTFacadeImpl 在 init 阶段预构建并暴露 replyLocationBaseUrl；RestServer 在异步 202 响应的 Location 头中使用该预构建 URL。 Changes
Sequence Diagram(s)sequenceDiagram
participant Config as 全局配置\n(REPLY_LOCATION_URL_*)
participant Init as RESTFacadeImpl\n(init)
participant Facade as RESTFacade\n(getReplyLocationBaseUrl)
participant Server as RestServer\n(handle request)
participant Client as 客户端
Config->>Init: 读取 protocol / ip / port / path / callbackHostName
Init->>Init: 规范化并计算 replyLocationBaseUrl，校验可解析性
Init-->>Facade: 缓存并暴露 replyLocationBaseUrl
Client->>Server: 发送异步 API 请求
Server->>Facade: 调用 getReplyLocationBaseUrl()
Facade-->>Server: 返回 replyLocationBaseUrl
Server->>Server: 拼接版本/async-job 路径/Job-UUID
Server->>Client: 返回 202 Accepted\nLocation: {replyLocationBaseUrl}/...
估计代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 minutes 庆祝诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@core/src/main/java/org/zstack/core/CoreGlobalProperty.java`:
- Around line 103-105: The default value for REPLY_LOCATION_URL_PORT in
CoreGlobalProperty conflicts with its `@NumberRange` constraint and
RESTFacadeImpl's sentinel usage of 0; change the numeric range to allow 0 by
updating the `@NumberRange` on REPLY_LOCATION_URL_PORT to include 0 (e.g.,
`@NumberRange`({0, 65535})) so the default "0" remains valid and continues to
serve as the "use management port" sentinel used by RESTFacadeImpl and related
code paths.
In `@core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java`:
- Around line 155-160: The replyLocationUrl construction in RESTFacadeImpl
currently appends "/" when path is blank, which can produce a double-slash like
"//v1/..." when RestServer later calls ub.path(...); change the path fragment
logic so it is empty when path is blank and only prefix with "/" when path is
non-blank (i.e., compute a pathPart = StringUtils.isBlank(path) ? "" : "/" +
path and use that in the String.format for replyLocationUrl), referencing the
replyLocationUrl assignment in RESTFacadeImpl and the CoreGlobalProperty fields
used there.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be894249-aebd-4fdc-bbfe-a9923077bcb9
📒 Files selected for processing (4)
core/src/main/java/org/zstack/core/CoreGlobalProperty.javacore/src/main/java/org/zstack/core/rest/RESTFacadeImpl.javaheader/src/main/java/org/zstack/header/rest/RESTFacade.javarest/src/main/java/org/zstack/rest/RestServer.java
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-8
branch
from
f78ea70 to
a41a289
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java`:
- Around line 159-163: The code builds replyLocationUrl from CoreGlobalProperty
values in RESTFacadeImpl (replyLocationUrl) but doesn't validate it at startup;
add a fail-fast URI/URL validation in RESTFacadeImpl.init() after constructing
replyLocationUrl (e.g. try new URI/String parsing or use
UriComponentsBuilder.build() to validate) and throw a clear
InitializationException if parsing fails so misconfigured CoreGlobalProperty
values (protocol/IP/port) are reported at startup rather than causing a runtime
error later in RestServer.sendMessage() where
UriComponentsBuilder.fromHttpUrl(restf.getReplyLocationUrl()) is called.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b6c2fced-9dc9-4c4d-b4b3-987c15924b2b
📒 Files selected for processing (4)
core/src/main/java/org/zstack/core/CoreGlobalProperty.javacore/src/main/java/org/zstack/core/rest/RESTFacadeImpl.javaheader/src/main/java/org/zstack/header/rest/RESTFacade.javarest/src/main/java/org/zstack/rest/RestServer.java
🚧 Files skipped from review as they are similar to previous changes (3)
- rest/src/main/java/org/zstack/rest/RestServer.java
- header/src/main/java/org/zstack/header/rest/RESTFacade.java
- core/src/main/java/org/zstack/core/CoreGlobalProperty.java
Collaborator
Author
|
Comment from yaohua.wu: Review: MR !9753 — ZSV-11815背景为解决金山客户场景下 async API 返回的 Location 头使用管理网内部 🔴 Critical
🟡 Warning
🟢 Suggestion
Coverage
Verdict: REVISION_REQUIRED需修复 #1 后再合并；建议同时合并修复 #2/#3 以彻底解决误配问题。Suggestion 类条目作者酌情采纳。 🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-8
branch
from
a41a289 to
7e4bcad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/rest/RESTFacade.java (1)
83-86: Javadoc 语义不够明确，建议直接写清 202 Location 的用途。Line [84] 当前描述不足以区分该方法与
getBaseUrl()的职责边界，后续维护时容易误用。✏️ 建议修改
- /** - * in location of API message reply body - */ + /** + * Returns the externally reachable base URL used to build the HTTP 202 + * Location header for async API job polling. + */ String getReplyLocationBaseUrl();As per coding guidelines, 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/rest/RESTFacade.java` around lines 83 - 86, Update the Javadoc for getReplyLocationBaseUrl() to explicitly state that it returns the base URL used to construct the HTTP Location header for 202 Accepted responses (i.e., where clients can retrieve the eventual reply), specify the expected format (e.g., scheme://host[:port] without path) and explicitly contrast it with getBaseUrl() (which is the general service base URL for APIs), so callers won't confuse the two methods; ensure the method has a concise, clear Javadoc and no extra access modifiers.core/src/main/java/org/zstack/core/CoreGlobalProperty.java (1)
99-105: 建议在属性层收紧协议取值，并把哨兵语义写成确定性注释。Line [99]-Line [105] 的功能已可用，但
REPLY_LOCATION_URL_PROTOCOL仍可配置任意字符串；另外REPLY_LOCATION_URL_PORT注释里的 “maybe” 不够确定，IP空串回退语义也建议就地说明，减少配置试错。✏️ 建议修改
`@GlobalProperty`(name = "core.reply.location.url.protocol", defaultValue = "http") + `@AvailableValues`(value = {"http", "https"}) public static String REPLY_LOCATION_URL_PROTOCOL; `@GlobalProperty`(name = "core.reply.location.url.ip", defaultValue = "") + // empty -> use callbackHostName public static String REPLY_LOCATION_URL_IP; `@GlobalProperty`(name = "core.reply.location.url.port", defaultValue = "0") - `@NumberRange`({0, 65535}) // Note: 0 -> use default value (maybe Platform.getManagementNodeServicePort()) + `@NumberRange`({0, 65535}) // 0 -> use management node service port public static int REPLY_LOCATION_URL_PORT;As per coding guidelines, 代码应尽量做到自解释，且注释需要随代码更新并确保内容正确。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/CoreGlobalProperty.java` around lines 99 - 105, REPLY_LOCATION_URL_PROTOCOL should be constrained to a small set of allowed values (e.g., "http" or "https") instead of allowing any string; enforce this by validating/normalizing the property when read (or replace the declaration with an enum-backed/global property validator) and reject or default unknown values; clarify sentinel semantics inline: document that REPLY_LOCATION_URL_IP == "" means "use management node IP" and REPLY_LOCATION_URL_PORT == 0 deterministically means "use Platform.getManagementNodeServicePort()", updating the comment on REPLY_LOCATION_URL_PORT and adding a short comment for REPLY_LOCATION_URL_IP; reference the symbols REPLY_LOCATION_URL_PROTOCOL, REPLY_LOCATION_URL_IP, and REPLY_LOCATION_URL_PORT when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java`:
- Around line 155-158: The current normalization sets normalizedPath = "/" +
StringUtils.strip(path, "/") when StringUtils.isNotBlank(path), which yields "/"
for inputs like "/" or "///" and can reintroduce double-slash paths; change the
logic in RESTFacadeImpl to strip the slashes first and only prepend a single "/"
if the stripped result is non-empty (i.e., use StringUtils.strip(path, "/") into
a temp variable, check for notBlank on that stripped value, then set
normalizedPath = "/" + stripped), ensuring normalizedPath stays empty for
root-only inputs and avoiding "//" when later calling ub.path(...).
---
Nitpick comments:
In `@core/src/main/java/org/zstack/core/CoreGlobalProperty.java`:
- Around line 99-105: REPLY_LOCATION_URL_PROTOCOL should be constrained to a
small set of allowed values (e.g., "http" or "https") instead of allowing any
string; enforce this by validating/normalizing the property when read (or
replace the declaration with an enum-backed/global property validator) and
reject or default unknown values; clarify sentinel semantics inline: document
that REPLY_LOCATION_URL_IP == "" means "use management node IP" and
REPLY_LOCATION_URL_PORT == 0 deterministically means "use
Platform.getManagementNodeServicePort()", updating the comment on
REPLY_LOCATION_URL_PORT and adding a short comment for REPLY_LOCATION_URL_IP;
reference the symbols REPLY_LOCATION_URL_PROTOCOL, REPLY_LOCATION_URL_IP, and
REPLY_LOCATION_URL_PORT when making these changes.
In `@header/src/main/java/org/zstack/header/rest/RESTFacade.java`:
- Around line 83-86: Update the Javadoc for getReplyLocationBaseUrl() to
explicitly state that it returns the base URL used to construct the HTTP
Location header for 202 Accepted responses (i.e., where clients can retrieve the
eventual reply), specify the expected format (e.g., scheme://host[:port] without
path) and explicitly contrast it with getBaseUrl() (which is the general service
base URL for APIs), so callers won't confuse the two methods; ensure the method
has a concise, clear Javadoc and no extra access modifiers.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d96975d-eafa-4832-9863-91c1dc82f25c
📒 Files selected for processing (4)
core/src/main/java/org/zstack/core/CoreGlobalProperty.javacore/src/main/java/org/zstack/core/rest/RESTFacadeImpl.javaheader/src/main/java/org/zstack/header/rest/RESTFacade.javarest/src/main/java/org/zstack/rest/RestServer.java
Comment on lines
+155
to
+158
| String normalizedPath = ""; | ||
| if (StringUtils.isNotBlank(path)) { | ||
| normalizedPath = "/" + StringUtils.strip(path, "/"); | ||
| } |
There was a problem hiding this comment.
path 仅含 / 时仍会生成尾部斜杠，可能回退出 //v1/...。
Line [156]-Line [158] 先判断 isNotBlank(path) 再拼接，path="/"（或 "///"）时 strip 后为空字符串，但仍会得到 normalizedPath="/"。这会把之前修复过的双斜杠风险带回来（下游继续 ub.path(...) 时）。
🐛 建议修复
- String normalizedPath = "";
- if (StringUtils.isNotBlank(path)) {
- normalizedPath = "/" + StringUtils.strip(path, "/");
- }
+ String normalizedPath = "";
+ String strippedPath = StringUtils.strip(path, "/");
+ if (StringUtils.isNotBlank(strippedPath)) {
+ normalizedPath = "/" + strippedPath;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java` around lines 155
- 158, The current normalization sets normalizedPath = "/" +
StringUtils.strip(path, "/") when StringUtils.isNotBlank(path), which yields "/"
for inputs like "/" or "///" and can reintroduce double-slash paths; change the
logic in RESTFacadeImpl to strip the slashes first and only prepend a single "/"
if the stripped result is non-empty (i.e., use StringUtils.strip(path, "/") into
a temp variable, check for notBlank on that stripped value, then set
normalizedPath = "/" + stripped), ensuring normalizedPath stays empty for
root-only inputs and avoiding "//" when later calling ub.path(...).
Introduces configurable reply-location URL pieces (protocol, IP, port) and has RESTFacadeImpl build a replyLocationUrl string from them. The REST API's async job Location header (used when returning 202 Accepted for async messages) now uses this replyLocationUrl instead of the internal baseUrl. This allows overriding the public-facing callback host/IP/port used by clients while keeping internal base URL configuration separate. Resolves: ZSV-11815 Change-Id: I67656e6d72687462676966616b72666372746d64
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-8
branch
from
7e4bcad to
2f9d87c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java (1)
155-158:⚠️ Potential issue | 🟡 Minor
path为仅斜杠时仍会回退到"/"，可能再次产生双斜杠 URLLine [156]-Line [157] 先判断原始
path再strip，当path="/"或"///"时，normalizedPath仍会被设置为"/"。下游继续拼接 path 时，可能出现//v1/...。🐛 建议修复
- String normalizedPath = ""; - if (StringUtils.isNotBlank(path)) { - normalizedPath = "/" + StringUtils.strip(path, "/"); - } + String normalizedPath = ""; + String strippedPath = StringUtils.strip(path, "/"); + if (StringUtils.isNotBlank(strippedPath)) { + normalizedPath = "/" + strippedPath; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java` around lines 155 - 158, The current normalization in RESTFacadeImpl sets normalizedPath = "/" + StringUtils.strip(path, "/") whenever path is not blank, which yields "/" for inputs like "/" or "///" and can cause double slashes downstream; change the logic to strip leading/trailing slashes first into a temporary String (e.g., stripped = StringUtils.strip(path, "/")), then set normalizedPath to empty string if stripped is empty, otherwise set normalizedPath = "/" + stripped, so that inputs of only slashes produce "" rather than "/".
🧹 Nitpick comments (2)
core/src/main/java/org/zstack/core/CoreGlobalProperty.java (1)
101-102: IP 回退注释建议改为“空串/空白”而非仅null当前注释写的是
null -> use callbackHostName，但实现实际按 blank（含空串/空白）回退。建议注释与真实语义保持一致，避免运维误解。📝 建议修改
- `@GlobalProperty`(name = "core.reply.location.url.ip", defaultValue = "") // Note: null -> use callbackHostName + `@GlobalProperty`(name = "core.reply.location.url.ip", defaultValue = "") // Note: blank -> use callbackHostName public static String REPLY_LOCATION_URL_IP;As per coding guidelines “对于较长的注释，需要仔细校对并随代码更新，确保内容正确”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/CoreGlobalProperty.java` around lines 101 - 102, Update the comment on the CoreGlobalProperty.REPLY_LOCATION_URL_IP declaration to reflect that the fallback is used when the property is blank/empty (empty string or whitespace) rather than only null; modify the inline comment "@GlobalProperty(name = "core.reply.location.url.ip", defaultValue = "") // Note: null -> use callbackHostName" to indicate blank/empty string -> use callbackHostName so the comment matches the actual fallback semantics.header/src/main/java/org/zstack/header/rest/RESTFacade.java (1)
83-86: 补充getReplyLocationBaseUrl()的 Javadoc 语义说明当前注释语义不清晰（Line [84]），建议明确这是“异步 API 返回
202 Accepted时用于Location头的外部可达基础 URL”，避免后续调用方误解。✏️ 建议修改
- /** - * in location of API message reply body - */ + /** + * Returns the externally reachable base URL used to build the HTTP Location + * header for async API responses (202 Accepted). + */ String getReplyLocationBaseUrl();As per coding guidelines 接口方法“必须配有有效的 Javadoc 注释”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/rest/RESTFacade.java` around lines 83 - 86, Update the Javadoc for the RESTFacade.getReplyLocationBaseUrl() method to clearly state that this string is the externally reachable base URL used to populate the HTTP Location response header for asynchronous APIs that return 202 Accepted; specify that callers should append the reply resource path/ID to this base URL (include expected format/behavior regarding trailing slashes or path concatenation) so there is no ambiguity about how to construct the full Location header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java`:
- Around line 155-158: The current normalization in RESTFacadeImpl sets
normalizedPath = "/" + StringUtils.strip(path, "/") whenever path is not blank,
which yields "/" for inputs like "/" or "///" and can cause double slashes
downstream; change the logic to strip leading/trailing slashes first into a
temporary String (e.g., stripped = StringUtils.strip(path, "/")), then set
normalizedPath to empty string if stripped is empty, otherwise set
normalizedPath = "/" + stripped, so that inputs of only slashes produce ""
rather than "/".
---
Nitpick comments:
In `@core/src/main/java/org/zstack/core/CoreGlobalProperty.java`:
- Around line 101-102: Update the comment on the
CoreGlobalProperty.REPLY_LOCATION_URL_IP declaration to reflect that the
fallback is used when the property is blank/empty (empty string or whitespace)
rather than only null; modify the inline comment "@GlobalProperty(name =
"core.reply.location.url.ip", defaultValue = "") // Note: null -> use
callbackHostName" to indicate blank/empty string -> use callbackHostName so the
comment matches the actual fallback semantics.
In `@header/src/main/java/org/zstack/header/rest/RESTFacade.java`:
- Around line 83-86: Update the Javadoc for the
RESTFacade.getReplyLocationBaseUrl() method to clearly state that this string is
the externally reachable base URL used to populate the HTTP Location response
header for asynchronous APIs that return 202 Accepted; specify that callers
should append the reply resource path/ID to this base URL (include expected
format/behavior regarding trailing slashes or path concatenation) so there is no
ambiguity about how to construct the full Location header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7826d0d6-e282-417b-a686-e910d3999cbe
📒 Files selected for processing (4)
core/src/main/java/org/zstack/core/CoreGlobalProperty.javacore/src/main/java/org/zstack/core/rest/RESTFacadeImpl.javaheader/src/main/java/org/zstack/header/rest/RESTFacade.javarest/src/main/java/org/zstack/rest/RestServer.java
🚧 Files skipped from review as they are similar to previous changes (1)
- rest/src/main/java/org/zstack/rest/RestServer.java
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.
Introduces configurable reply-location URL pieces (protocol, IP, port)
and has RESTFacadeImpl build a replyLocationUrl string from them.
The REST API's async job Location header (used when returning
202 Accepted for async messages) now uses this replyLocationUrl
instead of the internal baseUrl.
This allows overriding the public-facing callback host/IP/port
used by clients while keeping internal base URL configuration separate.
Resolves: ZSV-11815
Change-Id: I67656e6d72687462676966616b72666372746d64
sync from gitlab !9753