You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "Frun1na (via GitHub)" <gi...@apache.org> on 2026/10/15 17:07:12 UTC
[PR] fix(alert): reject an import envelope the mock store would not accept [rocketmq-dashboard]
Frun1na opened a new pull request, #6784:
URL: https://github.com/apache/rocketmq-dashboard/pull/6784
### Which Issue(s) This PR Fixes
- Fixes #6610
### Brief Description
The Mock branch of `importAlertRulesTransfer` (`web/src/services/opsService.ts:177`) mapped the document straight onto the inventory:
```ts
if (!isMockMode()) return opsApi.importAlertRulesTransfer(data, domain);
const startId = Date.now();
const imported = data.rules.map((rule, index) => ({ ...copyAlertRule(rule as AlertRule), id: startId + index }));
alertRulesState[domain].push(...imported);
```
Nothing checked the envelope, so the demo data source accepted four documents the endpoint answers 400 for. `AlertRuleTransferService.validateEnvelope` (`server/.../ops/alert/AlertRuleTransferService.java:51-64`) requires `version == 1`, the domain of the page it is imported from, and between 1 and `MAX_RULES_PER_IMPORT` (200) rules. An empty `rules` array therefore reported a successful import of zero rules, 201 rules were appended, a document for the other domain was appended to the domain passed in, and a `version: 2` document was accepted.
The page does not close the gap: `handleImportRules` checks only version, domain and `Array.isArray` (`web/src/pages/ops/alerts.tsx:456`), so the file-upload path reaches the service with an empty or oversized document and the operator is told it imported.
The Mock branch now performs the endpoint's four checks with the endpoint's messages before it touches the inventory. The real-API branch is untouched.
User-visible change: in demo mode an empty or oversized import document now takes the page's existing `alerts.importFailed` notice instead of reporting an import, and a document for another domain is no longer written into the wrong domain's inventory.
### Other consumers
- `web/src/services/opsService.ts:177` `importAlertRulesTransfer` — fixed here. The Mock branch is the only mock surface for alert-rule import.
- `web/src/services/opsService.ts:168` `exportAlertRulesTransfer` — produces `{version: 1, domain, rules}` and reads no envelope; unchanged, and its output still round-trips through the new checks.
- `web/src/pages/ops/alerts.tsx:456` — the page's own pre-check still runs first for the file path and is deliberately left as it is: it guards the JSON shape the operator uploaded, while the rule count is a contract the endpoint enforces in its service (not in the DTO), which is why the same check belongs at the service boundary. The page's `catch` already routes both to `alerts.importFailed`, so no new UI text is needed.
- `server/.../ops/alert/AlertRuleTransferDTO.java` — `@NotEmpty` on `rules` and `@Valid` on each entry; the 200-rule ceiling lives in `AlertRuleTransferService`. Nothing to change; the Mock now mirrors both layers.
- `server/.../ai/tool/**` — no AI tool imports alert rules (`git grep -n AlertRuleTransferDTO -- server/src/main/java/org/apache/rocketmq/studio/ops/ai` is empty), so no catalog or tool-output surface consumes this envelope.
- Per-rule field validation (the endpoint's `@Valid` plus `metricCatalogService.validate`) is a separate surface and is out of scope here: a rule whose `channels` are missing already fails loudly in the Mock, and this PR is about the envelope.
### How Did You Test This Change?
```
$ ./wb test --filter src/services/opsService.test.ts
✗ 2.1s npx vitest run src/services/opsService.test.ts --maxWorkers=1 --fileParallelism=false
+ Received:
[]
❯ src/services/opsService.test.ts:204:5
203| importAlertRulesTransfer({ version: 1, domain: 'CLUSTER', rules: …
204| ).rejects.toThrow('Alert rule import must contain between 1 and 20…
Test Files 1 failed (1)
Tests 1 failed | 17 passed (18)
$ ./wb test
✓ 1.3s npx eslint src/services/opsService.test.ts src/services/opsService.ts
✓ 0.6s npx prettier --check src/services/opsService.test.ts src/services/opsService.ts
✓ 94.8s npx vitest related --run src/services/opsService.test.ts src/services/opsService.ts
Test Files 8 passed (8)
Tests 121 passed (121)
✓ 4.3s npx tsc -b
```
The new test fails without the fix:
```
# only the thirteen validation lines removed from the Mock branch, the tests kept
$ ./wb test --filter src/services/opsService.test.ts
✗ 2.0s npx vitest run src/services/opsService.test.ts --maxWorkers=1 --fileParallelism=false
+ Received:
[]
Test Files 1 failed (1)
Tests 1 failed | 17 passed (18)
✓ 3.0s npx tsc -b
```
The assertion goes red because it asks an empty envelope to reject; without the guard `[].map()` yields `[]`, the push adds nothing, and the call resolves with `[]` — the failure reads `Received: []`, i.e. an empty document reported as a successful import of zero rules. Its sibling in the same test file, `still imports the alert rule counts the endpoint accepts Test`, imports one rule and then two hundred and passes both with and without the guard, which is what shows the assertions pin the envelope rather than "the function throws".
The report in #6610 and the earlier attempt in #6611 (closed on 2026-10-15 by the seven-day inactivity sweep, not on review) describe the same defect; this change is scoped to the envelope the issue names.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`feat:` / `fix:` / `refactor:` / `chore:` / `docs:` / `perf:`)
- [x] Tests added or updated for non-trivial changes, test methods named `...Test`
- [ ] New UI text has both Chinese and English entries under `web/src/i18n/`
- [ ] Architecture constraints stay green (`mvn test` runs the ArchUnit checks)
- [ ] New source files carry the ASF license header
- [ ] Documentation touched where behaviour changed (README / `docs/` / in-app help)
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(alert): reject an import envelope the mock store would not accept [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6784:
URL: https://github.com/apache/rocketmq-dashboard/pull/6784#issuecomment-6391327431
This pull request has been automatically closed because there has been no activity for 7 days. If you would like to continue working on this, please reopen the PR with updated changes.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(alert): reject an import envelope the mock store would not accept [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #6784: fix(alert): reject an import envelope the mock store would not accept
URL: https://github.com/apache/rocketmq-dashboard/pull/6784
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org