feat: Configuration cleanup - #2164
Open
cstamas wants to merge 1 commit into
Open
Conversation
New shared holders (each key now read once): - WebUiProperties (ovsx.webui.url, ovsx.webui.frontendRoutes) — now the single source, reused by WebConfig, SecurityConfig, LocalVSCodeService, LocalRegistryService, SitemapService, ServerErrorController. - MigrationsProperties (ovsx.migrations.delay.seconds, ovsx.registry.version) — new, reused by VSCodeIdService, ExtensionControlService, ScheduleMigrationsListener, LocalRegistryService. - MirrorConfig gained isEnabled() (ovsx.data.mirror.enabled) — reused by VSCodeIdService, ExtensionControlService, AccessTokenConfig, MigrationScheduler, RestTemplateConfig, instead of each redeclaring the same @value. Reused existing classes instead of new ones (ladder rung 2 — no new class needed): - ExtensionVersionIntegrityService gained getKeyPairMode(); GenerateKeyPairJobRequestHandler now injects it instead of its own ovsx.integrity.key-pair read. - AzureBlobStorageService gained getServiceEndpoint()/getBlobContainer(); AzureDownloadCountHandler now injects it instead of its own ovsx.storage.azure.* reads. Left alone: the 8 ovsx.caching.*.ttl keys still show up twice each — but both reads are inside CacheConfig itself (JCache vs. Redis cache-manager beans, mutually exclusive via @ConditionalOnProperty), so it's already one class, not real duplication.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new WebUiProperties/MigrationsProperties are currently unsafe to instantiate outside Spring (null defaults), which can cause NPEs in existing URL-building code paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR centralizes several repeatedly-read configuration keys into shared holders and updates services/configuration to consume those shared objects, reducing scattered @Value usage and aligning dependent components on a single source of truth.
Changes:
- Introduces shared config holders (
WebUiProperties,MigrationsProperties) and wires them into web/security/migration/registry components. - Routes mirror-mode checks through
MirrorConfig.isEnabled()and removes duplicated@Valuereads across multiple services. - Updates affected tests to provide/import the newly required beans/constructor parameters.
File summaries
| File | Description |
|---|---|
| server/src/main/java/org/eclipse/openvsx/web/WebUiProperties.java | New shared holder for Web UI URL and frontend routes. |
| server/src/main/java/org/eclipse/openvsx/web/WebConfig.java | Uses WebUiProperties for CORS and frontend route forwarding. |
| server/src/main/java/org/eclipse/openvsx/web/SitemapService.java | Uses WebUiProperties for base URL resolution. |
| server/src/main/java/org/eclipse/openvsx/web/ServerErrorController.java | Redirect builds from WebUiProperties rather than local @Value. |
| server/src/main/java/org/eclipse/openvsx/security/SecurityConfig.java | Uses WebUiProperties for frontend route matching and login redirect. |
| server/src/main/java/org/eclipse/openvsx/migration/MigrationsProperties.java | New shared holder for migration delay and registry version. |
| server/src/main/java/org/eclipse/openvsx/migration/ScheduleMigrationsListener.java | Uses MigrationsProperties for delay/once-per-version behavior. |
| server/src/main/java/org/eclipse/openvsx/migration/MigrationScheduler.java | Uses MirrorConfig.isEnabled() for mirror-mode gating. |
| server/src/main/java/org/eclipse/openvsx/migration/GenerateKeyPairJobRequestHandler.java | Reads keypair mode from ExtensionVersionIntegrityService. |
| server/src/main/java/org/eclipse/openvsx/mirror/MirrorConfig.java | Adds enabled flag and isEnabled() accessor. |
| server/src/main/java/org/eclipse/openvsx/extension_control/ExtensionControlService.java | Replaces duplicated @Value reads with MirrorConfig + MigrationsProperties. |
| server/src/main/java/org/eclipse/openvsx/adapter/VSCodeIdService.java | Replaces duplicated @Value reads with MirrorConfig + MigrationsProperties. |
| server/src/main/java/org/eclipse/openvsx/RestTemplateConfig.java | Switches REST template selection to MirrorConfig.isEnabled(). |
| server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java | Uses MirrorConfig.isEnabled() in validation; constructor-injects mirror config. |
| server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java | Uses MigrationsProperties and WebUiProperties instead of local @Value fields. |
| server/src/main/java/org/eclipse/openvsx/adapter/LocalVSCodeService.java | Uses WebUiProperties for item URL generation. |
| server/src/main/java/org/eclipse/openvsx/storage/AzureBlobStorageService.java | Exposes endpoint/container accessors for reuse. |
| server/src/main/java/org/eclipse/openvsx/storage/log/AzureDownloadCountHandler.java | Injects AzureBlobStorageService instead of duplicating azure storage config reads. |
| server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java | Adds getKeyPairMode() accessor for reuse. |
| server/src/test/java/org/eclipse/openvsx/web/SitemapControllerTest.java | Imports WebUiProperties and updates SitemapService construction. |
| server/src/test/java/org/eclipse/openvsx/web/ServerErrorControllerTest.java | Updates controller construction to pass WebUiProperties. |
| server/src/test/java/org/eclipse/openvsx/UserAPITest.java | Imports WebUiProperties; adapts test wiring to new constructors. |
| server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java | Imports WebUiProperties; adapts test wiring to new constructors. |
| server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java | Imports WebUiProperties; adapts test wiring to new constructors. |
| server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java | Updates LocalRegistryService construction for new dependencies. |
| server/src/test/java/org/eclipse/openvsx/migration/MigrationSchedulerTest.java | Adapts scheduler construction to pass MirrorConfig. |
| server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java | Imports WebUiProperties; adapts LocalVSCodeService wiring. |
| server/src/test/java/org/eclipse/openvsx/adapter/LocalVSCodeServiceTest.java | Imports WebUiProperties; adapts LocalVSCodeService wiring. |
| server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java | Imports WebUiProperties for slice-test context completeness. |
| server/src/test/java/org/eclipse/openvsx/admin/ScanAPITest.java | Imports WebUiProperties for slice-test context completeness. |
| server/src/test/java/org/eclipse/openvsx/admin/FileDecisionAPITest.java | Imports WebUiProperties for slice-test context completeness. |
| server/src/test/java/org/eclipse/openvsx/admin/ConsistencyAPITest.java | Imports WebUiProperties for slice-test context completeness. |
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+26
to
+30
| @Value("${ovsx.migrations.delay.seconds:0}") | ||
| private long delaySeconds; | ||
|
|
||
| @Value("${ovsx.registry.version:}") | ||
| private String registryVersion; |
Comment on lines
+25
to
+31
| @Value("${ovsx.webui.url:}") | ||
| private String url; | ||
|
|
||
| @Value( | ||
| "${ovsx.webui.frontendRoutes:/extension/**,/namespace/**,/search,/user-settings/**,/publish,/admin-dashboard/**}" | ||
| ) | ||
| private String[] frontendRoutes; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New shared holders (each key now read once):
Reused existing classes instead of new ones (ladder rung 2 — no new class needed):
Left alone: the 8 ovsx.caching.*.ttl keys still show up twice each — but both reads are inside CacheConfig itself (JCache vs. Redis cache-manager beans, mutually exclusive via @ConditionalOnProperty), so it's already one class, not real duplication.