This is an automated email from the ASF dual-hosted git repository.
lizhimins pushed a commit to branch rocketmq-studio
in repository https://gitbox.apache.org/repos/asf/rocketmq-dashboard.git
The following commit(s) were added to refs/heads/rocketmq-studio by this push:
new 8e7887921 fix(instance): resolve resource counts by the canonical
instance name (#4790)
8e7887921 is described below
commit 8e788792113c795b1b27da8d68b57d606b4c0401
Author: 烤化の初雪 <[email protected]>
AuthorDate: Thu Sep 24 18:14:00 2026 +0800
fix(instance): resolve resource counts by the canonical instance name
(#4790)
fix(instance): keep the canonical-name resolution the batch-delete test pins
The previous commit's teeth check restored the trunk version of
InstanceService.java and the restore was lost before commit, so the
branch head silently carried the unguarded call sites while the tests
passed the run before last's restore. Re-apply the canonical-name fix
that this branch exists for; no behavioural change beyond the original
commit.
test(instance): pin that batch delete resolves counts by canonical name
deleteInstances funnels into the same deleteInstance guard as the
single-delete path; pin the canonical-name contract there too with an
instance named "42" (id 3) so the numeric-id shadowing stays closed
on the batch route as well.
fix(instance): resolve resource counts by the canonical instance name
loadCounts and the delete guard held an already-resolved instance but
passed String.valueOf(instance.getId()) to the provider, whose
identifier resolution matches the unique name before the numeric-id
fallback. An instance named like another instance's numeric id
(instance names are free-form up to 64 chars) therefore shadowed the
id: the instance list showed another instance's topic/group counts and
the delete guard read them from the wrong instance, letting an
operator delete an instance whose managed resources still exist. Pass
the canonical instance name instead, which findByIdentifier always
resolves to the instance itself.
---
.../rocketmq/studio/instance/InstanceService.java | 16 ++-
.../studio/instance/InstanceServiceTest.java | 129 ++++++++++++++++-----
2 files changed, 108 insertions(+), 37 deletions(-)
diff --git
a/server/src/main/java/org/apache/rocketmq/studio/instance/InstanceService.java
b/server/src/main/java/org/apache/rocketmq/studio/instance/InstanceService.java
index c0c6a3bd7..04e9f8377 100644
---
a/server/src/main/java/org/apache/rocketmq/studio/instance/InstanceService.java
+++
b/server/src/main/java/org/apache/rocketmq/studio/instance/InstanceService.java
@@ -176,13 +176,16 @@ public class InstanceService {
/**
* Resource counts live on the vendor side (cloud APIs) or in the local
tables (Apache),
- * so resolve them uniformly through the vendor provider.
+ * so resolve them uniformly through the vendor provider. The canonical
instance name is
+ * passed, not the numeric id as a string: provider-side identifier
resolution matches the
+ * unique name first, so a name that happens to equal another instance's
numeric id would
+ * otherwise shadow it and attribute the counts to the wrong instance.
*/
private InstanceResourceCountRunner.ResourceCounts loadCounts(InstanceVO
instance) {
InstanceVendor vendor = instance.getVendor() == null ?
InstanceVendor.APACHE : instance.getVendor();
InstanceProvider provider = providerRegistry.forVendor(vendor);
- int topicCount =
provider.countTopics(String.valueOf(instance.getId()));
- int consumerGroupCount =
provider.countGroups(String.valueOf(instance.getId()));
+ int topicCount = provider.countTopics(instance.getName());
+ int consumerGroupCount = provider.countGroups(instance.getName());
return new InstanceResourceCountRunner.ResourceCounts(topicCount,
consumerGroupCount);
}
@@ -664,8 +667,11 @@ public class InstanceService {
InstanceVendor vendor = existing.getVendor() == null ?
InstanceVendor.APACHE : existing.getVendor();
if (vendor == InstanceVendor.APACHE) {
InstanceProvider provider =
providerRegistry.forVendor(InstanceVendor.APACHE);
- int topicCount = provider.countTopics(String.valueOf(id));
- int consumerGroupCount = provider.countGroups(String.valueOf(id));
+ // Pass the canonical name: identifier resolution is name-first,
so the numeric id
+ // string could resolve to a different instance whose name happens
to equal this id,
+ // reading the wrong instance's counts in the delete guard.
+ int topicCount = provider.countTopics(existing.getName());
+ int consumerGroupCount = provider.countGroups(existing.getName());
if (topicCount > 0 || consumerGroupCount > 0) {
throw new BusinessException(409, String.format(
"Cannot delete instance with managed resources:
topics=%d, consumerGroups=%d",
diff --git
a/server/src/test/java/org/apache/rocketmq/studio/instance/InstanceServiceTest.java
b/server/src/test/java/org/apache/rocketmq/studio/instance/InstanceServiceTest.java
index 91d6cd3ed..bfb8719dd 100644
---
a/server/src/test/java/org/apache/rocketmq/studio/instance/InstanceServiceTest.java
+++
b/server/src/test/java/org/apache/rocketmq/studio/instance/InstanceServiceTest.java
@@ -197,12 +197,12 @@ class InstanceServiceTest {
@Test
void listInstancesMarksCloudCountsUnavailableWhenProviderFails() {
- InstanceVO instance =
InstanceVO.builder().vendor(InstanceVendor.ALIYUN).build();
+ InstanceVO instance =
InstanceVO.builder().name("aliyun-fail").vendor(InstanceVendor.ALIYUN).build();
instance.setId(1L);
InstanceProvider provider =
org.mockito.Mockito.mock(InstanceProvider.class);
when(instanceRepository.findAll()).thenReturn(List.of(instance));
when(providerRegistry.forVendor(InstanceVendor.ALIYUN)).thenReturn(provider);
- when(provider.countTopics("1")).thenThrow(new
IllegalStateException("access denied"));
+ when(provider.countTopics("aliyun-fail")).thenThrow(new
IllegalStateException("access denied"));
InstanceVO result = instanceService.listInstances(null, null).get(0);
@@ -211,13 +211,13 @@ class InstanceServiceTest {
@Test
void listInstancesKeepsCloudCountsAvailableWhenProviderReturnsEmptyLists()
{
- InstanceVO instance =
InstanceVO.builder().vendor(InstanceVendor.ALIYUN).build();
+ InstanceVO instance =
InstanceVO.builder().name("aliyun-empty").vendor(InstanceVendor.ALIYUN).build();
instance.setId(2L);
InstanceProvider provider =
org.mockito.Mockito.mock(InstanceProvider.class);
when(instanceRepository.findAll()).thenReturn(List.of(instance));
when(providerRegistry.forVendor(InstanceVendor.ALIYUN)).thenReturn(provider);
- when(provider.countTopics("2")).thenReturn(0);
- when(provider.countGroups("2")).thenReturn(0);
+ when(provider.countTopics("aliyun-empty")).thenReturn(0);
+ when(provider.countGroups("aliyun-empty")).thenReturn(0);
InstanceVO result = instanceService.listInstances(null, null).get(0);
@@ -318,10 +318,10 @@ class InstanceServiceTest {
when(instanceRepository.findAll()).thenReturn(List.of(apache, aliyun));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
when(providerRegistry.forVendor(InstanceVendor.ALIYUN)).thenReturn(aliyunProvider);
- when(instanceProvider.countTopics("3")).thenReturn(3);
- when(instanceProvider.countGroups("3")).thenReturn(2);
- when(aliyunProvider.countTopics("4")).thenReturn(5);
- when(aliyunProvider.countGroups("4")).thenReturn(4);
+ when(instanceProvider.countTopics("apache")).thenReturn(3);
+ when(instanceProvider.countGroups("apache")).thenReturn(2);
+ when(aliyunProvider.countTopics("aliyun")).thenReturn(5);
+ when(aliyunProvider.countGroups("aliyun")).thenReturn(4);
List<InstanceVO> result = instanceService.listInstances(null, null);
@@ -337,7 +337,7 @@ class InstanceServiceTest {
instance.setId(5L);
when(instanceRepository.findAll()).thenReturn(List.of(instance));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("5"))
+ when(instanceProvider.countTopics("broken"))
.thenThrow(new IllegalStateException("admin unavailable"));
List<InstanceVO> result = instanceService.listInstances(null, null);
@@ -355,7 +355,7 @@ class InstanceServiceTest {
instance.setResourceCountsAvailable(true);
when(instanceRepository.findAll()).thenReturn(List.of(instance));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("6"))
+ when(instanceProvider.countTopics("stale-counts"))
.thenThrow(new IllegalStateException("provider unavailable"));
InstanceVO result = instanceService.listInstances(null, null).get(0);
@@ -402,7 +402,7 @@ class InstanceServiceTest {
CountDownLatch topicCountStarted = new CountDownLatch(1);
CountDownLatch releaseProvider = new CountDownLatch(1);
CountDownLatch providerFinished = new CountDownLatch(1);
- when(instanceProvider.countTopics("24")).thenAnswer(invocation -> {
+ when(instanceProvider.countTopics("slow")).thenAnswer(invocation -> {
topicCountStarted.countDown();
boolean interrupted = false;
while (true) {
@@ -420,7 +420,7 @@ class InstanceServiceTest {
}
return 7;
});
- when(instanceProvider.countGroups("24")).thenAnswer(invocation -> {
+ when(instanceProvider.countGroups("slow")).thenAnswer(invocation -> {
providerFinished.countDown();
return 5;
});
@@ -971,8 +971,8 @@ class InstanceServiceTest {
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenReturn(0);
- when(instanceProvider.countGroups("1")).thenReturn(0);
+ when(instanceProvider.countTopics("to-delete")).thenReturn(0);
+ when(instanceProvider.countGroups("to-delete")).thenReturn(0);
when(instanceRepository.deleteById(1L)).thenReturn(true);
instanceService.deleteInstance(1L);
@@ -1004,8 +1004,8 @@ class InstanceServiceTest {
.thenReturn(Optional.empty());
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenReturn(0);
- when(instanceProvider.countGroups("1")).thenReturn(0);
+ when(instanceProvider.countTopics("inst-a")).thenReturn(0);
+ when(instanceProvider.countGroups("inst-a")).thenReturn(0);
when(instanceRepository.deleteById(1L)).thenReturn(true);
ReflectionTestUtils.setField(instanceService, "self", instanceService);
@@ -1026,9 +1026,9 @@ class InstanceServiceTest {
when(instanceRepository.findById(1L)).thenReturn(Optional.of(failedInstance));
when(instanceRepository.findById(2L)).thenReturn(Optional.of(deletedInstance));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenThrow(new
IllegalStateException("broker unavailable"));
- when(instanceProvider.countTopics("2")).thenReturn(0);
- when(instanceProvider.countGroups("2")).thenReturn(0);
+ when(instanceProvider.countTopics("inst-a")).thenThrow(new
IllegalStateException("broker unavailable"));
+ when(instanceProvider.countTopics("inst-b")).thenReturn(0);
+ when(instanceProvider.countGroups("inst-b")).thenReturn(0);
when(instanceRepository.deleteById(2L)).thenReturn(true);
ReflectionTestUtils.setField(instanceService, "self", instanceService);
@@ -1036,10 +1036,10 @@ class InstanceServiceTest {
assertThat(result.getDeleted()).isEqualTo(1);
assertThat(result.getFailed()).containsExactly("inst-a: broker
unavailable");
- verify(instanceProvider).countTopics("1");
+ verify(instanceProvider).countTopics("inst-a");
verify(instanceRepository).findByIdentifier("inst-b");
- verify(instanceProvider).countTopics("2");
- verify(instanceProvider).countGroups("2");
+ verify(instanceProvider).countTopics("inst-b");
+ verify(instanceProvider).countGroups("inst-b");
verify(instanceRepository).deleteById(2L);
}
@@ -1051,7 +1051,7 @@ class InstanceServiceTest {
when(instanceRepository.findByIdentifier("inst-a")).thenReturn(Optional.of(existing));
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenThrow(new
IllegalStateException(oversizedMessage));
+ when(instanceProvider.countTopics("inst-a")).thenThrow(new
IllegalStateException(oversizedMessage));
ReflectionTestUtils.setField(instanceService, "self", instanceService);
BatchDeleteResultVO result =
instanceService.deleteInstances(List.of("inst-a"));
@@ -1076,8 +1076,8 @@ class InstanceServiceTest {
when(instanceRepository.findByIdentifier("inst-a")).thenReturn(Optional.of(existing));
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenReturn(0);
- when(instanceProvider.countGroups("1")).thenReturn(0);
+ when(instanceProvider.countTopics("inst-a")).thenReturn(0);
+ when(instanceProvider.countGroups("inst-a")).thenReturn(0);
when(instanceRepository.deleteById(1L)).thenReturn(true);
ReflectionTestUtils.setField(instanceService, "self", instanceService);
@@ -1105,8 +1105,8 @@ class InstanceServiceTest {
existing.setId(1L);
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenReturn(2);
- when(instanceProvider.countGroups("1")).thenReturn(0);
+ when(instanceProvider.countTopics("with-topics")).thenReturn(2);
+ when(instanceProvider.countGroups("with-topics")).thenReturn(0);
assertThatThrownBy(() -> instanceService.deleteInstance(1L))
.isInstanceOf(BusinessException.class)
@@ -1116,6 +1116,71 @@ class InstanceServiceTest {
verify(instanceRepository, never()).deleteById(1L);
}
+ @Test
+ void deleteInstanceShouldResolveResourceCountsByInstanceNameTest() {
+ // findByIdentifier resolves a unique name before the numeric-id
fallback, so a name that
+ // looks like another instance's id would shadow it. The guard holds
the already-resolved
+ // instance and must pass its canonical name, not the numeric id as a
string.
+ InstanceVO existing = InstanceVO.builder().name("with-topics").build();
+ existing.setId(1L);
+
+
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
+
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
+ when(instanceProvider.countTopics("with-topics")).thenReturn(2);
+ when(instanceProvider.countGroups("with-topics")).thenReturn(0);
+
+ assertThatThrownBy(() -> instanceService.deleteInstance(1L))
+ .isInstanceOf(BusinessException.class)
+ .hasMessage("Cannot delete instance with managed resources:
topics=2, consumerGroups=0");
+
+ verify(instanceProvider).countTopics("with-topics");
+ verify(instanceProvider).countGroups("with-topics");
+ verify(instanceProvider, never()).countTopics("1");
+ verify(instanceRepository, never()).deleteById(1L);
+ }
+
+ @Test
+ void batchDeleteShouldRouteItsResourceGuardThroughTheCanonicalNameTest() {
+ // The PR contract: batch delete funnels into the same deleteInstance
guard, so the
+ // canonical name (not the numeric id string) is what reaches the
provider there too.
+ InstanceVO existing = InstanceVO.builder().name("42").build();
+ existing.setId(3L);
+
when(instanceRepository.findByIdentifier("42")).thenReturn(Optional.of(existing));
+
when(instanceRepository.findById(3L)).thenReturn(Optional.of(existing));
+
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
+ when(instanceProvider.countTopics("42")).thenReturn(0);
+ when(instanceProvider.countGroups("42")).thenReturn(0);
+ when(instanceRepository.deleteById(3L)).thenReturn(true);
+ ReflectionTestUtils.setField(instanceService, "self", instanceService);
+
+ BatchDeleteResultVO result =
instanceService.deleteInstances(List.of("42"));
+
+ assertThat(result.getDeleted()).isEqualTo(1);
+ verify(instanceProvider).countTopics("42");
+ verify(instanceProvider).countGroups("42");
+ verify(instanceProvider, never()).countTopics("3");
+ verify(instanceRepository).deleteById(3L);
+ }
+
+ @Test
+ void listInstancesShouldResolveResourceCountsByInstanceNameTest() {
+ // Same shadowing contract for the list counts: the numeric id string
must not be sent
+ // through the name-first identifier resolution.
+ InstanceVO apache = InstanceVO.builder().name("42").build();
+ apache.setId(3L);
+ when(instanceRepository.findAll()).thenReturn(List.of(apache));
+
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
+ when(instanceProvider.countTopics("42")).thenReturn(3);
+ when(instanceProvider.countGroups("42")).thenReturn(2);
+
+ List<InstanceVO> result = instanceService.listInstances(null, null);
+
+ assertThat(result.get(0).getTopicCount()).isEqualTo(3);
+ assertThat(result.get(0).getConsumerGroupCount()).isEqualTo(2);
+ verify(instanceProvider, never()).countTopics("3");
+ verify(instanceProvider, never()).countGroups("3");
+ }
+
@Test
void deleteInstanceShouldRejectInstanceWithConsumerGroups() {
InstanceVO existing = InstanceVO.builder()
@@ -1125,8 +1190,8 @@ class InstanceServiceTest {
existing.setId(1L);
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenReturn(0);
- when(instanceProvider.countGroups("1")).thenReturn(3);
+
when(instanceProvider.countTopics("with-consumer-groups")).thenReturn(0);
+
when(instanceProvider.countGroups("with-consumer-groups")).thenReturn(3);
assertThatThrownBy(() -> instanceService.deleteInstance(1L))
.isInstanceOf(BusinessException.class)
@@ -1195,8 +1260,8 @@ class InstanceServiceTest {
.instanceIds(List.of("to-delete", "inst-2")).build();
when(instanceRepository.findById(1L)).thenReturn(Optional.of(existing));
when(providerRegistry.forVendor(InstanceVendor.APACHE)).thenReturn(instanceProvider);
- when(instanceProvider.countTopics("1")).thenReturn(0);
- when(instanceProvider.countGroups("1")).thenReturn(0);
+ when(instanceProvider.countTopics("to-delete")).thenReturn(0);
+ when(instanceProvider.countGroups("to-delete")).thenReturn(0);
when(instanceRepository.deleteById(1L)).thenReturn(true);
when(settingsRepository.findAllDataSources()).thenReturn(List.of(dataSource));
when(settingsRepository.replaceDataSource(dataSource)).thenReturn(true);
