Skip to content

Remove side-effect in RequestRecordFactory #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 23, 2021
Merged

Remove side-effect in RequestRecordFactory #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d4d752
Remove side-effect in RequestRecordFactory
KarstenSchnitter Jul 19, 2021
a0a8007
Correct typo and remove obsolete method
KarstenSchnitter Jul 21, 2021
3831cf3
Remove unnecessary header in test request
KarstenSchnitter Jul 21, 2021
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
3 changes: 0 additions & 3 deletions ...ort-servlet/src/main/java/com/sap/hcp/cf/logging/servlet/filter/RequestRecordFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ public RequestRecord create(HttpServletRequest request) {
getHeader(request, HttpHeaders.X_FORWARDED_FOR))
.addOptionalTag(isLogRemoteUserField, Fields.REMOTE_USER, getValue(request.getRemoteUser()))
.addOptionalTag(isLogRefererField, Fields.REFERER, getHeader(request, HttpHeaders.REFERER));
for (HttpHeader header : HttpHeaders.propagated()) {
rrb.addContextTag(header.getField(), getHeaderValue(request, header, null));
}
return rrb.build();
}

Expand Down
143 changes: 143 additions & 0 deletions ...support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/CustomFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
package com.sap.hcp.cf.logging.servlet.filter;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.is;

import java.util.EnumSet;

import javax.servlet.DispatcherType;
import javax.servlet.Filter;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.FilterHolder;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.junit.Rule;
import org.junit.Test;
import org.slf4j.MDC;

import com.sap.hcp.cf.logging.common.Fields;
import com.sap.hcp.cf.logging.common.request.HttpHeaders;

public class CustomFilterTest {

@Rule
public SystemOutRule systemOutRule = new SystemOutRule();

@Test
public void setsFixedTenantId() throws Exception {
Server jetty = initJetty(constantTenantId("my_tenant"));
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
jetty.start();
try (CloseableHttpResponse response = client.execute(createBasicGetRequest(jetty))) {
assertThat(response.getStatusLine().getStatusCode(), is(equalTo(200)));
assertThat(systemOutRule.findLineAsMapWith(Fields.MSG, LoggingTestServlet.LOG_MESSAGE), hasEntry(
Fields.TENANT_ID,
"my_tenant"));
}
} finally {
jetty.stop();
}
}

/**
* This test case addresses
* <a href="https://github.com/SAP/cf-java-logging-support/issues/111">
* Github issue #111</a>, by ensuring the elimination of a side-effect in
* {@link RequestRecordFactory}, which used to overwrite custom set log
* fields with values extracted from http headers.
*
* @throws Exception
*/
@Test
public void usesCustomTenantIdInRequestLog() throws Exception {
Filter filter = new CompositeFilter(constantTenantId("custom_tenant"), new GenerateRequestLogFilter());
Server jetty = initJetty(filter);
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
jetty.start();
HttpGet request = createBasicGetRequest(jetty);
request.addHeader(HttpHeaders.TENANT_ID.getName(), "other_tenant");
try (CloseableHttpResponse response = client.execute(request)) {
assertThat(response.getStatusLine().getStatusCode(), is(equalTo(200)));
assertThat(systemOutRule.findLineAsMapWith(Fields.MSG, LoggingTestServlet.LOG_MESSAGE), hasEntry(
Fields.TENANT_ID,
"custom_tenant"));
assertThat(systemOutRule.findLineAsMapWith(Fields.LAYER, "[SERVLET]"), hasEntry(Fields.TENANT_ID,
"custom_tenant"));
}
} finally {
jetty.stop();
}
}

@Test
public void canOverwriteGeneratedCorrelationId() throws Exception {
Filter filter = new CompositeFilter(new CorrelationIdFilter(), constantCorrelationId("my_correlation"),
new GenerateRequestLogFilter());
Server jetty = initJetty(filter);
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
jetty.start();
try (CloseableHttpResponse response = client.execute(createBasicGetRequest(jetty))) {
assertThat(response.getStatusLine().getStatusCode(), is(equalTo(200)));
assertThat(systemOutRule.findLineAsMapWith(Fields.MSG, LoggingTestServlet.LOG_MESSAGE), hasEntry(
Fields.CORRELATION_ID,
"my_correlation"));
assertThat(systemOutRule.findLineAsMapWith(Fields.LAYER, "[SERVLET]"), hasEntry(Fields.CORRELATION_ID,
"my_correlation"));
}
} finally {
jetty.stop();
}
}

private Server initJetty(Filter filter) {
Server jetty = new Server(0);
ServletContextHandler contextHandler = new ServletContextHandler(jetty, null);
EnumSet<DispatcherType> dispatches = EnumSet.of(DispatcherType.INCLUDE, DispatcherType.REQUEST,
DispatcherType.ERROR, DispatcherType.FORWARD,
DispatcherType.ASYNC);
contextHandler.addFilter(new FilterHolder(filter), "/*", dispatches);
contextHandler.addServlet(LoggingTestServlet.class, "/test");
return jetty;
}

private HttpGet createBasicGetRequest(Server jetty) {
return new HttpGet(getBaseUrl(jetty) + "/test");
}

private String getBaseUrl(Server server) {
int port = ((ServerConnector) server.getConnectors()[0]).getLocalPort();
return "http://localhost:" + port;
}

private static Filter constantTenantId(String tenantId) {
return constantField(Fields.TENANT_ID, tenantId);
}

private static Filter constantCorrelationId(String correlationId) {
return constantField(Fields.CORRELATION_ID, correlationId);
}

private static Filter constantField(String field, String value) {
return new AbstractLoggingFilter() {
@Override
protected void beforeFilter(HttpServletRequest request, HttpServletResponse response) {
MDC.put(field, value);
}

@Override
protected void cleanup(HttpServletRequest request, HttpServletResponse response) {
MDC.remove(field);
}
};
}

}
25 changes: 25 additions & 0 deletions ...pport-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/LoggingTestServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.sap.hcp.cf.logging.servlet.filter;

import java.io.IOException;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class LoggingTestServlet extends HttpServlet {

public static final String LOG_MESSAGE = "request received";

private static final long serialVersionUID = -4594560302550583354L;
private static final Logger LOG = LoggerFactory.getLogger(LoggingTestServlet.class);

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
LOG.info(LOG_MESSAGE);
}

}
29 changes: 6 additions & 23 deletions ...g-support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/RequestLogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@
import java.util.UUID;

import javax.servlet.DispatcherType;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
Expand All @@ -29,7 +25,6 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

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

public class RequestLogTest {

private static final Logger LOG = LoggerFactory.getLogger(RequestLogTest.class);
private static final String REQUEST_RECEIVED = "Request received";

@Rule
@Rule
public SystemOutRule systemOut = new SystemOutRule();

private Server server;
Expand All @@ -65,7 +57,7 @@ private Server initJetty() throws Exception {
handler.addFilter(RequestLoggingFilter.class, "/*", EnumSet.of(DispatcherType.INCLUDE,
DispatcherType.REQUEST,
DispatcherType.ERROR, DispatcherType.FORWARD, DispatcherType.ASYNC));
handler.addServlet(TestServlet.class, "/test");
handler.addServlet(LoggingTestServlet.class, "/test");
server.start();
return server;
}
Expand Down Expand Up @@ -196,7 +188,7 @@ private String getBaseUrl() {
}

private String getCorrelationIdGenerated() throws IOException {
Map<String, Object> generationLog = systemOut.fineLineAsMapWith("logger", CorrelationIdFilter.class.getName());
Map<String, Object> generationLog = systemOut.findLineAsMapWith("logger", CorrelationIdFilter.class.getName());
if (generationLog == null) {
return null;
}
Expand All @@ -205,24 +197,15 @@ private String getCorrelationIdGenerated() throws IOException {
}

private Map<String, Object> getRequestMessage() throws IOException {
return systemOut.fineLineAsMapWith("msg", REQUEST_RECEIVED);
return systemOut.findLineAsMapWith("msg", LoggingTestServlet.LOG_MESSAGE);
}

private Map<String, Object> getRequestLog() throws IOException {
return systemOut.fineLineAsMapWith("layer", "[SERVLET]");
private Map<String, Object> getRequestLog() throws IOException {
return systemOut.findLineAsMapWith("layer", "[SERVLET]");
}

private static void assertFirstHeaderValue(String expected, CloseableHttpResponse response, HttpHeader header) {
String headerValue = response.getFirstHeader(header.getName()).getValue();
assertThat(headerValue, is(equalTo(expected)));
}

@SuppressWarnings("serial")
public static class TestServlet extends HttpServlet {

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
LOG.info(REQUEST_RECEIVED);
}
}
}
2 changes: 1 addition & 1 deletion ...ng-support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/SystemOutRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public String toString() {
return output.toString();
}

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