[ISSUE #10688] Skip malformed ordered route items

[Aias00]

Which Issue(s) This PR Fixes

Fixes #10688

Brief Description

MessageQueueSelector.buildWrite previously assumed every ordered route fragment from orderTopicConf used a valid brokerName:queueNum format. Malformed fragments could throw ArrayIndexOutOfBoundsException or NumberFormatException, and unknown broker names could make TopicRouteWrapper.getMasterAddr throw before the selector's null check could skip them.

This PR validates each ordered route fragment before using it, skips malformed entries with a warning, and makes TopicRouteWrapper.getMasterAddr return null for unknown brokers so existing skip logic works as intended. Valid ordered route entries keep the existing behavior.

How Did You Test This Change?

mvn -pl proxy -Dtest=MessageQueueSelectorTest -DfailIfNoTests=false test

Result: BUILD SUCCESS, Tests run: 3, Failures: 0, Errors: 0, Skipped: 0.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Defensive fix for MessageQueueSelector.buildWrite: validates each ordered route fragment (broker:count format) before parsing, skips malformed entries with a warning. Also fixes a potential NPE in TopicRouteWrapper.getMasterAddr when brokerData is null. Includes tests.

Findings

• [Info] proxy/src/main/java/org/apache/rocketmq/proxy/service/route/MessageQueueSelector.java:118-120 — The validation checks item.length != 2 and blank segments, which is good. Consider also logging the topicRoute.getTopicName() in the NumberFormatException catch block (it is already included — confirmed).

• [Info] proxy/src/main/java/org/apache/rocketmq/proxy/service/route/TopicRouteWrapper.java — The NPE guard in getMasterAddr (if (brokerData == null) return null) is a good complementary fix. Without it, a malformed route entry could cause a NPE downstream even if the format check passes.

• [Info] Test coverage in MessageQueueSelectorTest looks solid — tests valid input, wrong segment count, blank broker name, blank queue count, and non-numeric queue count.

Overall

Well-structured defensive fix with two layers of protection (format validation + NumberFormatException catch). The NPE guard in TopicRouteWrapper is a good catch. LGTM.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.25%. Comparing base (00e45b8) to head (f6abff7).

Files with missing lines	Patch %	Lines
...etmq/proxy/service/route/MessageQueueSelector.java	96.15%	0 Missing and 1 partial ⚠️
...ocketmq/proxy/service/route/TopicRouteWrapper.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10689      +/-   ##
=============================================
- Coverage      48.31%   48.25%   -0.07%     
+ Complexity     13511    13498      -13     
=============================================
  Files           1380     1380              
  Lines         101091   101116      +25     
  Branches       13101    13105       +4     
=============================================
- Hits           48844    48795      -49     
- Misses         46285    46337      +52     
- Partials        5962     5984      +22     

☔ 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.

[fuyou001]

Suggestion: consider changing TopicRouteWrapper.getMasterAddr to return Optional<String> instead of a nullable String. There are only three direct call sites, and Optional would force each one to handle a missing broker explicitly.

• In the ordered-route branch of MessageQueueSelector.buildWrite, use an empty result to log and skip the malformed/unknown broker entry.
• In the normal QueueData write branch, use an empty result to skip entries without a master address.
• In ClusterTopicRouteService.getBrokerAddr, convert an empty result into the existing explicit broker-not-found exception rather than returning null.

The third point is important because ClusterTopicRouteService.getBrokerAddr has many indirect callers. Returning null can bypass the exception handling in resolveBrokerAddr and resolveBrokerAddrInReceiptHandle, allowing a null address to reach the remoting client instead of producing INVALID_BROKER_NAME or INVALID_RECEIPT_HANDLE.

Please also add regression tests asserting that an unknown broker produces Optional.empty(), while ClusterTopicRouteService.getBrokerAddr still fails with the expected domain exception.

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

PR #10689: [ISSUE #10688] Skip malformed ordered route items
Re-review scope: New commits 31dd11b ("Make missing master broker address explicit") and 9c1a41e ("Trigger CI rerun")

Changes Since Last Review

• TopicRouteWrapper.getMasterAddr() now returns Optional<String> instead of raw String — this is a stronger API contract that makes the "broker not found" case explicit at the call site
• ClusterTopicRouteService.getBrokerAddr() updated to use .orElseThrow() with a descriptive ProxyException
• Other callers updated to handle the Optional return properly

Assessment

Correctness — The Optional<String> return type is a clean improvement over the previous null-returning approach. Callers are forced to handle the missing-broker case explicitly.
Compatibility — TopicRouteWrapper.getMasterAddr() is an internal API (proxy module), so the signature change is contained.
Tests — Existing tests cover the route selection logic; the Optional migration is straightforward.

Overall: LGTM — Good evolution of the null-safety pattern.

---

Automated re-review by github-manager-bot

[fuyou001]

The validation direction looks useful, but I think a few compatibility and resource-safety cases should be addressed before merging.

1. TopicRouteWrapper.getMasterAddr changes its public return type from String to Optional<String>. This is source- and binary-incompatible for external extensions or code compiled against the proxy module. Please consider retaining the existing signature and adding a separate safe lookup method, unless this type is explicitly guaranteed not to be an extension API.
2. The queue count is only checked for integer syntax. A negative value silently produces no queues, while a very large positive value can allocate a large number of AddressableMessageQueue objects during route refresh. Please require a positive count and enforce a reasonable upper bound based on RocketMQ queue limits.
3. Partially accepting a damaged ordered-topic configuration changes the ordered queue set. Please confirm that partial acceptance is the desired consistency policy; otherwise, rejecting the whole new configuration and retaining the last valid route would avoid different nodes constructing different queue views during mixed-version operation.

Please add regression coverage for zero, negative, Integer.MAX_VALUE, overflow, all-items-invalid, and missing-master cases; route refresh/cache recovery with a previously valid route; and mixed old/new proxy behavior. Please also assert how the new ProxyException from ClusterTopicRouteService.getBrokerAddr is mapped and propagated through ConsumerProcessor, ClusterMessageService, buildAddressableMessageQueue, and the background fault-strategy callback, including exceptional future completion.

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

PR #10689: [ISSUE #10688] Skip malformed ordered route items
Re-review trigger: New maintainer feedback from @fuyou001 (2026-07-30)

Maintainer Feedback Summary

@fuyou001 raised three concerns:

1. [Compatibility] TopicRouteWrapper.getMasterAddr changes its public return type from String to Optional<String>. This is source- and binary-incompatible for external extensions compiled against the proxy module.

2. [Correctness/Resource Safety] Queue count is only checked for integer syntax. A negative value silently produces no queues; a very large positive value can allocate many AddressableMessageQueue objects during route refresh.

3. [Consistency] Partially accepting a damaged ordered-topic configuration changes the ordered queue set. Confirm whether partial acceptance is the desired policy; otherwise rejecting the whole configuration and retaining the last valid route would prevent different nodes from constructing different queue views during mixed-version operation.

Assessment

All three concerns are valid:

1. Binary compatibility: Changing String → Optional<String> breaks any external code compiled against the old signature. Unless TopicRouteWrapper is explicitly internal, consider retaining the existing method and adding a new Optional<String> getOptionalMasterAddr(...) or similar.

2. Queue count bounds: The parseQueueCount method validates integer syntax but not range. A negative count or Integer.MAX_VALUE could cause silent data loss or OOM during route refresh. Suggest enforcing 1 <= count <= reasonable_upper_bound (e.g., 512 or whatever RocketMQ's documented queue limit is).

3. Partial acceptance policy: This is a design decision that the author should explicitly confirm. If partial acceptance is intentional, document it. If not, reject the entire configuration on any validation failure.

Additional Note

The ClusterTopicRouteService.getBrokerAddr change to throw ProxyException on missing broker is a positive improvement — it prevents null addresses from reaching the remoting client. However, as @fuyou001 noted, please verify that the exception is correctly mapped through all callers (ConsumerProcessor, ClusterMessageService, buildAddressableMessageQueue, background fault-strategy callback).

Status

No new commits since the maintainer feedback. Awaiting author response.

---

Automated review by github-manager-bot

[Aias00]

Addressed the latest feedback in commit 77c9853:\n\n- Restored TopicRouteWrapper.getMasterAddr(String) with its original nullable String signature to preserve source/binary compatibility. Added getOptionalMasterAddr(String) for the new explicit missing-broker handling.\n- Updated the new call sites to use getOptionalMasterAddr while keeping existing callers compatible.\n- Added ordered-topic queue count validation: only 1..1024 is accepted. This covers zero, negative, Integer.MAX_VALUE, and integer overflow values without allocating large queue lists.\n- Added regression coverage for zero, negative, max-int, overflow, all-items-invalid, and missing-master ordered route cases.\n\nFor the consistency policy: this PR intentionally keeps the existing defensive behavior of building a view from valid route fragments and skipping malformed fragments. Fully rejecting a refreshed ordered-topic route and retaining the last valid view would require a cache-level policy change in TopicRouteService, which is broader than this small parsing/validation fix and probably should be discussed separately.\n\nLocal verification passed:\nmvn -pl proxy -Dtest=MessageQueueSelectorTest,ClusterTopicRouteServiceTest -DfailIfNoTests=false test

[Aias00]

Added one more regression commit for the exceptional future propagation part of the review feedback.

New coverage in ClusterMessageServiceTest asserts that broker lookup failures complete returned futures exceptionally for both paths:

• ClusterMessageService.request(...)
• ClusterMessageService.requestOneway(...)

Both tests verify the propagated ProxyExceptionCode.INVALID_BROKER_NAME, so the invalid broker-name path does not proceed to remoting with a missing address.

Local verification with JDK 8:
JAVA_HOME=$(/usr/libexec/java_home -v 1.8) mvn -pl proxy -Dtest=MessageQueueSelectorTest,ClusterTopicRouteServiceTest,ClusterMessageServiceTest -DfailIfNoTests=false test

Result: Tests run: 14, Failures: 0, Errors: 0, Skipped: 0; checkstyle and spotbugs also passed in the same run.

[RockteMQ-AI]

Review by github-manager-bot (Re-review after new commits)

PR #10689: [ISSUE #10688] Skip malformed ordered route items
Re-review trigger: New commits pushed on Jul 31 (after previous review on Jul 31 00:37)

Previous Concerns Resolution

The previous review raised three concerns:

1. Binary Compatibility (getMasterAddr → getOptionalMasterAddr)

✅ Resolved. getMasterAddr(String) is preserved and delegates to getOptionalMasterAddr. The new getOptionalMasterAddr returns Optional<String>. This is backward compatible — existing callers of getMasterAddr are unaffected.

2. Queue Count Bounds Validation

✅ Resolved. parseOrderTopicQueueCount now validates range [1, MAX_ORDER_TOPIC_WRITE_QUEUE_COUNT] where MAX_ORDER_TOPIC_WRITE_QUEUE_COUNT = 1024. Invalid values (0, negative, >1024, overflow) are rejected with a warning log.

3. Partial Acceptance Policy

✅ Resolved. Invalid ordered route items are skipped with log.warn and continue. Valid items are processed normally. This is the correct approach — malformed fragments no longer crash the entire route building.

Code Quality

• API design: Clean separation — old API preserved, new Optional-based API added ✓
• Error handling: ClusterTopicRouteService.getBrokerAddr throws ProxyException(INVALID_BROKER_NAME) for unknown brokers ✓
• Bounds validation: Queue count range is reasonable (1-1024) ✓
• Logging: Warning logs for skipped items aid diagnostics ✓
• Tests: Comprehensive coverage including malformed fragments, invalid queue counts, and Integer overflow ✓

Verdict

Approve. All three previous concerns are fully addressed with clean, backward-compatible changes.