Skip to content

Configuration Processor should use the constructor as a source only with @ConstructorBinding #17035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bdemers opened this issue May 31, 2019 · 4 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@bdemers
Copy link
Contributor

bdemers commented May 31, 2019

Starting with 2.2.0.M3 spring-boot-configuration-processor generates an incorrect spring-configuration-metadata.json when configuration property class contains another autowired ConfigurationProperties.

For example:

    @Autowired
    public BrokenConfigProperties(@Autowired(required = false) NestedConfigProperties nested) {
        this.nested = nested;
    }

This works in 2.1.5.RELEASE and 2.2.0.M1.
Full example: https://github.com/bdemers/spring-config-problem

Running this example with M2 or M3 creates metadata:

{
  "groups": [
    {
      "name": "another.namespace",
      "type": "com.example.spring.configproblem.NestedConfigProperties",
      "sourceType": "com.example.spring.configproblem.NestedConfigProperties"
    },
    {
      "name": "example.broken",
      "type": "com.example.spring.configproblem.BrokenConfigProperties",
      "sourceType": "com.example.spring.configproblem.BrokenConfigProperties"
    }
  ],
  "properties": [
    {
      "name": "another.namespace.other-property",
      "type": "java.lang.String",
      "sourceType": "com.example.spring.configproblem.NestedConfigProperties"
    },
    {
      "name": "example.broken.nested",
      "type": "com.example.spring.configproblem.NestedConfigProperties",
      "sourceType": "com.example.spring.configproblem.BrokenConfigProperties"
    }
  ],
  "hints": []
}

expected (and 2.1.5.RELEASE output):

{
  "groups": [
    {
      "name": "another.namespace",
      "type": "com.example.spring.configproblem.NestedConfigProperties",
      "sourceType": "com.example.spring.configproblem.NestedConfigProperties"
    },
    {
      "name": "example.broken",
      "type": "com.example.spring.configproblem.BrokenConfigProperties",
      "sourceType": "com.example.spring.configproblem.BrokenConfigProperties"
    }
  ],
  "properties": [
    {
      "name": "another.namespace.other-property",
      "type": "java.lang.String",
      "sourceType": "com.example.spring.configproblem.NestedConfigProperties"
    },
    {
      "name": "example.broken.a-string-value",
      "type": "java.lang.String",
      "sourceType": "com.example.spring.configproblem.BrokenConfigProperties"
    }
  ],
  "hints": []
}

In this case the property a-string-value is missing from the M3 version.

A work around (not fully tested), is to add a default constructor the the config properties object:

    private BrokenConfigProperties() {
        this(null);
    }

    @Autowired
    public BrokenConfigProperties(@Autowired(required = false) NestedConfigProperties nested) {
        this.nested = nested;
    }
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 31, 2019
@wilkinsona wilkinsona changed the title Configuration Processor does not generate correct metadata starting with 5.2.0.M2 Configuration Processor does not generate correct metadata starting with 2.2.0.M3 Jun 1, 2019
@snicoll
Copy link
Member

snicoll commented Jun 2, 2019

@bdemers that setup is slightly odd in the first place. If you use @EnableConfigurationProperties you're asking Spring Boot to instantiate those classes for you and bind them to the environment. In such case, injecting other collaborators (i.e. other beans) is not foreseen. There is actually some note in the documentation about this:

We recommend that @ConfigurationProperties only deal with the environment and, in particular, does not inject other beans from the context.

In the 2.2 line we've started to support constructor binding so it's not only about metadata-generation. That constructor is now flagged as the preferred way to create the bean and bind its content from the environment, see this part of the 2.2 doc and it won't inject another bean but rather try to resolve example.broken.nested as a NestedConfigProperties type. There is no example.broken.nested value in the environment so it's injecting null but the @Autowired annotations there are ignored.

If you really need to get this, I'll use a setter injection for NestedConfigProperties but this object should not expose NestedConfigProperties again (getNested()). As you're exposing it with a public getter, you're basically telling the binder to bind something that was already bound. Not injecting other beans in such class is also a good way to avoid that kind of mistake.

Having said all that, getNested wasn't exposed in the metadata in 2.1.x and it should have been so I'll keep this issue open to investigate why that is. Can you please apply the suggestion above on your current version and let us know how it goes? (Also, @Autowired on constructor parameter is not a thing).

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jun 2, 2019
@bdemers
Copy link
Contributor Author

bdemers commented Jun 3, 2019

@snicoll I wouldn't read that tip, as don't do it, but more be careful if you are doing this.
I also wouldn't expect this functionality to change in a minor revision (as this a breaking issue).

Good catch on the public Nested getNested() would expect that to show up in 2.1.5 as well. I actual project this is a private method (so i've updated my example project to reflect this)

The metadata generated was not effected by this change (both in 2.1.5 and 2.2.0.M3)

Thanks for the heads up on the @Autowire + constructor. For now, I'll try to work around this to support 2.1 and 2.2 with your suggestions.

Thanks again!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 3, 2019
bdemers added a commit to okta/okta-spring-boot that referenced this issue Jun 3, 2019
* work around configuration processor issue spring-projects/spring-boot#17035
* Adjust AutoConfig tests to apply EnvironmentPostProcessor when context is loaded.
bdemers added a commit to okta/okta-spring-boot that referenced this issue Jun 3, 2019
* work around configuration processor issue spring-projects/spring-boot#17035
* Adjust AutoConfig tests to apply EnvironmentPostProcessor when context is loaded.
@snicoll
Copy link
Member

snicoll commented Jun 4, 2019

Configuration properties are scanned now and their lifecycle is such that if a constructor is present we use it to bind from the environment. That’s why the nested one shows up btw as it is now considered an explicit property.

You are correct about the note and doing something we don’t recommend and for which there is a note against is the reason we think it is acceptable to change in a feature release. If you really need to do this, create the bean yourself (using @bean) and don’t use classpath scanning/registration per class.

bdemers added a commit to okta/okta-spring-boot that referenced this issue Jun 4, 2019
* work around configuration processor issue spring-projects/spring-boot#17035
* Adjust AutoConfig tests to apply EnvironmentPostProcessor when context is loaded.
@philwebb philwebb added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Oct 2, 2019
@philwebb philwebb added this to the 2.2.x milestone Oct 2, 2019
@snicoll snicoll modified the milestones: 2.2.x, 2.2.0.RC1 Oct 2, 2019
@snicoll
Copy link
Member

snicoll commented Oct 2, 2019

We've added a dedicated @ConstructorBinding directive in #18469 so we're going to reuse this issue to fix the metadata to check for a dedicated signal that constructor binding should be used.

@snicoll snicoll self-assigned this Oct 2, 2019
@snicoll snicoll changed the title Configuration Processor does not generate correct metadata starting with 2.2.0.M3 Configuration Processor should use the constructor as a source only with @ConstructorBinding Oct 2, 2019
snicoll added a commit to snicoll/spring-boot that referenced this issue Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants