Skip to content

Throw exception if spring.config.location uses classpath*: #21168

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
vkochnev opened this issue Apr 27, 2020 · 3 comments
Closed

Throw exception if spring.config.location uses classpath*: #21168

vkochnev opened this issue Apr 27, 2020 · 3 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@vkochnev
Copy link

SpringBoot version 2.3.0.M4

When trying to set spring.cloud.bootstrap.location (which is effectively mapped to spring.config.location) to something like classpath*:/config/ discovered that it doesn't work because org.springframework.boot.context.config.ConfigFileApplicationListener internally calls org.springframework.util.ResourceUtils#isUrl with supplied location and when receives false prepends it with file:. As a result it ends up with file:classpath*:/config/ in my case, which is obviously incorrect.
I filed spring-projects/spring-framework#24979 considering it might be a bug in SpringFramework, but was directed that if pattern support is desirable here then org.springframework.core.io.support.ResourcePatternUtils#isUrl should be used instead.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 27, 2020
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 27, 2020
@philwebb philwebb added this to the 2.1.x milestone Apr 27, 2020
@philwebb philwebb added type: enhancement A general enhancement and removed type: bug A general bug labels Apr 27, 2020
@philwebb
Copy link
Member

At first I thought it would be possible to switch to ResourcePatternUtils.isUrl but I'm afraid it's not that simple. The locations specified in spring.config.location must be exact locations that can be resolved by DefaultResourceLoader. Since we don't want to support wildcards in locations, I'm going to improve the code by throwing a more specific exception.

@vkochnev Is there any reason you can't use classpath:/config/ for your location?

@philwebb philwebb self-assigned this Apr 27, 2020
@philwebb philwebb changed the title spring.config.location doesn't allow classpath*: locations Throw exception is spring.config.location uses classpath*: Apr 27, 2020
@philwebb philwebb changed the title Throw exception is spring.config.location uses classpath*: Throw exception if spring.config.location uses classpath*: Apr 27, 2020
@philwebb philwebb modified the milestones: 2.1.x, 2.1.14 Apr 27, 2020
@vkochnev
Copy link
Author

Actually my location expected to be classpath*:/config/*/,classpath:/config/. I have several starters shared by several apps and I want them to specify default properties related to this starter, e.g. I want to pre-configure logging, metrics, database, messaging and not copy all these configs over applications. Maybe there is another legit way of doing this, but I cannot see it.
In fact I bypassed this check at the moment with specifying location as classpath*:/$config/*/,classpath:/$config/ and looks like it works, but it is a dirty hack and I want some cleaner solution. Also default spring.config.location already contain at least one wildcard file:./config/*/.
Finally org.springframework.boot.context.config.ConfigFileApplicationListener.Loader#getResources is called to resolve resources and internally it uses PathMatchingResourcePatternResolver which works just fine with classpath*:. Moreover org.springframework.boot.context.config.ConfigFileApplicationListener.Loader#resourceLoader is not used directly anywhere in Loader except constructor and, I believe, it may be removed from fields at all.

@philwebb
Copy link
Member

It looks like we added PathMatchingResourcePatternResolver as part of #19909. I've opened #21217 to discuss that. I'll leave this one as a fix for 2.1.x, since we don't have scanning in that branch.

Please subscribe to #21217 for updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants