From c530f12cc325ce119fb1ffbbd8a8d277dfac23f1 Mon Sep 17 00:00:00 2001 From: Guirong Hu Date: Mon, 11 Jul 2022 18:11:09 +0800 Subject: [PATCH 1/2] Call the value adapter during NamedContributorsMapAdapter construction Update `NamedContributorsMapAdapter` so that the adapter function is called only once per entry. Prior to this commit, the adapter was called dynamically which made `CompositeHealthContributor` behave differently from a regular `HealthContributor`. See gh-31676 --- .../health/NamedContributorsMapAdapter.java | 36 +++++++++++-------- .../NamedContributorsMapAdapterTests.java | 24 ++++++++++++- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java index 94d8c399b5..fcd4f8e0ea 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 the original author or authors. + * Copyright 2012-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -31,23 +31,29 @@ import org.springframework.util.Assert; * @param the value type * @param the contributor type * @author Phillip Webb + * @author Guirong Hu * @see CompositeHealthContributorMapAdapter * @see CompositeReactiveHealthContributorMapAdapter */ abstract class NamedContributorsMapAdapter implements NamedContributors { - private final Map map; - - private final Function valueAdapter; + private final Map namedContributorsMap; NamedContributorsMapAdapter(Map map, Function valueAdapter) { Assert.notNull(map, "Map must not be null"); Assert.notNull(valueAdapter, "ValueAdapter must not be null"); - map.keySet().forEach(this::validateKey); - map.values().stream().map(valueAdapter) - .forEach((value) -> Assert.notNull(value, "Map must not contain null values")); - this.map = Collections.unmodifiableMap(new LinkedHashMap<>(map)); - this.valueAdapter = valueAdapter; + this.namedContributorsMap = getContributorsMap(map, valueAdapter); + } + + private Map getContributorsMap(Map map, Function valueAdapter) { + Map contributorsMap = new LinkedHashMap<>(map.size()); + map.forEach((name, value) -> { + this.validateKey(name); + C contributor = adapt(value, valueAdapter); + Assert.notNull(contributor, "Map must not contain null values"); + contributorsMap.put(name, contributor); + }); + return Collections.unmodifiableMap(contributorsMap); } private void validateKey(String value) { @@ -58,7 +64,7 @@ abstract class NamedContributorsMapAdapter implements NamedContributors @Override public Iterator> iterator() { - Iterator> iterator = this.map.entrySet().iterator(); + Iterator> iterator = this.namedContributorsMap.entrySet().iterator(); return new Iterator>() { @Override @@ -68,8 +74,8 @@ abstract class NamedContributorsMapAdapter implements NamedContributors @Override public NamedContributor next() { - Entry entry = iterator.next(); - return NamedContributor.of(entry.getKey(), adapt(entry.getValue())); + Entry entry = iterator.next(); + return NamedContributor.of(entry.getKey(), entry.getValue()); } }; @@ -77,11 +83,11 @@ abstract class NamedContributorsMapAdapter implements NamedContributors @Override public C getContributor(String name) { - return adapt(this.map.get(name)); + return this.namedContributorsMap.get(name); } - private C adapt(V value) { - return (value != null) ? this.valueAdapter.apply(value) : null; + private C adapt(V value, Function valueAdapter) { + return (value != null) ? valueAdapter.apply(value) : null; } } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java index 636b3b7f63..9e6a23bdb6 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 the original author or authors. + * Copyright 2012-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ import java.util.Collections; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; import org.junit.jupiter.api.Test; @@ -92,6 +93,22 @@ class NamedContributorsMapAdapterTests { assertThat(adapter.getContributor("two")).isEqualTo("owt"); } + @Test + void eachValueAdapterShouldBeCalledOnlyOnce() { + Map map = new LinkedHashMap<>(); + map.put("one", "one"); + map.put("two", "two"); + int callCount = map.size(); + + AtomicInteger counter = new AtomicInteger(0); + TestNamedContributorsMapAdapter adapter = new TestNamedContributorsMapAdapter<>(map, + (name) -> count(name, counter)); + assertThat(adapter.getContributor("one")).isEqualTo("eno"); + assertThat(counter.get()).isEqualTo(callCount); + assertThat(adapter.getContributor("two")).isEqualTo("owt"); + assertThat(counter.get()).isEqualTo(callCount); + } + @Test void getContributorWhenNotInMapReturnsNull() { TestNamedContributorsMapAdapter adapter = createAdapter(); @@ -106,6 +123,11 @@ class NamedContributorsMapAdapterTests { return adapter; } + private String count(CharSequence charSequence, AtomicInteger counter) { + counter.incrementAndGet(); + return reverse(charSequence); + } + private String reverse(CharSequence charSequence) { return new StringBuilder(charSequence).reverse().toString(); } From 59c9a9cd8a5e6ebe2d60be102b8c10f804761aad Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 26 Jul 2022 18:12:08 +0100 Subject: [PATCH 2/2] Polish "Call the value adapter during NamedContributorsMapAdapter construction" See gh-31676 --- .../health/NamedContributorsMapAdapter.java | 32 +++++++------------ .../NamedContributorsMapAdapterTests.java | 4 +-- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java index fcd4f8e0ea..10040b8f01 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapter.java @@ -18,10 +18,10 @@ package org.springframework.boot.actuate.health; import java.util.Collections; import java.util.Iterator; -import java.util.LinkedHashMap; import java.util.Map; import java.util.Map.Entry; import java.util.function.Function; +import java.util.stream.Collectors; import org.springframework.util.Assert; @@ -37,34 +37,30 @@ import org.springframework.util.Assert; */ abstract class NamedContributorsMapAdapter implements NamedContributors { - private final Map namedContributorsMap; + private final Map map; NamedContributorsMapAdapter(Map map, Function valueAdapter) { Assert.notNull(map, "Map must not be null"); Assert.notNull(valueAdapter, "ValueAdapter must not be null"); - this.namedContributorsMap = getContributorsMap(map, valueAdapter); - } - - private Map getContributorsMap(Map map, Function valueAdapter) { - Map contributorsMap = new LinkedHashMap<>(map.size()); - map.forEach((name, value) -> { - this.validateKey(name); - C contributor = adapt(value, valueAdapter); - Assert.notNull(contributor, "Map must not contain null values"); - contributorsMap.put(name, contributor); - }); - return Collections.unmodifiableMap(contributorsMap); + map.keySet().forEach(this::validateKey); + this.map = Collections.unmodifiableMap(map.entrySet().stream() + .collect(Collectors.toMap(Entry::getKey, (entry) -> adapt(entry.getValue(), valueAdapter)))); } private void validateKey(String value) { Assert.notNull(value, "Map must not contain null keys"); Assert.isTrue(!value.contains("/"), "Map keys must not contain a '/'"); + } + private C adapt(V value, Function valueAdapter) { + C contributor = (value != null) ? valueAdapter.apply(value) : null; + Assert.notNull(contributor, "Map must not contain null values"); + return contributor; } @Override public Iterator> iterator() { - Iterator> iterator = this.namedContributorsMap.entrySet().iterator(); + Iterator> iterator = this.map.entrySet().iterator(); return new Iterator>() { @Override @@ -83,11 +79,7 @@ abstract class NamedContributorsMapAdapter implements NamedContributors @Override public C getContributor(String name) { - return this.namedContributorsMap.get(name); - } - - private C adapt(V value, Function valueAdapter) { - return (value != null) ? valueAdapter.apply(value) : null; + return this.map.get(name); } } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java index 9e6a23bdb6..c33214ef1d 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/health/NamedContributorsMapAdapterTests.java @@ -32,6 +32,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException * Tests for {@link NamedContributorsMapAdapter}. * * @author Phillip Webb + * @author Guirong Hu */ class NamedContributorsMapAdapterTests { @@ -94,12 +95,11 @@ class NamedContributorsMapAdapterTests { } @Test - void eachValueAdapterShouldBeCalledOnlyOnce() { + void getContributorCallsAdaptersOnlyOnce() { Map map = new LinkedHashMap<>(); map.put("one", "one"); map.put("two", "two"); int callCount = map.size(); - AtomicInteger counter = new AtomicInteger(0); TestNamedContributorsMapAdapter adapter = new TestNamedContributorsMapAdapter<>(map, (name) -> count(name, counter));