Thanks to the team for taking the time.

Affects: 5.3.18 and main branch

Affected code

https://github.com/spring-projects/spring-framework/blob/28ca0dd64209a003dcd577212ad06624dfbc2684/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java#L482-L502

Discussion

The addPropertySource method is affected by a bug IMO where the effect of the if statement is reverted by the fact that PropertySources are hashed using their name property.

When code reaches the affected if-statement, it means that a PropertySource of name foo is already registered in the Spring context, and that PropertySource is assigned to the existing variable. The idea is to replace (L499) the old property source with name foo with the CompositePropertySource containing both.

The problem is, given that

  • CompositePropertySource holds a Set of PropertySources
  • PropertySources are hashed by their getName(), which is not extended currently
  • Both PropertySources to compose have same name

In the end, the second PropertySource won't be added and CompositePropertySource will hold only one PropertySource of name foo

Affected case

We found a problem when multiple teams work on different Java libraries, each declaring some property sources from classpath files.

In the end, we discovered that if two libraries hold the same env.yaml in different packages, but the associated PropertySource name is computed by the file name (not fully qualified path), two libraries define a PropertySource named env.yaml and clash.

We expect this to happen even if teams define any kind of MapPropertySource with the same non-unique name. We have implemented a workaround by using the fully qualified resource URL.

Suggested solution

Have CompositePropertySource use List than Set?

Test case

Not yet available

Comment From: snicoll

@djechelon thank you, this looks legit to me. The logic has moved to PropertySourceProcessor so it should be easier for you to write a test case for it if have the time.

Comment From: jhoeller

This turns out to be by design since CompositePropertySource is not meant to hold property sources with different names.

In ConfigurationClassParser, the logic is primarily focused on ResourcePropertySource which is able to expose the full resource path as name. For custom property sources with hard names, they replace each other in case of the same name even then. Since existing setups may rely on the latter as well, we'll rather avoid changing this.

Instead, the recommended solution is exactly what you went with: let the algorithm choose a distinct name - such as the fully qualified resource path - as ResourcePropertySource name, or provide a similarly distinct name for a custom PropertySource if necessary. I assume your setup involves a custom PropertySourceFactory where this needs to be taken into account specifically. I'll turn this ticket into a documentation ticket for it.