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

ConfigFileApplicationListener filtering fails when the defaultPropertySource is a composite #17011

Closed
ryanjbaxter opened this issue May 29, 2019 · 17 comments

Comments

Projects
None yet
6 participants
@ryanjbaxter
Copy link

commented May 29, 2019

Prior to Boot 2.2.0 if you used spring-cloud-starter and within your bootstrap.yml set spring.config.name Boot would correctly load the property file specified in the property. Starting with Boot 2.2.0 this no longer works.

I have traced the breaking change down to this line added in commit d92c2f7

In the situation I described above defaultProperties is actually an instance of ExtendedDefaultPropertySource defined here.

The sources property, which is a composite, contains the value of spring.config.name in this case. However in ConfigFileApplicationListener.replaceDefaultPropertySourceIfNecessary the code just calls .getSource to construct the new defaultProperties so we loose the properties in the composite and therefore the spring.config.name property is never retrieved.

For a sample that reproduces the problem you can look at this test.

@philwebb

This comment has been minimized.

Copy link
Member

commented May 29, 2019

See #15445

@ryanjbaxter

This comment has been minimized.

Copy link
Author

commented May 29, 2019

@philwebb I read the issue, but that is all about profiles, this problem has nothing to do with profiles.

To add to this, I also think these changes broke this functionality as well, which does have to do with profiles.

@philwebb philwebb added this to the 2.2.x milestone May 29, 2019

@wilkinsona

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@ryanjbaxter The commit that you referenced fixed the issue that Phil referenced.

@philwebb

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@ryanjbaxter I just added a link to the issue referenced in commit d92c2f7

@ryanjbaxter

This comment has been minimized.

Copy link
Author

commented May 29, 2019

@wilkinsona @philwebb sorry I misinterpreted what you were saying ;)

@philwebb

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@ryanjbaxter I tried to run BootstrapClientApplicationTests but the test passed for me so I'm not sure what I'm doing wrong. Any hints?

For BootstrapConfigurationTests it appears that the bootstrap local.properties are defining a spring.profiles.active key which we are now actively filtering out. I'm not really sure how we can fix this one and also fix #15445. They seem to be at odds with each other. Is there any reason that the bootstrap context needs to use a property source named defaultProperties? If it does, perhaps we need to offer a way to switch off the filtering just for that bootstrap context.

@ryanjbaxter

This comment has been minimized.

Copy link
Author

commented May 30, 2019

@philwebb did you remove the @Ignore on the test in BootstrapClientApplicationTests?

Is there any reason that the bootstrap context needs to use a property source named defaultProperties?

@spencergibb @dsyer do you know? I am a little unsure.

@spencergibb

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I'm not sure either.

@philwebb

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@Ignore on the test

Doh! 🤦‍♂

@philwebb

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Actually, even with the @Ignore removed it appears to work

@ryanjbaxter

This comment has been minimized.

Copy link
Author

commented Jun 3, 2019

Weird not sure, it is failing for me on my machine

@mbhave

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

I'm able to reproduce this. As @ryanjbaxter mentioned, it's because we use defaultProperties.getSource() here. The ExtendedDefaultPropertySource used by Spring Cloud has the property sources wrapped inside a CompositePropertySource and defaultProperties.getSource() just returns an empty map. We could change the filtering logic so that it only applies to Spring Boot's default property source or only if the default property source is a MapPropertySource.

@mbhave mbhave self-assigned this Jun 6, 2019

@philwebb philwebb changed the title Replacing Default Properties Breaks Functionality In Spring Cloud Bootstrap ConfigFileApplicationListener filtering fails when the defaultPropertySource is a composite Jun 10, 2019

@philwebb philwebb assigned philwebb and unassigned mbhave Jun 10, 2019

@philwebb philwebb modified the milestones: 2.2.x, 2.2.0.M4 Jun 10, 2019

@philwebb philwebb closed this in 8d44e31 Jun 10, 2019

@philwebb

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@ryanjbaxter I've implemented the fix suggested by @mbhave, both Spring Cloud tests referenced in this issue work for me locally now.

@ryanjbaxter

This comment has been minimized.

Copy link
Author

commented Jun 13, 2019

@philwebb I removed the @Ignore in BootstrapConfurationTests but the build still fails using Snapshots (I am going to add it back so we have a green build for now).
https://jenkins.spring.io/job/spring-cloud-commons-master-ci/8853/testReport/org.springframework.cloud.bootstrap.config/BootstrapConfigurationTests/includeProfileFromBootstrapProperties/

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

FWIW, it also fails for me when built locally with ./mvnw -U clean verify.

@wilkinsona wilkinsona reopened this Jun 13, 2019

@mbhave

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Looks like I was wrong about the same fix being able to fix both tests. I share what @philwebb said here:

I'm not really sure how we can fix this one and also fix 15445.

I don't think a solution where the filtering only applies for Boot's defaultProperties will work either. The ExtendedDefaultPropertySource wraps Boot's defaultProperties, so it would need to apply to that to be consistent.

@philwebb

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

I swear it worked in my IDE :/

I've opened #17141 to look at the failing test. It feels like a different issue and the composite property source problem has been fixed so let's keep this issue just for that one.

@philwebb philwebb closed this Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.