<test>[testlib]: cjlib improve#4162
Open
MatheMatrix wants to merge 1 commit into
Open
Conversation
cjlib improve Resolves: ZSTAC-63145 Change-Id: I7077737376616b66696c6b617269786b6e72617a (cherry picked from commit 6d8e56c)
概览为 TProxy 代理类新增参数化构造函数，支持在创建代理对象时传递构造参数给被代理类，并保留适配器字段值的复制逻辑。 变更TProxy 参数化构造函数
代码审查工作量估计🎯 2 (简单) | ⏱️ ~10 分钟 庆祝诗
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testlib/src/main/java/org/zstack/testlib/util/TProxy.groovy`:
- Around line 90-96: The catch block handling IllegalArgumentException in TProxy
(where adaptee.getClass() is referenced) uses a misleading message "has no
non-argument constructor"; update that exception message to reflect the actual
situation when a parameterized constructor cannot be found (e.g., "has no
matching parameterized constructor for the provided arguments") and rethrow the
new Exception with the original cause so the message correctly describes the
lookup failure for a parameterized constructor.
- Around line 72-73: 构造函数 TProxy(Object adaptee, Class[] argumentTypes, Object[]
arguments) 缺少对 argumentTypes 和 arguments 的校验，会导致 NPE 或参数数量/类型不匹配的运行时错误；在方法开始处验证
argumentTypes 和 arguments 非 null，并且检查 argumentTypes.length ==
arguments.length（不满足时抛出 IllegalArgumentException 或 AssertionError），同时保留对 adaptee
的现有断言并在错误消息中说明期望的参数数量和类型以便调试。
- Around line 72-97: The two constructors TProxy(Object adaptee) and
TProxy(Object adaptee, Class[] argumentTypes, Object[] arguments) duplicate
field-copy and exception-handling logic; refactor by extracting the shared logic
into a private helper (e.g., initProxyFields(Object proxyedObject, Object
adaptee)) that performs the FieldUtils.getAllFields loop and the
IllegalArgumentException handling, and have each constructor only create the
Enhancer, call enhancer.create(...) with the appropriate signature, then pass
the created proxy object and adaptee into that helper; ensure the helper
preserves the current behavior (skip static/final fields, setAccessible, copy
values, and translate the specific IllegalArgumentException message).
🪄 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: eb06b31a-a585-4773-a1aa-21b43899111d
📒 Files selected for processing (1)
testlib/src/main/java/org/zstack/testlib/util/TProxy.groovy
Comment on lines
+72
to
+73
| TProxy(Object adaptee, Class[] argumentTypes, Object[] arguments) { | ||
| assert adaptee != null : "call TProxy(Class adapteeClass) instead" |
There was a problem hiding this comment.
添加参数验证以防止运行时错误。
构造函数缺少对 argumentTypes 和 arguments 的验证：
- 未检查 null 值，可能导致 NPE
- 未验证
argumentTypes.length == arguments.length，类型与参数数量不匹配会在运行时失败
🛡️ 建议添加参数验证
TProxy(Object adaptee, Class[] argumentTypes, Object[] arguments) {
assert adaptee != null : "call TProxy(Class adapteeClass) instead"
+ assert argumentTypes != null : "argumentTypes cannot be null"
+ assert arguments != null : "arguments cannot be null"
+ assert argumentTypes.length == arguments.length : "argumentTypes and arguments must have the same length"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@testlib/src/main/java/org/zstack/testlib/util/TProxy.groovy` around lines 72
- 73, 构造函数 TProxy(Object adaptee, Class[] argumentTypes, Object[] arguments) 缺少对
argumentTypes 和 arguments 的校验，会导致 NPE 或参数数量/类型不匹配的运行时错误；在方法开始处验证 argumentTypes 和
arguments 非 null，并且检查 argumentTypes.length == arguments.length（不满足时抛出
IllegalArgumentException 或 AssertionError），同时保留对 adaptee
的现有断言并在错误消息中说明期望的参数数量和类型以便调试。
Comment on lines
+72
to
+97
| TProxy(Object adaptee, Class[] argumentTypes, Object[] arguments) { | ||
| assert adaptee != null : "call TProxy(Class adapteeClass) instead" | ||
|
|
||
| Enhancer enhancer = new Enhancer() | ||
| enhancer.setSuperclass(adaptee.getClass()) | ||
| enhancer.setCallback(new ProxyHandler()) | ||
| try { | ||
| proxyedObject = enhancer.create(argumentTypes, arguments) | ||
| if (adaptee != null) { | ||
| FieldUtils.getAllFields(adaptee.getClass()).each { f -> | ||
| if (Modifier.isStatic(f.modifiers) || Modifier.isFinal(f.modifiers)) { | ||
| return | ||
| } | ||
|
|
||
| f.setAccessible(true) | ||
| f.set(proxyedObject, f.get(adaptee)) | ||
| } | ||
| } | ||
| } catch (IllegalArgumentException e) { | ||
| if (e.getMessage().contains("Superclass has no null constructors" )) { | ||
| throw new Exception("the class ${adaptee.getClass()} has no non-argument constructor", e) | ||
| } | ||
|
|
||
| throw e | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
消除与现有构造函数的代码重复。
新构造函数与 TProxy(Object adaptee)（第 45-70 行）几乎完全重复，唯一区别是第 79 行调用 enhancer.create(argumentTypes, arguments) 而非无参的 enhancer.create()。字段复制和异常处理逻辑完全相同，违反 DRY 原则。
建议提取公共逻辑到私有辅助方法中，以提高可维护性并减少未来出现不一致的风险。
♻️ 建议重构以消除重复
+ private void copyFields(Object source, Object target) {
+ FieldUtils.getAllFields(source.getClass()).each { f ->
+ if (Modifier.isStatic(f.modifiers) || Modifier.isFinal(f.modifiers)) {
+ return
+ }
+ f.setAccessible(true)
+ f.set(target, f.get(source))
+ }
+ }
+
+ private Object createProxy(Object adaptee, Closure createEnhancer) {
+ Enhancer enhancer = new Enhancer()
+ enhancer.setSuperclass(adaptee.getClass())
+ enhancer.setCallback(new ProxyHandler())
+ try {
+ def proxy = createEnhancer(enhancer)
+ if (adaptee != null) {
+ copyFields(adaptee, proxy)
+ }
+ return proxy
+ } catch (IllegalArgumentException e) {
+ if (e.getMessage().contains("Superclass has no null constructors")) {
+ throw new Exception("the class ${adaptee.getClass()} has no matching constructor", e)
+ }
+ throw e
+ }
+ }
+
TProxy(Object adaptee) {
assert adaptee != null : "call TProxy(Class adapteeClass) instead"
- Enhancer enhancer = new Enhancer()
- enhancer.setSuperclass(adaptee.getClass())
- enhancer.setCallback(new ProxyHandler())
- try {
- proxyedObject = enhancer.create()
- if (adaptee != null) {
- FieldUtils.getAllFields(adaptee.getClass()).each { f ->
- if (Modifier.isStatic(f.modifiers) || Modifier.isFinal(f.modifiers)) {
- return
- }
-
- f.setAccessible(true)
- f.set(proxyedObject, f.get(adaptee))
- }
- }
- } catch (IllegalArgumentException e) {
- if (e.getMessage().contains("Superclass has no null constructors" )) {
- throw new Exception("the class ${adaptee.getClass()} has no non-argument constructor", e)
- }
-
- throw e
- }
+ proxyedObject = createProxy(adaptee) { enhancer -> enhancer.create() }
}
TProxy(Object adaptee, Class[] argumentTypes, Object[] arguments) {
assert adaptee != null : "call TProxy(Class adapteeClass) instead"
- Enhancer enhancer = new Enhancer()
- enhancer.setSuperclass(adaptee.getClass())
- enhancer.setCallback(new ProxyHandler())
- try {
- proxyedObject = enhancer.create(argumentTypes, arguments)
- if (adaptee != null) {
- FieldUtils.getAllFields(adaptee.getClass()).each { f ->
- if (Modifier.isStatic(f.modifiers) || Modifier.isFinal(f.modifiers)) {
- return
- }
-
- f.setAccessible(true)
- f.set(proxyedObject, f.get(adaptee))
- }
- }
- } catch (IllegalArgumentException e) {
- if (e.getMessage().contains("Superclass has no null constructors" )) {
- throw new Exception("the class ${adaptee.getClass()} has no non-argument constructor", e)
- }
-
- throw e
- }
+ proxyedObject = createProxy(adaptee) { enhancer -> enhancer.create(argumentTypes, arguments) }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@testlib/src/main/java/org/zstack/testlib/util/TProxy.groovy` around lines 72
- 97, The two constructors TProxy(Object adaptee) and TProxy(Object adaptee,
Class[] argumentTypes, Object[] arguments) duplicate field-copy and
exception-handling logic; refactor by extracting the shared logic into a private
helper (e.g., initProxyFields(Object proxyedObject, Object adaptee)) that
performs the FieldUtils.getAllFields loop and the IllegalArgumentException
handling, and have each constructor only create the Enhancer, call
enhancer.create(...) with the appropriate signature, then pass the created proxy
object and adaptee into that helper; ensure the helper preserves the current
behavior (skip static/final fields, setAccessible, copy values, and translate
the specific IllegalArgumentException message).
Comment on lines
+90
to
+96
| } catch (IllegalArgumentException e) { | ||
| if (e.getMessage().contains("Superclass has no null constructors" )) { | ||
| throw new Exception("the class ${adaptee.getClass()} has no non-argument constructor", e) | ||
| } | ||
|
|
||
| throw e | ||
| } |
There was a problem hiding this comment.
修正异常消息以匹配参数化构造的上下文。
第 92 行的错误消息"has no non-argument constructor"在此上下文中不正确。当调用带参数的构造函数时，该消息具有误导性——实际问题应该是找不到匹配的参数化构造函数，而非无参构造函数缺失。
🐛 建议修正错误消息
} catch (IllegalArgumentException e) {
if (e.getMessage().contains("Superclass has no null constructors" )) {
- throw new Exception("the class ${adaptee.getClass()} has no non-argument constructor", e)
+ throw new Exception("the class ${adaptee.getClass()} has no matching constructor for the given argument types", e)
}
throw e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (IllegalArgumentException e) { | |
| if (e.getMessage().contains("Superclass has no null constructors" )) { | |
| throw new Exception("the class ${adaptee.getClass()} has no non-argument constructor", e) | |
| } | |
| throw e | |
| } | |
| } catch (IllegalArgumentException e) { | |
| if (e.getMessage().contains("Superclass has no null constructors" )) { | |
| throw new Exception("the class ${adaptee.getClass()} has no matching constructor for the given argument types", e) | |
| } | |
| throw e | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@testlib/src/main/java/org/zstack/testlib/util/TProxy.groovy` around lines 90
- 96, The catch block handling IllegalArgumentException in TProxy (where
adaptee.getClass() is referenced) uses a misleading message "has no non-argument
constructor"; update that exception message to reflect the actual situation when
a parameterized constructor cannot be found (e.g., "has no matching
parameterized constructor for the provided arguments") and rethrow the new
Exception with the original cause so the message correctly describes the lookup
failure for a parameterized constructor.
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.
Backport ZSTAC-85588 to 4.8.38 (cross-repo @@2)
Cherry-pick of original fix (含配套测试) to 4.8.38. 本 issue 同时涉及 zstack 与 premium 两仓,分支统一命名 bugfix/ZSTAC-85588@@2 以便 DevOps 关联跑 case。
Sibling repo MR: premium MR (see below)
Commits
Related: ZSTAC-85588
sync from gitlab !10067