lizhimins commented on PR #5606: URL: https://github.com/apache/rocketmq-dashboard/pull/5606#issuecomment-6076051068
The finding is correct and well-argued: SettingsService.java:211-222 documents the cache as full-list-only, CacheConfig.java:50-51 returns a bare ConcurrentMapCacheManager with no TTL or bound, and GET /api/settings/datasources/page is not in AuthInterceptor.isAdminOnlyGetPath (:174-183), so any authenticated reader can pin one PageResult per distinct search term. The regression counts you cite (43/1/23/5) all check out, and SettingsServiceCachingTest genuinely stays green because it calls each key once. Two things block it as submitted. First, both new @Test methods must end with "Test" (fullDataSourceListIsCachedAsDocumentedTest / parameterizedDataSourceSearchMustNotAccumulatePermanentCacheEntriesTest) - the repo convention applies in full to new files, and the sibling SettingsServiceCachingTest already follows it. Second, both files lost their trailing newline. Also please correct the severity claim: the cached type DataSourceVO has no authType/username/password/bearerToken fields (see DataSourceVO.java:31-42 - key, name, type, url, auth, status, instanceIds; bearerToken only exists on DataSourceTestDTO:40, which is never cached). The unbounded-heap argument stands on its own; the credential-exposure framing does not. Minor: use imports for @Configuration/@Bean instead of fully-qualified inline annotations, as SettingsServiceCachingTest does. And this PR targets master, which is 60 commits behind the rocketmq-studio integration branch - please retarget. -- 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]
