Polish "Include properties in source merge algorithm"

See gh-25507

spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java

@@ -61,7 +61,7 @@ public class SimpleConfigurationMetadataRepository implements ConfigurationMetad
 			}
 			String sourceType = source.getType();
 			if (sourceType != null) {
-				putIfAbsent(group.getSources(), sourceType, source);
+				addOrMergeSource(group.getSources(), sourceType, source);
 			}
 		}
 	}
@@ -93,7 +93,7 @@ public class SimpleConfigurationMetadataRepository implements ConfigurationMetad
 				// Merge properties
 				group.getProperties().forEach((name, value) -> putIfAbsent(existingGroup.getProperties(), name, value));
 				// Merge sources
-				group.getSources().forEach((name, value) -> putIfAbsent(existingGroup.getSources(), name, value));
+				group.getSources().forEach((name, value) -> addOrMergeSource(existingGroup.getSources(), name, value));
 			}
 		}
 
@@ -111,23 +111,21 @@ public class SimpleConfigurationMetadataRepository implements ConfigurationMetad
 		return this.allGroups.get(source.getGroupId());
 	}
 
+	private void addOrMergeSource(Map<String, ConfigurationMetadataSource> sources, String name,
+			ConfigurationMetadataSource source) {
+		ConfigurationMetadataSource existingSource = sources.get(name);
+		if (existingSource == null) {
+			sources.put(name, source);
+		}
+		else {
+			source.getProperties().forEach((k, v) -> putIfAbsent(existingSource.getProperties(), k, v));
+		}
+	}
+
 	private <V> void putIfAbsent(Map<String, V> map, String key, V value) {
 		if (!map.containsKey(key)) {
 			map.put(key, value);
 		}
 	}
 
-/*
-Uncomment this code to fix issue revealed by ConfigurationMetadataRepositoryJsonBuilderTests#severalRepositoriesIdenticalGroups3()
-
-	private void putIfAbsent(Map<String, ConfigurationMetadataSource> sources, String name,
-			ConfigurationMetadataSource source) {
-		ConfigurationMetadataSource existing = sources.get(name);
-		if (existing == null) {
-			sources.put(name, source);
-		} else {
-			source.getProperties().forEach((k, v) -> putIfAbsent(existing.getProperties(), k, v));
-		}
-	}
-*/
 }

spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java

@@ -18,6 +18,7 @@ package org.springframework.boot.configurationmetadata;
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.Arrays;
 import java.util.Map;
 
 import org.junit.jupiter.api.Test;
@@ -99,116 +100,60 @@ class ConfigurationMetadataRepositoryJsonBuilderTests extends AbstractConfigurat
 			try (InputStream foo2 = getInputStreamFor("foo2")) {
 				ConfigurationMetadataRepository repo = ConfigurationMetadataRepositoryJsonBuilder.create(foo, foo2)
 						.build();
-				assertThat(repo.getAllGroups()).hasSize(1);
+				Iterable<String> allKeys = Arrays.asList("spring.foo.name", "spring.foo.description",
+						"spring.foo.counter", "spring.foo.enabled", "spring.foo.type");
+				assertThat(repo.getAllProperties()).containsOnlyKeys(allKeys);
+				assertThat(repo.getAllGroups()).containsOnlyKeys("spring.foo");
 				ConfigurationMetadataGroup group = repo.getAllGroups().get("spring.foo");
-				contains(group.getSources(), "org.acme.Foo", "org.acme.Foo2", "org.springframework.boot.FooProperties");
-				assertThat(group.getSources()).hasSize(3);
-				contains(group.getProperties(), "spring.foo.name", "spring.foo.description", "spring.foo.counter",
-						"spring.foo.enabled", "spring.foo.type");
-				assertThat(group.getProperties()).hasSize(5);
-				contains(repo.getAllProperties(), "spring.foo.name", "spring.foo.description", "spring.foo.counter",
-						"spring.foo.enabled", "spring.foo.type");
-				assertThat(repo.getAllProperties()).hasSize(5);
+				assertThat(group.getProperties()).containsOnlyKeys(allKeys);
+				assertThat(group.getSources()).containsOnlyKeys("org.acme.Foo", "org.acme.Foo2",
+						"org.springframework.boot.FooProperties");
+				assertThat(group.getSources().get("org.acme.Foo").getProperties()).containsOnlyKeys("spring.foo.name",
+						"spring.foo.description");
+				assertThat(group.getSources().get("org.acme.Foo2").getProperties())
+						.containsOnlyKeys("spring.foo.enabled", "spring.foo.type");
+				assertThat(group.getSources().get("org.springframework.boot.FooProperties").getProperties())
+						.containsOnlyKeys("spring.foo.name", "spring.foo.counter");
 			}
 		}
 	}
 
-	
-	/*
-	 * A rewrite of severalRepositoriesIdenticalGroups() using "containsOnlyKeys" to show actual vs. expected when assert fails.
-	 */
 	@Test
-	void severalRepositoriesIdenticalGroups_rewritten() throws IOException {
+	void severalRepositoriesIdenticalGroupsWithSameType() throws IOException {
 		try (InputStream foo = getInputStreamFor("foo")) {
-			try (InputStream foo2 = getInputStreamFor("foo2")) {
-				ConfigurationMetadataRepository repo = ConfigurationMetadataRepositoryJsonBuilder.create(foo, foo2)
+			try (InputStream foo3 = getInputStreamFor("foo3")) {
+				ConfigurationMetadataRepository repo = ConfigurationMetadataRepositoryJsonBuilder.create(foo, foo3)
 						.build();
-				
-				// assert all properties are found
-				assertThat(repo.getAllProperties()).containsOnlyKeys(
-						"spring.foo.name", 
-						"spring.foo.description", 
-						"spring.foo.counter", 
-						"spring.foo.enabled", 
-						"spring.foo.type");
-				
-				// we have a single group containing all properties
+				Iterable<String> allKeys = Arrays.asList("spring.foo.name", "spring.foo.description",
+						"spring.foo.counter", "spring.foo.enabled", "spring.foo.type");
+				assertThat(repo.getAllProperties()).containsOnlyKeys(allKeys);
 				assertThat(repo.getAllGroups()).containsOnlyKeys("spring.foo");
-				
 				ConfigurationMetadataGroup group = repo.getAllGroups().get("spring.foo");
-				assertThat(group.getProperties()).containsOnlyKeys(
-						"spring.foo.name", 
-						"spring.foo.description", 
-						"spring.foo.counter", 
-						"spring.foo.enabled", 
-						"spring.foo.type");
-				
-				// the group contains 3 different sources
-				assertThat(group.getSources()).containsOnlyKeys(
-						"org.acme.Foo", "org.acme.Foo2", "org.springframework.boot.FooProperties");
-				
-				assertThat(group.getSources().get("org.acme.Foo").getProperties()).containsOnlyKeys(
-						"spring.foo.name",
-						"spring.foo.description");
-				
-				assertThat(group.getSources().get("org.acme.Foo2").getProperties()).containsOnlyKeys(
-						"spring.foo.enabled",
-						"spring.foo.type");
-				
-				assertThat(group.getSources().get("org.springframework.boot.FooProperties").getProperties()).containsOnlyKeys(
-						"spring.foo.name",
-						"spring.foo.counter");
+				assertThat(group.getProperties()).containsOnlyKeys(allKeys);
+				assertThat(group.getSources()).containsOnlyKeys("org.acme.Foo",
+						"org.springframework.boot.FooProperties");
+				assertThat(group.getSources().get("org.acme.Foo").getProperties()).containsOnlyKeys("spring.foo.name",
+						"spring.foo.description", "spring.foo.enabled", "spring.foo.type");
+				assertThat(group.getSources().get("org.springframework.boot.FooProperties").getProperties())
+						.containsOnlyKeys("spring.foo.name", "spring.foo.counter");
 			}
 		}
 	}
-	
-	/*
-	 * "foo3" contains the same properties as "foo2" except they refer to a group that already exists in 
-	 * "foo1" (same NAME, same TYPE).
-	 * 
-	 * This test shows that the union of properties collected from the sources is less than what the group actually 
-	 * contains (some properties are missing).
-	 */
+
 	@Test
-	void severalRepositoriesIdenticalGroups3() throws IOException {
+	void severalRepositoriesIdenticalGroupsWithSameTypeDoesNotOverrideSource() throws IOException {
 		try (InputStream foo = getInputStreamFor("foo")) {
 			try (InputStream foo3 = getInputStreamFor("foo3")) {
 				ConfigurationMetadataRepository repo = ConfigurationMetadataRepositoryJsonBuilder.create(foo, foo3)
 						.build();
-				
-				assertThat(repo.getAllProperties()).containsOnlyKeys(
-						"spring.foo.name", 
-						"spring.foo.description", 
-						"spring.foo.counter", 
-						"spring.foo.enabled", 
-						"spring.foo.type");
-				
-				assertThat(repo.getAllGroups()).containsOnlyKeys("spring.foo");
-				
 				ConfigurationMetadataGroup group = repo.getAllGroups().get("spring.foo");
-				assertThat(group.getProperties()).containsOnlyKeys(
-						"spring.foo.name", 
-						"spring.foo.description", 
-						"spring.foo.counter", 
-						"spring.foo.enabled", 
-						"spring.foo.type");
-				
-				
-				assertThat(group.getSources()).containsOnlyKeys("org.acme.Foo", "org.springframework.boot.FooProperties");
-				assertThat(group.getSources().get("org.acme.Foo").getProperties()).containsOnlyKeys(
-						"spring.foo.name",
-						"spring.foo.description",
-						"spring.foo.enabled",		// <-- missing although present in repo.getAllProperties()
-						"spring.foo.type");			// <-- missing although present in repo.getAllProperties()
-				
-				assertThat(group.getSources().get("org.springframework.boot.FooProperties").getProperties()).containsOnlyKeys(
-						"spring.foo.name",
-						"spring.foo.counter");
+				ConfigurationMetadataSource fooSource = group.getSources().get("org.acme.Foo");
+				assertThat(fooSource.getSourceMethod()).isEqualTo("foo()");
+				assertThat(fooSource.getDescription()).isEqualTo("This is Foo.");
 			}
 		}
 	}
-	
-	
+
 	@Test
 	void emptyGroups() throws IOException {
 		try (InputStream in = getInputStreamFor("empty-groups")) {

spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json

@@ -4,8 +4,8 @@
       "name": "spring.foo",
       "type": "org.acme.Foo",
       "sourceType": "org.acme.config.FooApp",
-      "sourceMethod": "foo2()",
-      "description": "This is Foo."
+      "sourceMethod": "foo3()",
+      "description": "This is Foo3."
     }
   ],
   "properties": [