[ISSUE #11158] Allow '+' separator in topic names for V2 pop retry topics

[jokerzsd]

What is the purpose of the change

Fixes #11158.

KeyBuilder.buildPopRetryTopicV2() generates V2 pop retry topics using + as the separator between the consumer group and topic (POP_RETRY_SEPARATOR_V2), producing names like %RETRY%<cid>+<topic>. However TopicValidator.VALID_CHAR_BIT_MAP did not include +, so brokers with enableRetryTopicV2=true rejected these generated topics with "contains illegal characters".

Brief changelog

• Add + to TopicValidator.VALID_CHAR_BIT_MAP.
• Update the "allowing only" regex in both error messages from ^[%|a-zA-Z0-9_-]+$ to ^[%|a-zA-Z0-9_+-]+$.
• Add a regression test verifying %RETRY%GID_test+normal_topic is valid.

Verifying this change

mvn -f common/pom.xml test -Dtest=TopicValidatorTest — all 10 tests pass.

[RockteMQ-AI]

Summary

This PR modifies 2 files (+13/-3) in apache/rocketmq.

Observations

• Diff size: 59 lines — moderate change, recommend careful review
• Test coverage: ✅ Test files included

Recommendations

• Please ensure backward compatibility if any public API is modified
• Verify thread safety for any concurrent code paths
• Confirm error handling is adequate for new code paths

---

Automated review by github-manager-bot

[RockteMQ-AI]

Summary

Fixes #11158. KeyBuilder.buildPopRetryTopicV2() generates V2 pop retry topics using + as the separator (POP_RETRY_SEPARATOR_V2), producing names like %RETRY%<cid>+<topic>. However TopicValidator.VALID_CHAR_BIT_MAP did not include +, so brokers with enableRetryTopicV2=true rejected these auto-generated topics with "contains illegal characters".

The fix adds + to the valid character set and updates the regex comments and error messages accordingly. Test covers the V2 retry topic scenario.

One consideration: since isTopicOrGroupIllegal() is shared between topic and group validation, this also allows + in consumer group names. This is a broader change than strictly needed for the V2 retry topic fix, but unlikely to cause issues in practice — and consistency between topic/group character sets is generally desirable.

LGTM.

---

Automated review by github-manager-bot

[qianye1001]

The validation mismatch for V2 retry topics is real. Please keep + forbidden in ordinary topic and group names, and add dedicated validation for well-formed V2 retry topics.

Adding + to the shared VALID_CHAR_BIT_MAP introduces ambiguous retry-topic names. For example, both of these newly accepted pairs generate the same topic:

group=A,   topic=B+C  -> %RETRY%A+B+C
group=A+B, topic=C    -> %RETRY%A+B+C

The current KeyBuilder.parseGroup() also expects exactly two parts when splitting on +. It returns A+B+C for the example above, which is neither intended group. Even a traditional retry topic %RETRY%A+B for group A+B is interpreted as a V2 retry topic and parsed as group A. SendMessageProcessor.handleRetryAndDLQ() uses this parsed group to look up the subscription configuration and handle retries/DLQ routing, so this has behavioral consequences. I reproduced these results against the current PR head.

A scoped fix would recognize %RETRY%<group>+<topic> in topic validation, require exactly one + separator and non-empty components, and validate the components using the existing character rules and applicable length limits. Ordinary topic validation and group validation should continue rejecting +.

Please also cover valid V2 retry topics, + in ordinary topic/group names, and malformed V2 names with empty components or multiple separators in the regression tests.

[jokerzsd]

Thanks for the detailed review, @qianye1001. Agreed — allowing + in the shared VALID_CHAR_BIT_MAP created the ambiguity you described. I've reworked the fix:

• + remains forbidden in ordinary topic and group names (reverted the bit-map change and error-message regexes).
• validateTopic now short-circuits to a dedicated validatePopRetryTopicV2 for %RETRY%<group>+<topic>: it requires exactly one +, non-empty group/topic, validates each component against the existing character rules (which still reject +), and enforces the 255 retry-topic length limit.

Regression tests now cover: a valid V2 retry topic, + in ordinary topic names, + in group names, and malformed V2 names (multiple separators, empty group, empty topic).

mvn -f common/pom.xml test -Dtest=TopicValidatorTest → 13/13 pass.

[RockteMQ-AI]

Summary

The updated implementation looks great. Instead of broadly allowing + in the valid character map, this version introduces a dedicated validatePopRetryTopicV2() that:

1. Detects V2 retry topics by checking for the %RETRY% prefix and + separator
2. Validates the structure: exactly one + separating a valid consumer group and a valid topic
3. Enforces length constraints on the composite retry topic name
4. Keeps + rejected in ordinary topic names and group names

The test coverage is thorough — valid V2 retry topics, malformed inputs (multiple +, empty group/topic), and negative cases (ordinary topics with +) are all covered.

LGTM. 👍

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.41%. Comparing base (1a50c6e) to head (9938c07).
⚠️ Report is 5 commits behind head on develop.

Files with missing lines	Patch %	Lines
...g/apache/rocketmq/common/topic/TopicValidator.java	73.33%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #11165      +/-   ##
=============================================
+ Coverage      49.39%   49.41%   +0.01%     
- Complexity     14237    14267      +30     
=============================================
  Files           1390     1390              
  Lines         103123   103144      +21     
  Branches       13484    13490       +6     
=============================================
+ Hits           50940    50967      +27     
+ Misses         46031    45996      -35     
- Partials        6152     6181      +29     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[qianye1001]

LGTM