Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ dependencies {
// OpenFeature SDK
implementation(libs.openfeature)

// okhttp client instrumentation
implementation(projects.sentryOkhttp)
implementation(libs.okhttp)

// database query tracing
implementation(projects.sentryJdbc)
runtimeOnly(libs.hsqldb)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import io.sentry.samples.spring.boot.jakarta.quartz.SampleJob;
import java.util.Collections;
import okhttp3.OkHttpClient;
import org.quartz.JobDetail;
import org.quartz.SimpleTrigger;
import org.springframework.boot.SpringApplication;
Expand Down Expand Up @@ -42,6 +43,12 @@ RestClient restClient(RestClient.Builder builder) {
return builder.build();
}

@Bean
OkHttpClient okHttpClient() {
// automatically instrumented by Sentry via sentry.clients.ok-http-enabled=true
return new OkHttpClient.Builder().build();
}

@Bean
public JobDetailFactoryBean jobDetail() {
JobDetailFactoryBean jobDetailFactory = new JobDetailFactoryBean();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
package io.sentry.samples.spring.boot.jakarta;

import com.fasterxml.jackson.databind.ObjectMapper;
import io.sentry.reactor.SentryReactorUtils;
import java.io.IOException;
import java.io.UncheckedIOException;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RestController;
Expand All @@ -16,11 +22,20 @@ public class TodoController {
private final RestTemplate restTemplate;
private final WebClient webClient;
private final RestClient restClient;
private final OkHttpClient okHttpClient;
private final ObjectMapper objectMapper;

public TodoController(RestTemplate restTemplate, WebClient webClient, RestClient restClient) {
public TodoController(
RestTemplate restTemplate,
WebClient webClient,
RestClient restClient,
OkHttpClient okHttpClient,
ObjectMapper objectMapper) {
this.restTemplate = restTemplate;
this.webClient = webClient;
this.restClient = restClient;
this.okHttpClient = okHttpClient;
this.objectMapper = objectMapper;
}

@GetMapping("/todo/{id}")
Expand Down Expand Up @@ -54,4 +69,15 @@ Todo todoRestClient(@PathVariable Long id) {
.retrieve()
.body(Todo.class);
}

@GetMapping("/todo-okhttp/{id}")
Todo todoOkHttp(@PathVariable Long id) {
final Request request =
new Request.Builder().url("https://jsonplaceholder.typicode.com/todos/" + id).build();
try (Response response = okHttpClient.newCall(request).execute()) {
return objectMapper.readValue(response.body().byteStream(), Todo.class);
Comment on lines +77 to +78

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The code does not check if response.body() is null before calling .byteStream(), which can lead to a NullPointerException.
Severity: LOW

Suggested Fix

Add a null check on the ResponseBody object returned by response.body() before attempting to access its methods. If the body is null, handle the case appropriately, for instance, by throwing an IOException or returning an empty or error response.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
sentry-samples/sentry-samples-spring-boot-jakarta/src/main/java/io/sentry/samples/spring/boot/jakarta/TodoController.java#L77-L78

Potential issue: The code in `TodoController.java` makes a call to an external service
using OkHttp. The result of `response.body()` can be `null` in certain situations, such
as when a response is served from a cache. The code directly calls `.byteStream()` on
the result of `response.body()` without a null check. If `response.body()` returns
`null`, this will cause a `NullPointerException`. The surrounding `try-catch` block only
catches `IOException`, so the `NullPointerException` will be unhandled, causing the
request to fail.

Also affects:

  • sentry-samples/sentry-samples-spring-boot/src/main/java/io/sentry/samples/spring/boot/TodoController.java:53~54

Did we get this right? 👍 / 👎 to inform future reviews.

} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ spring.quartz.job-store-type=memory

# Cache tracing
sentry.enable-cache-tracing=true
# Automatically instrument Spring-managed OkHttpClient beans
sentry.clients.ok-http-enabled=true

spring.cache.cache-names=todos
spring.cache.caffeine.spec=maximumSize=500,expireAfterAccess=600s
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,20 @@ class TodoSystemTest {
)
}
}

@Test
fun `get todo okhttp works`() {
val restClient = testHelper.restClient
restClient.getTodoOkHttp(1L)
assertEquals(200, restClient.lastKnownStatusCode)

testHelper.ensureTransactionReceived { transaction, envelopeHeader ->
transaction.transaction == "GET /todo-okhttp/{id}" &&
testHelper.doesTransactionContainSpanWithOpAndDescription(
transaction,
"http.client",
"GET https://jsonplaceholder.typicode.com/todos/1",
)
}
}
}
4 changes: 4 additions & 0 deletions sentry-samples/sentry-samples-spring-boot/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ dependencies {
implementation(projects.sentryQuartz)
implementation(projects.sentryAsyncProfiler)

// okhttp client instrumentation
implementation(projects.sentryOkhttp)
implementation(libs.okhttp)

// database query tracing
implementation(projects.sentryJdbc)
runtimeOnly(libs.hsqldb)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import io.sentry.samples.spring.boot.quartz.SampleJob;
import java.util.Collections;
import okhttp3.OkHttpClient;
import org.quartz.JobDetail;
import org.quartz.SimpleTrigger;
import org.springframework.boot.SpringApplication;
Expand Down Expand Up @@ -36,6 +37,12 @@ WebClient webClient(WebClient.Builder builder) {
return builder.build();
}

@Bean
OkHttpClient okHttpClient() {
// automatically instrumented by Sentry via sentry.clients.ok-http-enabled=true
return new OkHttpClient.Builder().build();
}

@Bean
public JobDetailFactoryBean jobDetail() {
JobDetailFactoryBean jobDetailFactory = new JobDetailFactoryBean();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
package io.sentry.samples.spring.boot;

import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
import java.io.UncheckedIOException;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RestController;
Expand All @@ -10,10 +16,18 @@
public class TodoController {
private final RestTemplate restTemplate;
private final WebClient webClient;
private final OkHttpClient okHttpClient;
private final ObjectMapper objectMapper;

public TodoController(RestTemplate restTemplate, WebClient webClient) {
public TodoController(
RestTemplate restTemplate,
WebClient webClient,
OkHttpClient okHttpClient,
ObjectMapper objectMapper) {
this.restTemplate = restTemplate;
this.webClient = webClient;
this.okHttpClient = okHttpClient;
this.objectMapper = objectMapper;
}

@GetMapping("/todo/{id}")
Expand All @@ -31,4 +45,15 @@ Todo todoWebClient(@PathVariable Long id) {
.bodyToMono(Todo.class)
.block();
}

@GetMapping("/todo-okhttp/{id}")
Todo todoOkHttp(@PathVariable Long id) {
final Request request =
new Request.Builder().url("https://jsonplaceholder.typicode.com/todos/" + id).build();
try (Response response = okHttpClient.newCall(request).execute()) {
return objectMapper.readValue(response.body().byteStream(), Todo.class);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ sentry.profile-lifecycle=TRACE

# Cache tracing
sentry.enable-cache-tracing=true
# Automatically instrument Spring-managed OkHttpClient beans
sentry.clients.ok-http-enabled=true
spring.cache.cache-names=todos
spring.cache.caffeine.spec=maximumSize=500,expireAfterAccess=600s

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,20 @@ class TodoSystemTest {
)
}
}

@Test
fun `get todo okhttp works`() {
val restClient = testHelper.restClient
restClient.getTodoOkHttp(1L)
assertEquals(200, restClient.lastKnownStatusCode)

testHelper.ensureTransactionReceived { transaction, envelopeHeader ->
transaction.transaction == "GET /todo-okhttp/{id}" &&
testHelper.doesTransactionContainSpanWithOpAndDescription(
transaction,
"http.client",
"GET https://jsonplaceholder.typicode.com/todos/1",
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public class io/sentry/spring/boot/jakarta/SentryProfilerAutoConfiguration {

public class io/sentry/spring/boot/jakarta/SentryProperties : io/sentry/SentryOptions {
public fun <init> ()V
public fun getClients ()Lio/sentry/spring/boot/jakarta/SentryProperties$Clients;
public fun getExceptionResolverOrder ()I
public fun getGraphql ()Lio/sentry/spring/boot/jakarta/SentryProperties$Graphql;
public fun getLogging ()Lio/sentry/spring/boot/jakarta/SentryProperties$Logging;
Expand All @@ -49,6 +50,7 @@ public class io/sentry/spring/boot/jakarta/SentryProperties : io/sentry/SentryOp
public fun isEnableAotCompatibility ()Z
public fun isKeepTransactionsOpenForAsyncResponses ()Z
public fun isUseGitCommitIdAsRelease ()Z
public fun setClients (Lio/sentry/spring/boot/jakarta/SentryProperties$Clients;)V
public fun setEnableAotCompatibility (Z)V
public fun setExceptionResolverOrder (I)V
public fun setGraphql (Lio/sentry/spring/boot/jakarta/SentryProperties$Graphql;)V
Expand All @@ -59,6 +61,12 @@ public class io/sentry/spring/boot/jakarta/SentryProperties : io/sentry/SentryOp
public fun setUserFilterOrder (Ljava/lang/Integer;)V
}

public class io/sentry/spring/boot/jakarta/SentryProperties$Clients {
public fun <init> ()V
public fun isOkHttpEnabled ()Z
public fun setOkHttpEnabled (Z)V
}

public class io/sentry/spring/boot/jakarta/SentryProperties$Graphql {
public fun <init> ()V
public fun getIgnoredErrorTypes ()Ljava/util/List;
Expand Down
8 changes: 6 additions & 2 deletions sentry-spring-boot-jakarta/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ dependencies {
api(projects.sentrySpringJakarta)
compileOnly(projects.sentryLogback)
compileOnly(projects.sentryLog4j2)
compileOnly(projects.sentryOkhttp)
compileOnly(libs.okhttp)
compileOnly(projects.sentryApacheHttpClient5)
compileOnly(libs.log4j.api)
compileOnly(libs.log4j.core)
Expand Down Expand Up @@ -70,10 +72,9 @@ dependencies {

// tests
testImplementation(projects.sentryLogback)
testImplementation(projects.sentryOkhttp)
testImplementation(projects.sentryLog4j2)
testImplementation(projects.sentryApacheHttpClient5)
testImplementation(libs.log4j.api)
testImplementation(libs.log4j.core)
testImplementation(projects.sentryGraphql)
testImplementation(projects.sentryGraphql22)
testImplementation(projects.sentryKafka)
Expand All @@ -88,8 +89,11 @@ dependencies {
testImplementation(platform(SpringBootPlugin.BOM_COORDINATES))
testImplementation(libs.context.propagation)
testImplementation(libs.kotlin.test.junit)
testImplementation(libs.google.truth)
testImplementation(libs.mockito.kotlin)
testImplementation(libs.okhttp)
testImplementation(libs.log4j.api)
testImplementation(libs.log4j.core)
testImplementation(libs.okhttp.mockwebserver)
testImplementation(libs.otel)
testImplementation(libs.otel.extension.autoconfigure.spi)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,28 @@ static class SentryKafkaQueueConfiguration {
}
}

@Configuration(proxyBeanMethods = false)
@ConditionalOnClass(
name = {
"okhttp3.OkHttpClient",
"io.sentry.okhttp.SentryOkHttpInterceptor",
"io.sentry.okhttp.SentryOkHttpEventListener"
})
@ConditionalOnProperty(name = "sentry.clients.ok-http-enabled", havingValue = "true")
@ConditionalOnMissingClass({
"io.sentry.opentelemetry.SentryAutoConfigurationCustomizerProvider",
"io.sentry.opentelemetry.agent.AgentMarker"
})
@Open
static class SentryOkHttpConfiguration {

@Bean
public static @NotNull SentryOkHttpClientBeanPostProcessor
sentryOkHttpClientBeanPostProcessor() {
return new SentryOkHttpClientBeanPostProcessor();
}
}

@Configuration(proxyBeanMethods = false)
@ConditionalOnClass(ProceedingJoinPoint.class)
@ConditionalOnProperty(
Expand Down
Loading
Loading