MapPropertySource changes are not reflected in the adapted ConfigurationPropertySource

[fahimfarookme]

Issue

In spring-boot 2x Maps in @ConfigurationProperties are not being updated from the second EnvironmentChangeEvent onwards. i.e. Following test fails in spring-boot 2x, but passed in spring-boot 1x.

@ConfigurationProperties
class Props {
   private Map<String, String> map = new HashMap<>();
   ...
}


TestPropertyValues.of("map.key1=value1").applyTo(this.environment);

// first EnvironmentChangeEvent
context.publishEvent(new EnvironmentChangeEvent(..."map.key1"));
// 'value1' gets reflected in @ConfigurationProperties bean - in both spring-boot 1x and 2x
assertEquals("value1", context.getBean(Props.class).getMap().get("key1"));

TestPropertyValues.of("map.key2=value2").applyTo(this.environment);

// second EnvironmentChangeEvent
context.publishEvent(new EnvironmentChangeEvent(..."map.key2"));

// 'value2' NOT getting reflected in @ConfigurationProperties bean - in spring-boot 2x.
assertEquals("value2", context.getBean(Props.class).getMap().get("key2"));

Analysis

• In case of MapPropertySource, SpringIterableConfigurationPropertySource#getCacheKey()
  here always returns the reference to the same object in the heap (for different invocations), even though the property-source state has changed. i.e.
  
  return ((MapPropertySource) getPropertySource()).getSource().keySet();

• As a result, if (ObjectUtils.nullSafeEquals(cacheKey, this.cacheKey)) here always evaluates to true (same reference) and getCache() returns the old/ initial cache.

Consequently, SpringIterableConfigurationPropertySource#iterator() having reference only to the initial/ old property-keys - hence the MapBinder fails here to bind updated properties.

Fix

SpringIterableConfigurationPropertySource#getCacheKey() should return a new object. This branch - if (getPropertySource() instanceof MapPropertySource) is not necessary.

private Object getCacheKey() {
   return getPropertySource().getPropertyNames(); // returns a new array
}

Whenever the cacheKey is updated, Cache is reinitialized (with all the property keys)

Sample

• PoC here to confirm/ recreate the issue - the same set of test cases passed here (spring-boot 1x), but failed here (spring-boot 2x).

This issue supersedes this one.

[fahimfarookme]

Below test case was passing before due to a side-effect of the issue I reported in SpringIterableConfigurationPropertySource. ${pid} was resolvable.

[philwebb]

Thanks for pull-request and the detailed analysis. I need to look in a a bit more detail to see what's going on before merging this. The use of keySet for MapPropertySource was an intentional optimization designed to prevent unnecessary creation of String[]. I'm worried that removing this might cause performance problems.

[spencergibb]

@philwebb I just want to point out that there are failures in spring cloud if you didn't notice the link above spring-cloud/spring-cloud-netflix#2538

[philwebb]

@spencergibb Thanks! We fixed something around this before didn't we?

[spencergibb]

Indeed it was a refresh that only happened on the first request. The key wasn't checked because the property source was new.

[philwebb]

@fahimfarookme Thanks very much for the pull-request. I've merged this into 2.0.x and master with an additional commit that should hopefully mean we keep the performance characteristics of the previous version.