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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
