From d8141e6a8d4b51b7d9b41c2d3a8b2bc92227face Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 4 Oct 2021 20:45:21 -0700 Subject: [PATCH 1/2] Allow Producible enums to indicate a default value Add an `isDefault()` method to `Producible` which can be used to indicate which of the enum values should be used when the accept header is `*/*` or `null`. Prior to this commit, the last enum value was always used as the default. See gh-28130 --- .../boot/actuate/endpoint/Producible.java | 12 +++ .../ProducibleOperationArgumentResolver.java | 39 ++++++++-- ...ducibleOperationArgumentResolverTests.java | 75 ++++++++++++++++++- 3 files changed, 117 insertions(+), 9 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Producible.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Producible.java index fc2c487422..2bfe151766 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Producible.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Producible.java @@ -47,4 +47,16 @@ public interface Producible & Producible> { */ MimeType getProducedMimeType(); + /** + * Return if this enum value should be used as the default value when an accept header + * of */* is provided, or if the accept header is missing. Only one value + * can be marked as default. If no value is marked, then the value with the highest + * {@link Enum#ordinal() ordinal} is used as the default. + * @return if this value + * @since 2.5.6 + */ + default boolean isDefault() { + return false; + } + } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolver.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolver.java index e407286c7f..6e9b639a2f 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolver.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolver.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.List; import java.util.function.Supplier; +import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import org.springframework.util.MimeType; import org.springframework.util.MimeTypeUtils; @@ -29,6 +30,7 @@ import org.springframework.util.MimeTypeUtils; * An {@link OperationArgumentResolver} for {@link Producible producible enums}. * * @author Andy Wilkinson + * @author Phillip Webb * @since 2.5.0 */ public class ProducibleOperationArgumentResolver implements OperationArgumentResolver { @@ -56,30 +58,35 @@ public class ProducibleOperationArgumentResolver implements OperationArgumentRes private Enum> resolveProducible(Class>> type) { List accepts = this.accepts.get(); - List>> values = Arrays.asList(type.getEnumConstants()); - Collections.reverse(values); + List>> values = getValues(type); if (CollectionUtils.isEmpty(accepts)) { - return values.get(0); + return getDefaultValue(values); } Enum> result = null; for (String accept : accepts) { for (String mimeType : MimeTypeUtils.tokenize(accept)) { - result = mostRecent(result, forType(values, MimeTypeUtils.parseMimeType(mimeType))); + result = mostRecent(result, forMimeType(values, mimeType)); } } return result; } - private static Enum> mostRecent(Enum> existing, + private Enum> mostRecent(Enum> existing, Enum> candidate) { int existingOrdinal = (existing != null) ? existing.ordinal() : -1; int candidateOrdinal = (candidate != null) ? candidate.ordinal() : -1; return (candidateOrdinal > existingOrdinal) ? candidate : existing; } - private static Enum> forType(List>> candidates, - MimeType mimeType) { - for (Enum> candidate : candidates) { + private Enum> forMimeType(List>> values, String mimeType) { + if ("*/*".equals(mimeType)) { + return getDefaultValue(values); + } + return forMimeType(values, MimeTypeUtils.parseMimeType(mimeType)); + } + + private Enum> forMimeType(List>> values, MimeType mimeType) { + for (Enum> candidate : values) { if (mimeType.isCompatibleWith(((Producible) candidate).getProducedMimeType())) { return candidate; } @@ -87,4 +94,20 @@ public class ProducibleOperationArgumentResolver implements OperationArgumentRes return null; } + private List>> getValues(Class>> type) { + List>> values = Arrays.asList(type.getEnumConstants()); + Collections.reverse(values); + Assert.state(values.stream().filter(this::isDefault).count() <= 1, + "Multiple default values declared in " + type.getName()); + return values; + } + + private Enum> getDefaultValue(List>> values) { + return values.stream().filter(this::isDefault).findFirst().orElseGet(() -> values.get(0)); + } + + private boolean isDefault(Enum> value) { + return ((Producible) value).isDefault(); + } + } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolverTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolverTests.java index a27350dd55..a63dd31640 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolverTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ProducibleOperationArgumentResolverTests.java @@ -22,12 +22,16 @@ import java.util.function.Supplier; import org.junit.jupiter.api.Test; +import org.springframework.util.MimeType; + import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** * Test for {@link ProducibleOperationArgumentResolver}. * * @author Andy Wilkinson + * @author Phillip Webb */ class ProducibleOperationArgumentResolverTests { @@ -40,11 +44,21 @@ class ProducibleOperationArgumentResolverTests { assertThat(resolve(acceptHeader())).isEqualTo(ApiVersion.V3); } + @Test + void whenAcceptHeaderIsEmptyAndWithDefaultThenDefaultIsReturned() { + assertThat(resolve(acceptHeader(), WithDefault.class)).isEqualTo(WithDefault.TWO); + } + @Test void whenEverythingIsAcceptableThenHighestOrdinalIsReturned() { assertThat(resolve(acceptHeader("*/*"))).isEqualTo(ApiVersion.V3); } + @Test + void whenEverythingIsAcceptableWithDefaultThenDefaultIsReturned() { + assertThat(resolve(acceptHeader("*/*"), WithDefault.class)).isEqualTo(WithDefault.TWO); + } + @Test void whenNothingIsAcceptableThenNullIsReturned() { assertThat(resolve(acceptHeader("image/png"))).isEqualTo(null); @@ -68,13 +82,72 @@ class ProducibleOperationArgumentResolverTests { assertThat(resolve(acceptHeader(V2_JSON + "," + V3_JSON))).isEqualTo(ApiVersion.V3); } + @Test + void withMultipleValuesOneOfWhichIsAllReturnsDefault() { + assertThat(resolve(acceptHeader("one/one", "*/*"), WithDefault.class)).isEqualTo(WithDefault.TWO); + } + + @Test + void whenMultipleDefaultsThrowsException() { + assertThatIllegalStateException().isThrownBy(() -> resolve(acceptHeader("one/one"), WithMultipleDefaults.class)) + .withMessageContaining("Multiple default values"); + } + private Supplier> acceptHeader(String... types) { List value = Arrays.asList(types); return () -> (value.isEmpty() ? null : value); } private ApiVersion resolve(Supplier> accepts) { - return new ProducibleOperationArgumentResolver(accepts).resolve(ApiVersion.class); + return resolve(accepts, ApiVersion.class); + } + + private T resolve(Supplier> accepts, Class type) { + return new ProducibleOperationArgumentResolver(accepts).resolve(type); + } + + enum WithDefault implements Producible { + + ONE("one/one"), + + TWO("two/two") { + + @Override + public boolean isDefault() { + return true; + } + + }, + + THREE("three/three"); + + private final MimeType mimeType; + + WithDefault(String mimeType) { + this.mimeType = MimeType.valueOf(mimeType); + } + + @Override + public MimeType getProducedMimeType() { + return this.mimeType; + } + + } + + enum WithMultipleDefaults implements Producible { + + ONE, TWO, THREE; + + @Override + public boolean isDefault() { + return true; + } + + @Override + public MimeType getProducedMimeType() { + return MimeType.valueOf("image/jpeg"); + } + } } From 437a1601efe8bd7b9983bbeec27b360368bfe80b Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Mon, 4 Oct 2021 20:10:46 +0100 Subject: [PATCH 2/2] Fix broken content negotiation for Prometheus with OpenMetrics Update Prometheus `TextOutputFormat` so that OpenMetrics is used in preference to text output when an appropriate accept header is found. If the accept header contains `*/*` or is missing then the text format will be used. See gh-28130 --- .../export/prometheus/TextOutputFormat.java | 17 +++++++++++------ ...rometheusScrapeEndpointIntegrationTests.java | 8 ++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/prometheus/TextOutputFormat.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/prometheus/TextOutputFormat.java index 4831d37cf3..54b16b7110 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/prometheus/TextOutputFormat.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/prometheus/TextOutputFormat.java @@ -36,25 +36,30 @@ import org.springframework.util.MimeTypeUtils; public enum TextOutputFormat implements Producible { /** - * OpenMetrics text version 1.0.0. + * Prometheus text version 0.0.4. */ - CONTENT_TYPE_OPENMETRICS_100(TextFormat.CONTENT_TYPE_OPENMETRICS_100) { + CONTENT_TYPE_004(TextFormat.CONTENT_TYPE_004) { @Override void write(Writer writer, Enumeration samples) throws IOException { - TextFormat.writeOpenMetrics100(writer, samples); + TextFormat.write004(writer, samples); + } + + @Override + public boolean isDefault() { + return true; } }, /** - * Prometheus text version 0.0.4. + * OpenMetrics text version 1.0.0. */ - CONTENT_TYPE_004(TextFormat.CONTENT_TYPE_004) { + CONTENT_TYPE_OPENMETRICS_100(TextFormat.CONTENT_TYPE_OPENMETRICS_100) { @Override void write(Writer writer, Enumeration samples) throws IOException { - TextFormat.write004(writer, samples); + TextFormat.writeOpenMetrics100(writer, samples); } }; diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/prometheus/PrometheusScrapeEndpointIntegrationTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/prometheus/PrometheusScrapeEndpointIntegrationTests.java index 9b0419efee..dbce312303 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/prometheus/PrometheusScrapeEndpointIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/prometheus/PrometheusScrapeEndpointIntegrationTests.java @@ -55,6 +55,14 @@ class PrometheusScrapeEndpointIntegrationTests { .contains("counter1_total").contains("counter2_total").contains("counter3_total")); } + @WebEndpointTest + void scrapePrefersToProduceOpenMetrics100(WebTestClient client) { + MediaType openMetrics = MediaType.parseMediaType(TextFormat.CONTENT_TYPE_OPENMETRICS_100); + MediaType textPlain = MediaType.parseMediaType(TextFormat.CONTENT_TYPE_004); + client.get().uri("/actuator/prometheus").accept(openMetrics, textPlain).exchange().expectStatus().isOk() + .expectHeader().contentType(openMetrics); + } + @WebEndpointTest void scrapeWithIncludedNames(WebTestClient client) { client.get().uri("/actuator/prometheus?includedNames=counter1_total,counter2_total").exchange().expectStatus()