Skip to content

Commit 2f192a1

Browse files
KarstenSchnitterKarstenSchnitter
authored
Remove side-effect in RequestRecordFactory (#117)
* Remove side-effect in RequestRecordFactory This change removes the http header propagation to log fields in the RequestRecordFactory. This feature was refactored to the AddHttpHeadersToLogContextFilter. The change include several test to ensure correct behaviour.
1 parent 671d10b commit 2f192a1

File tree

5 files changed

+175
-27
lines changed

5 files changed

+175
-27
lines changed

‎cf-java-logging-support-servlet/src/main/java/com/sap/hcp/cf/logging/servlet/filter/RequestRecordFactory.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,6 @@ public RequestRecord create(HttpServletRequest request) {
3636
getHeader(request, HttpHeaders.X_FORWARDED_FOR))
3737
.addOptionalTag(isLogRemoteUserField, Fields.REMOTE_USER, getValue(request.getRemoteUser()))
3838
.addOptionalTag(isLogRefererField, Fields.REFERER, getHeader(request, HttpHeaders.REFERER));
39-
for (HttpHeader header : HttpHeaders.propagated()) {
40-
rrb.addContextTag(header.getField(), getHeaderValue(request, header, null));
41-
}
4239
return rrb.build();
4340
}
4441

‎cf-java-logging-support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/CustomFilterTest.java

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
package com.sap.hcp.cf.logging.servlet.filter;
2+
3+
import static org.hamcrest.MatcherAssert.assertThat;
4+
import static org.hamcrest.Matchers.equalTo;
5+
import static org.hamcrest.Matchers.hasEntry;
6+
import static org.hamcrest.Matchers.is;
7+
8+
import java.util.EnumSet;
9+
10+
import javax.servlet.DispatcherType;
11+
import javax.servlet.Filter;
12+
import javax.servlet.http.HttpServletRequest;
13+
import javax.servlet.http.HttpServletResponse;
14+
15+
import org.apache.http.client.methods.CloseableHttpResponse;
16+
import org.apache.http.client.methods.HttpGet;
17+
import org.apache.http.impl.client.CloseableHttpClient;
18+
import org.apache.http.impl.client.HttpClientBuilder;
19+
import org.eclipse.jetty.server.Server;
20+
import org.eclipse.jetty.server.ServerConnector;
21+
import org.eclipse.jetty.servlet.FilterHolder;
22+
import org.eclipse.jetty.servlet.ServletContextHandler;
23+
import org.junit.Rule;
24+
import org.junit.Test;
25+
import org.slf4j.MDC;
26+
27+
import com.sap.hcp.cf.logging.common.Fields;
28+
import com.sap.hcp.cf.logging.common.request.HttpHeaders;
29+
30+
public class CustomFilterTest {
31+
32+
@Rule
33+
public SystemOutRule systemOutRule = new SystemOutRule();
34+
35+
@Test
36+
public void setsFixedTenantId() throws Exception {
37+
Server jetty = initJetty(constantTenantId("my_tenant"));
38+
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
39+
jetty.start();
40+
try (CloseableHttpResponse response = client.execute(createBasicGetRequest(jetty))) {
41+
assertThat(response.getStatusLine().getStatusCode(), is(equalTo(200)));
42+
assertThat(systemOutRule.findLineAsMapWith(Fields.MSG, LoggingTestServlet.LOG_MESSAGE), hasEntry(
43+
Fields.TENANT_ID,
44+
"my_tenant"));
45+
}
46+
} finally {
47+
jetty.stop();
48+
}
49+
}
50+
51+
/**
52+
* This test case addresses
53+
* <a href="https://github.com/SAP/cf-java-logging-support/issues/111">
54+
* Github issue #111</a>, by ensuring the elimination of a side-effect in
55+
* {@link RequestRecordFactory}, which used to overwrite custom set log
56+
* fields with values extracted from http headers.
57+
*
58+
* @throws Exception
59+
*/
60+
@Test
61+
public void usesCustomTenantIdInRequestLog() throws Exception {
62+
Filter filter = new CompositeFilter(constantTenantId("custom_tenant"), new GenerateRequestLogFilter());
63+
Server jetty = initJetty(filter);
64+
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
65+
jetty.start();
66+
HttpGet request = createBasicGetRequest(jetty);
67+
request.addHeader(HttpHeaders.TENANT_ID.getName(), "other_tenant");
68+
try (CloseableHttpResponse response = client.execute(request)) {
69+
assertThat(response.getStatusLine().getStatusCode(), is(equalTo(200)));
70+
assertThat(systemOutRule.findLineAsMapWith(Fields.MSG, LoggingTestServlet.LOG_MESSAGE), hasEntry(
71+
Fields.TENANT_ID,
72+
"custom_tenant"));
73+
assertThat(systemOutRule.findLineAsMapWith(Fields.LAYER, "[SERVLET]"), hasEntry(Fields.TENANT_ID,
74+
"custom_tenant"));
75+
}
76+
} finally {
77+
jetty.stop();
78+
}
79+
}
80+
81+
@Test
82+
public void canOverwriteGeneratedCorrelationId() throws Exception {
83+
Filter filter = new CompositeFilter(new CorrelationIdFilter(), constantCorrelationId("my_correlation"),
84+
new GenerateRequestLogFilter());
85+
Server jetty = initJetty(filter);
86+
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
87+
jetty.start();
88+
try (CloseableHttpResponse response = client.execute(createBasicGetRequest(jetty))) {
89+
assertThat(response.getStatusLine().getStatusCode(), is(equalTo(200)));
90+
assertThat(systemOutRule.findLineAsMapWith(Fields.MSG, LoggingTestServlet.LOG_MESSAGE), hasEntry(
91+
Fields.CORRELATION_ID,
92+
"my_correlation"));
93+
assertThat(systemOutRule.findLineAsMapWith(Fields.LAYER, "[SERVLET]"), hasEntry(Fields.CORRELATION_ID,
94+
"my_correlation"));
95+
}
96+
} finally {
97+
jetty.stop();
98+
}
99+
}
100+
101+
private Server initJetty(Filter filter) {
102+
Server jetty = new Server(0);
103+
ServletContextHandler contextHandler = new ServletContextHandler(jetty, null);
104+
EnumSet<DispatcherType> dispatches = EnumSet.of(DispatcherType.INCLUDE, DispatcherType.REQUEST,
105+
DispatcherType.ERROR, DispatcherType.FORWARD,
106+
DispatcherType.ASYNC);
107+
contextHandler.addFilter(new FilterHolder(filter), "/*", dispatches);
108+
contextHandler.addServlet(LoggingTestServlet.class, "/test");
109+
return jetty;
110+
}
111+
112+
private HttpGet createBasicGetRequest(Server jetty) {
113+
return new HttpGet(getBaseUrl(jetty) + "/test");
114+
}
115+
116+
private String getBaseUrl(Server server) {
117+
int port = ((ServerConnector) server.getConnectors()[0]).getLocalPort();
118+
return "http://localhost:" + port;
119+
}
120+
121+
private static Filter constantTenantId(String tenantId) {
122+
return constantField(Fields.TENANT_ID, tenantId);
123+
}
124+
125+
private static Filter constantCorrelationId(String correlationId) {
126+
return constantField(Fields.CORRELATION_ID, correlationId);
127+
}
128+
129+
private static Filter constantField(String field, String value) {
130+
return new AbstractLoggingFilter() {
131+
@Override
132+
protected void beforeFilter(HttpServletRequest request, HttpServletResponse response) {
133+
MDC.put(field, value);
134+
}
135+
136+
@Override
137+
protected void cleanup(HttpServletRequest request, HttpServletResponse response) {
138+
MDC.remove(field);
139+
}
140+
};
141+
}
142+
143+
}

‎cf-java-logging-support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/LoggingTestServlet.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
package com.sap.hcp.cf.logging.servlet.filter;
2+
3+
import java.io.IOException;
4+
5+
import javax.servlet.ServletException;
6+
import javax.servlet.http.HttpServlet;
7+
import javax.servlet.http.HttpServletRequest;
8+
import javax.servlet.http.HttpServletResponse;
9+
10+
import org.slf4j.Logger;
11+
import org.slf4j.LoggerFactory;
12+
13+
public class LoggingTestServlet extends HttpServlet {
14+
15+
public static final String LOG_MESSAGE = "request received";
16+
17+
private static final long serialVersionUID = -4594560302550583354L;
18+
private static final Logger LOG = LoggerFactory.getLogger(LoggingTestServlet.class);
19+
20+
@Override
21+
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
22+
LOG.info(LOG_MESSAGE);
23+
}
24+
25+
}

‎cf-java-logging-support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/RequestLogTest.java

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,6 @@
1313
import java.util.UUID;
1414

1515
import javax.servlet.DispatcherType;
16-
import javax.servlet.ServletException;
17-
import javax.servlet.http.HttpServlet;
18-
import javax.servlet.http.HttpServletRequest;
19-
import javax.servlet.http.HttpServletResponse;
2016

2117
import org.apache.http.client.methods.CloseableHttpResponse;
2218
import org.apache.http.client.methods.HttpGet;
@@ -29,7 +25,6 @@
2925
import org.junit.Before;
3026
import org.junit.Rule;
3127
import org.junit.Test;
32-
import org.slf4j.Logger;
3328
import org.slf4j.LoggerFactory;
3429

3530
import com.sap.hcp.cf.logging.common.Fields;
@@ -41,10 +36,7 @@
4136

4237
public class RequestLogTest {
4338

44-
private static final Logger LOG = LoggerFactory.getLogger(RequestLogTest.class);
45-
private static final String REQUEST_RECEIVED = "Request received";
46-
47-
@Rule
39+
@Rule
4840
public SystemOutRule systemOut = new SystemOutRule();
4941

5042
private Server server;
@@ -65,7 +57,7 @@ private Server initJetty() throws Exception {
6557
handler.addFilter(RequestLoggingFilter.class, "/*", EnumSet.of(DispatcherType.INCLUDE,
6658
DispatcherType.REQUEST,
6759
DispatcherType.ERROR, DispatcherType.FORWARD, DispatcherType.ASYNC));
68-
handler.addServlet(TestServlet.class, "/test");
60+
handler.addServlet(LoggingTestServlet.class, "/test");
6961
server.start();
7062
return server;
7163
}
@@ -196,7 +188,7 @@ private String getBaseUrl() {
196188
}
197189

198190
private String getCorrelationIdGenerated() throws IOException {
199-
Map<String, Object> generationLog = systemOut.fineLineAsMapWith("logger", CorrelationIdFilter.class.getName());
191+
Map<String, Object> generationLog = systemOut.findLineAsMapWith("logger", CorrelationIdFilter.class.getName());
200192
if (generationLog == null) {
201193
return null;
202194
}
@@ -205,24 +197,15 @@ private String getCorrelationIdGenerated() throws IOException {
205197
}
206198

207199
private Map<String, Object> getRequestMessage() throws IOException {
208-
return systemOut.fineLineAsMapWith("msg", REQUEST_RECEIVED);
200+
return systemOut.findLineAsMapWith("msg", LoggingTestServlet.LOG_MESSAGE);
209201
}
210202

211-
private Map<String, Object> getRequestLog() throws IOException {
212-
return systemOut.fineLineAsMapWith("layer", "[SERVLET]");
203+
private Map<String, Object> getRequestLog() throws IOException {
204+
return systemOut.findLineAsMapWith("layer", "[SERVLET]");
213205
}
214206

215207
private static void assertFirstHeaderValue(String expected, CloseableHttpResponse response, HttpHeader header) {
216208
String headerValue = response.getFirstHeader(header.getName()).getValue();
217209
assertThat(headerValue, is(equalTo(expected)));
218210
}
219-
220-
@SuppressWarnings("serial")
221-
public static class TestServlet extends HttpServlet {
222-
223-
@Override
224-
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
225-
LOG.info(REQUEST_RECEIVED);
226-
}
227-
}
228211
}

‎cf-java-logging-support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/SystemOutRule.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public String toString() {
3333
return output.toString();
3434
}
3535

36-
public Map<String, Object> fineLineAsMapWith(String key, String expected) throws IOException {
36+
public Map<String, Object> findLineAsMapWith(String key, String expected) throws IOException {
3737
for (String line : output.toString().split("\n")) {
3838
Map<String, Object> map = JSON.std.mapFrom(line);
3939
if (expected.equals(getAsString(map, key))) {

0 commit comments

Comments
 (0)