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 ecd7d62ad fix(alert): reject native rules without a supported operator
(#6642)
ecd7d62ad is described below
commit ecd7d62adee6b7e3722b718b7f5370ed347cd8e4
Author: Xiao Yang <[email protected]>
AuthorDate: Mon Oct 19 20:08:20 2026 +0800
fix(alert): reject native rules without a supported operator (#6642)
Verified against `rocketmq-studio` at 056af345c before merging: the defect
is still present on the
current trunk and the change merges cleanly.
Maintainer follow-ups on top of the contributor commit:
- fix(alert): judge the trimmed native operator and pin the whitelist to
the evaluator
Each follow-up commit message records the review finding it addresses.
Locally verified: targeted suites green on the integration branch; a full
backend and frontend
build plus test pass runs on the trunk once this batch lands.
---
.../studio/ops/alert/NativeAlertRulePolicy.java | 11 +++-
.../ops/alert/AlertRuleTransferServiceTest.java | 28 ++++++++
.../studio/ops/alert/AlertServiceTest.java | 23 +++++++
.../ops/alert/NativeAlertRulePolicyTest.java | 74 +++++++++++++++++++++-
4 files changed, 133 insertions(+), 3 deletions(-)
diff --git
a/server/src/main/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicy.java
b/server/src/main/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicy.java
index 36654aff2..b8a1085be 100644
---
a/server/src/main/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicy.java
+++
b/server/src/main/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicy.java
@@ -45,6 +45,7 @@ final class NativeAlertRulePolicy {
private static final Set<String> AVAILABILITY_METRICS = Set.of(
"nameserver.availability", "broker.availability",
"proxy.availability", "cloud.instance.availability");
private static final Set<String> NOTIFICATION_CHANNELS =
Set.of("dingtalk", "sms", "email");
+ static final Set<String> NATIVE_OPERATORS = Set.of(">", ">=", "<", "<=",
"==", "!=", "UNAVAILABLE");
private NativeAlertRulePolicy() {
}
@@ -69,7 +70,15 @@ final class NativeAlertRulePolicy {
if (!StringUtils.hasText(rule.getInstanceId())) {
throw new BusinessException(400, "instanceId is required for
native alert rules");
}
- if ("UNAVAILABLE".equals(rule.getOperator()) &&
!AVAILABILITY_METRICS.contains(rule.getMetric())) {
+ // AlertRuleEvaluator trims the operator before comparing and
NativeAlertRuleTestService stores the
+ // trimmed value, so the whitelist has to judge the trimmed operator
as well. Otherwise " > " is
+ // rejected here while every consumer downstream would have accepted
it.
+ String operator = StringUtils.hasText(rule.getOperator()) ?
rule.getOperator().trim() : null;
+ if (operator == null || !NATIVE_OPERATORS.contains(operator)) {
+ throw new BusinessException(400,
+ "operator must be one of >, >=, <, <=, ==, != or
UNAVAILABLE for native alert rules");
+ }
+ if ("UNAVAILABLE".equals(operator) &&
!AVAILABILITY_METRICS.contains(rule.getMetric())) {
throw new BusinessException(400, "UNAVAILABLE is only supported
for native availability metrics");
}
if (StringUtils.hasText(rule.getConsumerGroup()) &&
!GROUP_SCOPED_METRICS.contains(rule.getMetric())) {
diff --git
a/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertRuleTransferServiceTest.java
b/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertRuleTransferServiceTest.java
index a3aaaf982..b9849bb84 100644
---
a/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertRuleTransferServiceTest.java
+++
b/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertRuleTransferServiceTest.java
@@ -7,6 +7,9 @@
package org.apache.rocketmq.studio.ops.alert;
import org.apache.rocketmq.studio.common.exception.BusinessException;
+import org.apache.rocketmq.studio.audit.OperationAuditService;
+import org.apache.rocketmq.studio.cluster.metrics.MetricProfileService;
+import org.apache.rocketmq.studio.cluster.metrics.PrometheusProperties;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.ArgumentCaptor;
@@ -23,6 +26,8 @@ import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
@@ -97,6 +102,29 @@ class AlertRuleTransferServiceTest {
verify(alertService, never()).createRule(eq(AlertDomain.CLUSTER),
any(AlertRuleVO.class));
}
+ @Test
+ void rejectsImportedNativeRuleWithoutOperatorBeforePersistenceTest() {
+ AlertRepository repository = mock(AlertRepository.class);
+ OperationAuditService auditService = mock(OperationAuditService.class);
+ AlertService realService = new AlertService(repository,
mock(AlertStateRepository.class),
+ new AlertRuleAssetService(), auditService, new
MetricProfileService(new PrometheusProperties()));
+ AlertRuleTransferService realTransfer = new
AlertRuleTransferService(realService, metricCatalogService);
+ lenient().when(repository.insertRule(any(AlertRuleVO.class)))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ AlertRuleRequestDTO invalid = request("Missing operator");
+ invalid.setMetric("broker.availability");
+ invalid.setInstanceId("local");
+ invalid.setOperator(null);
+
+ BusinessException error = assertThrows(BusinessException.class,
+ () -> realTransfer.importRules(AlertDomain.CLUSTER,
+ transfer(AlertDomain.CLUSTER, invalid)));
+
+ assertEquals(400, error.getCode());
+ verify(repository, never()).insertRule(any(AlertRuleVO.class));
+ verifyNoInteractions(auditService);
+ }
+
@Test
void importsRulesAsNewRulesInTheRequestedDomainTest() {
AlertRuleRequestDTO request = request("Broker unavailable");
diff --git
a/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertServiceTest.java
b/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertServiceTest.java
index 82e4d1a89..cd726daa2 100644
---
a/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertServiceTest.java
+++
b/server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertServiceTest.java
@@ -49,6 +49,8 @@ import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@@ -71,6 +73,27 @@ class AlertServiceTest {
operationAuditService, new MetricProfileService(new
PrometheusProperties()));
}
+ @Test
+ void createNativeRuleShouldRejectMissingOperatorBeforePersistenceTest() {
+ AlertRuleRequestDTO request = new AlertRuleRequestDTO();
+ request.setName("Orders lag");
+ request.setMetric("consumer.lag.total");
+ request.setInstanceId("local");
+ request.setConsumerGroup("orders");
+ request.setThreshold(100);
+ request.setEnabled(true);
+ lenient().when(alertRepository.insertRule(any(AlertRuleVO.class)))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+
+ assertThatThrownBy(() -> alertService.createRule(AlertDomain.BUSINESS,
request.toAlertRuleVO()))
+ .isInstanceOf(BusinessException.class)
+ .hasMessageContaining("operator")
+ .satisfies(error -> assertThat(((BusinessException)
error).getCode()).isEqualTo(400));
+
+ verify(alertRepository, never()).insertRule(any(AlertRuleVO.class));
+ verifyNoInteractions(operationAuditService);
+ }
+
@Test
void listRulesShouldReturnAllRulesTest() {
AlertRuleVO rule1 = AlertRuleVO.builder().id(1L).name("High
CPU").metric("cpu_usage")
diff --git
a/server/src/test/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicyTest.java
b/server/src/test/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicyTest.java
index a2de0f1f2..5b304c9da 100644
---
a/server/src/test/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicyTest.java
+++
b/server/src/test/java/org/apache/rocketmq/studio/ops/alert/NativeAlertRulePolicyTest.java
@@ -16,12 +16,20 @@
*/
package org.apache.rocketmq.studio.ops.alert;
+import org.apache.rocketmq.studio.cluster.metrics.MetricAvailability;
+import org.apache.rocketmq.studio.cluster.metrics.MetricSample;
import org.apache.rocketmq.studio.common.exception.BusinessException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullAndEmptySource;
+import org.junit.jupiter.params.provider.ValueSource;
+import java.time.Instant;
import java.util.List;
import java.util.Locale;
+import java.util.Map;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
@@ -84,7 +92,50 @@ class NativeAlertRulePolicyTest {
@Test
void leavesLegacyPrometheusRulesCompatibleTest() {
assertThatCode(() ->
NativeAlertRulePolicy.validate(rule(AlertDomain.BUSINESS,
-
"rocketmq_consumer_lag_messages").build())).doesNotThrowAnyException();
+
"rocketmq_consumer_lag_messages").operator(null).build())).doesNotThrowAnyException();
+ }
+
+ @ParameterizedTest
+ @NullAndEmptySource
+ @ValueSource(strings = {" ", "INVALID", "GT", "unavailable"})
+ void rejectsNativeRulesWithoutSupportedOperatorTest(String operator) {
+ assertThatThrownBy(() ->
NativeAlertRulePolicy.validate(rule(AlertDomain.CLUSTER, "broker.availability")
+ .instanceId("local").operator(operator).build()))
+ .isInstanceOf(BusinessException.class)
+ .hasMessageContaining("operator")
+ .satisfies(error -> assertThat(((BusinessException)
error).getCode()).isEqualTo(400));
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {">", ">=", "<", "<=", "==", "!=", "UNAVAILABLE"})
+ void acceptsSupportedNativeOperatorsTest(String operator) {
+ assertThatCode(() ->
NativeAlertRulePolicy.validate(rule(AlertDomain.CLUSTER, "broker.availability")
+
.instanceId("local").operator(operator).build())).doesNotThrowAnyException();
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {" > ", " UNAVAILABLE "})
+ void acceptsPaddedOperatorsLikeTheEvaluatorDoesTest(String operator) {
+ // AlertRuleEvaluator.compare() trims the operator, so validation has
to accept the padded form
+ // instead of rejecting a rule the evaluator would happily run.
+ assertThatCode(() ->
NativeAlertRulePolicy.validate(rule(AlertDomain.CLUSTER, "broker.availability")
+
.instanceId("local").operator(operator).build())).doesNotThrowAnyException();
+ }
+
+ @Test
+ void keepsNativeOperatorWhitelistInSyncWithTheEvaluatorTest() {
+ // The whitelist exists three times: AlertRuleRequestDTO's @Pattern,
AlertRuleEvaluator.compare()
+ // and NativeAlertRulePolicy.NATIVE_OPERATORS. Pinning the evaluator
here means an operator added
+ // to the policy without a matching switch case fails the build
instead of silently evaluating to
+ // false at runtime.
+ assertThat(NativeAlertRulePolicy.NATIVE_OPERATORS)
+ .containsExactlyInAnyOrder(">", ">=", "<", "<=", "==", "!=",
"UNAVAILABLE");
+ AlertRuleEvaluator evaluator = new AlertRuleEvaluator();
+ for (String operator : NativeAlertRulePolicy.NATIVE_OPERATORS) {
+ assertThat(evaluatorSupports(evaluator, operator))
+ .as("AlertRuleEvaluator must implement operator %s",
operator)
+ .isTrue();
+ }
}
@Test
@@ -118,6 +169,25 @@ class NativeAlertRulePolicyTest {
}
private static AlertRuleVO.AlertRuleVOBuilder rule(AlertDomain domain,
String metric) {
- return AlertRuleVO.builder().domain(domain).name("Test
rule").metric(metric).consecutiveSamples(1);
+ return AlertRuleVO.builder().domain(domain).name("Test
rule").metric(metric).operator(">")
+ .consecutiveSamples(1);
+ }
+
+ /**
+ * Probes one operator against the sample values that make every supported
comparison fire at least
+ * once, so an operator missing from AlertRuleEvaluator.compare() reports
false here.
+ */
+ private static boolean evaluatorSupports(AlertRuleEvaluator evaluator,
String operator) {
+ List<MetricSample> samples = List.of(sample(4D,
MetricAvailability.AVAILABLE),
+ sample(5D, MetricAvailability.AVAILABLE), sample(6D,
MetricAvailability.AVAILABLE),
+ sample(null, MetricAvailability.UNAVAILABLE));
+ AlertRuleVO rule = rule(AlertDomain.CLUSTER,
"broker.availability").instanceId("local")
+ .operator(operator).threshold(5D).build();
+ return samples.stream().anyMatch(sample -> evaluator.evaluate(rule,
sample).conditionMet());
+ }
+
+ private static MetricSample sample(Double value, MetricAvailability
availability) {
+ return new MetricSample("broker.availability", AlertDomain.CLUSTER,
"local", "local", Map.of(), value,
+ availability, Instant.EPOCH);
}
}
