[PR] Fix-resource [rocketmq-clients]

["zhaohai666 (via GitHub)" <gi...@apache.org>]

zhaohai666 opened a new pull request, #1119:
URL: https://github.com/apache/rocketmq-clients/pull/1119

   Fixed an issue where addTopics() was called twice in each loop, creating two different Resource objects.


-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix-resource [rocketmq-clients]

["zk-drizzle (via GitHub)" <gi...@apache.org>]

zk-drizzle commented on PR #1119:
URL: https://github.com/apache/rocketmq-clients/pull/1119#issuecomment-3483512791

   GDTM


-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix-resource [rocketmq-clients]

["RockteMQ-AI (via GitHub)" <gi...@apache.org>]

RockteMQ-AI commented on PR #1119:
URL: https://github.com/apache/rocketmq-clients/pull/1119#issuecomment-4707477738

   ## Review by github-manager-bot
   
   ### Summary
   This PR fixes a bug in the Node.js client where `PublishingSettings` incorrectly creates two separate topic entries per topic instead of chaining `setName()` and `setResourceNamespace()` on the same entry.
   
   ### Strengths
   - **Correct root cause fix**: The original code called `publishing.addTopics()` twice, which creates two distinct `Topic` entries in the protobuf repeated field. The fix chains both setters on a single `addTopics()` call, which is the correct protobuf builder pattern.
   - **Minimal change**: Only 1 file changed, +1 −2. The fix is surgical and does not introduce unrelated modifications.
   - **Clear commit message**: The commit message accurately describes the bug and fix.
   
   ### Issues
   
   #### 🔴 Must Fix
   _None found._
   
   #### 🟡 Should Fix
   _None found._
   
   #### 🟢 Suggestions
   1. **Test coverage**: Consider adding a unit test that verifies the number of topic entries in the published settings matches the expected count. This would prevent regression if someone accidentally reintroduces the double-`addTopics()` pattern.
      ```typescript
      // Example assertion
      const settings = publishingSettings.toProtobufObj();
      const topics = settings.getPublishing().getTopicsList();
      assert.strictEqual(topics.length, expectedTopicCount);
      ```
   
   2. **Similar pattern audit**: It may be worth auditing other `Settings` classes (e.g., `PushConsumerSettings`, `SimpleConsumerSettings`) for the same `addXxx()` called-twice pattern to ensure no similar bugs exist elsewhere.
   
   ### Assessment
   - [x] **Ready to merge**
   - Clean one-line fix for a real bug. The protobuf builder pattern mistake is easy to make and this PR corrects it properly.
   
   ---
   _This review was generated by github-manager-bot · 2026-06-15_


-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix-resource [rocketmq-clients]

["RongtongJin (via GitHub)" <gi...@apache.org>]

RongtongJin merged PR #1119:
URL: https://github.com/apache/rocketmq-clients/pull/1119


-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org