Repository navigation
fix: Add charset to application/json and related content types (RFC 7231 §3.1.1.2) - #81
Conversation
…ypes (RFC 7231 §3.1.1.2) Extend isTextBasedContentType() to cover: - application/json - application/ld+json - application/xhtml+xml Add tests for each new type, existing-charset preservation, and application/octet-stream exclusion. Closes #73 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates DefaultCharsetMiddleware so it treats common text-based application/* media types (JSON, LD+JSON, XHTML+XML) as eligible for automatically appending a default charset=UTF-8 to the Content-Type response header, aligning behavior with RFC 7231 §3.1.1.2 and closing #73.
Changes:
- Extend
isTextBasedContentType()to includeapplication/json,application/ld+json, andapplication/xhtml+xml. - Update/expand unit tests to assert charset addition for these media types (and unchanged behavior when charset is already present / for octet-stream).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| enkan-web/src/main/java/enkan/middleware/DefaultCharsetMiddleware.java | Expands the set of media types considered “text-based” for default charset injection. |
| enkan-web/src/test/java/enkan/middleware/DefaultCharsetMiddlewareTest.java | Renames/updates the JSON charset behavior test and adds coverage for LD+JSON and XHTML+XML. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected boolean isTextBasedContentType(String contentType) { | ||
| return contentType != null && (contentType.startsWith("text/") || contentType.startsWith("application/xml")); | ||
| if (contentType == null) return false; | ||
| return contentType.startsWith("text/") | ||
| || contentType.startsWith("application/xml") | ||
| || contentType.startsWith("application/json") | ||
| || contentType.startsWith("application/ld+json") | ||
| || contentType.startsWith("application/xhtml+xml"); |
There was a problem hiding this comment.
isTextBasedContentType uses case-sensitive startsWith(...) checks, but media types are case-insensitive. As a result, responses like Content-Type: Application/JSON (or TEXT/HTML) won’t get a charset added. Consider normalizing the type/subtype to lower-case (Locale.ROOT) or using regionMatches(true, ...) for the prefix comparisons.
| || contentType.startsWith("application/xml") | ||
| || contentType.startsWith("application/json") | ||
| || contentType.startsWith("application/ld+json") | ||
| || contentType.startsWith("application/xhtml+xml"); |
There was a problem hiding this comment.
By treating application/json, application/ld+json, and application/xhtml+xml as text-based, this middleware will now call HttpResponseUtils.charset(...) for those types. HttpResponseUtils.charset rebuilds the Content-Type by truncating at the first ;, which drops any existing parameters other than charset (e.g., application/json; profile=... or vendor/version params). This is a behavior regression for these newly included content types; consider adding charset while preserving existing non-charset parameters.
| || contentType.startsWith("application/xml") | |
| || contentType.startsWith("application/json") | |
| || contentType.startsWith("application/ld+json") | |
| || contentType.startsWith("application/xhtml+xml"); | |
| || contentType.startsWith("application/xml"); |
| @Test | ||
| void doesNotAddCharsetToApplicationJson() { | ||
| void addsCharsetToApplicationJson() { | ||
| HttpResponse response = builder(HttpResponse.of("body")) | ||
| .set(HttpResponse::setHeaders, Headers.of("Content-Type", "application/json")) | ||
| .build(); | ||
|
|
||
| HttpResponse result = handleWith(response); | ||
|
|
||
| String contentType = getHeader(result, "Content-Type"); | ||
| assertThat(contentType).contains("charset=UTF-8"); | ||
| } | ||
|
|
||
| @Test | ||
| void addsCharsetToApplicationJsonWithExistingCharsetUnchanged() { | ||
| HttpResponse response = builder(HttpResponse.of("body")) | ||
| .set(HttpResponse::setHeaders, Headers.of("Content-Type", "application/json; charset=UTF-8")) | ||
| .build(); | ||
|
|
||
| HttpResponse result = handleWith(response); | ||
|
|
||
| String contentType = getHeader(result, "Content-Type"); | ||
| assertThat(contentType).containsOnlyOnce("charset=UTF-8"); | ||
| } | ||
|
|
||
| @Test | ||
| void addsCharsetToApplicationLdJson() { | ||
| HttpResponse response = builder(HttpResponse.of("body")) | ||
| .set(HttpResponse::setHeaders, Headers.of("Content-Type", "application/ld+json")) | ||
| .build(); | ||
|
|
||
| HttpResponse result = handleWith(response); | ||
|
|
||
| String contentType = getHeader(result, "Content-Type"); | ||
| assertThat(contentType).contains("charset=UTF-8"); |
There was a problem hiding this comment.
The new JSON/LD+JSON/XHTML tests only assert that the resulting Content-Type contains charset=UTF-8. Please add a test covering a Content-Type with additional parameters (e.g., application/json; profile=...) to ensure the middleware adds charset without removing existing non-charset parameters, since this PR expands the set of types affected.
- isTextBasedContentType: normalize to lowercase (Locale.ROOT) for case-insensitive media type comparison per RFC 7231 §3.1.1.1 - HttpResponseUtils.charset: preserve non-charset parameters when adding charset (e.g. profile=, version= are no longer dropped) - Add addsCharsetPreservingExistingNonCharsetParameters test - Add addsCharsetToCaseInsensitiveContentType test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Rebuild Content-Type preserving non-charset parameters, then append charset. | ||
| String[] parts = type.split(";"); | ||
| StringBuilder sb = new StringBuilder(parts[0].trim()); | ||
| for (int i = 1; i < parts.length; i++) { | ||
| String param = parts[i].trim(); | ||
| if (!param.regionMatches(true, 0, "charset=", 0, 8)) { | ||
| sb.append("; ").append(param); |
There was a problem hiding this comment.
HttpResponseUtils.charset() now uses String.split(";") which invokes regex-based splitting and allocates an array/substrings on every call. Since this method can be on the response hot path (e.g., DefaultCharsetMiddleware), consider switching back to a non-regex parse (indexOf loop) to preserve parameters while avoiding regex/pattern overhead.
| // Rebuild Content-Type preserving non-charset parameters, then append charset. | |
| String[] parts = type.split(";"); | |
| StringBuilder sb = new StringBuilder(parts[0].trim()); | |
| for (int i = 1; i < parts.length; i++) { | |
| String param = parts[i].trim(); | |
| if (!param.regionMatches(true, 0, "charset=", 0, 8)) { | |
| sb.append("; ").append(param); | |
| // Rebuild Content-Type preserving non-charset parameters, then append charset, | |
| // without using regex-based String.split to avoid overhead on hot paths. | |
| StringBuilder sb; | |
| int semicolonIndex = type.indexOf(';'); | |
| if (semicolonIndex < 0) { | |
| // No parameters present. | |
| sb = new StringBuilder(type.trim()); | |
| } else { | |
| // Base media type is before the first semicolon. | |
| sb = new StringBuilder(type.substring(0, semicolonIndex).trim()); | |
| int length = type.length(); | |
| int pos = semicolonIndex + 1; | |
| while (pos < length) { | |
| int nextSemicolon = type.indexOf(';', pos); | |
| String param; | |
| if (nextSemicolon == -1) { | |
| param = type.substring(pos); | |
| pos = length; | |
| } else { | |
| param = type.substring(pos, nextSemicolon); | |
| pos = nextSemicolon + 1; | |
| } | |
| param = param.trim(); | |
| if (param.isEmpty()) { | |
| continue; | |
| } | |
| if (!param.regionMatches(true, 0, "charset=", 0, 8)) { | |
| sb.append("; ").append(param); | |
| } |
| StringBuilder sb = new StringBuilder(parts[0].trim()); | ||
| for (int i = 1; i < parts.length; i++) { | ||
| String param = parts[i].trim(); | ||
| if (!param.regionMatches(true, 0, "charset=", 0, 8)) { |
There was a problem hiding this comment.
When rebuilding Content-Type parameters, empty segments from consecutive semicolons (e.g., "type;; param=1") will be preserved and emitted as an empty parameter ("; "), producing a malformed header value. Consider skipping empty trimmed params before appending.
| if (!param.regionMatches(true, 0, "charset=", 0, 8)) { | |
| if (!param.isEmpty() && !param.regionMatches(true, 0, "charset=", 0, 8)) { |
| String lower = contentType.toLowerCase(java.util.Locale.ROOT); | ||
| return lower.startsWith("text/") | ||
| || lower.startsWith("application/xml") | ||
| || lower.startsWith("application/json") | ||
| || lower.startsWith("application/ld+json") | ||
| || lower.startsWith("application/xhtml+xml"); |
There was a problem hiding this comment.
isTextBasedContentType() lowercases the entire Content-Type value, allocating a new String for every response. To keep the case-insensitive behavior without extra allocation, consider using regionMatches(true, 0, ...) checks for each prefix (similar to isContainsCharset) instead of toLowerCase(Locale.ROOT).
| String lower = contentType.toLowerCase(java.util.Locale.ROOT); | |
| return lower.startsWith("text/") | |
| || lower.startsWith("application/xml") | |
| || lower.startsWith("application/json") | |
| || lower.startsWith("application/ld+json") | |
| || lower.startsWith("application/xhtml+xml"); | |
| int len = contentType.length(); | |
| return (len >= 5 && contentType.regionMatches(true, 0, "text/", 0, 5)) | |
| || (len >= 15 && contentType.regionMatches(true, 0, "application/xml", 0, 15)) | |
| || (len >= 16 && contentType.regionMatches(true, 0, "application/json", 0, 16)) | |
| || (len >= 21 && contentType.regionMatches(true, 0, "application/ld+json", 0, 21)) | |
| || (len >= 24 && contentType.regionMatches(true, 0, "application/xhtml+xml", 0, 24)); |
- DefaultCharsetMiddleware.isTextBasedContentType: replace toLowerCase(Locale.ROOT) with regionMatches(true,...) to avoid String allocation per call
- HttpResponseUtils.charset: replace String.split(";") with indexOf loop to avoid regex overhead and skip empty segments from consecutive semicolons
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Uses indexOf loop instead of String.split to avoid regex overhead on hot path. | ||
| int semicolonIndex = type.indexOf(';'); | ||
| StringBuilder sb = semicolonIndex < 0 | ||
| ? new StringBuilder(type.trim()) | ||
| : new StringBuilder(type.substring(0, semicolonIndex).trim()); | ||
| int pos = semicolonIndex + 1; | ||
| int len = type.length(); | ||
| while (semicolonIndex >= 0 && pos <= len) { | ||
| int next = type.indexOf(';', pos); | ||
| String param = (next < 0 ? type.substring(pos) : type.substring(pos, next)).trim(); | ||
| pos = next < 0 ? len + 1 : next + 1; | ||
| if (!param.isEmpty() && !param.regionMatches(true, 0, "charset=", 0, 8)) { | ||
| sb.append("; ").append(param); | ||
| } |
There was a problem hiding this comment.
HttpResponseUtils.charset() rebuilds the Content-Type by splitting on every ; via indexOf. This breaks valid quoted-string parameter values that contain semicolons (e.g., profile="a;b"), producing corrupted/invalid parameters. Consider parsing parameters with a small state machine that skips semicolons inside quotes (or reuse an existing media type parser) so quoted values are preserved correctly.
| // Uses indexOf loop instead of String.split to avoid regex overhead on hot path. | |
| int semicolonIndex = type.indexOf(';'); | |
| StringBuilder sb = semicolonIndex < 0 | |
| ? new StringBuilder(type.trim()) | |
| : new StringBuilder(type.substring(0, semicolonIndex).trim()); | |
| int pos = semicolonIndex + 1; | |
| int len = type.length(); | |
| while (semicolonIndex >= 0 && pos <= len) { | |
| int next = type.indexOf(';', pos); | |
| String param = (next < 0 ? type.substring(pos) : type.substring(pos, next)).trim(); | |
| pos = next < 0 ? len + 1 : next + 1; | |
| if (!param.isEmpty() && !param.regionMatches(true, 0, "charset=", 0, 8)) { | |
| sb.append("; ").append(param); | |
| } | |
| // Parse parameters with a simple state machine that skips semicolons inside quotes. | |
| int semicolonIndex = type.indexOf(';'); | |
| StringBuilder sb = semicolonIndex < 0 | |
| ? new StringBuilder(type.trim()) | |
| : new StringBuilder(type.substring(0, semicolonIndex).trim()); | |
| int len = type.length(); | |
| if (semicolonIndex >= 0 && semicolonIndex + 1 < len) { | |
| String paramsPart = type.substring(semicolonIndex + 1); | |
| StringBuilder currentParam = new StringBuilder(); | |
| boolean inQuotes = false; | |
| boolean escape = false; | |
| for (int i = 0; i < paramsPart.length(); i++) { | |
| char c = paramsPart.charAt(i); | |
| if (escape) { | |
| // Inside an escape sequence within a quoted-string. | |
| currentParam.append(c); | |
| escape = false; | |
| continue; | |
| } | |
| if (c == '\\') { | |
| // Start escape only if we're inside quotes. | |
| if (inQuotes) { | |
| escape = true; | |
| } else { | |
| currentParam.append(c); | |
| } | |
| continue; | |
| } | |
| if (c == '"') { | |
| inQuotes = !inQuotes; | |
| currentParam.append(c); | |
| continue; | |
| } | |
| if (c == ';' && !inQuotes) { | |
| String param = currentParam.toString().trim(); | |
| if (!param.isEmpty() && !param.regionMatches(true, 0, "charset=", 0, 8)) { | |
| sb.append("; ").append(param); | |
| } | |
| currentParam.setLength(0); | |
| } else { | |
| currentParam.append(c); | |
| } | |
| } | |
| // Flush last parameter, if any. | |
| String lastParam = currentParam.toString().trim(); | |
| if (!lastParam.isEmpty() && !lastParam.regionMatches(true, 0, "charset=", 0, 8)) { | |
| sb.append("; ").append(lastParam); | |
| } |
…micolons Replace indexOf-loop with a state machine in HttpResponseUtils.charset() so that semicolons inside quoted-string parameter values (e.g. profile="a;b") are not treated as parameter delimiters, preventing corrupted Content-Type headers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Thank you for the thorough review.
isTextBasedContentType — case-insensitive comparison: Already implemented using regionMatches(true, ...) in the current code. The test addsCharsetToCaseInsensitiveContentType (with Application/JSON) covers this.
charset() — quoted-string semicolons: Fixed in the latest commit. Replaced the indexOf loop with a state machine that tracks inQuotes and escape state, so semicolons inside quoted-string values (e.g. profile="a;b") are correctly ignored as parameter delimiters.
Parameter preservation test: The test addsCharsetPreservingExistingNonCharsetParameters already covers application/json; profile=... and asserts both charset=UTF-8 is added and profile= is retained.
- isTextBasedContentType: normalize to lowercase (Locale.ROOT) for case-insensitive media type comparison per RFC 7231 §3.1.1.1 - HttpResponseUtils.charset: preserve non-charset parameters when adding charset (e.g. profile=, version= are no longer dropped) - Add addsCharsetPreservingExistingNonCharsetParameters test - Add addsCharsetToCaseInsensitiveContentType test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
isTextBasedContentType()to coverapplication/json,application/ld+json, andapplication/xhtml+xmlin addition to the existingtext/*andapplication/xmldoesNotAddCharsetToApplicationJsontest toaddsCharsetToApplicationJson(behavior change)Test plan
addsCharsetToApplicationJson—application/jsongets; charset=UTF-8addsCharsetToApplicationJsonWithExistingCharsetUnchanged—application/json; charset=UTF-8unchangedaddsCharsetToApplicationLdJson—application/ld+jsongets; charset=UTF-8addsCharsetToApplicationXhtmlXml—application/xhtml+xmlgets; charset=UTF-8doesNotAddCharsetToApplicationOctetStream—application/octet-streamunchangedCloses #73
🤖 Generated with Claude Code