Skip to content

Commit

Permalink
Polish "Include properties in source merge algorithm"
Browse files Browse the repository at this point in the history 
See gh-25507
  • Loading branch information
@snicoll
snicoll committed Mar 5, 2021
1 parent cf4bc6e commit 6ebc69d
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 105 deletions.
28 changes: 13 additions & 15 deletions ...org/springframework/boot/configurationmetadata/SimpleConfigurationMetadataRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public void add(Collection<ConfigurationMetadataSource> sources) {
}
String sourceType = source.getType();
if (sourceType != null) {
putIfAbsent(group.getSources(), sourceType, source);
addOrMergeSource(group.getSources(), sourceType, source);
}
}
}
Expand Down Expand Up @@ -93,7 +93,7 @@ public void include(ConfigurationMetadataRepository repository) {
// 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));
}
}

Expand All @@ -111,23 +111,21 @@ private ConfigurationMetadataGroup getGroup(ConfigurationMetadataSource source)
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));
}
}
*/
}
121 changes: 33 additions & 88 deletions ...framework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.Arrays;
import java.util.Map;

import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -99,116 +100,60 @@ void severalRepositoriesIdenticalGroups() throws IOException {
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")) {
Expand Down
4 changes: 2 additions & 2 deletions ...-boot-configuration-metadata/src/test/resources/metadata/configuration-metadata-foo3.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down

0 comments on commit 6ebc69d

Please sign in to comment.