Skip to content

refactor: migrate @ConfigurationProperties from @Component to @EnableConfigurationProperties#264

Merged
devondragon merged 2 commits intomainfrom
issue-263-config-properties-refactor
Feb 22, 2026
Merged

refactor: migrate @ConfigurationProperties from @Component to @EnableConfigurationProperties#264
devondragon merged 2 commits intomainfrom
issue-263-config-properties-refactor

Conversation

@devondragon
Copy link
Owner

Summary

  • Remove @Component and @PropertySource from DevLoginConfigProperties and WebAuthnConfigProperties so they are passive data holders per Spring Boot convention
  • Add DevLoginAutoConfiguration with @Profile("local") + @ConditionalOnProperty guards and @EnableConfigurationProperties to register DevLoginConfigProperties
  • Add WebAuthnAutoConfiguration with @EnableConfigurationProperties to register WebAuthnConfigProperties (always active since WebSecurityConfig requires it)

Closes #263

Test plan

  • ./gradlew test — all existing tests pass with no changes needed
  • DevLoginIntegrationTest — beans register with local profile + enabled property
  • DevLoginDisabledTest — beans don't register when disabled
  • DevLoginControllerTest — unit test unaffected
  • WebAuthnFeatureEnabledIntegrationTest — WebAuthn beans register when enabled
  • WebAuthnFeatureDisabledIntegrationTest — WebAuthn beans don't register when disabled

… @EnableConfigurationProperties

Remove @component and @propertysource from DevLoginConfigProperties and
WebAuthnConfigProperties so they are passive data holders per Spring Boot
convention. Registration is now handled by dedicated @configuration
classes with @EnableConfigurationProperties:

- DevLoginAutoConfiguration: guards with @Profile("local") and
  @ConditionalOnProperty, owns @propertysource
- WebAuthnAutoConfiguration: always active (WebSecurityConfig requires
  the properties), owns @propertysource

Closes #263
Copilot AI review requested due to automatic review settings February 22, 2026 17:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors two @ConfigurationProperties classes to follow Spring Boot conventions by removing @Component from the properties classes and introducing dedicated auto-configuration classes with @EnableConfigurationProperties.

Changes:

  • DevLoginConfigProperties and WebAuthnConfigProperties are now passive data holders without lifecycle annotations
  • New DevLoginAutoConfiguration and WebAuthnAutoConfiguration classes register the properties and own the activation logic
  • Guards (@Profile, @ConditionalOnProperty) and property source declarations were moved from properties classes to configuration classes

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
WebAuthnConfigProperties.java Removed @Component and @PropertySource annotations to make it a passive data holder
WebAuthnAutoConfiguration.java New configuration class that registers WebAuthnConfigProperties via @EnableConfigurationProperties and declares the property source
DevLoginConfigProperties.java Removed @Component, @PropertySource, @Profile, and @ConditionalOnProperty annotations
DevLoginAutoConfiguration.java New configuration class that registers DevLoginConfigProperties via @EnableConfigurationProperties, owns the activation guards (@Profile, @ConditionalOnProperty), and declares the property source

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@claude
Copy link

claude bot commented Feb 22, 2026

PR Review: Migrate @ConfigurationProperties from @Component to @EnableConfigurationProperties

Summary: This is a sound, well-motivated refactor. Moving @ConfigurationProperties binding off @Component and onto dedicated @EnableConfigurationProperties configuration classes follows the Spring Boot recommendation (Spring docs) and makes the intent clearer. The overall direction is correct.


Issues

1. @PropertySource on a conditional class is effectively dead code

DevLoginAutoConfiguration carries @PropertySource("classpath:config/dsspringuserconfig.properties"), but the class is only loaded when:

  • The local Spring profile is active and
  • user.dev.auto-login-enabled=true is already in the Environment

This creates a subtle but real problem: the @ConditionalOnProperty check is evaluated before @PropertySource annotations on the same class are processed. If a consumer relies solely on the default value in dsspringuserconfig.properties (which is false) to resolve the condition, it never will — they must set the property explicitly in their own application.yml. That's correct behavior, but it means the @PropertySource on this conditional class cannot meaningfully influence its own condition evaluation and is therefore redundant.

WebAuthnAutoConfiguration is unconditional and loads the same file, so defaults are always available via that path. The @PropertySource on DevLoginAutoConfiguration can be removed without any change in behavior.

// DevLoginAutoConfiguration.java — @PropertySource here is redundant and misleading
@Configuration
@Profile("local")
@ConditionalOnProperty(name = "user.dev.auto-login-enabled", havingValue = "true", matchIfMissing = false)
@PropertySource("classpath:config/dsspringuserconfig.properties")  // <-- remove this
@EnableConfigurationProperties(DevLoginConfigProperties.class)
public class DevLoginAutoConfiguration {}

2. Naming: *AutoConfiguration implies Spring Boot SPI; these are component-scanned beans

UserConfiguration does @ComponentScan(basePackages = "com.digitalsanctuary.spring.user"), so DevLoginAutoConfiguration and WebAuthnAutoConfiguration are registered via component scan — not via META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports. The AutoConfiguration suffix conventionally implies the SPI loading mechanism. Consumers or future contributors may incorrectly expect these to honor Spring Boot's deferred auto-configuration ordering (e.g., @AutoConfigureAfter).

Consider either:

  • Naming them DevLoginConfiguration / WebAuthnConfiguration to match the style of the rest of the package, or
  • Accepting the name and adding a comment clarifying that they're component-scanned (not SPI-registered)

This is a minor naming/convention concern, not a functional bug.


What's Good

  • Properties classes are now passive POJOs — removing @Component from @ConfigurationProperties classes is exactly right. They should not self-register.
  • DevLoginController and DevLoginStartupWarning retain their own guards — defense-in-depth is preserved. Even if someone misconfigures the auto-configuration, the individual beans won't activate.
  • WebAuthnAutoConfiguration is correctly unconditional — the comment explaining that WebSecurityConfig requires WebAuthnConfigProperties regardless of whether WebAuthn is enabled is accurate and helpful.
  • JavaDoc is clear and accurate on all new and modified classes.
  • No security regressions — the @Profile("local") + @ConditionalOnProperty pairing for dev login remains in place.

Verdict

Two concrete suggestions:

  1. Remove @PropertySource from DevLoginAutoConfiguration — it cannot affect its own conditional evaluation and is already covered by WebAuthnAutoConfiguration.
  2. Consider renaming to DevLoginConfiguration / WebAuthnConfiguration to avoid implying SPI auto-configuration semantics.

Neither blocks merge if you disagree, but item 1 removes dead code that could mislead future readers.

- Rename DevLoginAutoConfiguration → DevLoginConfiguration and
  WebAuthnAutoConfiguration → WebAuthnConfiguration to avoid implying
  Spring Boot SPI semantics (these are component-scanned, not
  SPI-registered via AutoConfiguration.imports)
- Remove @propertysource from DevLoginConfiguration: the annotation
  cannot influence its own @ConditionalOnProperty evaluation (condition
  is checked before @propertysource is processed), and defaults are
  already available via WebAuthnConfiguration which unconditionally
  loads dsspringuserconfig.properties
@claude
Copy link

claude bot commented Feb 22, 2026

Follow-up Review

The second commit (23047ca) directly addresses both issues raised in the initial review:

  • Naming fixed: DevLoginAutoConfigurationDevLoginConfiguration, WebAuthnAutoConfigurationWebAuthnConfiguration — no longer implies SPI auto-configuration semantics.
  • Dead @PropertySource removed from DevLoginConfiguration: The commit message correctly explains why it was removed (condition is evaluated before @PropertySource is processed), and the updated JavaDoc notes the implicit dependency on WebAuthnConfiguration loading the defaults.

Current state looks good

One minor observation (non-blocking): matchIfMissing = false in @ConditionalOnProperty is the default and could be omitted, but leaving it explicit is a reasonable choice for clarity.

One design note worth acknowledging: DevLoginConfiguration now implicitly relies on WebAuthnConfiguration being in the context to supply the dsspringuserconfig.properties defaults. The JavaDoc calls this out, which is the right mitigation. Just worth being aware that if WebAuthnConfiguration were ever made conditional in the future, the dev-login property defaults could silently stop loading. Not a concern for the current codebase.

Verdict

Both review items resolved. The refactor is clean — properties classes are now passive POJOs, activation guards live in dedicated configuration classes, and the @PropertySource is consolidated in one always-active location. Ready to merge.

@devondragon devondragon merged commit 8c90bbd into main Feb 22, 2026
5 checks passed
@devondragon devondragon deleted the issue-263-config-properties-refactor branch February 22, 2026 20:41
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.

refactor: migrate @ConfigurationProperties classes from @Component to @EnableConfigurationProperties pattern

2 participants