fix(alerting): reject null fireMode on ExchangeMatchCondition + repair in-flight rows
All checks were successful
All checks were successful
The rule editor wizard reset the condition payload on kind-change without
seeding a fireMode default; the ExchangeMatchCondition ctor allowed null to
pass through; AlertEvaluatorJob then NPE-looped every tick on a saved rule.
- core: compact ctor now rejects null fireMode (Jackson-deser path only — all
production callers already pass a value).
- V14: repair existing EXCHANGE_MATCH rows with fireMode=null to
PER_EXCHANGE + perExchangeLingerSeconds=300 (default matches the wizard).
- ui: ConditionStep.onKindChange seeds EXCHANGE_MATCH defaults so the
Select's displayed fallback ("Per exchange") is actually in form state.
- ui: validateStep('condition', ...) now enforces fireMode presence + the
mode-specific fields before the user reaches Review.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -70,6 +70,7 @@ PostgreSQL (Flyway): `cameleer-server-app/src/main/resources/db/migration/`
|
|||||||
- V11 — Outbound connections (outbound_connections table, enums)
|
- V11 — Outbound connections (outbound_connections table, enums)
|
||||||
- V12 — Alerting tables (alert_rules, alert_rule_targets, alert_instances, alert_notifications, alert_reads, alert_silences)
|
- V12 — Alerting tables (alert_rules, alert_rule_targets, alert_instances, alert_notifications, alert_reads, alert_silences)
|
||||||
- V13 — alert_instances open-rule unique index (alert_instances_open_rule_uq partial index on rule_id WHERE state IN PENDING/FIRING/ACKNOWLEDGED)
|
- V13 — alert_instances open-rule unique index (alert_instances_open_rule_uq partial index on rule_id WHERE state IN PENDING/FIRING/ACKNOWLEDGED)
|
||||||
|
- V14 — Repair EXCHANGE_MATCH alert_rules persisted with fireMode=null (sets fireMode=PER_EXCHANGE + perExchangeLingerSeconds=300); paired with stricter `ExchangeMatchCondition` ctor that now rejects null fireMode.
|
||||||
|
|
||||||
ClickHouse: `cameleer-server-app/src/main/resources/clickhouse/init.sql` (run idempotently on startup)
|
ClickHouse: `cameleer-server-app/src/main/resources/clickhouse/init.sql` (run idempotently on startup)
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,19 @@
|
|||||||
|
-- V14 — Repair EXCHANGE_MATCH rules persisted with fireMode=null.
|
||||||
|
-- Root cause: the rule editor wizard reset the condition payload on kind
|
||||||
|
-- change without seeding a fireMode default, and the backend ctor allowed
|
||||||
|
-- null to pass. Stricter ctor in ExchangeMatchCondition now rejects null
|
||||||
|
-- fireMode — existing rows need to be repaired so startup doesn't fail
|
||||||
|
-- deserialization and the evaluator stops NPE-looping on them.
|
||||||
|
--
|
||||||
|
-- Default to PER_EXCHANGE + perExchangeLingerSeconds=300 — the same default
|
||||||
|
-- the UI now seeds when a user picks EXCHANGE_MATCH.
|
||||||
|
UPDATE alert_rules
|
||||||
|
SET condition = jsonb_set(
|
||||||
|
jsonb_set(condition, '{fireMode}', '"PER_EXCHANGE"', true),
|
||||||
|
'{perExchangeLingerSeconds}',
|
||||||
|
COALESCE(condition->'perExchangeLingerSeconds', '300'::jsonb),
|
||||||
|
true
|
||||||
|
),
|
||||||
|
updated_at = now()
|
||||||
|
WHERE condition_kind = 'EXCHANGE_MATCH'
|
||||||
|
AND (condition->>'fireMode') IS NULL;
|
||||||
@@ -14,6 +14,8 @@ public record ExchangeMatchCondition(
|
|||||||
) implements AlertCondition {
|
) implements AlertCondition {
|
||||||
|
|
||||||
public ExchangeMatchCondition {
|
public ExchangeMatchCondition {
|
||||||
|
if (fireMode == null)
|
||||||
|
throw new IllegalArgumentException("fireMode is required (PER_EXCHANGE or COUNT_IN_WINDOW)");
|
||||||
if (fireMode == FireMode.COUNT_IN_WINDOW && (threshold == null || windowSeconds == null))
|
if (fireMode == FireMode.COUNT_IN_WINDOW && (threshold == null || windowSeconds == null))
|
||||||
throw new IllegalArgumentException("COUNT_IN_WINDOW requires threshold + windowSeconds");
|
throw new IllegalArgumentException("COUNT_IN_WINDOW requires threshold + windowSeconds");
|
||||||
if (fireMode == FireMode.PER_EXCHANGE && perExchangeLingerSeconds == null)
|
if (fireMode == FireMode.PER_EXCHANGE && perExchangeLingerSeconds == null)
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import java.util.List;
|
|||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
|
|
||||||
import static org.assertj.core.api.Assertions.assertThat;
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
|
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||||
|
|
||||||
class AlertConditionJsonTest {
|
class AlertConditionJsonTest {
|
||||||
|
|
||||||
@@ -43,6 +44,34 @@ class AlertConditionJsonTest {
|
|||||||
assertThat(((ExchangeMatchCondition) parsed).threshold()).isEqualTo(5);
|
assertThat(((ExchangeMatchCondition) parsed).threshold()).isEqualTo(5);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void exchangeMatchRejectsNullFireMode() {
|
||||||
|
assertThatThrownBy(() -> new ExchangeMatchCondition(
|
||||||
|
new AlertScope(null, null, null),
|
||||||
|
new ExchangeMatchCondition.ExchangeFilter("FAILED", Map.of()),
|
||||||
|
null, null, null, null))
|
||||||
|
.isInstanceOf(IllegalArgumentException.class)
|
||||||
|
.hasMessageContaining("fireMode");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void exchangeMatchRejectsNullFireModeOnDeserialization() throws Exception {
|
||||||
|
String json = """
|
||||||
|
{
|
||||||
|
"kind": "EXCHANGE_MATCH",
|
||||||
|
"scope": {},
|
||||||
|
"filter": {"status": "FAILED", "attributes": {}},
|
||||||
|
"fireMode": null,
|
||||||
|
"threshold": null,
|
||||||
|
"windowSeconds": null,
|
||||||
|
"perExchangeLingerSeconds": null
|
||||||
|
}
|
||||||
|
""";
|
||||||
|
assertThatThrownBy(() -> om.readValue(json, AlertCondition.class))
|
||||||
|
.hasRootCauseInstanceOf(IllegalArgumentException.class)
|
||||||
|
.hasMessageContaining("fireMode");
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void roundtripAgentState() throws Exception {
|
void roundtripAgentState() throws Exception {
|
||||||
var c = new AgentStateCondition(new AlertScope("orders", null, null), "DEAD", 60);
|
var c = new AgentStateCondition(new AlertScope("orders", null, null), "DEAD", 60);
|
||||||
|
|||||||
@@ -14,10 +14,19 @@ export function ConditionStep({ form, setForm }: { form: FormState; setForm: (f:
|
|||||||
// Reset the condition payload so stale fields from a previous kind don't leak
|
// Reset the condition payload so stale fields from a previous kind don't leak
|
||||||
// into the save request. Preserve scope — it's managed on the scope step.
|
// into the save request. Preserve scope — it's managed on the scope step.
|
||||||
const prev = form.condition as Record<string, unknown>;
|
const prev = form.condition as Record<string, unknown>;
|
||||||
|
const base: Record<string, unknown> = { kind, scope: prev.scope };
|
||||||
|
// EXCHANGE_MATCH must carry a fireMode — the backend ctor now rejects null.
|
||||||
|
// Seed PER_EXCHANGE + 300s linger so a user can save without touching every
|
||||||
|
// sub-field (matches what the form's Select displays by default).
|
||||||
|
if (kind === 'EXCHANGE_MATCH') {
|
||||||
|
base.fireMode = 'PER_EXCHANGE';
|
||||||
|
base.perExchangeLingerSeconds = 300;
|
||||||
|
base.filter = {};
|
||||||
|
}
|
||||||
setForm({
|
setForm({
|
||||||
...form,
|
...form,
|
||||||
conditionKind: kind,
|
conditionKind: kind,
|
||||||
condition: { kind, scope: prev.scope } as FormState['condition'],
|
condition: base as FormState['condition'],
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
@@ -47,4 +47,37 @@ describe('validateStep', () => {
|
|||||||
f.evaluationIntervalSeconds = 1;
|
f.evaluationIntervalSeconds = 1;
|
||||||
expect(validateStep('trigger', f).some((e) => /Evaluation interval/.test(e))).toBe(true);
|
expect(validateStep('trigger', f).some((e) => /Evaluation interval/.test(e))).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('flags missing fireMode on EXCHANGE_MATCH condition step', () => {
|
||||||
|
const f = initialForm();
|
||||||
|
f.conditionKind = 'EXCHANGE_MATCH';
|
||||||
|
// Simulate the old bug: condition payload without a fireMode.
|
||||||
|
f.condition = { kind: 'EXCHANGE_MATCH', scope: {} } as typeof f.condition;
|
||||||
|
expect(validateStep('condition', f).some((e) => /Fire mode is required/.test(e))).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('flags missing threshold/window on COUNT_IN_WINDOW', () => {
|
||||||
|
const f = initialForm();
|
||||||
|
f.conditionKind = 'EXCHANGE_MATCH';
|
||||||
|
f.condition = {
|
||||||
|
kind: 'EXCHANGE_MATCH',
|
||||||
|
scope: {},
|
||||||
|
fireMode: 'COUNT_IN_WINDOW',
|
||||||
|
} as unknown as typeof f.condition;
|
||||||
|
const errs = validateStep('condition', f);
|
||||||
|
expect(errs.some((e) => /Threshold is required/.test(e))).toBe(true);
|
||||||
|
expect(errs.some((e) => /Window/.test(e))).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('passes when EXCHANGE_MATCH PER_EXCHANGE has linger seconds', () => {
|
||||||
|
const f = initialForm();
|
||||||
|
f.conditionKind = 'EXCHANGE_MATCH';
|
||||||
|
f.condition = {
|
||||||
|
kind: 'EXCHANGE_MATCH',
|
||||||
|
scope: {},
|
||||||
|
fireMode: 'PER_EXCHANGE',
|
||||||
|
perExchangeLingerSeconds: 300,
|
||||||
|
} as unknown as typeof f.condition;
|
||||||
|
expect(validateStep('condition', f)).toEqual([]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -147,6 +147,19 @@ export function validateStep(step: WizardStep, f: FormState): string[] {
|
|||||||
}
|
}
|
||||||
if (step === 'condition') {
|
if (step === 'condition') {
|
||||||
if (!f.conditionKind) errs.push('Condition kind is required.');
|
if (!f.conditionKind) errs.push('Condition kind is required.');
|
||||||
|
if (f.conditionKind === 'EXCHANGE_MATCH') {
|
||||||
|
const c = f.condition as Record<string, unknown>;
|
||||||
|
if (!c.fireMode) {
|
||||||
|
errs.push('Fire mode is required.');
|
||||||
|
} else if (c.fireMode === 'PER_EXCHANGE') {
|
||||||
|
if (c.perExchangeLingerSeconds == null) {
|
||||||
|
errs.push('Linger seconds is required for PER_EXCHANGE.');
|
||||||
|
}
|
||||||
|
} else if (c.fireMode === 'COUNT_IN_WINDOW') {
|
||||||
|
if (c.threshold == null) errs.push('Threshold is required for COUNT_IN_WINDOW.');
|
||||||
|
if (c.windowSeconds == null) errs.push('Window (seconds) is required for COUNT_IN_WINDOW.');
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
if (step === 'trigger') {
|
if (step === 'trigger') {
|
||||||
if (f.evaluationIntervalSeconds < 5) errs.push('Evaluation interval must be \u2265 5 s.');
|
if (f.evaluationIntervalSeconds < 5) errs.push('Evaluation interval must be \u2265 5 s.');
|
||||||
|
|||||||
Reference in New Issue
Block a user