From 0f2a6e2528c7f0649b467e4002a27af8e7e8546b Mon Sep 17 00:00:00 2001
From: Hamza Remmal <2861-remmal@users.noreply.gitlab.epfl.ch>
Date: Thu, 26 Dec 2024 15:27:32 +0000
Subject: [PATCH] chore: unify the generation of the request id + return it in
the response
---
autograde-service/pom.xml | 2 +-
.../epfl/autograde/config/FiltersConfig.java | 28 ++++++
.../filters/AssignRequestIdFilter.java | 54 +++++++++++
.../autograde/logging/MdcInterceptor.java | 30 ------
.../logging/WebMvcConfiguration.java | 22 -----
.../src/main/resources/logback-spring.xml | 2 +-
...AssignRequestIdFilterIntegrationTests.java | 54 +++++++++++
.../AssignRequestIdFilterUnitTests.java | 95 +++++++++++++++++++
8 files changed, 233 insertions(+), 54 deletions(-)
create mode 100644 autograde-service/src/main/java/ch/epfl/autograde/config/FiltersConfig.java
create mode 100644 autograde-service/src/main/java/ch/epfl/autograde/filters/AssignRequestIdFilter.java
delete mode 100644 autograde-service/src/main/java/ch/epfl/autograde/logging/MdcInterceptor.java
delete mode 100644 autograde-service/src/main/java/ch/epfl/autograde/logging/WebMvcConfiguration.java
create mode 100644 autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterIntegrationTests.java
create mode 100644 autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterUnitTests.java
diff --git a/autograde-service/pom.xml b/autograde-service/pom.xml
index df959467..9a871e68 100644
--- a/autograde-service/pom.xml
+++ b/autograde-service/pom.xml
@@ -6,7 +6,7 @@
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
- <version>3.1.1</version>
+ <version>3.3.4</version>
<relativePath/> <!-- lookup parent from repository -->
</parent>
diff --git a/autograde-service/src/main/java/ch/epfl/autograde/config/FiltersConfig.java b/autograde-service/src/main/java/ch/epfl/autograde/config/FiltersConfig.java
new file mode 100644
index 00000000..cb681e20
--- /dev/null
+++ b/autograde-service/src/main/java/ch/epfl/autograde/config/FiltersConfig.java
@@ -0,0 +1,28 @@
+package ch.epfl.autograde.config;
+
+import ch.epfl.autograde.filters.AssignRequestIdFilter;
+import org.springframework.boot.web.servlet.FilterRegistrationBean;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.core.Ordered;
+
+/**
+ *
+ * @author Hamza REMMAL (hamza.remmal@epfl.ch)
+ * @since 1.3.0
+ */
+@Configuration
+public class FiltersConfig {
+
+ @Bean
+ public FilterRegistrationBean<?> requestIdFilterRegistration(AssignRequestIdFilter filter) {
+ final var registration = new FilterRegistrationBean<>();
+ registration.setFilter(filter);
+ // We should generate a request id only for api endpoints
+ registration.addUrlPatterns("/api/v1/*");
+ // Ensure that all the requests will have a request id by running the filter first
+ registration.setOrder(Ordered.HIGHEST_PRECEDENCE);
+ return registration;
+ }
+
+}
diff --git a/autograde-service/src/main/java/ch/epfl/autograde/filters/AssignRequestIdFilter.java b/autograde-service/src/main/java/ch/epfl/autograde/filters/AssignRequestIdFilter.java
new file mode 100644
index 00000000..8bead833
--- /dev/null
+++ b/autograde-service/src/main/java/ch/epfl/autograde/filters/AssignRequestIdFilter.java
@@ -0,0 +1,54 @@
+package ch.epfl.autograde.filters;
+
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.slf4j.MDC;
+import org.springframework.stereotype.Component;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import java.io.IOException;
+import java.util.UUID;
+
+/**
+ * A filter that assigns a unique Request ID to every incoming HTTP request.
+ *
+ * <p>
+ * This filter generates a unique {@code Request ID} for each HTTP request and ensures it is:
+ * <ul>
+ * <li>Added to the request attributes, making it accessible to downstream components.</li>
+ * <li>Included as a header in the HTTP response ({@code X-Request-Id}).</li>
+ * <li>Logged in the Mapped Diagnostic Context (MDC) for consistent tracking in logs.</li>
+ * </ul>
+ * This filter is designed to run once per request and should be one of the earliest filters
+ * in the filter chain.
+ * </p>
+ *
+ * @author Hamza REMMAL (hamza.remmal@epfl.ch)
+ * @since 1.3.0
+ * @see ch.epfl.autograde.config.FiltersConfig#requestIdFilterRegistration(AssignRequestIdFilter)
+ * @see org.springframework.web.filter.OncePerRequestFilter
+ */
+@Slf4j
+@Component
+@RequiredArgsConstructor
+public final class AssignRequestIdFilter extends OncePerRequestFilter {
+
+ public static final String REQUEST_ID_HEADER = "X-Request-Id";
+
+ @Override
+ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws ServletException, IOException {
+ // Generate a unique Request ID
+ // TODO: Extract the UUID to be a distributed service to have a real UUID and not an UUID per node
+ final var requestId = UUID.randomUUID().toString();
+ request.setAttribute(REQUEST_ID_HEADER, requestId);
+ response.setHeader(REQUEST_ID_HEADER, requestId);
+ MDC.put(REQUEST_ID_HEADER, requestId);
+ log.trace("Generated Request-Id '{}' for request from '{}'", requestId, request.getRemoteAddr());
+ chain.doFilter(request, response);
+ }
+
+}
diff --git a/autograde-service/src/main/java/ch/epfl/autograde/logging/MdcInterceptor.java b/autograde-service/src/main/java/ch/epfl/autograde/logging/MdcInterceptor.java
deleted file mode 100644
index 1d375ae1..00000000
--- a/autograde-service/src/main/java/ch/epfl/autograde/logging/MdcInterceptor.java
+++ /dev/null
@@ -1,30 +0,0 @@
-package ch.epfl.autograde.logging;
-
-import jakarta.servlet.http.HttpServletRequest;
-import jakarta.servlet.http.HttpServletResponse;
-import org.slf4j.MDC;
-import org.springframework.stereotype.Component;
-import org.springframework.web.servlet.HandlerInterceptor;
-
-import java.util.UUID;
-
-/**
- * Interceptor that adds a unique request id to the MDC,
- * so that it can be used in the logs to track a request through the system.
- *
- * @implNote To work with async requests, you need to create a TaskDecorator that copies the MDC to the new thread.
- */
-@Component
-public class MdcInterceptor implements HandlerInterceptor {
-
- @Override
- public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
- MDC.put("requestId", UUID.randomUUID().toString());
- return true;
- }
-
- @Override
- public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex) {
- MDC.remove("requestId");
- }
-}
diff --git a/autograde-service/src/main/java/ch/epfl/autograde/logging/WebMvcConfiguration.java b/autograde-service/src/main/java/ch/epfl/autograde/logging/WebMvcConfiguration.java
deleted file mode 100644
index 9c3a8a23..00000000
--- a/autograde-service/src/main/java/ch/epfl/autograde/logging/WebMvcConfiguration.java
+++ /dev/null
@@ -1,22 +0,0 @@
-package ch.epfl.autograde.logging;
-
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.stereotype.Component;
-import org.springframework.web.servlet.config.annotation.InterceptorRegistry;
-import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
-
-@Component
-public class WebMvcConfiguration implements WebMvcConfigurer {
-
- private final MdcInterceptor mdcInterceptor;
-
- @Autowired
- public WebMvcConfiguration(MdcInterceptor mdcInterceptor) {
- this.mdcInterceptor = mdcInterceptor;
- }
-
- @Override
- public void addInterceptors(InterceptorRegistry registry) {
- registry.addInterceptor(mdcInterceptor);
- }
-}
diff --git a/autograde-service/src/main/resources/logback-spring.xml b/autograde-service/src/main/resources/logback-spring.xml
index 64736462..5a2b05fd 100644
--- a/autograde-service/src/main/resources/logback-spring.xml
+++ b/autograde-service/src/main/resources/logback-spring.xml
@@ -1,7 +1,7 @@
<configuration>
<appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
<encoder>
- <pattern>%d{ISO8601} %X{requestId} [%thread] %-5level %logger{36} - %msg%n</pattern>
+ <pattern>%d{ISO8601} %X{X-Request-Id} [%thread] %-5level %logger{36} - %msg%n</pattern>
</encoder>
</appender>
diff --git a/autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterIntegrationTests.java b/autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterIntegrationTests.java
new file mode 100644
index 00000000..2e5cb0dd
--- /dev/null
+++ b/autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterIntegrationTests.java
@@ -0,0 +1,54 @@
+package ch.epfl.autograde.filters;
+
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.test.web.servlet.MockMvc;
+
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
+
+/**
+ *
+ * @author Hamza REMMAL (hamza.remmal@epfl.ch)
+ * @since 1.3.0
+ * @see ch.epfl.autograde.filters.AssignRequestIdFilter
+ */
+@AutoConfigureMockMvc
+@SpringBootTest
+final class AssignRequestIdFilterIntegrationTests {
+
+ @Autowired
+ private MockMvc mockMvc;
+
+ /**
+ * @implNote We call a fake-endpoint to test:
+ * <ul>
+ * <li>The filter is called when calling an API Endpoint</li>
+ * <li>The filter is executed before the authentication filters</li>
+ * <li>The filter is called before dispatching to the controller</li>
+ * </ul>
+ */
+ @Test @DisplayName("Request to API endpoints should have a 'X-Request-Id' header")
+ void apiRequestShouldHaveARequestID() throws Exception {
+ mockMvc.perform(get("/api/v1/fake-endpoint"))
+ .andExpect(header().exists(AssignRequestIdFilter.REQUEST_ID_HEADER));
+ }
+
+ /**
+ * @implNote We call a fake-endpoint to test:
+ * <ul>
+ * <li>The filter is not called when calling a non-API Endpoint</li>
+ * <li>The filter is executed before the authentication filters</li>
+ * <li>The filter is called before dispatching to the controller</li>
+ * </ul>
+ */
+ @Test @DisplayName("Request to non-API endpoints should not have a 'X-Request-Id' header")
+ void nonApiRequestShouldNotHaveARequestID() throws Exception {
+ mockMvc.perform(get("/not-api/v1/fake-endpoint"))
+ .andExpect(header().doesNotExist(AssignRequestIdFilter.REQUEST_ID_HEADER));
+ }
+
+}
diff --git a/autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterUnitTests.java b/autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterUnitTests.java
new file mode 100644
index 00000000..c56f2de2
--- /dev/null
+++ b/autograde-service/src/test/java/ch/epfl/autograde/filters/AssignRequestIdFilterUnitTests.java
@@ -0,0 +1,95 @@
+package ch.epfl.autograde.filters;
+
+import org.junit.jupiter.api.*;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.*;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.slf4j.MDC;
+
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.Mockito.*;
+
+/**
+ *
+ * @author Hamza REMMAL (hamza.remmal@epfl.ch)
+ * @since 1.3.0
+ * @see ch.epfl.autograde.filters.AssignRequestIdFilter
+ */
+@DisplayName("AssignRequestIdFilter Unit Tests")
+@ExtendWith(MockitoExtension.class)
+final class AssignRequestIdFilterUnitTests {
+
+ private final AssignRequestIdFilter filter = new AssignRequestIdFilter();
+
+ @Test @DisplayName("Filter adds the 'X-Request-Id' attribute to the HttpServletRequest")
+ void filterSetsTheAttributeInTheRequest(
+ @Mock HttpServletRequest request,
+ @Mock HttpServletResponse response,
+ @Mock FilterChain chain
+ ) throws Exception {
+ filter.doFilter(request, response, chain);
+ verify(request).setAttribute(eq(AssignRequestIdFilter.REQUEST_ID_HEADER), notNull());
+ }
+
+ @Test @DisplayName("Filter adds the 'X-Request-Id' header to the HttpServletResponse")
+ void filterSetsTheHeaderInTheResponse(
+ @Mock HttpServletRequest request,
+ @Mock HttpServletResponse response,
+ @Mock FilterChain chain
+ ) throws Exception {
+ filter.doFilter(request, response, chain);
+ verify(response).setHeader(eq(AssignRequestIdFilter.REQUEST_ID_HEADER), notNull());
+ }
+
+ @Test @DisplayName("Filter adds the 'X-Request-Id' to the MDC")
+ void filterSetsMDCContext(
+ @Mock HttpServletRequest request,
+ @Mock HttpServletResponse response,
+ @Mock FilterChain chain
+ ) throws Exception {
+ filter.doFilter(request, response, chain);
+ assertNotNull(MDC.get(AssignRequestIdFilter.REQUEST_ID_HEADER));
+ }
+
+ @Test @DisplayName("Filter calls the rest of the FilterChain")
+ void filterCallsTheRestOfTheChain(
+ @Mock HttpServletRequest request,
+ @Mock HttpServletResponse response,
+ @Mock FilterChain chain
+ ) throws Exception {
+ filter.doFilter(request, response, chain);
+ verify(chain).doFilter(request, response);
+ }
+
+ @Test
+ @DisplayName("Filter generates a unique 'X-Request-Id'")
+ void filterUsesSameRequestIdAcrossAll(
+ @Mock HttpServletRequest request,
+ @Mock HttpServletResponse response,
+ @Mock FilterChain chain,
+ @Captor ArgumentCaptor<String> requestCaptor,
+ @Captor ArgumentCaptor<String> responseCaptor
+ ) throws Exception {
+ filter.doFilter(request, response, chain);
+
+ verify(request).setAttribute(eq(AssignRequestIdFilter.REQUEST_ID_HEADER), requestCaptor.capture());
+ verify(response).setHeader(eq(AssignRequestIdFilter.REQUEST_ID_HEADER), responseCaptor.capture());
+
+ final var requestAttrId = requestCaptor.getValue();
+ final var responseHeaderId = responseCaptor.getValue();
+ final var mdcRequestId = MDC.get(AssignRequestIdFilter.REQUEST_ID_HEADER);
+
+ assertAll(
+ () -> assertNotNull(requestAttrId),
+ () -> assertNotNull(responseHeaderId),
+ () -> assertNotNull(mdcRequestId),
+ () -> assertEquals(requestAttrId, responseHeaderId),
+ () -> assertEquals(requestAttrId, mdcRequestId)
+ );
+ }
+
+}
--
GitLab