Skip to content

Commit 47450fa

Browse files
committed
refactor: Collapse the duplicated gzip decision in the renderer
All three render paths repeated the same guard — already coded, not compressible, or a status that carries no content — then announced Vary, then checked threshold and Accept-Encoding. That is now one shouldCompress method the three paths share, with a negative length meaning "unknown, treat as over the threshold" so streams keep their semantics. Also drops the single-argument ResponseRenderer constructor, which no longer had a caller in main, and trims incidental weight in the header parsers: the boxed tri-state in AcceptEncodingHeader becomes two plain booleans, the compressible-suffix Set becomes three endsWith calls, and single-use string constants are inlined to match ContentTypeHeader. 530 to 498 lines across the five files; behaviour unchanged.
1 parent bbcde95 commit 47450fa

7 files changed

Lines changed: 43 additions & 67 deletions

File tree

src/main/java/com/retailsvc/http/internal/AcceptEncodingHeader.java

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,6 @@
55
/** Parses {@code Accept-Encoding} request header values (RFC 9110 §12.5.3). */
66
public final class AcceptEncodingHeader {
77

8-
private static final String GZIP = "gzip";
9-
private static final String X_GZIP = "x-gzip";
10-
private static final String WILDCARD = "*";
11-
private static final String QUALITY = "q";
128
private static final double DEFAULT_QUALITY = 1.0;
139

1410
private AcceptEncodingHeader() {}
@@ -22,8 +18,9 @@ public static boolean acceptsGzip(String header) {
2218
if (header == null) {
2319
return false;
2420
}
25-
Boolean gzipAccepted = null;
26-
Boolean wildcardAccepted = null;
21+
boolean gzipSeen = false;
22+
boolean gzipAccepted = false;
23+
boolean wildcardAccepted = false;
2724
for (String token : header.split(",")) {
2825
String trimmed = token.trim();
2926
if (trimmed.isEmpty()) {
@@ -33,16 +30,14 @@ public static boolean acceptsGzip(String header) {
3330
String coding =
3431
(semi < 0 ? trimmed : trimmed.substring(0, semi)).trim().toLowerCase(Locale.ROOT);
3532
boolean accepted = quality(semi < 0 ? null : trimmed.substring(semi + 1)) > 0;
36-
if (GZIP.equals(coding) || X_GZIP.equals(coding)) {
37-
gzipAccepted = gzipAccepted == null ? accepted : gzipAccepted || accepted;
38-
} else if (WILDCARD.equals(coding)) {
39-
wildcardAccepted = wildcardAccepted == null ? accepted : wildcardAccepted || accepted;
33+
if ("gzip".equals(coding) || "x-gzip".equals(coding)) {
34+
gzipSeen = true;
35+
gzipAccepted |= accepted;
36+
} else if ("*".equals(coding)) {
37+
wildcardAccepted |= accepted;
4038
}
4139
}
42-
if (gzipAccepted != null) {
43-
return gzipAccepted;
44-
}
45-
return wildcardAccepted != null && wildcardAccepted;
40+
return gzipSeen ? gzipAccepted : wildcardAccepted;
4641
}
4742

4843
/**
@@ -60,7 +55,7 @@ private static double quality(String parameters) {
6055
continue;
6156
}
6257
String name = trimmed.substring(0, equals).trim().toLowerCase(Locale.ROOT);
63-
if (QUALITY.equals(name)) {
58+
if ("q".equals(name)) {
6459
try {
6560
return Double.parseDouble(trimmed.substring(equals + 1).trim());
6661
} catch (NumberFormatException e) {

src/main/java/com/retailsvc/http/internal/ContentEncodingHeader.java

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,15 @@
55
/** Classifies a request {@code Content-Encoding} into the codings the server can decode. */
66
public final class ContentEncodingHeader {
77

8-
private static final String IDENTITY_CODING = "identity";
9-
private static final String GZIP_CODING = "gzip";
10-
private static final String X_GZIP_CODING = "x-gzip";
11-
128
private ContentEncodingHeader() {}
139

14-
/** The content coding applied to a request body. */
10+
/**
11+
* The coding applied to a request body: none (absent or the {@code identity} no-op), a single
12+
* gzip, or one this server cannot decode and the caller renders 415 for.
13+
*/
1514
public enum Coding {
16-
/** No coding, or the explicit {@code identity} no-op. */
1715
NONE,
18-
/** A single gzip coding. */
1916
GZIP,
20-
/** A coding this server cannot decode; the caller renders 415. */
2117
UNSUPPORTED
2218
}
2319

@@ -33,13 +29,13 @@ public static Coding parse(String header) {
3329
Coding result = Coding.NONE;
3430
for (String token : header.split(",")) {
3531
String coding = token.trim().toLowerCase(Locale.ROOT);
36-
if (coding.isEmpty() || IDENTITY_CODING.equals(coding)) {
32+
if (coding.isEmpty() || "identity".equals(coding)) {
3733
continue;
3834
}
3935
if (result != Coding.NONE) {
4036
return Coding.UNSUPPORTED;
4137
}
42-
if (GZIP_CODING.equals(coding) || X_GZIP_CODING.equals(coding)) {
38+
if ("gzip".equals(coding) || "x-gzip".equals(coding)) {
4339
result = Coding.GZIP;
4440
} else {
4541
return Coding.UNSUPPORTED;

src/main/java/com/retailsvc/http/internal/ResponseCompression.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ public final class ResponseCompression {
2121
"application/javascript",
2222
"application/x-ndjson");
2323

24-
private static final Set<String> COMPRESSIBLE_SUFFIXES = Set.of("+json", "+xml", "+yaml");
25-
2624
private ResponseCompression() {}
2725

2826
/**
@@ -41,10 +39,8 @@ public static boolean isCompressible(String contentType) {
4139
if (mediaType.startsWith(TEXT_PREFIX)) {
4240
return !EVENT_STREAM.equals(mediaType);
4341
}
44-
for (String suffix : COMPRESSIBLE_SUFFIXES) {
45-
if (mediaType.endsWith(suffix)) {
46-
return true;
47-
}
42+
if (mediaType.endsWith("+json") || mediaType.endsWith("+xml") || mediaType.endsWith("+yaml")) {
43+
return true;
4844
}
4945
return COMPRESSIBLE_TYPES.contains(mediaType);
5046
}

src/main/java/com/retailsvc/http/internal/ResponseRenderer.java

Lines changed: 13 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,6 @@ public final class ResponseRenderer {
3535
private final Map<String, TypeMapper> mappers;
3636
private final long minimumGzipBytes;
3737

38-
public ResponseRenderer(Map<String, TypeMapper> mappers) {
39-
this(mappers, DEFAULT_MINIMUM_GZIP_BYTES);
40-
}
41-
4238
public ResponseRenderer(Map<String, TypeMapper> mappers, long minimumGzipBytes) {
4339
this.mappers = Map.copyOf(mappers);
4440
this.minimumGzipBytes = minimumGzipBytes;
@@ -72,15 +68,11 @@ private void renderEmpty(HttpExchange exchange, Headers headers, int status, Str
7268
if (contentType != null && !headers.containsKey(CONTENT_TYPE)) {
7369
headers.add(CONTENT_TYPE, contentType);
7470
}
75-
if (!headers.containsKey(CONTENT_ENCODING)
76-
&& ResponseCompression.isCompressible(contentType)
77-
&& bodyAllowed(status)) {
78-
addVary(headers);
79-
if (declaredLength(headers) >= minimumGzipBytes && acceptsGzip(exchange)) {
80-
headers.remove(CONTENT_LENGTH);
81-
}
71+
long declared = declaredLength(headers);
72+
if (shouldCompress(exchange, headers, status, contentType, declared) && declared >= 0) {
73+
headers.remove(CONTENT_LENGTH);
8274
}
83-
exchange.sendResponseHeaders(status, -1);
75+
exchange.sendResponseHeaders(status, UNKNOWN_LENGTH);
8476
}
8577

8678
private void renderStream(
@@ -90,7 +82,7 @@ private void renderStream(
9082
headers.add(CONTENT_TYPE, contentType);
9183
}
9284
long declared = writer instanceof BodyWriter.Sized sized ? sized.length() : UNKNOWN_LENGTH;
93-
if (compressStream(exchange, headers, status, contentType, declared)) {
85+
if (shouldCompress(exchange, headers, status, contentType, declared)) {
9486
headers.set(CONTENT_ENCODING, GZIP);
9587
exchange.sendResponseHeaders(status, CHUNKED);
9688
try (OutputStream out = ResponseCompression.gzipStream(exchange.getResponseBody())) {
@@ -105,20 +97,19 @@ private void renderStream(
10597
}
10698

10799
/**
108-
* A coded stream has to go out chunked, because the length a {@code Sized} body declares measures
109-
* the uncoded form. A body of unknown length is compressed regardless of the threshold —
110-
* buffering it to find out how big it is would defeat streaming it.
100+
* Whether a body of {@code length} bytes should be gzipped, marking the response as varying by
101+
* {@code Accept-Encoding} whenever it could have been. A negative length means unknown, which
102+
* counts as over the threshold: measuring a stream to find out would defeat streaming it.
111103
*/
112-
private boolean compressStream(
113-
HttpExchange exchange, Headers headers, int status, String contentType, long declaredLength) {
104+
private boolean shouldCompress(
105+
HttpExchange exchange, Headers headers, int status, String contentType, long length) {
114106
if (headers.containsKey(CONTENT_ENCODING)
115107
|| !ResponseCompression.isCompressible(contentType)
116108
|| !bodyAllowed(status)) {
117109
return false;
118110
}
119111
addVary(headers);
120-
boolean worthCoding = declaredLength < 0 || declaredLength >= minimumGzipBytes;
121-
return worthCoding && acceptsGzip(exchange);
112+
return (length < 0 || length >= minimumGzipBytes) && acceptsGzip(exchange);
122113
}
123114

124115
/** The length a handler declared for a body it did not write, or -1 when absent or unreadable. */
@@ -158,21 +149,11 @@ private void renderBytes(
158149
}
159150
}
160151

161-
/**
162-
* Gzips the body when the client asked for it and the payload is big enough to be worth it. A
163-
* handler that coded the body itself is left alone, and so is a payload that gzip fails to
164-
* shrink.
165-
*/
152+
/** Gzips the body when it is worth it, leaving a payload gzip fails to shrink uncoded. */
166153
private byte[] maybeCompress(
167154
HttpExchange exchange, Headers headers, int status, String contentType, byte[] bytes)
168155
throws IOException {
169-
if (headers.containsKey(CONTENT_ENCODING)
170-
|| !ResponseCompression.isCompressible(contentType)
171-
|| !bodyAllowed(status)) {
172-
return bytes;
173-
}
174-
addVary(headers);
175-
if (bytes.length < minimumGzipBytes || !acceptsGzip(exchange)) {
156+
if (!shouldCompress(exchange, headers, status, contentType, bytes.length)) {
176157
return bytes;
177158
}
178159
byte[] gzipped = ResponseCompression.gzip(bytes);

src/test/java/com/retailsvc/http/internal/DispatchHandlerTest.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.retailsvc.http.internal;
22

3+
import static com.retailsvc.http.internal.ResponseRenderer.DEFAULT_MINIMUM_GZIP_BYTES;
34
import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
45
import static java.net.HttpURLConnection.HTTP_OK;
56
import static org.assertj.core.api.Assertions.assertThat;
@@ -44,14 +45,19 @@ private static HttpExchange stubExchange() {
4445
}
4546

4647
private static DispatchHandler dispatcher(Map<String, RequestHandler> handlers) {
47-
return new DispatchHandler(handlers, List.of(), List.of(), new ResponseRenderer(Map.of()));
48+
return new DispatchHandler(
49+
handlers, List.of(), List.of(), new ResponseRenderer(Map.of(), DEFAULT_MINIMUM_GZIP_BYTES));
4850
}
4951

5052
private static DispatchHandler dispatcher(
5153
Map<String, RequestHandler> handlers,
5254
List<RequestInterceptor> interceptors,
5355
List<ResponseDecorator> decorators) {
54-
return new DispatchHandler(handlers, interceptors, decorators, new ResponseRenderer(Map.of()));
56+
return new DispatchHandler(
57+
handlers,
58+
interceptors,
59+
decorators,
60+
new ResponseRenderer(Map.of(), DEFAULT_MINIMUM_GZIP_BYTES));
5561
}
5662

5763
private static void withRequest(String operationId, ScopedValue.CallableOp<Void, Exception> body)

src/test/java/com/retailsvc/http/internal/ExtrasRouterTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.retailsvc.http.internal;
22

3+
import static com.retailsvc.http.internal.ResponseRenderer.DEFAULT_MINIMUM_GZIP_BYTES;
34
import static org.assertj.core.api.Assertions.assertThat;
45
import static org.assertj.core.api.Assertions.assertThatThrownBy;
56
import static org.mockito.Mockito.mock;
@@ -145,7 +146,7 @@ private static ExtrasRouter newRouter(Map<String, RequestHandler> extras) {
145146
Map<String, TypeMapper> mappers = Map.of("application/json", new GsonTypeMapper());
146147
return new ExtrasRouter(
147148
extras,
148-
new ResponseRenderer(mappers),
149+
new ResponseRenderer(mappers, DEFAULT_MINIMUM_GZIP_BYTES),
149150
new RequestBodyReader(RequestBodyReader.DEFAULT_MAX_DECOMPRESSED_BYTES));
150151
}
151152

src/test/java/com/retailsvc/http/internal/RequestPreparationFilterTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.retailsvc.http.internal;
22

3+
import static com.retailsvc.http.internal.ResponseRenderer.DEFAULT_MINIMUM_GZIP_BYTES;
34
import static java.net.HttpURLConnection.HTTP_UNSUPPORTED_TYPE;
45
import static org.assertj.core.api.Assertions.assertThat;
56
import static org.assertj.core.api.Assertions.assertThatThrownBy;
@@ -114,7 +115,7 @@ public byte[] writeTo(Object value) {
114115
new DefaultValidator(spec::resolveSchema),
115116
mappers,
116117
rethrow,
117-
new ResponseRenderer(mappers),
118+
new ResponseRenderer(mappers, DEFAULT_MINIMUM_GZIP_BYTES),
118119
List.of(),
119120
new RequestBodyReader(RequestBodyReader.DEFAULT_MAX_DECOMPRESSED_BYTES));
120121
}

0 commit comments

Comments
 (0)