You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "yyqdbngt (via GitHub)" <gi...@apache.org> on 2026/09/01 21:41:06 UTC
[PR] fix(services): tolerate paged results without items in export walks [rocketmq-dashboard]
yyqdbngt opened a new pull request, #2938:
URL: https://github.com/apache/rocketmq-dashboard/pull/2938
## Summary
- `topicService.listAllTopics`, `consumerService.listAllConsumerGroups`, `settings.listAllDataSources`, and `studioUsers.listAllStudioUsers` now treat a success envelope whose `data` is `null` or lacks `items` as an empty page instead of crashing
- `total` is only trusted when it is a finite number
- Adds regression tests for every export walk (null payload, item-less payload, and the multi-page happy path)
## Why
All four export flows walk the list in pages with `page <= MAX` and spread `result.items` into an accumulator. The client interceptor validates the business `code`, but a `code: 0` envelope can still carry `data: null` or a page object without `items` — in that case `items` is `undefined`, `push(...undefined)` throws a `TypeError`, and the entire export (topics/groups CSV, data-source export, studio-user export) dies on a single malformed page.
## Testing
- `./node_modules/.bin/vitest run src/services/topicService.export.test.ts src/services/consumerService.test.ts src/api/settings.test.ts src/api/studioUsers.test.ts src/services/topicService.test.ts` → 37 passed (8 new; the 6 tolerance tests fail with the pre-fix code, the 2 happy-path tests pass both before and after)
- `./node_modules/.bin/tsc --noEmit` → clean
- `./node_modules/.bin/eslint` on the 8 changed files → 0 errors
--
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(services): tolerate paged results without items in export walks [rocketmq-dashboard]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
RockteMQ-AI commented on code in PR #2938:
URL: https://github.com/apache/rocketmq-dashboard/pull/2938#discussion_r3909139252
##########
web/src/services/consumerService.ts:
##########
@@ -138,9 +138,15 @@ export async function listAllConsumerGroups(
page,
pageSize: EXPORT_PAGE_SIZE,
});
- groups.push(...result.items);
- const total = result.total ?? groups.length;
- if (result.items.length === 0 || groups.length >= total) {
+ // A success envelope may still carry a null or item-less payload; treat it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ groups.push(...items);
+ const total =
Review Comment:
When `result.total` is not a finite number (e.g. `Infinity`, `NaN`, or a string) but `result.items` contains entries, `total` falls back to `groups.length`. The exit guard `groups.length >= total` then evaluates to `true` immediately, silently truncating the export after the first page. Consider logging a warning when `total` is discarded, or treating a missing/invalid `total` as a signal to continue until an empty page rather than stopping.
##########
web/src/services/consumerService.test.ts:
##########
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-import { describe, expect, it, vi } from 'vitest';
+import { beforeEach, describe, expect, it, vi } from 'vitest';
Review Comment:
`afterEach` is used on line 51 but is not imported from `vitest`. The import on line 18 adds `beforeEach` but omits `afterEach`. This will throw a `ReferenceError` at test time, causing the entire `describe` block to fail. Fix: add `afterEach` to the import list.
##########
web/src/api/settings.ts:
##########
@@ -129,9 +129,15 @@ export async function listAllDataSources(params: { search?: string; type?: strin
page,
pageSize: DATA_SOURCE_EXPORT_PAGE_SIZE,
});
- allDataSources.push(...result.items);
- const total = result.total ?? allDataSources.length;
- if (result.items.length === 0 || allDataSources.length >= total) {
+ // A success envelope may still carry a null or item-less payload; treat it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ allDataSources.push(...items);
+ const total =
Review Comment:
Same truncation risk: a non-finite `result.total` with valid items causes the walk to exit after one page because `total` collapses to `allDataSources.length`. Data-source exports would silently lose entries beyond the first page.
##########
web/src/services/topicService.ts:
##########
@@ -95,9 +95,15 @@ export const listAllTopics = async (params: TopicQuery = {}): Promise<Topic[]> =
while (page <= MAX_EXPORT_PAGES) {
const result = await listTopicsPage({ ...params, page, pageSize: EXPORT_PAGE_SIZE });
- topics.push(...result.items);
- const total = result.total ?? topics.length;
- if (result.items.length === 0 || topics.length >= total) return topics;
+ // A success envelope may still carry a null or item-less payload; treat it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ topics.push(...items);
+ const total =
Review Comment:
Same truncation risk as consumerService: if the server returns items but a non-finite `total`, the fallback `total = topics.length` causes immediate exit after one page. This silently drops data for large exports where the backend sends a malformed total. A defensive log or a 'continue until empty page' strategy would be safer.
##########
web/src/services/topicService.export.test.ts:
##########
@@ -0,0 +1,55 @@
+import { afterEach, describe, expect, it, vi } from 'vitest';
Review Comment:
This new test file is missing the Apache 2.0 license header that the surrounding test files (e.g. `consumerService.test.ts`) include. This may cause CI license-check failures.
##########
web/src/api/studioUsers.ts:
##########
@@ -63,9 +63,15 @@ export const listAllStudioUsers = async (
page,
pageSize: STUDIO_USER_EXPORT_PAGE_SIZE,
});
- allUsers.push(...result.items);
- const total = result.total ?? allUsers.length;
- if (result.items.length === 0 || allUsers.length >= total) return allUsers;
+ // A success envelope may still carry a null or item-less payload; treat it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
+ allUsers.push(...items);
+ const total =
Review Comment:
Same truncation risk: a non-finite `result.total` with valid items causes immediate exit. Studio-user exports would silently drop users beyond the first page.
##########
web/src/services/topicService.ts:
##########
@@ -95,9 +95,15 @@ export const listAllTopics = async (params: TopicQuery = {}): Promise<Topic[]> =
while (page <= MAX_EXPORT_PAGES) {
const result = await listTopicsPage({ ...params, page, pageSize: EXPORT_PAGE_SIZE });
- topics.push(...result.items);
- const total = result.total ?? topics.length;
- if (result.items.length === 0 || topics.length >= total) return topics;
+ // A success envelope may still carry a null or item-less payload; treat it as an
+ // empty page instead of crashing the whole export walk.
+ const items = Array.isArray(result?.items) ? result.items : [];
Review Comment:
The null-safe pagination logic (items fallback, total validation, exit guard) is duplicated identically across four files (`topicService.ts`, `consumerService.ts`, `settings.ts`, `studioUsers.ts`). Consider extracting a shared helper like `walkPages<T>(fetchPage, pageSize, maxPages)` to centralize the tolerance logic and avoid drift as the pattern evolves.
--
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(services): tolerate paged results without items in export walks [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #2938: fix(services): tolerate paged results without items in export walks
URL: https://github.com/apache/rocketmq-dashboard/pull/2938
--
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(services): tolerate paged results without items in export walks [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2938:
URL: https://github.com/apache/rocketmq-dashboard/pull/2938#issuecomment-5593751191
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.
--
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