Fix 1935

[wind57]

No description provided.

[wind57]

this and all the subclasses (4 in total) test that overriding works correctly, as in the bug description

[wind57]

no changes in tests logic at all, just adjusted the code to the fix

[wind57]

yamlParserGenerator now returns a Map<String, Object> instead of Properties, so we can get rid of one step

[wind57]

instead of using YamlPropertiesFactoryBean, we are now using our own CustomYamlPropertiesFactoryBean. It is heavily based on the spring boot ones, but takes care to override collections and maps.

I wanted not to do this, but use the same process that spring-boot does and I can't. The thing is , spring-boot reads the application.yaml into a PropertySource directly and then the IndexedElementsBinder wires things based on what exists in @ConfigurationProperties. The process is very coupled to its internals

[ryanjbaxter]

@wind57 did you open an issue with Spring Boot about this? Ideally we can change this in Boot 4.0.0 and override it instead

[wind57]

I thought about that, but the truth is that we will still have to drill into it and extract lots of code outside like we do it now. The org.springframework.beans.factory.config.YamlProcessor::protected void process(MatchCallback callback) specifically is documented as:

Provide an opportunity for subclasses to process the Yaml parsed from the supplied resources.

So I think that the point of that class is : if you plan to override, then everything is on you now.

[wind57]

spring-projects/spring-boot#46575

[wind57]

@ryanjbaxter the issue was closed and the suggestion to create one PropertySource per each document in the yaml is something I can not agree enough. I've seen and debugged the code in spring-boot, and yes, it is the best option out there. But it's not an option for us as far as I see, we can't do that unless we re-think the entire PropertySource creation in our project.

[wind57]

ideally, this one should have overridden the YamlPropertiesFactoryBean that spring-boot uses, but we can't do that. The main reason is that it has such a method:

public void setDocumentMatchers(DocumentMatcher... matchers) {
		this.documentMatchers = List.of(matchers);
}

but this method is used only in an internal, private method. It looks like this:

protected process(...) -> private process(...) -> use `documentMatchers`

So if I override protected process(...), the documentMatchers would not be used anymore, but we need them.

---

Otherwise, this class does almost the same as the original YamlPropertiesFactoryBean: it parses yaml documents and computes properties. The only difference that I introduce here, is the ability to override collections and maps from profile-based sources. I mean step (3) and (4).

All other steps are 1-1 to the original code.

spring-boot does not do that, it computes PropertySources directly, on the spot, but we can't do that, since we need to read configmaps and secrets first, and then compute those.

[ryanjbaxter]

Can you add some documentation to the class around the code that is different than what is in spring boot?

[wind57]

done

[wind57]

@ryanjbaxter this is ready to be looked at. I understand it might be a lot to digest, it took me a while myself to figure things out, please don't hesitate and ask me anything as needed. Thank you!

[ryanjbaxter]

k8s 3.1.x has reach end of support so can you make this against the 3.2.x branch instead?

[wind57]

@ryanjbaxter did you had a change to look at this one?

[ryanjbaxter]

I think it would be useful to add why Spring Boot doesn't do the same thing and why we have a special case in k8s config maps and secrets

[wind57]

good point, now I get it. done

[ryanjbaxter]

This is where I am hung up. Why do we need to read everything into a single property source? The following sentence says because we need to read configmaps and secrets but what about configmap and secrets is different than reading a file?

[wind57]

This is where I am hung up. Why do we need to read everything into a single property source?

because this is they way our code was designed from the beginning. We have this for example:

ConfigMapPropertySourceLocator implements PropertySourceLocator {
    
     public PropertySource<?> locate(Environment environment) { ... }

}

and this method returns a CompositePropertySource composite = new CompositePropertySource("composite-configmap");.

So when we read a configmap, we assume we will compute a single property source, for each source. I mean:

Set<NormalizedSource> sources = new LinkedHashSet<>(this.properties.determineSources(environment));
				LOG.debug("Config Map normalized sources : " + sources);
				sources.forEach(s -> {
					MapPropertySource propertySource = getMapPropertySource(s, env);
					if ("true".equals(propertySource.getProperty(Constants.ERROR_PROPERTY))) {
						LOG.warn("Failed to load source: " + s);
					}
					else {
						LOG.debug("Adding config map property source " + propertySource.getName());
						composite.addFirstPropertySource(propertySource);
					}
				});

so :

• find the sources
• for each of them, create a single PropertySource.

But in case of a yaml multi document, things are not trivial to do. spring-boot does not impose this limitation, but we had it from the very beginning.

---

This is why I said, I totally agree with the comment from spring-boot maintainer, when he says : "create a single PropertySource just the say we do for each individual yaml document". I agree, but our code is far too involved at this point in time. We would need to revise the entire code and the way we create property sources.

[ryanjbaxter]

So basically for multi-document yaml files (configmaps/secrets) we have always created a single property source per file instead of separating each document into their own property source?

And when we use Boots YamlPropertiesFactoryBean it creates multiple properties sources and this somehow results in the weird overriding of properties?

[wind57]

So basically for multi-document yaml files (configmaps/secrets) we have always created a single property source per file instead of separating each document into their own property source?

exactly.

And when we use Boots YamlPropertiesFactoryBean it creates multiple properties sources and this somehow results in the weird overriding of properties?

The idea is correct, the details are not. We were already using YamlPropertiesFactoryBean and this one is overriding properties in a "bad way".

We can't use the Boots way, because it creates one PropertySource per yaml document.

Essentially, I think you are spot on with the idea here, yes.

[ryanjbaxter]

The idea is correct, the details are not. We were already using YamlPropertiesFactoryBean and this one is overriding properties in a "bad way".

Is it because the multiple property sources from the multiple documents results in incorrect ordering of the property sources? Do you have an example yaml document, the property sources Boot would create and the order they would be in as well as what we would expect? This should help me understand the crux of the issue.

[wind57]

yes, an example would make things simpler I guess. I will use what the OP presented to us. I will strip it down to the bare minimum. Suppose you have this yaml:

my-application:
  items:
    - "Item 1"
    - "Item 2"
    - "Item 3"
    - "Item 4"
    - "Item 5"
---
spring:
  config:
    activate:
      on-profile: dev

my-application:
  items:
    - "Item 1"
    - "Item 6"
    - "Item 7"

spring-boot, using YamlPropertySourceLoader, will create two property sources:

• name of the first property source : Config resource 'class path resource [application.yaml]' via location 'optional:classpath:/' (document #0)

• values of the fist property source : my-application.items[0] = Item 1, my-application.items[1] = Item 2 ....

• name of the second property source: Config resource 'class path resource [application.yaml]' via location 'optional:classpath:/' (document #1)

• value of the second property source: my-application.items[0] = Item 1, my-application.items[1] = Item 6, my-application.items[2] = Item 7 ....

Internally, later, when you need to wire things, IndexedElementsBinder will correctly chose the profile based properties (second yaml document from the example).

---

In our case, we were using YamlPropertiesFactoryBean. So when that one was reading the yaml file, it would do :

    protected Properties createProperties() {
        Properties result = CollectionFactory.createStringAdaptingProperties();
        this.process((properties, map) -> result.putAll(properties));
        return result;
    }

In simpler words, it will "merge" properties from yaml documents, into a single Map, using : result.putAll(properties).

So, it will read the first yaml document, create the properties:

my-application.items[0] = Item 1
my-application.items[1] = Item 2
my-application.items[2] = Item 3
my-application.items[3] = Item 4
my-application.items[4] = Item 5

Then, it will read the second yaml document, create the proeprties:

my-application.items[0] = Item 1
my-application.items[1] = Item 6
my-application.items[2] = Item 7

then, again using result.putAll(properties), merge these. Resulting obviously, in :

my-application.items[0] = Item 1
my-application.items[1] = Item 6
my-application.items[2] = Item 7
my-application.items[3] = Item 4
my-application.items[4] = Item 5

which is not correct.

---

As we talked above, if we had the option to create separate PropertySources from these two documents, we would and spring-boot injection mechanism would have taken care of the rest. But we are in a situation were this is not an option. And this is why I created the workaround.

[ryanjbaxter]

Why is this incorrect? The dev profile is active so it is overriding the items in the array from the default profile.

[wind57]

it was supposed to override the entire array, exactly like spring-boot does, not individual items. no?

[ryanjbaxter]

Ah yes OK, thats where I was all caught up. You are right it should override the property my-application.items. Thanks for being so patient!

[wind57]

if you wire properties using spring-boot only, no kubernetes involved, you will end up with :

List<String> items; // [Item 1, Item 6, Item 7]

but if users use configmap to read proeprties and we have the same yaml file, they will end up with :

List<String> items; // [Item 1, Item 6, Item 7, Item 4, Item 5]