From d00722594da449318b4cbcc7ce104d2bbc1c050b Mon Sep 17 00:00:00 2001 From: Lukas Bloder Date: Mon, 19 Feb 2024 09:30:11 +0100 Subject: [PATCH] Resolve spring properties in @SentryCheckIn annotation (#3194) * resolve spring properties in @SentryCheckIn annotation * format * add changelog * guard against exceptions when resolving property, add test * Format code * Update sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java Co-authored-by: Alexander Dinauer * Format code * cr changes, fix tests --------- Co-authored-by: Sentry Github Bot Co-authored-by: Alexander Dinauer --- CHANGELOG.md | 1 + .../api/sentry-spring-jakarta.api | 3 +- .../jakarta/checkin/SentryCheckInAdvice.java | 31 ++++- .../spring/jakarta/SentryCheckInAdviceTest.kt | 107 +++++++++++++++++- sentry-spring/api/sentry-spring.api | 3 +- .../spring/checkin/SentryCheckInAdvice.java | 34 +++++- .../sentry/spring/SentryCheckInAdviceTest.kt | 106 +++++++++++++++++ 7 files changed, 278 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 424ab1d889..754646ddc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Add new threshold parameters to monitor config ([#3181](https://github.com/getsentry/sentry-java/pull/3181)) - Report process init time as a span for app start performance ([#3159](https://github.com/getsentry/sentry-java/pull/3159)) - (perf-v2): Calculate frame delay on a span level ([#3197](https://github.com/getsentry/sentry-java/pull/3197)) +- Resolve spring properties in @SentryCheckIn annotation ([#3194](https://github.com/getsentry/sentry-java/pull/3194)) ### Fixes diff --git a/sentry-spring-jakarta/api/sentry-spring-jakarta.api b/sentry-spring-jakarta/api/sentry-spring-jakarta.api index 90cfc493f9..536c9e9af8 100644 --- a/sentry-spring-jakarta/api/sentry-spring-jakarta.api +++ b/sentry-spring-jakarta/api/sentry-spring-jakarta.api @@ -94,10 +94,11 @@ public abstract interface annotation class io/sentry/spring/jakarta/checkin/Sent public abstract fun value ()Ljava/lang/String; } -public class io/sentry/spring/jakarta/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor { +public class io/sentry/spring/jakarta/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor, org/springframework/context/EmbeddedValueResolverAware { public fun ()V public fun (Lio/sentry/IHub;)V public fun invoke (Lorg/aopalliance/intercept/MethodInvocation;)Ljava/lang/Object; + public fun setEmbeddedValueResolver (Lorg/springframework/util/StringValueResolver;)V } public class io/sentry/spring/jakarta/checkin/SentryCheckInAdviceConfiguration { diff --git a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java index fe3cfd8e0e..5a4e329fa8 100644 --- a/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java +++ b/sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java @@ -17,8 +17,10 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.springframework.aop.support.AopUtils; +import org.springframework.context.EmbeddedValueResolverAware; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.util.ObjectUtils; +import org.springframework.util.StringValueResolver; /** * Reports execution of every bean method annotated with {@link SentryCheckIn} as a monitor @@ -27,9 +29,11 @@ @ApiStatus.Internal @ApiStatus.Experimental @Open -public class SentryCheckInAdvice implements MethodInterceptor { +public class SentryCheckInAdvice implements MethodInterceptor, EmbeddedValueResolverAware { private final @NotNull IHub hub; + private @Nullable StringValueResolver resolver; + public SentryCheckInAdvice() { this(HubAdapter.getInstance()); } @@ -51,7 +55,25 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl } final boolean isHeartbeatOnly = checkInAnnotation.heartbeat(); - final @Nullable String monitorSlug = checkInAnnotation.value(); + + @Nullable String monitorSlug = checkInAnnotation.value(); + + if (resolver != null) { + try { + monitorSlug = resolver.resolveStringValue(checkInAnnotation.value()); + } catch (Throwable e) { + // When resolving fails, we fall back to the original string which may contain unresolved + // expressions. Testing shows this can also happen if properties cannot be resolved (without + // an exception being thrown). Sentry should alert the user about missed checkins in this + // case since the monitor slug won't match what is configured in Sentry. + hub.getOptions() + .getLogger() + .log( + SentryLevel.WARNING, + "Slug for method annotated with @SentryCheckIn could not be resolved from properties.", + e); + } + } if (ObjectUtils.isEmpty(monitorSlug)) { hub.getOptions() @@ -85,4 +107,9 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl hub.popScope(); } } + + @Override + public void setEmbeddedValueResolver(StringValueResolver resolver) { + this.resolver = resolver; + } } diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryCheckInAdviceTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryCheckInAdviceTest.kt index d6cdf8668c..05b97ba24c 100644 --- a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryCheckInAdviceTest.kt +++ b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryCheckInAdviceTest.kt @@ -21,13 +21,16 @@ import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.springframework.beans.factory.annotation.Autowired +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.EnableAspectJAutoProxy import org.springframework.context.annotation.Import +import org.springframework.context.support.PropertySourcesPlaceholderConfigurer +import org.springframework.test.context.TestPropertySource import org.springframework.test.context.junit.jupiter.SpringJUnitConfig import org.springframework.test.context.junit4.SpringRunner -import kotlin.RuntimeException +import org.springframework.util.StringValueResolver import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals @@ -35,6 +38,7 @@ import kotlin.test.assertNotNull @RunWith(SpringRunner::class) @SpringJUnitConfig(SentryCheckInAdviceTest.Config::class) +@TestPropertySource(properties = ["my.cron.slug = mypropertycronslug"]) class SentryCheckInAdviceTest { @Autowired @@ -46,6 +50,9 @@ class SentryCheckInAdviceTest { @Autowired lateinit var sampleServiceHeartbeat: SampleServiceHeartbeat + @Autowired + lateinit var sampleServiceSpringProperties: SampleServiceSpringProperties + @Autowired lateinit var hub: IHub @@ -157,6 +164,66 @@ class SentryCheckInAdviceTest { verify(hub, never()).popScope() } + @Test + fun `when @SentryCheckIn is passed a spring property it is resolved correctly`() { + val checkInId = SentryId() + val checkInCaptor = argumentCaptor() + whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId) + val result = sampleServiceSpringProperties.hello() + assertEquals(1, result) + assertEquals(1, checkInCaptor.allValues.size) + + val doneCheckIn = checkInCaptor.lastValue + assertEquals("mypropertycronslug", doneCheckIn.monitorSlug) + assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status) + assertNotNull(doneCheckIn.duration) + + val order = inOrder(hub) + order.verify(hub).pushScope() + order.verify(hub).captureCheckIn(any()) + order.verify(hub).popScope() + } + + @Test + fun `when @SentryCheckIn is passed a spring property that does not exist, raw value is used`() { + val checkInId = SentryId() + val checkInCaptor = argumentCaptor() + whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId) + val result = sampleServiceSpringProperties.helloUnresolvedProperty() + assertEquals(1, result) + assertEquals(1, checkInCaptor.allValues.size) + + val doneCheckIn = checkInCaptor.lastValue + assertEquals("\${my.cron.unresolved.property}", doneCheckIn.monitorSlug) + assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status) + assertNotNull(doneCheckIn.duration) + + val order = inOrder(hub) + order.verify(hub).pushScope() + order.verify(hub).captureCheckIn(any()) + order.verify(hub).popScope() + } + + @Test + fun `when @SentryCheckIn is passed a spring property that causes an exception, raw value is used`() { + val checkInId = SentryId() + val checkInCaptor = argumentCaptor() + whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId) + val result = sampleServiceSpringProperties.helloExceptionProperty() + assertEquals(1, result) + assertEquals(1, checkInCaptor.allValues.size) + + val doneCheckIn = checkInCaptor.lastValue + assertEquals("\${my.cron.exception.property}", doneCheckIn.monitorSlug) + assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status) + assertNotNull(doneCheckIn.duration) + + val order = inOrder(hub) + order.verify(hub).pushScope() + order.verify(hub).captureCheckIn(any()) + order.verify(hub).popScope() + } + @Configuration @EnableAspectJAutoProxy(proxyTargetClass = true) @Import(SentryCheckInAdviceConfiguration::class, SentryCheckInPointcutConfiguration::class) @@ -171,12 +238,21 @@ class SentryCheckInAdviceTest { @Bean open fun sampleServiceHeartbeat() = SampleServiceHeartbeat() + @Bean + open fun sampleServiceSpringProperties() = SampleServiceSpringProperties() + @Bean open fun hub(): IHub { val hub = mock() Sentry.setCurrentHub(hub) return hub } + + companion object { + @Bean + @JvmStatic + fun propertySourcesPlaceholderConfigurer() = MyPropertyPlaceholderConfigurer() + } } open class SampleService { @@ -206,4 +282,33 @@ class SentryCheckInAdviceTest { throw RuntimeException("thrown on purpose") } } + + open class SampleServiceSpringProperties { + + @SentryCheckIn("\${my.cron.slug}", heartbeat = true) + open fun hello() = 1 + + @SentryCheckIn("\${my.cron.unresolved.property}", heartbeat = true) + open fun helloUnresolvedProperty() = 1 + + @SentryCheckIn("\${my.cron.exception.property}", heartbeat = true) + open fun helloExceptionProperty() = 1 + } + + class MyPropertyPlaceholderConfigurer : PropertySourcesPlaceholderConfigurer() { + + override fun doProcessProperties( + beanFactoryToProcess: ConfigurableListableBeanFactory, + valueResolver: StringValueResolver + ) { + val wrappedResolver = StringValueResolver { strVal: String -> + if ("\${my.cron.exception.property}".equals(strVal)) { + throw IllegalArgumentException("Cannot resolve property: $strVal") + } else { + valueResolver.resolveStringValue(strVal) + } + } + super.doProcessProperties(beanFactoryToProcess, wrappedResolver) + } + } } diff --git a/sentry-spring/api/sentry-spring.api b/sentry-spring/api/sentry-spring.api index 857e29fe6d..9ef6b5bb3a 100644 --- a/sentry-spring/api/sentry-spring.api +++ b/sentry-spring/api/sentry-spring.api @@ -94,10 +94,11 @@ public abstract interface annotation class io/sentry/spring/checkin/SentryCheckI public abstract fun value ()Ljava/lang/String; } -public class io/sentry/spring/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor { +public class io/sentry/spring/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor, org/springframework/context/EmbeddedValueResolverAware { public fun ()V public fun (Lio/sentry/IHub;)V public fun invoke (Lorg/aopalliance/intercept/MethodInvocation;)Ljava/lang/Object; + public fun setEmbeddedValueResolver (Lorg/springframework/util/StringValueResolver;)V } public class io/sentry/spring/checkin/SentryCheckInAdviceConfiguration { diff --git a/sentry-spring/src/main/java/io/sentry/spring/checkin/SentryCheckInAdvice.java b/sentry-spring/src/main/java/io/sentry/spring/checkin/SentryCheckInAdvice.java index dc87745faf..c29ad44900 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/checkin/SentryCheckInAdvice.java +++ b/sentry-spring/src/main/java/io/sentry/spring/checkin/SentryCheckInAdvice.java @@ -17,8 +17,10 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.springframework.aop.support.AopUtils; +import org.springframework.context.EmbeddedValueResolverAware; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.util.ObjectUtils; +import org.springframework.util.StringValueResolver; /** * Reports execution of every bean method annotated with {@link SentryCheckIn} as a monitor @@ -27,9 +29,11 @@ @ApiStatus.Internal @ApiStatus.Experimental @Open -public class SentryCheckInAdvice implements MethodInterceptor { +public class SentryCheckInAdvice implements MethodInterceptor, EmbeddedValueResolverAware { private final @NotNull IHub hub; + private @Nullable StringValueResolver resolver; + public SentryCheckInAdvice() { this(HubAdapter.getInstance()); } @@ -51,7 +55,28 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl } final boolean isHeartbeatOnly = checkInAnnotation.heartbeat(); - final @Nullable String monitorSlug = checkInAnnotation.value(); + + @Nullable String monitorSlug = checkInAnnotation.value(); + + if (resolver != null) { + try { + monitorSlug = resolver.resolveStringValue(checkInAnnotation.value()); + } catch (Throwable e) { + // When resolving fails, we fall back to the original string which may contain unresolved + // expressions. + // Testing shows this can also happen if properties cannot be resolved (without an exception + // being thrown). + // Sentry should alert the user about missed checkins in this case since the monitor slug + // won't match + // what is configured in Sentry. + hub.getOptions() + .getLogger() + .log( + SentryLevel.WARNING, + "Slug for method annotated with @SentryCheckIn could not be resolved from properties.", + e); + } + } if (ObjectUtils.isEmpty(monitorSlug)) { hub.getOptions() @@ -85,4 +110,9 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl hub.popScope(); } } + + @Override + public void setEmbeddedValueResolver(StringValueResolver resolver) { + this.resolver = resolver; + } } diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryCheckInAdviceTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryCheckInAdviceTest.kt index a04655a000..4f94fd7ce0 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryCheckInAdviceTest.kt +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryCheckInAdviceTest.kt @@ -21,12 +21,16 @@ import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.springframework.beans.factory.annotation.Autowired +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.EnableAspectJAutoProxy import org.springframework.context.annotation.Import +import org.springframework.context.support.PropertySourcesPlaceholderConfigurer +import org.springframework.test.context.TestPropertySource import org.springframework.test.context.junit.jupiter.SpringJUnitConfig import org.springframework.test.context.junit4.SpringRunner +import org.springframework.util.StringValueResolver import kotlin.RuntimeException import kotlin.test.BeforeTest import kotlin.test.Test @@ -35,6 +39,7 @@ import kotlin.test.assertNotNull @RunWith(SpringRunner::class) @SpringJUnitConfig(SentryCheckInAdviceTest.Config::class) +@TestPropertySource(properties = ["my.cron.slug = mypropertycronslug"]) class SentryCheckInAdviceTest { @Autowired @@ -46,6 +51,9 @@ class SentryCheckInAdviceTest { @Autowired lateinit var sampleServiceHeartbeat: SampleServiceHeartbeat + @Autowired + lateinit var sampleServiceSpringProperties: SampleServiceSpringProperties + @Autowired lateinit var hub: IHub @@ -157,6 +165,66 @@ class SentryCheckInAdviceTest { verify(hub, never()).popScope() } + @Test + fun `when @SentryCheckIn is passed a spring property it is resolved correctly`() { + val checkInId = SentryId() + val checkInCaptor = argumentCaptor() + whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId) + val result = sampleServiceSpringProperties.hello() + assertEquals(1, result) + assertEquals(1, checkInCaptor.allValues.size) + + val doneCheckIn = checkInCaptor.lastValue + assertEquals("mypropertycronslug", doneCheckIn.monitorSlug) + assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status) + assertNotNull(doneCheckIn.duration) + + val order = inOrder(hub) + order.verify(hub).pushScope() + order.verify(hub).captureCheckIn(any()) + order.verify(hub).popScope() + } + + @Test + fun `when @SentryCheckIn is passed a spring property that does not exist, raw value is used`() { + val checkInId = SentryId() + val checkInCaptor = argumentCaptor() + whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId) + val result = sampleServiceSpringProperties.helloUnresolvedProperty() + assertEquals(1, result) + assertEquals(1, checkInCaptor.allValues.size) + + val doneCheckIn = checkInCaptor.lastValue + assertEquals("\${my.cron.unresolved.property}", doneCheckIn.monitorSlug) + assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status) + assertNotNull(doneCheckIn.duration) + + val order = inOrder(hub) + order.verify(hub).pushScope() + order.verify(hub).captureCheckIn(any()) + order.verify(hub).popScope() + } + + @Test + fun `when @SentryCheckIn is passed a spring property that causes an exception, raw value is used`() { + val checkInId = SentryId() + val checkInCaptor = argumentCaptor() + whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId) + val result = sampleServiceSpringProperties.helloExceptionProperty() + assertEquals(1, result) + assertEquals(1, checkInCaptor.allValues.size) + + val doneCheckIn = checkInCaptor.lastValue + assertEquals("\${my.cron.exception.property}", doneCheckIn.monitorSlug) + assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status) + assertNotNull(doneCheckIn.duration) + + val order = inOrder(hub) + order.verify(hub).pushScope() + order.verify(hub).captureCheckIn(any()) + order.verify(hub).popScope() + } + @Configuration @EnableAspectJAutoProxy(proxyTargetClass = true) @Import(SentryCheckInAdviceConfiguration::class, SentryCheckInPointcutConfiguration::class) @@ -171,12 +239,21 @@ class SentryCheckInAdviceTest { @Bean open fun sampleServiceHeartbeat() = SampleServiceHeartbeat() + @Bean + open fun sampleServiceSpringProperties() = SampleServiceSpringProperties() + @Bean open fun hub(): IHub { val hub = mock() Sentry.setCurrentHub(hub) return hub } + + companion object { + @Bean + @JvmStatic + fun propertySourcesPlaceholderConfigurer() = MyPropertyPlaceholderConfigurer() + } } open class SampleService { @@ -206,4 +283,33 @@ class SentryCheckInAdviceTest { throw RuntimeException("thrown on purpose") } } + + open class SampleServiceSpringProperties { + + @SentryCheckIn("\${my.cron.slug}", heartbeat = true) + open fun hello() = 1 + + @SentryCheckIn("\${my.cron.unresolved.property}", heartbeat = true) + open fun helloUnresolvedProperty() = 1 + + @SentryCheckIn("\${my.cron.exception.property}", heartbeat = true) + open fun helloExceptionProperty() = 1 + } + + class MyPropertyPlaceholderConfigurer : PropertySourcesPlaceholderConfigurer() { + + override fun doProcessProperties( + beanFactoryToProcess: ConfigurableListableBeanFactory, + valueResolver: StringValueResolver + ) { + val wrappedResolver = StringValueResolver { strVal: String -> + if ("\${my.cron.exception.property}".equals(strVal)) { + throw IllegalArgumentException("Cannot resolve property: $strVal") + } else { + valueResolver.resolveStringValue(strVal) + } + } + super.doProcessProperties(beanFactoryToProcess, wrappedResolver) + } + } }