RockteMQ-AI commented on PR #2947:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2947#issuecomment-5502681656
I have everything needed. The production fix is sound, but I found a
critical issue with the new test — verified against the pinned MyBatis-Plus
3.5.7 source. Writing the review now.
---
## Review
### Summary
This PR escapes LIKE wildcards (`%`, `_`, `\`) in the audit-log free-text
search before building the `QueryWrapper` in
`MybatisPlusAuditRepository.findPage`, and adds a unit test asserting the
escaped pattern reaches the mapper. The production change is correct and
consistent with the existing `escapeLike` in `QueryHistoryService` — but **the
new test as diffed cannot pass**, so the PR description's "all tests pass"
claim doesn't match the code.
### Critical: the new test will fail as written
`MybatisPlusAuditRepositoryTest.findPageEscapesLikeWildcardsInSearch`
asserts:
```java
assertThat(wrapper.getParamNameValuePairs().values())
.allSatisfy(parameter ->
assertThat(parameter.toString()).isEqualTo("100\\%\\_a\\\\b"));
```
But MyBatis-Plus 3.5.7 (pinned in `server/pom.xml:73`) does **not** store
the raw `like()` value in `paramNameValuePairs`. In `AbstractWrapper.likeValue`
(v3.5.7 tag):
```java
return maybeDo(condition, () ->
appendSqlSegments(columnToSqlSegment(column), keyword,
() -> formatParam(null, SqlUtils.concatLike(val, sqlLike))));
```
`SqlUtils.concatLike(val, SqlLike.DEFAULT)` returns `PERCENT + str +
PERCENT`, and `formatParam` puts that into `paramNameValuePairs` unchanged. So
for search `100%_a\b`, the three bound values will be:
```
%100\%\_a\\b%
```
not `100\%\_a\\b`. The assertion fails on the first value. Please re-run the
suite — this suggests the test in the diff wasn't actually executed (or was
executed against different code than what's posted).
Suggested fix — either account for MP's wrapping:
```java
assertThat(wrapper.getParamNameValuePairs().values())
.allSatisfy(parameter -> assertThat(parameter.toString())
.isEqualTo("%100\\%\\_a\\\\b%"));
```
or, better, assert actual matching semantics with an H2 integration test
(the pattern already exists in `QueryHistoryServiceIntegrationTest`): insert
audit rows with operators `100x` and `100%`, then assert that searching `100%`
returns only the literal row. That would also cover the
`resource_name`/`detail` branches end-to-end and isn't coupled to MP's internal
param encoding.
### The production fix itself: correct
- Replacement order (backslash first, then `%`, then `_`) is right;
identical to the proven `QueryHistoryService.escapeLike`
(`server/src/main/java/org/apache/rocketmq/studio/instance/message/QueryHistoryService.java:229`).
The `hasText` guard is redundant with the `.and(StringUtils.hasText(search),
...)` condition but harmless.
- No injection concern: MP `like()` binds values as parameters, so this is
purely a matching-semantics fix (searching `%` currently returns every row,
including via the CSV export path in `AuditService`).
- Portability is fine for the supported backends: MySQL
(`application.yml:10`) and H2 in MySQL mode (`application-dev.yml:4`) both
treat `\` as the default LIKE escape character. (Oracle/SQL Server would not,
but they aren't supported here.)
- Behavior change to be aware of: users who relied on `%`/`_` as wildcards
in audit search will now get literal matching — that's the intent, and it now
matches message/trace history search behavior.
### Follow-up suggestions (not blockers)
1. **Same bug exists elsewhere.** This is now the second private copy of
`escapeLike`, while several other user-input `like()` call sites remain
unescaped: `AuthService.java:182` (username search),
`MybatisPlusInstanceRepository.java:67-82`,
`MybatisPlusCloudCredentialRepository.java:53`,
`MybatisPlusAclRepository.java:341-342`,
`MybatisPlusSettingsRepository.java:137`,
`RocketMQMetadataProvider.java:145/165/225`. Consider extracting a shared
utility (e.g., in `org.apache.rocketmq.studio.common`) and a follow-up PR
applying it to those sites.
2. **Javadoc nit**: the inline tags in the new `escapeLike` doc have a space
after the brace (`{ @code %}`), so they render literally instead of as code —
use `{@code %}`.
### Verdict
Request changes: the fix is right, but the regression test is broken as
posted and the claimed test run can't have included it. Fix the assertion (or
replace with an integration test), re-run, and this is good to merge.
--
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]
