Skip to content

fix(services): tolerate paged results without items in export walks - #2938

Open
yyqdbngt wants to merge 1 commit into
apache:rocketmq-studiofrom
yyqdbngt:codex/yy-listall-pages
Open

fix(services): tolerate paged results without items in export walks#2938
yyqdbngt wants to merge 1 commit into
apache:rocketmq-studiofrom
yyqdbngt:codex/yy-listall-pages

Conversation

@yyqdbngt

@yyqdbngt yyqdbngt commented Sep 1, 2026

Copy link
Copy Markdown

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

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

7 finding(s) to address.

Findings

  • [CRITICAL] web/src/services/consumerService.test.ts:18afterEach 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.
  • [WARNING] web/src/services/consumerService.ts:145 — 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.
  • [WARNING] web/src/services/topicService.ts:102 — 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.
  • [WARNING] web/src/api/settings.ts:136 — 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.
  • [WARNING] web/src/api/studioUsers.ts:70 — 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.
  • [INFO] web/src/services/topicService.ts:100 — 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.
  • [INFO] web/src/services/topicService.export.test.ts:1 — 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.

Automated review by github-manager-bot

*/

import { describe, expect, it, vi } from 'vitest';
import { beforeEach, describe, expect, it, vi } from 'vitest';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// empty page instead of crashing the whole export walk.
const items = Array.isArray(result?.items) ? result.items : [];
groups.push(...items);
const total =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// empty page instead of crashing the whole export walk.
const items = Array.isArray(result?.items) ? result.items : [];
topics.push(...items);
const total =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread web/src/api/settings.ts
// empty page instead of crashing the whole export walk.
const items = Array.isArray(result?.items) ? result.items : [];
allDataSources.push(...items);
const total =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// empty page instead of crashing the whole export walk.
const items = Array.isArray(result?.items) ? result.items : [];
allUsers.push(...items);
const total =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 : [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,55 @@
import { afterEach, describe, expect, it, vi } from 'vitest';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants