chore: let settings read their own value from the settings store by gnugomez · Pull Request #2339 · eclipse-openvsx/openvsx

gnugomez · 2026-10-15T11:33:28Z

When we introduced the per-setting beans in #2257 the idea was to have the settings open for extension and closed for modification, so adding a setting never touches SettingsService. We deprecated isReadOnly() on the service back then, but the max extension size that came in with #2287 ended up doing exactly the opposite and added getMaxExtensionSize() to the service, so we had one getter deprecated and another one alive.

This settles that the way the deprecation already pointed: a setting the server consults exposes its own accessor (ReadOnlySetting.isEnabled(), MaxExtensionSizeSetting.getValue()) over the cached snapshot, and callers inject the setting. The getters on the service are gone and the service keeps knowing nothing about individual settings, it still only takes the list.

SettingsCache is now SettingsStore: the store as this node sees it. It stays package-private, since every setting lives in the settings package. A setting's read/merge/validate/toRows still only see the rows they are handed, so a save reads the store exactly once. Tests outside the package use the key constants instead of spelling the keys.

We're also taking the json mapping out of the service. It returns SettingRows now and SettingsJsonMapper does the mapping for AdminAPI and SettingsAPI, so the DTOs don't name any setting codec anymore.

ReadOnlyEndpointAspect resolves the setting once through a SingletonSupplier instead of a bean lookup on every guarded request.

Fix: read-only mode did not cover trusted publishing

The aspect's class filter only admitted RegistryAPI, UserAPI and the *API classes of the admin package, so the three @MutatingOperation endpoints of TrustedPublishingAPI (create and delete a trusted publisher, request a publish token) kept working while the registry was read-only. The filter now admits every class named *API, and matchesEveryMutatingOperationInTheRegistry scans the classpath and fails for any @MutatingOperation the aspect does not guard. Added a changelog entry.

When we introduced the per-setting beans in #2257 the idea was to have the settings open for extension and closed for modification, so adding a setting never touches `SettingsService`. We deprecated `isReadOnly()` on the service back then, but the max extension size that came in with #2287 ended up doing exactly the opposite and added `getMaxExtensionSize()` to the service, so we had one getter deprecated and another one alive, and both settings reading the cache from the same bean that encodes and validates them. This settles that. A setting bean only knows how to encode and decode its rows, and a setting the server needs at runtime gets its own reader (`ReadOnlyMode`, `ExtensionSizeLimitService`) that asks the service for the current value through a package-private `current(setting)`. Everything else injects the reader, the setting beans, their keys and the cache are package-private now, and the getters on the service are gone. We're also taking the json mapping out of the service. It returns `SettingRows` now and `SettingsJsonMapper`, next to the beans since it's the only place that can see the keys, does the mapping for `AdminAPI` and `SettingsAPI`, so the DTOs don't name any setting codec anymore. Assisted-By: anthropic:claude-fable-5

Copilot

🟢 Approval recommended

Static review supports a behavior-preserving refactor with updated tests and no blocking defects.

1 open finding

Add coverage for ReadOnlyMode snapshot delegation · New

What changed in this PR

Refactors server settings access into dedicated readers, keeping SettingsService independent of individual settings and JSON payloads.

Changes:

Migrates runtime consumers to per-setting readers and hides setting implementations.
Moves JSON conversion into SettingsJsonMapper.
Updates affected tests and documents the settings architecture.

File	Description
server/src/test/java/org/eclipse/openvsx/settings/SettingsServiceTest.java	Tests row-based service operations.
server/src/test/java/org/eclipse/openvsx/settings/SettingsJsonMapperTest.java	Adds mapping coverage.
server/src/test/java/org/eclipse/openvsx/settings/SettingsAPITest.java	Exercises endpoint mapping.
server/src/test/java/org/eclipse/openvsx/settings/ReadOnlySettingTest.java	Removes obsolete accessor coverage.
server/src/test/java/org/eclipse/openvsx/settings/ReadOnlyEndpointAspectTest.java	Uses the reader provider.
server/src/test/java/org/eclipse/openvsx/settings/ExtensionSizeLimitServiceTest.java	Updates default-limit stubbing.
server/src/test/java/org/eclipse/openvsx/migration/MigrationItemJobRequestHandlerTest.java	Mocks the read-only reader.
server/src/test/java/org/eclipse/openvsx/json/SiteSettingsJsonTest.java	Keeps payload serialization coverage.
server/src/test/java/org/eclipse/openvsx/json/SettingsJsonTest.java	Moves mapping tests out.
server/src/test/java/org/eclipse/openvsx/extension_control/ExtensionControlJobRequestHandlerTest.java	Updates reader injection.
server/src/test/java/org/eclipse/openvsx/analytics/ingestion/DownloadIngestionRunnerTest.java	Updates read-only stubbing.
server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java	Uses row responses and real mapping.
server/src/main/java/org/eclipse/openvsx/settings/WritableSetting.java	Documents codec responsibilities.
server/src/main/java/org/eclipse/openvsx/settings/SettingsService.java	Exposes rows and generic setting reads.
server/src/main/java/org/eclipse/openvsx/settings/SettingsJsonMapper.java	Centralizes JSON conversion.
server/src/main/java/org/eclipse/openvsx/settings/SettingsCache.java	Makes the cache package-private.
server/src/main/java/org/eclipse/openvsx/settings/SettingsAPI.java	Maps published rows to JSON.
server/src/main/java/org/eclipse/openvsx/settings/ReadOnlySetting.java	Removes cache access and public visibility.
server/src/main/java/org/eclipse/openvsx/settings/ReadOnlyMode.java	Adds the runtime flag reader.
server/src/main/java/org/eclipse/openvsx/settings/ReadOnlyEndpointAspect.java	Resolves the reader lazily.
server/src/main/java/org/eclipse/openvsx/settings/package-info.java	Documents settings boundaries.
server/src/main/java/org/eclipse/openvsx/settings/MaxExtensionSizeSetting.java	Separates encoding from runtime access.
server/src/main/java/org/eclipse/openvsx/settings/ExtensionSizeLimitService.java	Reads the default through the service.
server/src/main/java/org/eclipse/openvsx/settings/BannerSetting.java	Hides implementation and keys.
server/src/main/java/org/eclipse/openvsx/ratelimit/jobs/CollectUsageStatsHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/ratelimit/jobs/CalculateDailyUsageStatsHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/publish/PublishingConfig.java	Updates a method name in documentation.
server/src/main/java/org/eclipse/openvsx/migration/MigrationItemJobRequestHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/json/SiteSettingsJson.java	Removes row conversion.
server/src/main/java/org/eclipse/openvsx/json/SettingsJson.java	Removes setting-specific mapping.
server/src/main/java/org/eclipse/openvsx/extension_control/ExtensionControlJobRequestHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/DownloadIngestionRunner.java	Migrates ingestion read-only checks.
server/src/main/java/org/eclipse/openvsx/admin/MonthlyAdminStatisticsJobRequestHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/admin/AdminAPI.java	Maps settings requests and responses.
server/src/main/java/org/eclipse/openvsx/adapter/VSCodeIdDailyUpdateJobRequestHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/accesstoken/UpgradePersonalAccessTokenHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/accesstoken/NotifyPersonalAccessTokenExpirationHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/accesstoken/LegacyPersonalAccessTokenExpirationHandler.java	Uses the read-only reader.
server/src/main/java/org/eclipse/openvsx/accesstoken/ExpirePersonalAccessTokensHandler.java	Uses the read-only reader.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

netomi · 2026-10-15T14:57:25Z

I am not convinced the separate reader components (ReadOnlyMode, and the same pattern for the max extension size) pay for themselves.

What the split gains: SettingsService stays generic, callers inject a narrow type instead of the whole service, and the setting beans, their keys and the cache become package-private.

What it costs:

Two classes per runtime setting: one that encodes the value (ReadOnlySetting) and one that reads it (ReadOnlyMode).
It is not really open for extension: current() is package-private, so every new runtime setting still needs a reader class inside org.eclipse.openvsx.settings. The edit moves from SettingsService to a new file next to it.
Knock-on effects: tests hardcode "read-only" and "max-extension-size" because the keys are no longer visible, ExtensionSizeLimitService has a public constructor taking a package-private type, and the javadoc grows to explain the layering.
All this for two settings that the server reads at runtime.

Suggestion: let the setting bean read its own value, as ReadOnlySetting.isEnabled() did before this change, and do the same for MaxExtensionSizeSetting. That is one class per setting, callers still inject a narrow type, SettingsService still knows nothing about individual settings, and no extra component is needed. The objection that a codec should not read the store seems like purity without a practical payoff for a key-value setting.

Plain getters on SettingsService would also be acceptable with only two runtime settings, but that reverses the deprecation from #2257.

Minor, on the hot path: ReadOnlyEndpointAspect now calls ObjectProvider.getObject() on every guarded request, which repeats the bean-factory lookup each time, whereas the previous @Lazy proxy cached its target. Resolving it once (e.g. SingletonSupplier.of(readOnly::getObject)) keeps the deferred lookup without the per-call cost.

A setting the server consults exposes its own accessor over the cached snapshot, so the reader beans go, SettingsCache becomes the public SettingsStore, and each consulted setting moves into a package of its own.

The previous commit captured only the file moves. This is the change itself: ReadOnlySetting.isEnabled() and MaxExtensionSizeSetting.getValue() over the public SettingsStore, the reader beans and SettingsService.current removed, callers injecting the setting, and the tests using the key constants.

gnugomez · 2026-10-15T19:28:18Z

I am not convinced the separate reader components (ReadOnlyMode, and the same pattern for the max extension size) pay for themselves.

What the split gains: SettingsService stays generic, callers inject a narrow type instead of the whole service, and the setting beans, their keys and the cache become package-private.

What it costs:
* Two classes per runtime setting: one that encodes the value (`ReadOnlySetting`) and one that reads it (`ReadOnlyMode`).

* It is not really open for extension: `current()` is package-private, so every new runtime setting still needs a reader class inside `org.eclipse.openvsx.settings`. The edit moves from `SettingsService` to a new file next to it.

* Knock-on effects: tests hardcode `"read-only"` and `"max-extension-size"` because the keys are no longer visible, `ExtensionSizeLimitService` has a public constructor taking a package-private type, and the javadoc grows to explain the layering.

* All this for two settings that the server reads at runtime.
Suggestion: let the setting bean read its own value, as ReadOnlySetting.isEnabled() did before this change, and do the same for MaxExtensionSizeSetting. That is one class per setting, callers still inject a narrow type, SettingsService still knows nothing about individual settings, and no extra component is needed. The objection that a codec should not read the store seems like purity without a practical payoff for a key-value setting.

Plain getters on SettingsService would also be acceptable with only two runtime settings, but that reverses the deprecation from #2257.

Minor, on the hot path: ReadOnlyEndpointAspect now calls ObjectProvider.getObject() on every guarded request, which repeats the bean-factory lookup each time, whereas the previous @Lazy proxy cached its target. Resolving it once (e.g. SingletonSupplier.of(readOnly::getObject)) keeps the deferred lookup without the per-call cost.

@netomi fair enough, I agree the readers were not paying for themselves. The real problem was the package-private current(): it would have stopped a setting the server consults from being defined outside the package, which is something we will need from a deployment. The part I cared about was not growing getters on the service, and your suggestion keeps that, so I went with it.

A setting the server consults now reads its own value over the cached snapshot (ReadOnlySetting.isEnabled(), MaxExtensionSizeSetting.getValue()), the readers are gone and callers inject the setting. SettingsCache is now SettingsStore and public, since it is what a setting injects; read/merge/validate/toRows still only see the rows they are handed. Since the consulted settings are public anyway, each got its own package (settings.readonly, settings.extensionsize), the banner has no accessor so it stays package-private. Tests use the key constants, the accessor test is back, and the aspect resolves the setting once through a SingletonSupplier.

#2340 follows the same shape with DownloadAnalyticsSetting.isVisible().

netomi

This looks much better ty.

The sub packages do not really make sense imho, we can introduce that when we have a lot of settings, right now this feels like an overkill, also its inconsistent as some settings do not have a subpackage.

I will revert that and update the readonly aspect, it misses some API classes afaict.

Move the settings out of the readonly and extensionsize subpackages back into settings, which shrinks the change and lets SettingsStore stay package-private. ReadOnlyEndpointAspect only matched RegistryAPI, UserAPI and the admin package, so the TrustedPublishingAPI mutations ran in read-only mode. Match every class named *API instead, and add a test that every @MutatingOperation is guarded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gnugomez requested review from cstamas and netomi October 15, 2026 11:33

gnugomez force-pushed the gnugomez/main/settings-reader-beans branch from 39f57c5 to 4f67eae Compare October 15, 2026 12:50

gnugomez requested a balanced review from Copilot October 15, 2026 12:52

Copilot started reviewing on behalf of gnugomez October 15, 2026 12:53

View session

Copilot AI reviewed Oct 15, 2026

View reviewed changes

Comment thread server/src/main/java/org/eclipse/openvsx/settings/ReadOnlyModeService.java Resolved

gnugomez added 2 commits October 15, 2026 17:27

chore: rename ReadOnlyMode to ReadOnlyModeService

e4f3c04

chore: update ReadOnlyMode references to ReadOnlyModeService

b2bfe39

gnugomez added this pull request to stack #2341 October 15, 2026 18:48

gnugomez added 2 commits October 15, 2026 21:16

chore: let a setting read its own value from the settings store

403259c

A setting the server consults exposes its own accessor over the cached snapshot, so the reader beans go, SettingsCache becomes the public SettingsStore, and each consulted setting moves into a package of its own.

netomi previously approved these changes Oct 16, 2026

View reviewed changes

netomi dismissed their stale review via d9b08cb October 16, 2026 06:48

docs: add a changelog entry for the read-only trusted publishing fix

ff5ab8c

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

netomi changed the title ~~chore: read site settings through per-setting reader beans~~ chore: let settings read their own value from the settings store Oct 16, 2026

netomi merged commit 72b6c54 into main Oct 16, 2026
5 checks passed

netomi deleted the gnugomez/main/settings-reader-beans branch October 16, 2026 07:02

Navigation Menu

Repository navigation

chore: let settings read their own value from the settings store - #2339

chore: let settings read their own value from the settings store#2339
netomi merged 7 commits into
mainfrom
gnugomez/main/settings-reader-beans

gnugomez commented Oct 15, 2026 •

edited by netomi

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

netomi commented Oct 15, 2026

Uh oh!

gnugomez commented Oct 15, 2026

Uh oh!

netomi left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gnugomez commented Oct 15, 2026 • edited by netomi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix: read-only mode did not cover trusted publishing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

netomi commented Oct 15, 2026

Uh oh!

gnugomez commented Oct 15, 2026

Uh oh!

netomi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gnugomez commented Oct 15, 2026 •

edited by netomi

Loading