<chore>[header]: replace CollectionUtil.find by findOneOrNull#3792
Open
zstack-robot-1 wants to merge 1 commit intozsv_5.0.0from
Open
<chore>[header]: replace CollectionUtil.find by findOneOrNull#3792zstack-robot-1 wants to merge 1 commit intozsv_5.0.0from
zstack-robot-1 wants to merge 1 commit intozsv_5.0.0from
Conversation
Related: ZSV-5936 Change-Id: I67756e797a647a6b6c6c796e766776747473616e
预览本次拉取请求将代码库中的多个文件从使用 变更清单
预估代码审查工作量🎯 3 (中等) | ⏱️ ~25 分钟 兔子的诗
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaComment |
![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
🧹 Nitpick comments (1)
compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java (1)
5739-5747: 可选：避免同一 NIC 的重复查找Line 5739 已拿到
vmNicVO，Line 5745-5747 再查一次同一对象是冗余的。可直接复用，减少一次遍历并提升可读性。♻️ 建议修改
- final VmNicInventory nic = VmNicInventory.valueOf( - CollectionUtils.findOneOrNull( - self.getVmNics(), - arg -> arg.getUuid().equals(nicUuid)) - ); + final VmNicInventory nic = VmNicInventory.valueOf(vmNicVO);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java` around lines 5739 - 5747, You've already found the NIC as vmNicVO via CollectionUtils.findOneOrNull(self.getVmNics(), arg -> arg.getUuid().equals(nicUuid)); reuse that vmNicVO when creating the VmNicInventory instead of performing a second find; replace the second CollectionUtils.findOneOrNull(...) inside VmNicInventory.valueOf(...) with vmNicVO so VmNicInventory.valueOf(vmNicVO) is used, eliminating the redundant traversal and improving readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmDetachNicFlow.java`:
- Around line 44-45: In VmDetachNicFlow, the predicate used to compute
`selected` currently matches the NIC being detached
(CollectionUtils.findOneOrNull(spec.getVmInventory().getVmNics(), arg ->
arg.getUuid().equals(nic.getUuid()))), which can cause `defaultL3NetworkUuid` to
be reset to the L3 of the NIC that is about to be removed; change the predicate
to select a different NIC (i.e., find any `arg` whose UUID is NOT equal to
`nic.getUuid()`) and use that NIC's L3 for `defaultL3NetworkUuid`, and if no
such remaining NIC exists set `defaultL3NetworkUuid` to null; apply the same
inverted-predicate fix for the other occurrence that sets
`defaultL3NetworkUuid`.
In
`@plugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/portforwarding/VirtualRouterPortForwardingBackend.java`:
- Around line 82-85: The code silently sets publicMac to null when no VM NIC
matches vip L3 network (selected from vr.getVmNics() by comparing
arg.getL3NetworkUuid() to struct.getVipL3Network().getUuid()), which defers
failure; change this to fail-fast like privateMac by asserting or throwing an
exception if selected is null (i.e., when computing publicMac) so the method
(VirtualRouterPortForwardingBackend logic around publicMac) aborts immediately
with a clear error rather than proceeding with a null MAC.
---
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java`:
- Around line 5739-5747: You've already found the NIC as vmNicVO via
CollectionUtils.findOneOrNull(self.getVmNics(), arg ->
arg.getUuid().equals(nicUuid)); reuse that vmNicVO when creating the
VmNicInventory instead of performing a second find; replace the second
CollectionUtils.findOneOrNull(...) inside VmNicInventory.valueOf(...) with
vmNicVO so VmNicInventory.valueOf(vmNicVO) is used, eliminating the redundant
traversal and improving readability.
🪄 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: cdb0d97d-22ed-4df9-8274-a9c9190791ae
📒 Files selected for processing (29)
compute/src/main/java/org/zstack/compute/vm/VmDetachNicFlow.javacompute/src/main/java/org/zstack/compute/vm/VmDownloadIsoFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaheader/src/main/java/org/zstack/header/vm/VmInstanceInventory.javanetwork/src/main/java/org/zstack/network/service/DhcpExtension.javaplugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmDeployAgentFlow.javaplugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmFirewallRuleVO.javaplugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmManagementIpChecker.javaplugin/eip/src/main/java/org/zstack/network/service/eip/EipExtension.javaplugin/flatNetworkProvider/src/main/java/org/zstack/network/service/flat/FlatUserdataBackend.javaplugin/flatNetworkProvider/src/main/java/org/zstack/network/service/userdata/UserdataExtension.javaplugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.javaplugin/sharedMountPointPrimaryStorage/src/main/java/org/zstack/storage/primary/smp/KvmBackend.javaplugin/sugonSdnController/src/main/java/org/zstack/sugonSdnController/userdata/TfUserdataBackend.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouterManagerImpl.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/dhcp/VirtualRouterDhcpBackend.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/eip/VirtualRouterEipBackend.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/eip/VirtualRouterSyncEipOnStartFlow.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/lifecycle/VirtualRouterAssembleDecoratorFlow.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/lifecycle/VirtualRouterDeployAgentFlow.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/portforwarding/VirtualRouterPortForwardingBackend.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/vyos/VyosConfigSshFlow.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/vyos/VyosDeployAgentFlow.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/vyos/VyosGetVersionFlow.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageTagAllocatorExtension.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javastorage/src/main/java/org/zstack/storage/volume/VolumeUpgradeExtension.java
Comment on lines
+44
to
+45
| VmNicInventory selected = CollectionUtils.findOneOrNull( | ||
| spec.getVmInventory().getVmNics(), arg -> arg.getUuid().equals(nic.getUuid())); |
There was a problem hiding this comment.
默认 L3 重置条件写反，导致可能引用已解绑 NIC 的 L3。
Line 44-45 当前匹配的是被解绑 NIC 本身，Line 48 因而可能把 defaultL3NetworkUuid 设回即将移除的 L3。这里应选择“非当前解绑 NIC”的剩余网卡；不存在时再置 null。
🔧 建议修复
- VmNicInventory selected = CollectionUtils.findOneOrNull(
- spec.getVmInventory().getVmNics(), arg -> arg.getUuid().equals(nic.getUuid()));
+ VmNicInventory selected = CollectionUtils.findOneOrNull(
+ spec.getVmInventory().getVmNics(), arg -> !arg.getUuid().equals(nic.getUuid()));
VmInstanceVO vm = dbf.findByUuid(spec.getVmInventory().getUuid(), VmInstanceVO.class);
vm.setDefaultL3NetworkUuid(selected == null ? null : selected.getL3NetworkUuid());Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/vm/VmDetachNicFlow.java` around
lines 44 - 45, In VmDetachNicFlow, the predicate used to compute `selected`
currently matches the NIC being detached
(CollectionUtils.findOneOrNull(spec.getVmInventory().getVmNics(), arg ->
arg.getUuid().equals(nic.getUuid()))), which can cause `defaultL3NetworkUuid` to
be reset to the L3 of the NIC that is about to be removed; change the predicate
to select a different NIC (i.e., find any `arg` whose UUID is NOT equal to
`nic.getUuid()`) and use that NIC's L3 for `defaultL3NetworkUuid`, and if no
such remaining NIC exists set `defaultL3NetworkUuid` to null; apply the same
inverted-predicate fix for the other occurrence that sets
`defaultL3NetworkUuid`.
Comment on lines
+82
to
85
| selected = CollectionUtils.findOneOrNull(vr.getVmNics(), | ||
| arg -> arg.getL3NetworkUuid().equals(struct.getVipL3Network().getUuid())); | ||
| String publicMac = selected == null ? null : selected.getMac(); | ||
|
|
There was a problem hiding this comment.
publicMac 变为静默 null 会弱化失败语义。
当前 privateMac 会显式断言，但 publicMac 缺失时会继续执行并写入 null，这会把错误延后到下游调用，增加排障成本。建议与 privateMac 保持一致，缺失即快速失败。
建议修复
selected = CollectionUtils.findOneOrNull(vr.getVmNics(),
arg -> arg.getL3NetworkUuid().equals(struct.getVipL3Network().getUuid()));
String publicMac = selected == null ? null : selected.getMac();
+ DebugUtils.Assert(publicMac != null, String.format("cannot find vip nic[l3NetworkUuid:%s] on virtual router[uuid:%s, name:%s]",
+ struct.getVipL3Network().getUuid(), vr.getUuid(), vr.getName()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/portforwarding/VirtualRouterPortForwardingBackend.java`
around lines 82 - 85, The code silently sets publicMac to null when no VM NIC
matches vip L3 network (selected from vr.getVmNics() by comparing
arg.getL3NetworkUuid() to struct.getVipL3Network().getUuid()), which defers
failure; change this to fail-fast like privateMac by asserting or throwing an
exception if selected is null (i.e., when computing publicMac) so the method
(VirtualRouterPortForwardingBackend logic around publicMac) aborts immediately
with a clear error rather than proceeding with a null MAC.
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.
Related: ZSV-5936
Change-Id: I67756e797a647a6b6c6c796e766776747473616e
sync from gitlab !9661