Skip to content

fix(studio): add explicit ESCAPE clause to LIKE searches - #4393

Closed
jokerzsd wants to merge 1 commit into
apache:masterfrom
jokerzsd:fix/studio-like-escape-escape-clause
Closed

jokerzsd wants to merge 1 commit into
apache:masterfrom
jokerzsd:fix/studio-like-escape-escape-clause

Conversation

@jokerzsd

Copy link
Copy Markdown
Contributor

Replaces #4363, closed by the rocketmq-studio → master branch switch. (Also a resubmit of #4236 per @lizhimins's review.)

Summary

User-supplied search strings in topic/group metadata search and data source search were passed into MyBatis-Plus like(), so %, _ and \ were treated as SQL wildcards/escape characters. The prior fix escaped the value but relied on MySQL's implicit backslash escape character, which is wrong on H2 (no default LIKE escape character).

Fix

  • Add a shared SqlLikeUtils helper (escape + contains + the ESCAPE clause constant).
  • Replace the five .like() predicates with .apply(..., "name LIKE {0} ESCAPE '\'", SqlLikeUtils.contains(search)) (four in RocketMQMetadataProvider, one in MybatisPlusSettingsRepository.findDataSources), so the backslash is explicitly the escape character on every dialect.

Tests

  • RocketMQMetadataProviderTest.searchEscapesLikeWildcardsAndAddsExplicitEscapeClause: asserts the wrapper contains LIKE/ESCAPE and the bound parameter is %prod\_user% for both topic and group search.
  • MybatisPlusSettingsRepositoryTest.findDataSourcesEscapesSearchWildcardsAndAddsEscapeClause: same assertion for the data source search.

Verification

mvn test -Dtest=RocketMQMetadataProviderTest → 42 passed.
mvn test -Dtest=MybatisPlusSettingsRepositoryTest → 11 passed.

Resubmit of the LIKE-escape fix with dialect-independent escaping. The
previous version relied on MySQL's implicit backslash escape character, which
is wrong on H2 (no default escape character), so the escaped pattern silently
changed meaning there.

Attach an explicit `ESCAPE '\'` clause to every user-search LIKE predicate and
route the escaped value through the shared SqlLikeUtils helper so `%`, `_` and
`\` match literally on every dialect. Cover topic/group metadata search and
data source search with regression tests asserting both the ESCAPE clause and
the escaped bound parameter.

Signed-off-by: jokerzsd <2701819133@qq.com>

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Important correctness fix — user search strings containing %, _, or \ were being interpreted as SQL LIKE wildcards instead of literal characters (e.g. searching prod_user would also match prodXuser). The new SqlLikeUtils utility properly escapes these characters and attaches an explicit ESCAPE clause for cross-dialect compatibility (H2 has no implicit default).

Applied consistently across topic, consumer group, and data source searches. Well-tested.

LGTM.


Automated review by "github-manager-bot"

@lizhimins lizhimins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the resubmission. The underlying bug is real and worth fixing: RocketMQMetadataProvider passes the caller's search straight to MyBatis-Plus like() on the topic and group name predicates, and MybatisPlusSettingsRepository.findDataSources does the same on the json column, so %, _ and \ keep their wildcard meaning. Putting the helper in common/util is also the right home per the project conventions, and both new tests are mutation-sensitive — reverting any one of the four .apply(...) call sites turns the provider test red.

There is one hard blocker and three things to close out.

1. The ESCAPE clause is a syntax error on MySQL. ESCAPE_CLAUSE = " ESCAPE '\\'" is a Java literal whose runtime value is ESCAPE '\', so the emitted SQL is name LIKE ? ESCAPE '\'. MySQL — the default production profile — treats \ as a string-literal escape character, so '\' is an escaped quote and the string is never terminated: ERROR 1064. H2, which is what mvn test runs against (jdbc:h2:...;MODE=MySQL in the dev profile), does not do backslash escaping and accepts it. Both new tests only assert on getSqlSegment() and the bound parameter, and never execute SQL, so the suite cannot catch this. As written the change is green in CI and broken in production on five query paths (topic list, topic page, group list, group page, data source list).

A spelling that works on both engines is ESCAPE CHAR(92), which names the backslash by code point. Note that ESCAPE '\\' is the mirror-image trap: MySQL accepts it, H2 rejects it with Error in LIKE ESCAPE. Whichever you choose, please add a test that actually executes the generated predicate against a database — that is the only thing that pins the dialect question down.

2. Coverage. After this patch, 18 user-supplied .like() call sites are still unescaped, across AuthService, MybatisPlusInstanceRepository, MybatisPlusAclRepository, MybatisPlusAlertRepository, MybatisPlusAuditRepository and MybatisPlusCloudCredentialRepository. On the current rocketmq-studio branch there is also a tenth class, MybatisPlusAiConversationRepository, whose title search calls its own private escapeLike. If you want to keep the PR narrow that is a legitimate choice, but then please say so explicitly in the description and open an issue for the remainder — a helper that escapes five call sites while eighteen others keep passing raw user input into like() is easy to mistake for a completed fix.

3. Duplication. QueryHistoryService.escapeLike and MybatisPlusAiConversationRepository.escapeLike are line-for-line copies of the rule your new helper implements (and the latter's own comment says a shared util is tracked upstream). The conventions ask for shared logic in common/util; landing a third copy without absorbing the other two moves the repository backwards. Please delete both private copies as part of this change, or explain why they have to stay.

4. Tests and naming. SqlLikeUtils itself has no unit test — the behaviour of escape on \, %, _, combinations, blank and null is entirely uncovered. And the two new test methods end in Clause rather than Test, which is the project convention for Java test methods.

Not blocking, but worth a look: .apply() takes the column as a bare string ("name", "json"), which gives up the type-safe RmqTopic::getName reference the LambdaQueryWrapper had. A column rename will no longer be caught by the compiler. Modelling that layer (a likePredicate(column) helper, say) at least centralises the loss.

Two process notes. This PR is based on master; Studio PRs now merge onto the rocketmq-studio branch, so please retarget and rebase. And #4458 is open on exactly this topic — it also adds a common/util/SqlLikeUtils and touches the same call sites, so the two are mutually exclusive and only one can land. We are returning this rather than closing it because the bug is real and your write-up is useful, but you should know that #4458 uses ESCAPE CHAR(92), sweeps all nine of the classes that call .like() on its base (we have asked it to pick up the tenth, MybatisPlusAiConversationRepository, on rebase), removes the private copies and ships an H2-executing dialect test. If you would rather not duplicate that work, reviewing #4458 is a genuinely useful contribution; if you do want to continue here, items 1 to 4 all need addressing first and we will then pick between the two.

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the stale label Oct 3, 2026
@github-actions github-actions Bot closed this Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants