diff --git a/CLAUDE.md b/CLAUDE.md index 77c14ce6..be7b1d8b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,6 +70,7 @@ PostgreSQL (Flyway): `cameleer-server-app/src/main/resources/db/migration/` - V11 — Outbound connections (outbound_connections table, enums) - 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) +- 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) diff --git a/cameleer-server-app/src/main/resources/db/migration/V14__fix_null_firemode_alert_rules.sql b/cameleer-server-app/src/main/resources/db/migration/V14__fix_null_firemode_alert_rules.sql new file mode 100644 index 00000000..dbe76313 --- /dev/null +++ b/cameleer-server-app/src/main/resources/db/migration/V14__fix_null_firemode_alert_rules.sql @@ -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; diff --git a/cameleer-server-core/src/main/java/com/cameleer/server/core/alerting/ExchangeMatchCondition.java b/cameleer-server-core/src/main/java/com/cameleer/server/core/alerting/ExchangeMatchCondition.java index 0ea59eb0..e8a56cc8 100644 --- a/cameleer-server-core/src/main/java/com/cameleer/server/core/alerting/ExchangeMatchCondition.java +++ b/cameleer-server-core/src/main/java/com/cameleer/server/core/alerting/ExchangeMatchCondition.java @@ -14,6 +14,8 @@ public record ExchangeMatchCondition( ) implements AlertCondition { 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)) throw new IllegalArgumentException("COUNT_IN_WINDOW requires threshold + windowSeconds"); if (fireMode == FireMode.PER_EXCHANGE && perExchangeLingerSeconds == null) diff --git a/cameleer-server-core/src/test/java/com/cameleer/server/core/alerting/AlertConditionJsonTest.java b/cameleer-server-core/src/test/java/com/cameleer/server/core/alerting/AlertConditionJsonTest.java index b17b056b..8ecf85f0 100644 --- a/cameleer-server-core/src/test/java/com/cameleer/server/core/alerting/AlertConditionJsonTest.java +++ b/cameleer-server-core/src/test/java/com/cameleer/server/core/alerting/AlertConditionJsonTest.java @@ -6,6 +6,7 @@ import java.util.List; import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; class AlertConditionJsonTest { @@ -43,6 +44,34 @@ class AlertConditionJsonTest { 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 void roundtripAgentState() throws Exception { var c = new AgentStateCondition(new AlertScope("orders", null, null), "DEAD", 60); diff --git a/ui/src/pages/Alerts/RuleEditor/ConditionStep.tsx b/ui/src/pages/Alerts/RuleEditor/ConditionStep.tsx index a0c4cb1b..2d5c21f6 100644 --- a/ui/src/pages/Alerts/RuleEditor/ConditionStep.tsx +++ b/ui/src/pages/Alerts/RuleEditor/ConditionStep.tsx @@ -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 // into the save request. Preserve scope — it's managed on the scope step. const prev = form.condition as Record; + const base: Record = { 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({ ...form, conditionKind: kind, - condition: { kind, scope: prev.scope } as FormState['condition'], + condition: base as FormState['condition'], }); }; diff --git a/ui/src/pages/Alerts/RuleEditor/form-state.test.ts b/ui/src/pages/Alerts/RuleEditor/form-state.test.ts index 40e68df8..ed1db316 100644 --- a/ui/src/pages/Alerts/RuleEditor/form-state.test.ts +++ b/ui/src/pages/Alerts/RuleEditor/form-state.test.ts @@ -47,4 +47,37 @@ describe('validateStep', () => { f.evaluationIntervalSeconds = 1; 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([]); + }); }); diff --git a/ui/src/pages/Alerts/RuleEditor/form-state.ts b/ui/src/pages/Alerts/RuleEditor/form-state.ts index 3de5b9d5..120c70af 100644 --- a/ui/src/pages/Alerts/RuleEditor/form-state.ts +++ b/ui/src/pages/Alerts/RuleEditor/form-state.ts @@ -147,6 +147,19 @@ export function validateStep(step: WizardStep, f: FormState): string[] { } if (step === 'condition') { if (!f.conditionKind) errs.push('Condition kind is required.'); + if (f.conditionKind === 'EXCHANGE_MATCH') { + const c = f.condition as Record; + 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 (f.evaluationIntervalSeconds < 5) errs.push('Evaluation interval must be \u2265 5 s.');