SpringBoot The org.springframework.boot.context.properties.bind.MapBinder#merge works in wrong result by deleting the cofniguration

the code like this:

@Data
@ToString
@ConfigurationProperties(value = MonitorPluginConfiguration.PREFIX)
public class MonitorPluginConfiguration {
    public static final String PREFIX = "monitor";
    private Map<String, String> plugins;
}

the yaml configuration in nacos:

monitor:
  plugins:
    p1: 1
    p2: 2
    p3: 3
    p4: 4

if i delete the key-value p4: 4 in the configuration from nacos ，the value for field MonitorPluginConfiguration#plugins will not be deleted.

the changed configuration in nacos after delete the the key-value p4: 4

monitor:
  plugins:
    p1: 1
    p2: 2
    p3: 3

I trace the code , i find the method in org.springframework.boot.context.properties.bind.MapBinder#merge，

        @Override
    protected Map<Object, Object> merge(Supplier<Map<Object, Object>> existing, Map<Object, Object> additional) {
        Map<Object, Object> existingMap = getExistingIfPossible(existing);
        if (existingMap == null) {
            return additional;
        }
        try {
            existingMap.putAll(additional);
            return copyIfPossible(existingMap);
        }
        catch (UnsupportedOperationException ex) {
            Map<Object, Object> result = createNewMap(additional.getClass(), existingMap);
            result.putAll(additional);
            return result;
        }
    }

the parmater additionalvalue is {p1=1, p2=2, p3=3}

and the variable existingMap value is{p1=1, p2=2, p3=3, p4=4}

for the code existingMap.putAll(additional), it will save the original value {p1=1, p2=2, p3=3, p4=4}. so the result is bad. I think that it is good to execute existingMap.clear() before executing the code existingMap.putAll(additional) The code like this:

        @Override
    protected Map<Object, Object> merge(Supplier<Map<Object, Object>> existing, Map<Object, Object> additional) {
        Map<Object, Object> existingMap = getExistingIfPossible(existing);
        if (existingMap == null) {
            return additional;
        }
        try {
            existingMap.clear(); // add this line
            existingMap.putAll(additional);
            return copyIfPossible(existingMap);
        }
        catch (UnsupportedOperationException ex) {
            Map<Object, Object> result = createNewMap(additional.getClass(), existingMap);
            result.putAll(additional);
            return result;
        }
    }

Comment From: wilkinsona

if i delete the key-value p4: 4 in the configuration from nacos

This won't have any effect in a standard Spring Boot application as Spring Boot doesn't support refreshing configuration at runtime. What's triggering an update of your MonitorPluginConfiguration class when the configuration is changed in Nacos?

Comment From: winjaychan

if i delete the key-value p4: 4 in the configuration from nacos

This won't have any effect in a standard Spring Boot application as Spring Boot doesn't support refreshing configuration at runtime. What's triggering an update of your MonitorPluginConfiguration class when the configuration is changed in Nacos?

it triggered a rebind action,

• org.springframework.cloud.context.properties.ConfigurationPropertiesRebinder#rebind(),
• org.springframework.cloud.context.properties.ConfigurationPropertiesRebinder#rebind(java.lang.String)
• org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory#initializeBean(java.lang.Object, java.lang.String)
• ......
• the last action org.springframework.boot.context.properties.bind.MapBinder#merge

Comment From: winjaychan

for additional discovery, i found the code in org.springframework.boot.context.properties.bind.CollectionBinder#merge

@Override
protected Collection<Object> merge(Supplier<Collection<Object>> existing, Collection<Object> additional) {
    Collection<Object> existingCollection = getExistingIfPossible(existing);
    if (existingCollection == null) {
        return additional;
    }
    try {
        existingCollection.clear();
        existingCollection.addAll(additional);
        return copyIfPossible(existingCollection);
    }
    catch (UnsupportedOperationException ex) {
        return createNewCollection(additional);
    }
}

before add all elements to the existingCollection, it clear the collection variable existingCollection by calling the method clear() , so for the Collection type, it works well.

what is the difference between MapBinder an CollectionBinder?

Comment From: wilkinsona

Thanks. From a Spring Boot perspective, this is working as designed and documented.

I believe the bug is in Spring Cloud. When they are rebinding configuration properties, an existing instance of MonitorPluginConfiguration should not be re-used. Instead, a fresh instance should be created and then have properties bound to it. This has been raised before but the Spring Cloud team declined it as a duplicate of https://github.com/spring-cloud/spring-cloud-commons/issues/818. You may want to raise the problem again as, IMO, it should be documented as a limitation at the least.

Comment From: winjaychan

Thanks. From a Spring Boot perspective, this is working as designed and documented.

I believe the bug is in Spring Cloud. When they are rebinding configuration properties, an existing instance of MonitorPluginConfiguration should not be re-used. Instead, a fresh instance should be created and then have properties bound to it. This has been raised before but the Spring Cloud team declined it as a duplicate of spring-cloud/spring-cloud-commons#818. You may want to raise the problem again as, IMO, it should be documented as a limitation at the least.

Thanks, but I hope to known why the org.springframework.boot.context.properties.bind.CollectionBinder#merge method, in that, it calls clear()

@Override
protected Collection<Object> merge(Supplier<Collection<Object>> existing, Collection<Object> additional) {
    Collection<Object> existingCollection = getExistingIfPossible(existing);
    if (existingCollection == null) {
        return additional;
    }
    try {
        existingCollection.clear();
        existingCollection.addAll(additional);
        return copyIfPossible(existingCollection);
    }
    catch (UnsupportedOperationException ex) {
        return createNewCollection(additional);
    }
}

before add all elements to the existingCollection, it clear the collection variable existingCollection by calling the method clear() , so for the Collection type, it works well.

So what is the difference between MapBinder an CollectionBinder? @wilkinsona

Comment From: wilkinsona

@winjaychan Have you read the reference documentation that I linked to earlier? It describes the difference in behaviour for binding lists and maps. Lists are only ever bound from a single source (so they are cleared). Maps can be bound from multiple sources (so they are not cleared).

If you have any further questions, please follow up on Stack Overflow or Gitter. As mentioned in the guidelines for contributing, we prefer to use GitHub issues only for bugs and enhancements.