spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.4k stars 40.51k forks source link

No metadata is generated with meta-annotated @ConfigurationProperties #18517

Closed ChristianIvicevic closed 1 month ago

ChristianIvicevic commented 4 years ago

In my project I have the following setup:

build.gradle.kts:

plugins {
    val springBootVersion = "2.2.0.M6"
    val kotlinVersion = "1.3.50"

    id("org.springframework.boot") version springBootVersion
    // ...
    kotlin("kapt") version kotlinVersion
}

dependencies {
    // ...
    kapt("org.springframework.boot:spring-boot-configuration-processor")
}

ServiceApplication.kt

package foo.service

import foo.service.config.properties.ServiceConfigurationProperties
import org.springframework.boot.autoconfigure.SpringBootApplication
import org.springframework.boot.context.properties.EnableConfigurationProperties
import org.springframework.boot.runApplication

@SpringBootApplication
@EnableConfigurationProperties(value = [ServiceConfigurationProperties::class])
class ServiceApplication

fun main(args: Array<String>) {
    runApplication<ServiceApplication>(*args)
}

ServiceConfigurationProperties.kt

package foo.service.config.properties

import org.springframework.boot.context.properties.ConfigurationProperties
import org.springframework.boot.context.properties.bind.DefaultValue

@ConfigurationProperties("foo.service")
data class ServiceConfigurationProperties(
    /**
     * Allows to override certain production environment behaviour to be different during test execution.
     */
    val testOverrides: TestOverrides?
) {
    data class TestOverrides(
        /**
         * Gets or sets a boolean value indicating whether scheduling should be disabled when running integration tests.
         */
        @DefaultValue("false")
        val isSchedulingDisabled: Boolean
    )
}

This setup works with v2.2.0.M6 and yields the following spring-configuration-metadata.json as expected (and enabling IntelliJ auto completion and hints in the yaml configuration files):

{
  "groups": [
    {
      "name": "foo.service",
      "type": "foo.service.config.properties.ServiceConfigurationProperties",
      "sourceType": "foo.service.config.properties.ServiceConfigurationProperties"
    },
    {
      "name": "foo.service.test-overrides",
      "type": "foo.service.config.properties.ServiceConfigurationProperties$TestOverrides",
      "sourceType": "foo.service.config.properties.ServiceConfigurationProperties",
      "sourceMethod": "getTestOverrides()"
    }
  ],
  "properties": [
    {
      "name": "foo.service.test-overrides.is-scheduling-disabled",
      "type": "java.lang.Boolean",
      "description": "Gets or sets a boolean value indicating whether scheduling should be disabled when running integration tests.",
      "sourceType": "foo.service.config.properties.ServiceConfigurationProperties$TestOverrides",
      "defaultValue": false
    }
  ],
  "hints": []
}

Unfortunately when upgrading to v2.2.0.RC1 the properties array ends up being empty in the json file:

{
  "groups": [
    {
      "name": "foo.service",
      "type": "foo.service.config.properties.ServiceConfigurationProperties",
      "sourceType": "foo.service.config.properties.ServiceConfigurationProperties"
    },
    {
      "name": "foo.service.test-overrides",
      "type": "foo.service.config.properties.ServiceConfigurationProperties$TestOverrides",
      "sourceType": "foo.service.config.properties.ServiceConfigurationProperties",
      "sourceMethod": "getTestOverrides()"
    }
  ],
  "properties": [],
  "hints": []
}
philwebb commented 4 years ago

This might be related to the recent constructor binding changes that we made. Check out this section of the release notes for details.

Do you get correct metadata if you change @ConfigurationProperties to @ImmutableConfigurationProperties?

ChristianIvicevic commented 4 years ago

@philwebb Using @ImmutableConfigurationProperties actually causes the metadata file to not be created at all or being deleted if it existed previously.

snicoll commented 4 years ago

Yep, that part is an oversight and we’ll use this issue to fix it. Add @ConstructorBinding instead.

ChristianIvicevic commented 4 years ago

@snicoll I can confirm, @ConstructorBinding and @ConfigurationProperties work together as expected for the time being.

snicoll commented 4 years ago

I'd like us to reconsider having @ImmutableConfigurationProperties at all. If we improve our current setup in the future so that @ConstructorBinding is not necessary for the idiomatic setup, the annotation will become irrelevant. Also, we created @ConstructorBinding to be able to be explicit about things and having @ImmutableConfigurationProperties is hiding that a bit again (not the fact the constructor will be used but the fact a unique constructor if present will be used).

I can see two problems:

All in all, I don't think it feels very consistent with our move of being more explicit about this. Flagging for team attention to see what the rest of the team thinks.

philwebb commented 4 years ago

If you use @ImmutableConfigurationProperties and the class has two constructors you have to put @ConstructorBinding again. If you don't, JavaBean properties binding is used which feels quite odd

I think it should throw an exception. It's the same as adding @ConstructorBinding on the type then having an ambiguous constructor.

If you do add @ConstructorBinding then you have @ImmutableConfigurationProperties + @ConstructorBinding again. This reduce the relevance of the dedicated annotation in this case

This is true, but it doesn't seem any worse than having @ConfigurationProperties and @ConstructorBinding. I also think that 90% of users won't have multiple constructors.

When updating the tests I found needing two annotations really clunky and I quite liked @ImmutableConfigurationProperties. In a lot of ways it's like @RestController. You could argue that @Controller and @ResponseBody are more explicit, but I like the convenience of @RestController.

snicoll commented 4 years ago

In a lot of ways it's like @RestController.

I can see how it's grouping one annotation with two features but there is a major difference IMO. With @RestController it has an impact on all mapping methods of the class and it makes sure you don't end up returning the name of a view if you forget to add @ResponseBody to one of them. This one is about a single use (the constructor to use or the fact of using a constructor at all).

I agree with the convenience for the 90% use case but I am still wondering about whether we should promote it as a first class citizen.

jnizet commented 4 years ago

I have another concern with the ImmutableConfigurationProperties annotation: its name.

It tends to imply that it is supposed to be used on immutable classes only. Or worse, that annotating a class with this annotation makes the class immutable. But it isn't the case. Using the annotation on a class which has setters, or var in Kotlin , doesn't cause any problem.

I understand that constructor bindings makes it possible to create immutable configuration properties classes, and that making that possible was the primary goal of constructor binding, but still, the name is confusing. ConstructorBoundConfigurationProperties would be a more correct name, IMHO.

wilkinsona commented 4 years ago

Thanks for the feedback, @jnizet. We've decided to remove @ImmutableConfigurationProperties.

ChristianIvicevic commented 4 years ago

Thank you for clarifying the usage with both annotations and removing the immutable annotation. Closing this issue.

wilkinsona commented 4 years ago

No problem, @ChristianIvicevic. I'm going to re-open this issue as we'd like to use it to track adding support to the annotation processor for finding @ConfigurationProperties when it's used as a meta-annotation.

snicoll commented 1 month ago

I don't really remember what we have in mind for this but the annotation processor is triggered by specific annotations and has no concept of meta annotations. We're also outside the boundary of our annotation utilities that handle composed and meta-annotations for us automatically so, even if we changed our processor to react to any annotation, we'd still need significant work to make it work in the API the AP uses.

I've added a note in the reference guide https://github.com/spring-projects/spring-boot/commit/9817201833503b157182db288aed7b1016c5f997.