From 62225e6905bb20ed4b34a46d4a5796a0bc308bdd Mon Sep 17 00:00:00 2001 From: Ivo Smid Date: Sun, 21 Jun 2020 20:35:46 +0200 Subject: [PATCH] Optional/Null parameter used within @SpanTag --- .../annotation/SpanTagAnnotationHandler.java | 13 +- .../NullSpanTagAnnotationHandlerTests.java | 136 ++++++++++++++++++ .../web/TraceFilterWebIntegrationTests.java | 39 +++++ 3 files changed, 182 insertions(+), 6 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/annotation/NullSpanTagAnnotationHandlerTests.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/annotation/SpanTagAnnotationHandler.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/annotation/SpanTagAnnotationHandler.java index 1e5c02ceb..c7b48b36e 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/annotation/SpanTagAnnotationHandler.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/annotation/SpanTagAnnotationHandler.java @@ -146,19 +146,20 @@ class SpanTagAnnotationHandler { } String resolveTagValue(SpanTag annotation, Object argument) { - if (argument == null) { - return ""; - } + String value = null; if (annotation.resolver() != NoOpTagValueResolver.class) { TagValueResolver tagValueResolver = this.beanFactory .getBean(annotation.resolver()); - return tagValueResolver.resolve(argument); + value = tagValueResolver.resolve(argument); } else if (StringUtils.hasText(annotation.expression())) { - return this.beanFactory.getBean(TagValueExpressionResolver.class) + value = this.beanFactory.getBean(TagValueExpressionResolver.class) .resolve(annotation.expression(), argument); } - return argument.toString(); + else if (argument != null) { + value = argument.toString(); + } + return value == null ? "" : value; } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/annotation/NullSpanTagAnnotationHandlerTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/annotation/NullSpanTagAnnotationHandlerTests.java new file mode 100644 index 000000000..7245cdf41 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/annotation/NullSpanTagAnnotationHandlerTests.java @@ -0,0 +1,136 @@ +/* + * Copyright 2013-2019 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.sleuth.annotation; + +import java.lang.annotation.Annotation; +import java.lang.reflect.Method; + +import brave.sampler.Sampler; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; + +@SpringBootTest(classes = NullSpanTagAnnotationHandlerTests.TestConfiguration.class) + +public class NullSpanTagAnnotationHandlerTests { + + @Autowired + BeanFactory beanFactory; + + @Autowired + TagValueResolver tagValueResolver; + + SpanTagAnnotationHandler handler; + + @BeforeEach + public void setup() { + this.handler = new SpanTagAnnotationHandler(this.beanFactory); + } + + @Test + public void shouldUseEmptyStringWheCustomTagValueResolverReturnsNull() + throws NoSuchMethodException, SecurityException { + Method method = AnnotationMockClass.class + .getMethod("getAnnotationForTagValueResolver", String.class); + Annotation annotation = method.getParameterAnnotations()[0][0]; + if (annotation instanceof SpanTag) { + String resolvedValue = this.handler.resolveTagValue((SpanTag) annotation, + "test"); + assertThat(resolvedValue).isEqualTo(""); + } + else { + fail("Annotation was not SleuthSpanTag"); + } + } + + @Test + public void shouldUseEmptyStringWhenTagValueExpressionReturnNull() + throws NoSuchMethodException, SecurityException { + Method method = AnnotationMockClass.class + .getMethod("getAnnotationForTagValueExpression", String.class); + Annotation annotation = method.getParameterAnnotations()[0][0]; + if (annotation instanceof SpanTag) { + String resolvedValue = this.handler.resolveTagValue((SpanTag) annotation, + "test"); + + assertThat(resolvedValue).isEqualTo(""); + } + else { + fail("Annotation was not SleuthSpanTag"); + } + } + + @Test + public void shouldUseEmptyStringWhenArgumentIsNull() + throws NoSuchMethodException, SecurityException { + Method method = AnnotationMockClass.class + .getMethod("getAnnotationForArgumentToString", Long.class); + Annotation annotation = method.getParameterAnnotations()[0][0]; + if (annotation instanceof SpanTag) { + String resolvedValue = this.handler.resolveTagValue((SpanTag) annotation, + null); + assertThat(resolvedValue).isEqualTo(""); + } + else { + fail("Annotation was not SleuthSpanTag"); + } + } + + @Configuration + @EnableAutoConfiguration + protected static class TestConfiguration { + + @Bean + public TagValueResolver tagValueResolver() { + return parameter -> null; + } + + @Bean + Sampler alwaysSampler() { + return Sampler.ALWAYS_SAMPLE; + } + + } + + protected class AnnotationMockClass { + + @NewSpan + public void getAnnotationForTagValueResolver( + @SpanTag(key = "test", resolver = TagValueResolver.class) String test) { + } + + @NewSpan + public void getAnnotationForTagValueExpression( + @SpanTag(key = "test", expression = "null") String test) { + } + + @NewSpan + public void getAnnotationForArgumentToString(@SpanTag("test") Long param) { + } + + } + +} diff --git a/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java b/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java index 10540bf6c..24707cfd4 100644 --- a/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java +++ b/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java @@ -47,6 +47,8 @@ import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.system.CapturedOutput; import org.springframework.boot.test.system.OutputCaptureExtension; +import org.springframework.cloud.sleuth.annotation.ContinueSpan; +import org.springframework.cloud.sleuth.annotation.SpanTag; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.env.Environment; @@ -54,6 +56,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.http.client.ClientHttpResponse; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.client.DefaultResponseErrorHandler; import org.springframework.web.client.HttpClientErrorException; @@ -95,6 +98,24 @@ public class TraceFilterWebIntegrationTests { then(spanHandler.takeRemoteSpan(Kind.SERVER).tags()).containsKey("http.url"); } + @Test + public void should_tag_with_value_from_null_expression() { + new RestTemplate().getForObject("http://localhost:" + port() + "/null-parameter", + String.class); + + then(this.currentTraceContext.get()).isNull(); + then(spanHandler.takeRemoteSpan(Kind.SERVER).tags()).containsEntry("foo", "1001"); + } + + @Test + public void should_tag_with_value_from_non_null_expression() { + new RestTemplate().getForObject( + "http://localhost:" + port() + "/null-parameter?bar=10", String.class); + + then(this.currentTraceContext.get()).isNull(); + then(spanHandler.takeRemoteSpan(Kind.SERVER).tags()).containsEntry("foo", "11"); + } + @Test public void exception_logging_span_handler_logs_synchronous_exceptions( CapturedOutput capture) { @@ -164,6 +185,11 @@ public class TraceFilterWebIntegrationTests { return new GoodController(); } + @Bean + NullParameterController nullParameterController() { + return new NullParameterController(); + } + @Bean ExceptionThrowingController badController() { return new ExceptionThrowingController(); @@ -258,6 +284,19 @@ public class TraceFilterWebIntegrationTests { } + @RestController + public static class NullParameterController { + + @RequestMapping("/null-parameter") + @ContinueSpan + public String nullParameter( + @SpanTag(key = "foo", expression = "(#root?:1000)+1") @RequestParam( + value = "bar", required = false) Integer param) { + return "ok param=" + param; + } + + } + @RestController public static class ExceptionThrowingController {