utafrali commented on code in PR #3769:
URL:
https://github.com/apache/rocketmq-dashboard/pull/3769#discussion_r3941999426
##########
server/src/test/java/org/apache/rocketmq/studio/instance/acl/ApacheAclReadServiceTest.java:
##########
@@ -123,4 +125,74 @@ private void executeWith(RuntimeAdminClientResolver
resolver, MQAdminExt admin)
return action.apply(admin);
});
}
+
+ @Test
+ void skipsDuplicateMasterAddressAcrossBrokerEntries() throws Exception {
+ RuntimeAdminClientResolver resolver =
mock(RuntimeAdminClientResolver.class);
+ MQAdminExt admin = mock(MQAdminExt.class);
+ BrokerData first = new BrokerData();
+ first.setBrokerAddrs(new HashMap<>(Map.of(0L, "broker-a:10911")));
+ BrokerData second = new BrokerData();
+ second.setBrokerAddrs(new HashMap<>(Map.of(0L, "broker-a:10911")));
+ ClusterInfo clusterInfo = new ClusterInfo();
+ clusterInfo.setBrokerAddrTable(Map.of("first", first, "second",
second));
+ when(admin.examineBrokerClusterInfo()).thenReturn(clusterInfo);
+ when(admin.listAcl("broker-a:10911", null,
null)).thenReturn(List.of());
+ executeWith(resolver, admin);
+
+ RemoteAclReadResult result = new
ApacheAclReadService(resolver).listRules("instance-1", null, null);
+
+ verify(admin).listAcl("broker-a:10911", null, null);
+
assertThat(result.getPoliciesByBroker()).containsOnlyKeys("broker-a:10911");
+ assertThat(result.getFailuresByBroker()).isEmpty();
+ }
+
+ @Test
+ void skipsBrokerEntriesWithoutMasterAddress() throws Exception {
+ RuntimeAdminClientResolver resolver =
mock(RuntimeAdminClientResolver.class);
+ MQAdminExt admin = mock(MQAdminExt.class);
+ BrokerData noAddrs = new BrokerData();
+ BrokerData slaveOnly = new BrokerData();
+ slaveOnly.setBrokerAddrs(new HashMap<>(Map.of(2L, "broker-a:10911")));
+ ClusterInfo clusterInfo = new ClusterInfo();
+ clusterInfo.setBrokerAddrTable(Map.of("no-addrs", noAddrs, "slave",
slaveOnly));
+ when(admin.examineBrokerClusterInfo()).thenReturn(clusterInfo);
+ executeWith(resolver, admin);
+
+ RemoteAclReadResult result = new
ApacheAclReadService(resolver).listRules("instance-1", null, null);
+
+ verify(admin, never()).listAcl(any(), any(), any());
+ assertThat(result.getPoliciesByBroker()).isEmpty();
+ assertThat(result.getFailuresByBroker()).isEmpty();
+ assertThat(result.isPartial()).isFalse();
+ }
+
+ @Test
+ void usesDeepestRootCauseAsFailureMessage() throws Exception {
+ RuntimeAdminClientResolver resolver =
mock(RuntimeAdminClientResolver.class);
+ MQAdminExt admin = mock(MQAdminExt.class);
+
when(admin.examineBrokerClusterInfo()).thenReturn(clusterInfo("broker-a:10911"));
+ when(admin.listAcl("broker-a:10911", null, null))
Review Comment:
The exception chain here is only two levels deep (`IllegalStateException ->
RuntimeException`), so a simple `getCause()` would pass this test just as
easily as a true root-cause traversal. To actually verify the algorithm walks
to the bottom, use a three-level chain:
```java
.thenThrow(new IllegalStateException("outer",
new RuntimeException("middle",
new RuntimeException("root cause"))));
```
Then the assertion `containsEntry("broker-a:10911", "root cause")` would
fail if the implementation stops at `getCause()` instead of traversing to the
deepest cause.
##########
server/src/test/java/org/apache/rocketmq/studio/instance/acl/ApacheAclReadServiceTest.java:
##########
@@ -123,4 +125,74 @@ private void executeWith(RuntimeAdminClientResolver
resolver, MQAdminExt admin)
return action.apply(admin);
});
}
+
+ @Test
+ void skipsDuplicateMasterAddressAcrossBrokerEntries() throws Exception {
+ RuntimeAdminClientResolver resolver =
mock(RuntimeAdminClientResolver.class);
+ MQAdminExt admin = mock(MQAdminExt.class);
+ BrokerData first = new BrokerData();
+ first.setBrokerAddrs(new HashMap<>(Map.of(0L, "broker-a:10911")));
+ BrokerData second = new BrokerData();
+ second.setBrokerAddrs(new HashMap<>(Map.of(0L, "broker-a:10911")));
+ ClusterInfo clusterInfo = new ClusterInfo();
+ clusterInfo.setBrokerAddrTable(Map.of("first", first, "second",
second));
+ when(admin.examineBrokerClusterInfo()).thenReturn(clusterInfo);
+ when(admin.listAcl("broker-a:10911", null,
null)).thenReturn(List.of());
+ executeWith(resolver, admin);
+
+ RemoteAclReadResult result = new
ApacheAclReadService(resolver).listRules("instance-1", null, null);
+
+ verify(admin).listAcl("broker-a:10911", null, null);
+
assertThat(result.getPoliciesByBroker()).containsOnlyKeys("broker-a:10911");
+ assertThat(result.getFailuresByBroker()).isEmpty();
+ }
+
+ @Test
+ void skipsBrokerEntriesWithoutMasterAddress() throws Exception {
+ RuntimeAdminClientResolver resolver =
mock(RuntimeAdminClientResolver.class);
+ MQAdminExt admin = mock(MQAdminExt.class);
+ BrokerData noAddrs = new BrokerData();
+ BrokerData slaveOnly = new BrokerData();
+ slaveOnly.setBrokerAddrs(new HashMap<>(Map.of(2L, "broker-a:10911")));
+ ClusterInfo clusterInfo = new ClusterInfo();
+ clusterInfo.setBrokerAddrTable(Map.of("no-addrs", noAddrs, "slave",
slaveOnly));
+ when(admin.examineBrokerClusterInfo()).thenReturn(clusterInfo);
+ executeWith(resolver, admin);
+
+ RemoteAclReadResult result = new
ApacheAclReadService(resolver).listRules("instance-1", null, null);
+
+ verify(admin, never()).listAcl(any(), any(), any());
+ assertThat(result.getPoliciesByBroker()).isEmpty();
+ assertThat(result.getFailuresByBroker()).isEmpty();
+ assertThat(result.isPartial()).isFalse();
+ }
+
+ @Test
+ void usesDeepestRootCauseAsFailureMessage() throws Exception {
+ RuntimeAdminClientResolver resolver =
mock(RuntimeAdminClientResolver.class);
+ MQAdminExt admin = mock(MQAdminExt.class);
+
when(admin.examineBrokerClusterInfo()).thenReturn(clusterInfo("broker-a:10911"));
+ when(admin.listAcl("broker-a:10911", null, null))
+ .thenThrow(new IllegalStateException("outer", new
RuntimeException("root cause")));
+ executeWith(resolver, admin);
+
+ RemoteAclReadResult result = new
ApacheAclReadService(resolver).listRules("instance-1", null, null);
+
Review Comment:
This test records a broker failure but never checks `result.isPartial()`.
Every other test that has a meaningful result state (tests 2 and 4) asserts on
`isPartial()`. Since a failure should mark the result partial, add:
```java
assertThat(result.isPartial()).isTrue();
```
##########
server/src/test/java/org/apache/rocketmq/studio/instance/acl/ApacheAclReadServiceTest.java:
##########
@@ -123,4 +125,74 @@ private void executeWith(RuntimeAdminClientResolver
resolver, MQAdminExt admin)
return action.apply(admin);
});
}
+
+ @Test
+ void skipsDuplicateMasterAddressAcrossBrokerEntries() throws Exception {
+ RuntimeAdminClientResolver resolver =
mock(RuntimeAdminClientResolver.class);
+ MQAdminExt admin = mock(MQAdminExt.class);
+ BrokerData first = new BrokerData();
+ first.setBrokerAddrs(new HashMap<>(Map.of(0L, "broker-a:10911")));
+ BrokerData second = new BrokerData();
+ second.setBrokerAddrs(new HashMap<>(Map.of(0L, "broker-a:10911")));
+ ClusterInfo clusterInfo = new ClusterInfo();
+ clusterInfo.setBrokerAddrTable(Map.of("first", first, "second",
second));
+ when(admin.examineBrokerClusterInfo()).thenReturn(clusterInfo);
+ when(admin.listAcl("broker-a:10911", null,
null)).thenReturn(List.of());
+ executeWith(resolver, admin);
+
+ RemoteAclReadResult result = new
ApacheAclReadService(resolver).listRules("instance-1", null, null);
+
+ verify(admin).listAcl("broker-a:10911", null, null);
+
assertThat(result.getPoliciesByBroker()).containsOnlyKeys("broker-a:10911");
Review Comment:
`skipsDuplicateMasterAddressAcrossBrokerEntries` is missing an `isPartial()`
assertion. Tests 2 and 4 both check `assertThat(result.isPartial()).isFalse()`
after successful/empty outcomes. Add the same here for consistency and to
document the expected state:
```java
assertThat(result.isPartial()).isFalse();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
