From cf4bc6e9e0d57a148e955b53433916e7b9ea1840 Mon Sep 17 00:00:00 2001 From: Bertrand Renuart Date: Fri, 5 Mar 2021 11:30:16 +0100 Subject: [PATCH 1/2] Include properties in source merge algorithm This commit improves SimpleConfigurationMetadataRepository to include properties that are contributed to an existing configuration metadata source. See gh-25507 --- ...SimpleConfigurationMetadataRepository.java | 13 +++ ...ionMetadataRepositoryJsonBuilderTests.java | 96 +++++++++++++++++++ .../metadata/configuration-metadata-foo3.json | 23 +++++ 3 files changed, 132 insertions(+) create mode 100644 spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java index 5becb4d6906..1d9a2553495 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java @@ -117,4 +117,17 @@ public class SimpleConfigurationMetadataRepository implements ConfigurationMetad } } +/* +Uncomment this code to fix issue revealed by ConfigurationMetadataRepositoryJsonBuilderTests#severalRepositoriesIdenticalGroups3() + + private void putIfAbsent(Map 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)); + } + } +*/ } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java index 0706b9036d1..b22da1be470 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java @@ -113,6 +113,102 @@ class ConfigurationMetadataRepositoryJsonBuilderTests extends AbstractConfigurat } } + + /* + * A rewrite of severalRepositoriesIdenticalGroups() using "containsOnlyKeys" to show actual vs. expected when assert fails. + */ + @Test + void severalRepositoriesIdenticalGroups_rewritten() throws IOException { + try (InputStream foo = getInputStreamFor("foo")) { + try (InputStream foo2 = getInputStreamFor("foo2")) { + ConfigurationMetadataRepository repo = ConfigurationMetadataRepositoryJsonBuilder.create(foo, foo2) + .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 + 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"); + } + } + } + + /* + * "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 { + 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"); + } + } + } + + @Test void emptyGroups() throws IOException { try (InputStream in = getInputStreamFor("empty-groups")) { diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json new file mode 100644 index 00000000000..417cf5d613e --- /dev/null +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json @@ -0,0 +1,23 @@ +{ + "groups": [ + { + "name": "spring.foo", + "type": "org.acme.Foo", + "sourceType": "org.acme.config.FooApp", + "sourceMethod": "foo2()", + "description": "This is Foo." + } + ], + "properties": [ + { + "name": "spring.foo.enabled", + "type": "java.lang.Boolean", + "sourceType": "org.acme.Foo" + }, + { + "name": "spring.foo.type", + "type": "java.lang.String", + "sourceType": "org.acme.Foo" + } + ] +} \ No newline at end of file From 6ebc69d704f0abebf0a0ab6d9b490a98720ecbab Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Fri, 5 Mar 2021 15:45:00 +0100 Subject: [PATCH 2/2] Polish "Include properties in source merge algorithm" See gh-25507 --- ...SimpleConfigurationMetadataRepository.java | 28 ++-- ...ionMetadataRepositoryJsonBuilderTests.java | 135 ++++++------------ .../metadata/configuration-metadata-foo3.json | 4 +- 3 files changed, 55 insertions(+), 112 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java index 1d9a2553495..03948f699cb 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/main/java/org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java +++ b/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 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 void putIfAbsent(Map 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 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)); - } - } -*/ } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java index b22da1be470..fd5dbcd3c19 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java +++ b/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 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 { - try (InputStream foo = getInputStreamFor("foo")) { - try (InputStream foo2 = getInputStreamFor("foo2")) { - ConfigurationMetadataRepository repo = ConfigurationMetadataRepositoryJsonBuilder.create(foo, foo2) - .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 - 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"); - } - } - } - - /* - * "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 severalRepositoriesIdenticalGroupsWithSameType() 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"); - + Iterable 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"); - - - 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"); + 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"); } } } - - + + @Test + void severalRepositoriesIdenticalGroupsWithSameTypeDoesNotOverrideSource() throws IOException { + try (InputStream foo = getInputStreamFor("foo")) { + try (InputStream foo3 = getInputStreamFor("foo3")) { + ConfigurationMetadataRepository repo = ConfigurationMetadataRepositoryJsonBuilder.create(foo, foo3) + .build(); + ConfigurationMetadataGroup group = repo.getAllGroups().get("spring.foo"); + 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")) { diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json b/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json index 417cf5d613e..e3ea2f120ce 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json +++ b/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": [