zjncs opened a new pull request, #5775:
URL: https://github.com/apache/rocketmq-dashboard/pull/5775
Closes #5605
Re-submission of #5606 (closed for targeting the stale `master` branch — a
maintainer-closed PR cannot be reopened by the author). All four review points
from @lizhimins are addressed here: test methods renamed to end with `Test`,
trailing newlines restored, `@Configuration`/`@Bean` moved to imports, and the
credential-exposure framing dropped — `DataSourceVO` carries no credential
fields, so this PR now argues the unbounded-heap case only. Rebased onto
`rocketmq-studio` (c99b9ad5).
## Problem
The paginated `listDataSources(search, type, page, pageSize)` overload
carried the same `@Cacheable(DATA_SOURCE_CACHE)` as the full-list method,
contradicting the comment that documents the cache as a cache of the **full
list only**. Consequences:
- the Spring cache key is `SimpleKey(search, type, page, pageSize)` —
`search`/`type` are arbitrary caller strings on `GET
/api/settings/datasources/page`, which is **not** admin-only, so any
authenticated reader can drive it
- the configured `ConcurrentMapCacheManager` has no TTL, size bound, or
eviction (entries only clear on data-source create/update/delete)
- every distinct search term permanently pins one `PageResult<DataSourceVO>`
in the heap
## Fix
Remove the annotation from the parameterized overload only (the full-list
method keeps it, and the write-path `@CacheEvict(allEntries=true)` methods
continue to maintain it); leave a comment stating why the paged overload is
intentionally uncached.
## Verification (on rocketmq-studio, docker maven 3.9 / temurin 21)
- New `SettingsServiceDataSourceCacheBoundTest` boots an
`AnnotationConfigApplicationContext` registering the **production**
`CacheConfig`:
- control `fullDataSourceListIsCachedAsDocumentedTest` — the full list is
served once and cached (proves the harness is live)
-
`parameterizedDataSourceSearchMustNotAccumulatePermanentCacheEntriesTest` —
pins the contract; **fails with the fix reverted** (50 permanent entries after
50 distinct searches: `Expecting empty but was: {SimpleKey [search-1, null, 1,
20]=PageResult@…, …}`), passes with this change
- Regression: sibling `SettingsServiceCachingTest` 1/1 green
- Mutation check: restoring only the `@Cacheable` on the parameterised
overload makes the new test fail (`Tests run: 2, Failures: 1`); removing it
again passes (3/3 green)
## Collision note
`SettingsService.java` is touched by #5484 (open, hunks ~@160-170); this
change is confined to the parameterised `listDataSources` overload (~@223-236),
disjoint. The new test file and `CacheConfig.java` are untouched by any open PR.
--
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]
