Skip to content
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

'spring.config.import' placeholders can resolve from profile-specific documents when they should fail #29386

Closed
tauparticle opened this issue Jan 14, 2022 · 6 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@tauparticle
Copy link

tauparticle commented Jan 14, 2022

Greetings Spring Boot. I ran into an unexpected results with the following setup with config loading. Let's consider we have the following application.yml file.

test:
  environment: test

---
spring:
  config:
    activate:
      on-profile: profileA

test:
  environment: prod


---
spring:
  config:
    activate:
      on-profile: profileB
    import:
      - application/${test.environment}.yml

otherProp: ${test.environment}  

And we have application/prod.yml with the following contents:

myTestProp: testProp/prod

And application/test.yml with the following contents:

myTestProp: testProp/test

And finally we have the following test that will activate profileB and assert on expected results.

@ActiveProfiles(profiles = "profileB")
@SpringBootTest
public class ProfileBPropsTest {

    @Autowired
    private Environment env;

    @Test
    public void Test() {
        assertEquals("test", env.getProperty("test.environment", String.class));
        assertEquals("test", env.getProperty("otherProp", String.class));
        assertEquals("testProp/test", env.getProperty("myTestProp", String.class));
    }

    @Configuration
    class TestConfig {}

And we have the following test to activate both profiles profileA and profileB and assert on expected results.

@ActiveProfiles(profiles = {"profileA", "profileB"})
@SpringBootTest
public class ProfileABPropsTest {

    @Autowired
    private Environment env;

    @Test
    public void Test() {
        assertEquals("prod", env.getProperty("test.environment", String.class));
        assertEquals("prod", env.getProperty("otherProp", String.class));
        assertEquals("testProp/prod", env.getProperty("myTestProp", String.class));
    }

    @Configuration
    class TestConfig {}

}

What I've observed is ProfileABPropsTest passes expectedly. The file-order profile activation picks up the override for test.environment in profileA which is then used in profileB to import prod.yml as a property source.

However what fails is ProfileBPropsTest. Only profileB is active, so we assume the default value of test.environment=test will cause test.yml to be imported as a property source. Instead what we find is the value of myTestProp=testProp/prod which would only happen if prod.yml was loaded. But as we can see by the test, test.environment=test, so how is it that we get this property in this scenario unless the expression ${test.environment} is pulling in an incorrect value that should technically only happen if profileA is activated.

I cannot explain this from reading the Spring documentation. For context, I am seeing this in spring boot 2.5.x.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 14, 2022
@tauparticle
Copy link
Author

I uploaded the code to reproduce this problem here. https://github.com/tauparticle/SpringBootPropLoadingDemo

@philwebb
Copy link
Member

@tauparticle https://github.com/tauparticle/SpringBootPropLoadingDemo gives a 404 for me. Can you double check the link is correct.

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Jan 15, 2022
@tauparticle
Copy link
Author

@philwebb apologies. I think it was made private originally. It should work now.

@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 Jan 15, 2022
@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Jan 18, 2022
@philwebb
Copy link
Member

Thanks. The problem looks somewhat related to #23020. The placeholder resolver logic isn't working with contributors that have Kind.UNBOUND_IMPORT.

@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 18, 2022
@philwebb philwebb added this to the 2.5.x milestone Jan 18, 2022
@philwebb philwebb self-assigned this Jan 18, 2022
@philwebb philwebb changed the title Unexpected property values with multi-document YAML config and import expressions spring.config.import expressions can import from profile-specific documents when they should fail Jan 18, 2022
@philwebb philwebb changed the title spring.config.import expressions can import from profile-specific documents when they should fail 'spring.config.import' placeholders can resolve from profile-specific documents when they should fail Jan 18, 2022
@philwebb
Copy link
Member

@tauparticle I've pushed a fix for this, but I'm afraid it's not going to allow you to import configuration in the way you're trying to do it. Placeholder resolution should have always failed when you try to reference a property in an inactive document. This was an intentional design decision that we made when we introduced spring.config.import support. The idea was to prevent some of the issues that make application.yaml processing hard to reason about.

I would suggest changing your YAML so that the imports are directly declared, rather than using placeholders. For example:

---
spring:
  config:
    activate:
      on-profile: profileA
    import:
      - application/prod.yml
---
spring:
  config:
    activate:
      on-profile: profileB
    import:
      - application/test.yml

@philwebb philwebb modified the milestones: 2.5.x, 2.5.9 Jan 19, 2022
@tauparticle
Copy link
Author

Thanks for the clarification @philwebb!

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