RockteMQ-AI commented on PR #2946:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2946#issuecomment-5502688980
I have enough context to write the review.
---
## PR Review: fix(audit): reject result filters outside the recorded
vocabulary
### Summary
This PR adds input validation for the `result` query parameter on the audit
log endpoints. `AuditService.findPage()` now rejects any `result` filter value
that is not `SUCCESS`, `FAILED`, or absent, returning a 400 error instead of
silently producing an empty page. Since `findPage` is the shared internal entry
point for both `queryLogs` and `exportLogs`, both paths are covered. Two
regression tests are added.
### Issues
**1. Javadoc syntax is broken (`AuditService.java`, new
`validateResultFilter` method)**
The Javadoc uses `{ @code record}` with a space between the brace and the
tag. Javadoc will not interpret this as an inline tag — it will render the
literal text `{ @code record}`. Should be `{@code record}` (no space) in all
three occurrences:
```java
/**
* Every {@code record} call site writes {@code SUCCESS} or {@code FAILED}
to the result
* column, so a filter outside that vocabulary can never match a row:
surface a 400 instead
* of a silent empty page.
*/
```
**2. Test annotations are corrupted (`AuditServiceTest.java`)**
The diff shows the `@Test` annotation for both new test methods as:
```
@server/src/main/java/org/apache/rocketmq/studio/settings/DataSourceTestDTO.java
```
This should simply be `@Test`. If this is not a diff-rendering artifact and
is the actual committed content, the test class will not compile. Please verify
the file on disk.
**3. No regression test for `exportLogs` with an invalid result filter**
The new tests cover `queryLogs` only. While `exportLogs` delegates to the
same `findPage()` and is therefore covered by the validation at runtime, there
is no test asserting that `exportLogs(... , "success")` also throws. Given the
PR description explicitly calls out the CSV export path, a parallel assertion
would strengthen the regression net and document the intent.
**4. Hardcoded vocabulary may drift**
The allowed set (`"SUCCESS"`, `"FAILED"`) is repeated as string literals in
`validateResultFilter` and at every `record()` call site across the codebase.
If a future change introduces a third value (e.g. `PARTIAL`), the validator
must be updated in lock-step or valid queries will start returning 400.
Consider extracting an enum or a `Set<String> ALLOWED_RESULTS` constant shared
by both the `record()` call sites and this validator, so the vocabulary has a
single source of truth.
### Minor Observations
- **Ordering**: `validateResultFilter` runs before `parseDate` inside
`findPage`, so an invalid result fails fast without unnecessary date parsing.
Good.
- **Null/empty semantics**: `null` and `""` are both treated as "no filter",
consistent with how the rest of the query layer works.
- **Backward compatibility**: This is technically a breaking change —
clients that previously sent `result=success` (lowercase) will now get a 400
instead of an empty page. The new behavior is strictly better UX, but worth a
mention in release notes if external consumers exist.
- **Thread safety**: `validateResultFilter` is `static` and stateless. No
concerns.
### Verdict
The core logic is sound and the fix is well-targeted. Items 1 and 2 must be
fixed before merge (broken Javadoc, potentially broken test annotations). Items
3 and 4 are constructive improvements to consider.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
