<fix>[rest]: improve markdown validation error reporting#3669
Open
zstack-robot-1 wants to merge 1 commit intofeature-5.5.12-znsfrom
Open
<fix>[rest]: improve markdown validation error reporting#3669zstack-robot-1 wants to merge 1 commit intofeature-5.5.12-znsfrom
zstack-robot-1 wants to merge 1 commit intofeature-5.5.12-znsfrom
Conversation
Change-Id: I1234567890abcdef1234567890abcdef12345678
高层概述此变更改进了文档生成脚本中的错误报告机制。 变更
代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 分钟 诗句
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rest/src/main/resources/scripts/RestDocumentationGenerator.groovy`:
- Around line 2841-2844: The error message when a markdown file is missing
currently only logs it.value.name; update the message to include the actual file
path mdPath and use clear English. Locate the block that constructs mdFile and
pushes to allErrors (variables mdFile, mdPath, allErrors, it.value.name) and
replace the string with a descriptive message such as "Global config markdown
not found: ${mdPath}" so each aggregated error shows the real path and is
unambiguous.
- Around line 805-839: The mismatch messages currently label the markdown
(md.globalConfig.*) values as "expected" and the code/current (globalConfig /
validatorString / newClasses) values as "actual", which is reversed; update each
mismatches.add call (for name, defaultValue, description, type, category,
resources/classes, valueRange) to either swap the two sides so expected= current
code value and actual= markdown value or — clearer — replace the labels with
unambiguous keys like "markdown=" and "current=" (modify the mismatches.add
string constructions that reference md.globalConfig.name, globalConfig.name,
md.globalConfig.defaultValue, globalConfig.defaultValue,
StringUtils.trimToEmpty(md.globalConfig.description),
StringUtils.trimToEmpty(globalConfig.description), md.globalConfig.type,
globalConfig.type, md.globalConfig.category, globalConfig.category,
oldClasses/newClasses, and md.globalConfig.valueRange/validatorString while
keeping the mdPath context intact).
- Around line 790-799: The code in isConsistent currently builds mdPath from
md.globalConfig.category and md.globalConfig.name which can be stale; change the
path construction to use the authoritative globalConfig values (e.g.,
globalConfig.category and globalConfig.name) instead of md.globalConfig.*, and
guard nulls (md or globalConfig) already checked; update the mdPath assignment
(the PathUtil.join call that sets mdPath) to reference globalConfig.category and
globalConfig.name so reported paths reflect the actual files being validated.
🪄 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: c602d03e-0b3b-44ab-a386-9c9835e25021
📒 Files selected for processing (1)
rest/src/main/resources/scripts/RestDocumentationGenerator.groovy
Comment on lines
+790
to
799
| List<String> isConsistent(GlobalConfigMarkDown md, GlobalConfig globalConfig) { | ||
| if (md == null) { | ||
| return ["GlobalConfigMarkDown is null"] | ||
| } | ||
| if (globalConfig == null) { | ||
| return ["GlobalConfig is null"] | ||
| } | ||
| String mdPath = | ||
| PathUtil.join(PathUtil.join(Paths.get("../doc").toAbsolutePath().normalize().toString(), | ||
| "globalconfig"), md.globalConfig.category, md.globalConfig.name) + ".md" |
There was a problem hiding this comment.
不要用 Markdown 内容回推报错路径。
这里重新用 md.globalConfig.category/name 组装 mdPath，但这两个字段本身就是后面要校验的内容。只要 Name 或 Category 过期，最终报错就会指向旧路径，而不是当前实际校验的文件，新的聚合报错会误导排查。
💡 建议修改
- List<String> isConsistent(GlobalConfigMarkDown md, GlobalConfig globalConfig) {
+ List<String> isConsistent(GlobalConfigMarkDown md, GlobalConfig globalConfig, String mdPath) {
if (md == null) {
return ["GlobalConfigMarkDown is null"]
}
if (globalConfig == null) {
return ["GlobalConfig is null"]
}
- String mdPath =
- PathUtil.join(PathUtil.join(Paths.get("../doc").toAbsolutePath().normalize().toString(),
- "globalconfig"), md.globalConfig.category, md.globalConfig.name) + ".md"- List<String> inconsistencies = isConsistent(markDown, globalConfig)
+ List<String> inconsistencies = isConsistent(markDown, globalConfig, mdPath)Also applies to: 862-862
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rest/src/main/resources/scripts/RestDocumentationGenerator.groovy` around
lines 790 - 799, The code in isConsistent currently builds mdPath from
md.globalConfig.category and md.globalConfig.name which can be stale; change the
path construction to use the authoritative globalConfig values (e.g.,
globalConfig.category and globalConfig.name) instead of md.globalConfig.*, and
guard nulls (md or globalConfig) already checked; update the mdPath assignment
(the PathUtil.join call that sets mdPath) to reference globalConfig.category and
globalConfig.name so reported paths reflect the actual files being validated.
Comment on lines
+805
to
+839
| if (md.globalConfig.name != globalConfig.name) { | ||
| logger.info("name of ${mdPath} is not latest") | ||
| mismatches.add("name mismatch in ${mdPath}: expected='${md.globalConfig.name}', actual='${globalConfig.name}'") | ||
| } | ||
| if (md.globalConfig.defaultValue != globalConfig.defaultValue) { | ||
| logger.info("defaultValue of ${mdPath} is not latest") | ||
| mismatches.add("defaultValue mismatch in ${mdPath}: expected='${md.globalConfig.defaultValue}', actual='${globalConfig.defaultValue}'") | ||
| } | ||
| if (StringUtils.trimToEmpty(md.globalConfig.description) != StringUtils.trimToEmpty(globalConfig.description)) { | ||
| logger.info("desc of ${mdPath} is not latest") | ||
| mismatches.add("description mismatch in ${mdPath}: expected='${StringUtils.trimToEmpty(md.globalConfig.description)}', actual='${StringUtils.trimToEmpty(globalConfig.description)}'") | ||
| } | ||
| if (md.globalConfig.type != globalConfig.type) { | ||
| if (globalConfig.type != null) { | ||
| logger.info("type of ${mdPath} is not latest") | ||
| mismatches.add("type mismatch in ${mdPath}: expected='${md.globalConfig.type}', actual='${globalConfig.type}'") | ||
| } | ||
| } | ||
| if (md.globalConfig.category != globalConfig.category) { | ||
| logger.info("category of ${mdPath} is not latest") | ||
| mismatches.add("category mismatch in ${mdPath}: expected='${md.globalConfig.category}', actual='${globalConfig.category}'") | ||
| } | ||
| List<String> oldClasses = md.globalConfig.resources.sort() | ||
| if (oldClasses != newClasses) { | ||
| logger.info("classes of ${mdPath} is not latest") | ||
| mismatches.add("resources mismatch in ${mdPath}: expected='${oldClasses}', actual='${newClasses}'") | ||
| } | ||
|
|
||
| if (md.globalConfig.valueRange != (validatorString)) { | ||
| boolean useBooleanValidator = (globalConfig.type == "java.lang.Boolean" | ||
| && md.globalConfig.valueRange == "{true, false}") | ||
| if (validatorString != null || !useBooleanValidator) { | ||
| logger.info("valueRange of ${mdPath} is not latest") | ||
| logger.info("valueRange = ${md.globalConfig.valueRange} validatorString = ${validatorString}") | ||
| flag = false | ||
| } | ||
| } | ||
| return flag | ||
| } | ||
| if (validatorString != null || !useBooleanValidator) { | ||
| logger.info("valueRange of ${mdPath} is not latest") | ||
| logger.info("valueRange = ${md.globalConfig.valueRange} validatorString = ${validatorString}") | ||
| mismatches.add("valueRange mismatch in ${mdPath}: expected='${md.globalConfig.valueRange}', actual='${validatorString}'") |
There was a problem hiding this comment.
expected / actual 的方向写反了。
现在的 mismatch 文案把 Markdown 里的旧值写成 expected，把代码里的最新值写成 actual。这会让人按反方向修文档。建议至少对调两者，或者直接改成 markdown= / current= 这类不含歧义的键。
💡 示例
- mismatches.add("name mismatch in ${mdPath}: expected='${md.globalConfig.name}', actual='${globalConfig.name}'")
+ mismatches.add("name mismatch in ${mdPath}: markdown='${md.globalConfig.name}', current='${globalConfig.name}'")其余 defaultValue / description / type / category / resources / valueRange 的报错也需要同样调整。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rest/src/main/resources/scripts/RestDocumentationGenerator.groovy` around
lines 805 - 839, The mismatch messages currently label the markdown
(md.globalConfig.*) values as "expected" and the code/current (globalConfig /
validatorString / newClasses) values as "actual", which is reversed; update each
mismatches.add call (for name, defaultValue, description, type, category,
resources/classes, valueRange) to either swap the two sides so expected= current
code value and actual= markdown value or — clearer — replace the labels with
unambiguous keys like "markdown=" and "current=" (modify the mismatches.add
string constructions that reference md.globalConfig.name, globalConfig.name,
md.globalConfig.defaultValue, globalConfig.defaultValue,
StringUtils.trimToEmpty(md.globalConfig.description),
StringUtils.trimToEmpty(globalConfig.description), md.globalConfig.type,
globalConfig.type, md.globalConfig.category, globalConfig.category,
oldClasses/newClasses, and md.globalConfig.valueRange/validatorString while
keeping the mdPath context intact).
Comment on lines
+2841
to
+2844
| File mdFile = new File(mdPath) | ||
| if (!mdFile.exists()) { | ||
| allErrors.add("Not found the document markdown of the global config ${it.value.name} , please generate it first.") | ||
| return |
There was a problem hiding this comment.
缺失 Markdown 的报错需要带上真实路径。
这里只输出了 name，同名配置分布在不同 category 时会变得不可区分；而且这句英文也不够自然。聚合报错里最好直接输出 mdPath，例如 Global config markdown not found: ${mdPath}。
💡 建议修改
- allErrors.add("Not found the document markdown of the global config ${it.value.name} , please generate it first.")
+ allErrors.add("Global config markdown not found: ${mdPath}. Please generate it first.")As per coding guidelines, "代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rest/src/main/resources/scripts/RestDocumentationGenerator.groovy` around
lines 2841 - 2844, The error message when a markdown file is missing currently
only logs it.value.name; update the message to include the actual file path
mdPath and use clear English. Locate the block that constructs mdFile and pushes
to allErrors (variables mdFile, mdPath, allErrors, it.value.name) and replace
the string with a descriptive message such as "Global config markdown not found:
${mdPath}" so each aggregated error shows the real path and is unambiguous.
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.
Change-Id: I1234567890abcdef1234567890abcdef12345678
sync from gitlab !9529