Skip to content

Commit 21237c5

Browse files
loadkrnisloadkrnis
authored
Fix Slf4jLogger and JavaLogger returning unbuffered response when logging disabled (#3172)
When SLF4J/java.util.logging level is above DEBUG/FINE, logAndRebufferResponse() was returning the original response without rebuffering. This caused: 1. ErrorDecoder failing to read response body from error responses 2. Inconsistent behavior where response.body() returns different types (byte[] vs InputStream) based solely on logging configuration Now always delegates to super.logAndRebufferResponse() to ensure consistent rebuffering based on Feign's Logger.Level, while logging output is still controlled by the underlying logger's level check in log() method. - Remove conditional in Slf4jLogger.logAndRebufferResponse() - Remove conditional in JavaLogger.logAndRebufferResponse() - Add unit tests for rebuffering at various logging levels Fixes #1336
1 parent 5345da5 commit 21237c5

5 files changed

Lines changed: 408 additions & 8 deletions

File tree

‎core/src/main/java/feign/Logger.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -230,10 +230,7 @@ protected void logRequest(String configKey, Level logLevel, Request request) {
230230
@Override
231231
protected Response logAndRebufferResponse(
232232
String configKey, Level logLevel, Response response, long elapsedTime) throws IOException {
233-
if (logger.isLoggable(java.util.logging.Level.FINE)) {
234-
return super.logAndRebufferResponse(configKey, logLevel, response, elapsedTime);
235-
}
236-
return response;
233+
return super.logAndRebufferResponse(configKey, logLevel, response, elapsedTime);
237234
}
238235

239236
@Override

‎core/src/test/java/feign/JavaLoggerTest.java‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
/*
2+
* Copyright © 2012 The Feign Authors ([email protected])
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package feign;
17+
18+
import feign.Logger.JavaLogger;
19+
import feign.Request.HttpMethod;
20+
import java.util.Collection;
21+
import java.util.Collections;
22+
import java.util.logging.Level;
23+
import org.junit.jupiter.api.BeforeEach;
24+
import org.junit.jupiter.api.Test;
25+
26+
public class JavaLoggerTest {
27+
28+
private static final String CONFIG_KEY = "TestApi#testMethod()";
29+
private java.util.logging.Logger julLogger;
30+
private JavaLogger logger;
31+
32+
@BeforeEach
33+
void setUp() {
34+
julLogger = java.util.logging.Logger.getLogger(JavaLoggerTest.class.getName());
35+
logger = new JavaLogger(JavaLoggerTest.class);
36+
}
37+
38+
@Test
39+
void rebuffersResponseBodyWhenJulLevelIsInfo() throws Exception {
40+
// given
41+
julLogger.setLevel(Level.INFO);
42+
Response responseWithBody =
43+
Response.builder()
44+
.status(200)
45+
.reason("OK")
46+
.request(
47+
Request.create(HttpMethod.GET, "/api", Collections.emptyMap(), null, Util.UTF_8))
48+
.headers(Collections.<String, Collection<String>>emptyMap())
49+
.body("{\"error\":\"test\"}", Util.UTF_8)
50+
.build();
51+
52+
// when
53+
Response result =
54+
logger.logAndRebufferResponse(
55+
CONFIG_KEY, feign.Logger.Level.HEADERS, responseWithBody, 273);
56+
57+
// then
58+
String body1 = Util.toString(result.body().asReader(Util.UTF_8));
59+
String body2 = Util.toString(result.body().asReader(Util.UTF_8));
60+
assert body1.equals("{\"error\":\"test\"}") : "First read should return body content";
61+
assert body2.equals("{\"error\":\"test\"}") : "Second read should return same body content";
62+
}
63+
64+
@Test
65+
void rebuffersResponseBodyWhenJulLevelIsWarning() throws Exception {
66+
// given
67+
julLogger.setLevel(Level.WARNING);
68+
Response responseWithBody =
69+
Response.builder()
70+
.status(500)
71+
.reason("Internal Server Error")
72+
.request(
73+
Request.create(HttpMethod.GET, "/api", Collections.emptyMap(), null, Util.UTF_8))
74+
.headers(Collections.<String, Collection<String>>emptyMap())
75+
.body("{\"message\":\"error details\"}", Util.UTF_8)
76+
.build();
77+
78+
// when
79+
Response result =
80+
logger.logAndRebufferResponse(CONFIG_KEY, feign.Logger.Level.FULL, responseWithBody, 100);
81+
82+
// then
83+
byte[] bodyBytes = Util.toByteArray(result.body().asInputStream());
84+
assert new String(bodyBytes, Util.UTF_8).equals("{\"message\":\"error details\"}")
85+
: "Body should be readable after rebuffering";
86+
}
87+
88+
@Test
89+
void responseBodyReadableMultipleTimesForErrorDecoder() throws Exception {
90+
// given
91+
julLogger.setLevel(Level.SEVERE);
92+
String originalBody = "{\"errorCode\":\"E001\",\"message\":\"Validation failed\"}";
93+
Response responseWithBody =
94+
Response.builder()
95+
.status(400)
96+
.reason("Bad Request")
97+
.request(
98+
Request.create(
99+
HttpMethod.POST, "/api/submit", Collections.emptyMap(), null, Util.UTF_8))
100+
.headers(Collections.<String, Collection<String>>emptyMap())
101+
.body(originalBody, Util.UTF_8)
102+
.build();
103+
104+
// when
105+
Response result =
106+
logger.logAndRebufferResponse(
107+
CONFIG_KEY, feign.Logger.Level.HEADERS, responseWithBody, 150);
108+
109+
// then
110+
String read1 = Util.toString(result.body().asReader(Util.UTF_8));
111+
String read2 = Util.toString(result.body().asReader(Util.UTF_8));
112+
String read3 = Util.toString(result.body().asReader(Util.UTF_8));
113+
assert read1.equals(originalBody) : "First read should match original body";
114+
assert read2.equals(originalBody) : "Second read should match original body";
115+
assert read3.equals(originalBody) : "Third read should match original body";
116+
}
117+
}

‎core/src/test/java/feign/LoggerRebufferTest.java‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
/*
2+
* Copyright © 2012 The Feign Authors ([email protected])
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package feign;
17+
18+
import static org.junit.jupiter.api.Assertions.assertEquals;
19+
import static org.junit.jupiter.api.Assertions.assertNotNull;
20+
import static org.junit.jupiter.api.Assertions.assertNull;
21+
22+
import feign.Request.HttpMethod;
23+
import java.util.Collection;
24+
import java.util.Collections;
25+
import org.junit.jupiter.api.Test;
26+
27+
public class LoggerRebufferTest {
28+
29+
private static final String CONFIG_KEY = "TestApi#testMethod()";
30+
31+
private static class TestLogger extends Logger {
32+
@Override
33+
protected void log(String configKey, String format, Object... args) {}
34+
}
35+
36+
@Test
37+
void headersLevelRebuffersResponseBody() throws Exception {
38+
// given
39+
TestLogger logger = new TestLogger();
40+
String originalBody = "{\"status\":\"error\",\"message\":\"Not found\"}";
41+
Response response =
42+
Response.builder()
43+
.status(404)
44+
.reason("Not Found")
45+
.request(
46+
Request.create(
47+
HttpMethod.GET, "/api/resource", Collections.emptyMap(), null, Util.UTF_8))
48+
.headers(Collections.<String, Collection<String>>emptyMap())
49+
.body(originalBody, Util.UTF_8)
50+
.build();
51+
52+
// when
53+
Response result =
54+
logger.logAndRebufferResponse(CONFIG_KEY, Logger.Level.HEADERS, response, 100);
55+
56+
// then
57+
String read1 = Util.toString(result.body().asReader(Util.UTF_8));
58+
String read2 = Util.toString(result.body().asReader(Util.UTF_8));
59+
assertEquals(originalBody, read1, "First read should return original body");
60+
assertEquals(originalBody, read2, "Second read should return same body (rebuffered)");
61+
}
62+
63+
@Test
64+
void basicLevelDoesNotRebufferResponseBody() throws Exception {
65+
// given
66+
TestLogger logger = new TestLogger();
67+
String originalBody = "{\"status\":\"ok\"}";
68+
Response response =
69+
Response.builder()
70+
.status(200)
71+
.reason("OK")
72+
.request(
73+
Request.create(
74+
HttpMethod.GET, "/api/resource", Collections.emptyMap(), null, Util.UTF_8))
75+
.headers(Collections.<String, Collection<String>>emptyMap())
76+
.body(originalBody, Util.UTF_8)
77+
.build();
78+
79+
// when
80+
Response result = logger.logAndRebufferResponse(CONFIG_KEY, Logger.Level.BASIC, response, 100);
81+
82+
// then
83+
String read1 = Util.toString(result.body().asReader(Util.UTF_8));
84+
assertEquals(originalBody, read1, "First read should return original body");
85+
}
86+
87+
@Test
88+
void fullLevelRebuffersResponseBody() throws Exception {
89+
// given
90+
TestLogger logger = new TestLogger();
91+
String originalBody = "{\"data\":{\"id\":123,\"name\":\"test\"}}";
92+
Response response =
93+
Response.builder()
94+
.status(200)
95+
.reason("OK")
96+
.request(
97+
Request.create(
98+
HttpMethod.POST, "/api/create", Collections.emptyMap(), null, Util.UTF_8))
99+
.headers(Collections.<String, Collection<String>>emptyMap())
100+
.body(originalBody, Util.UTF_8)
101+
.build();
102+
103+
// when
104+
Response result = logger.logAndRebufferResponse(CONFIG_KEY, Logger.Level.FULL, response, 50);
105+
106+
// then
107+
String read1 = Util.toString(result.body().asReader(Util.UTF_8));
108+
String read2 = Util.toString(result.body().asReader(Util.UTF_8));
109+
String read3 = Util.toString(result.body().asReader(Util.UTF_8));
110+
assertEquals(originalBody, read1, "First read should return original body");
111+
assertEquals(originalBody, read2, "Second read should return same body");
112+
assertEquals(originalBody, read3, "Third read should return same body");
113+
}
114+
115+
@Test
116+
void noneLevelDoesNotRebufferResponseBody() throws Exception {
117+
// given
118+
TestLogger logger = new TestLogger();
119+
String originalBody = "{\"result\":\"success\"}";
120+
Response response =
121+
Response.builder()
122+
.status(200)
123+
.reason("OK")
124+
.request(
125+
Request.create(
126+
HttpMethod.GET, "/api/status", Collections.emptyMap(), null, Util.UTF_8))
127+
.headers(Collections.<String, Collection<String>>emptyMap())
128+
.body(originalBody, Util.UTF_8)
129+
.build();
130+
131+
// When
132+
Response result = logger.logAndRebufferResponse(CONFIG_KEY, Logger.Level.NONE, response, 100);
133+
134+
// then
135+
String read1 = Util.toString(result.body().asReader(Util.UTF_8));
136+
assertEquals(originalBody, read1, "Body should be readable");
137+
}
138+
139+
@Test
140+
void http204DoesNotRebufferEvenAtHeadersLevel() throws Exception {
141+
// given
142+
TestLogger logger = new TestLogger();
143+
Response response =
144+
Response.builder()
145+
.status(204)
146+
.reason("No Content")
147+
.request(
148+
Request.create(
149+
HttpMethod.DELETE, "/api/resource/1", Collections.emptyMap(), null, Util.UTF_8))
150+
.headers(Collections.<String, Collection<String>>emptyMap())
151+
.body("should be ignored", Util.UTF_8)
152+
.build();
153+
154+
// when
155+
Response result =
156+
logger.logAndRebufferResponse(CONFIG_KEY, Logger.Level.HEADERS, response, 100);
157+
158+
// then
159+
assertNotNull(result.body(), "Response body object should exist");
160+
}
161+
162+
@Test
163+
void http205DoesNotRebufferEvenAtHeadersLevel() throws Exception {
164+
// given
165+
TestLogger logger = new TestLogger();
166+
Response response =
167+
Response.builder()
168+
.status(205)
169+
.reason("Reset Content")
170+
.request(
171+
Request.create(
172+
HttpMethod.POST, "/api/form", Collections.emptyMap(), null, Util.UTF_8))
173+
.headers(Collections.<String, Collection<String>>emptyMap())
174+
.body("should be ignored", Util.UTF_8)
175+
.build();
176+
177+
// when
178+
Response result =
179+
logger.logAndRebufferResponse(CONFIG_KEY, Logger.Level.HEADERS, response, 100);
180+
181+
// then
182+
assertNotNull(result.body(), "Response body object should exist");
183+
}
184+
185+
@Test
186+
void nullBodyHandledCorrectlyAtHeadersLevel() throws Exception {
187+
// given
188+
TestLogger logger = new TestLogger();
189+
Response response =
190+
Response.builder()
191+
.status(200)
192+
.reason("OK")
193+
.request(
194+
Request.create(
195+
HttpMethod.HEAD, "/api/resource", Collections.emptyMap(), null, Util.UTF_8))
196+
.headers(Collections.<String, Collection<String>>emptyMap())
197+
.body((byte[]) null)
198+
.build();
199+
200+
// when
201+
Response result =
202+
logger.logAndRebufferResponse(CONFIG_KEY, Logger.Level.HEADERS, response, 100);
203+
204+
// then
205+
assertNull(result.body(), "Body should remain null");
206+
}
207+
}

‎slf4j/src/main/java/feign/slf4j/Slf4jLogger.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,10 +56,7 @@ protected void logRequest(String configKey, Level logLevel, Request request) {
5656
@Override
5757
protected Response logAndRebufferResponse(
5858
String configKey, Level logLevel, Response response, long elapsedTime) throws IOException {
59-
if (logger.isDebugEnabled()) {
60-
return super.logAndRebufferResponse(configKey, logLevel, response, elapsedTime);
61-
}
62-
return response;
59+
return super.logAndRebufferResponse(configKey, logLevel, response, elapsedTime);
6360
}
6461

6562
@Override

0 commit comments

Comments
 (0)