Conversation
📝 WalkthroughWalkthroughIntroduces two new classes for HTTP request parsing: a parser utility that reads HTTP requests from input streams and extracts method, path, query string, version, headers, and body; and an immutable record data structure to represent parsed HTTP requests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant InputStream
participant HttpParser
participant HttpRequest
Client->>InputStream: Send HTTP request
HttpParser->>InputStream: Read request line
InputStream-->>HttpParser: method, path, version
HttpParser->>HttpParser: Parse path & query string
HttpParser->>InputStream: Read headers (until empty line)
InputStream-->>HttpParser: Headers map
HttpParser->>HttpParser: Check Content-Length header
alt Content-Length present
HttpParser->>InputStream: Read body bytes
InputStream-->>HttpParser: body data
else No Content-Length
HttpParser->>HttpParser: Use empty body
end
HttpParser->>HttpRequest: Construct with parsed components
HttpRequest-->>HttpParser: HttpRequest object
HttpParser-->>Client: Return populated HttpRequest
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Line 12: The code currently creates a BufferedReader (BufferedReader reader =
new BufferedReader(new InputStreamReader(in))) but later reads the body from the
underlying InputStream via in.readNBytes(length), which leads to corruption
because BufferedReader has already read ahead; fix by using a single stream for
both headers and body: either (A) read the request line and headers with
BufferedReader (reader.readLine(...)) and then read the body from the same
reader (reader.read(char[] / readFully) and convert to bytes using the request
charset) or (B) stop using BufferedReader entirely and implement header parsing
byte-by-byte from the InputStream, then use in.readNBytes(length) for the body;
update the code paths that reference BufferedReader and the in.readNBytes(...)
call so they consistently use the same source.
- Around line 48-50: In HttpParser, the headers Map check uses "Content-Length"
but the fetch uses the misspelled key "Content-Lenght", causing
Integer.parseInt(null) and no body read; fix by retrieving the header with the
correct key ("Content-Length") before parsing and then call
in.readNBytes(length) to populate body (refer to headers, length, body, and
in.readNBytes in HttpParser).
- Around line 37-44: The headers map in HttpParser is case-sensitive (headers
variable) which breaks RFC 7230; change headers to use a case-insensitive key
lookup (e.g., replace new HashMap<>() with a case-insensitive Map implementation
such as new TreeMap<>(String.CASE_INSENSITIVE_ORDER) or normalize keys to
lower-case when inserting) and ensure any subsequent lookups (e.g., the
Content-Length check that reads from headers) use that same case-insensitive
map; update the header-parsing loop to put keys into the chosen case-insensitive
map (or toLowerCase() keys) so alternate casings like "content-length" are
recognized.
- Around line 40-43: The parser currently assumes every header line contains ':'
and will throw on malformed lines; update the parsing in HttpParser (the block
using 'int colon = line.indexOf(':')', 'String key', 'String value', and
'headers.put') to guard against colon == -1: if colon < 0 either skip the
malformed line (or treat the whole trimmed line as a header key with an empty
value) and do not call substring with -1, ensuring no
StringIndexOutOfBoundsException is thrown.
- Around line 39-44: In HttpParser where headers are read (the loop calling
reader.readLine()), guard against readLine() returning null before calling
isEmpty() — capture the return into a variable, check for null (treat as
EOF/early close) and break or throw an IOException as appropriate, then proceed
to parse the line; also ensure you handle a missing ':' by validating colon !=
-1 before using substring to avoid further exceptions when building the headers
map.
- Around line 20-23: Validate the parsed request-line before indexing: change
the split to requestLine.split(" ", 3) and check that the resulting parts array
has length >= 3 (or otherwise treat as malformed), then handle the error path
instead of directly accessing parts[1]/parts[2]; update the code around
HttpParser (the requestLine, parts, method, fullPath, version variables) to
throw or return a parse error (e.g., an HttpParseException / specific error
response) when the request-line is malformed.
🧹 Nitpick comments (1)
src/main/java/org/example/httpparser/HttpRequest.java (1)
5-12: Record exposes mutableMapandbyte[]— consider defensive copies.The record's auto-generated accessors hand out direct references to
headersandbody. Callers can mutate the map or array after construction, undermining the immutability guarantee. A compact constructor with defensive copies would close this:♻️ Suggested defensive-copy constructor
+import java.util.Collections; + public record HttpRequest( String method, String path, String queryString, String httpVersion, Map<String, String> headers, byte[] body -) {} +) { + public HttpRequest { + headers = headers == null + ? Map.of() + : Collections.unmodifiableMap(new HashMap<>(headers)); + body = body == null ? new byte[0] : body.clone(); + } +}
|
|
||
| public class HttpParser { | ||
| public HttpRequest parse(InputStream in) throws IOException { | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(in)); |
There was a problem hiding this comment.
Mixing BufferedReader and raw InputStream reads corrupts the body.
BufferedReader reads ahead into an internal buffer (default 8 KiB). When you later call in.readNBytes(length) on the underlying InputStream (line 50), those bytes have already been consumed by the BufferedReader, so the body read will either block, return wrong data, or return fewer bytes than expected.
Read the body through the same BufferedReader, or avoid BufferedReader entirely and parse the request line / headers byte-by-byte from the InputStream.
Also applies to: 46-51
🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` at line 12, The code
currently creates a BufferedReader (BufferedReader reader = new
BufferedReader(new InputStreamReader(in))) but later reads the body from the
underlying InputStream via in.readNBytes(length), which leads to corruption
because BufferedReader has already read ahead; fix by using a single stream for
both headers and body: either (A) read the request line and headers with
BufferedReader (reader.readLine(...)) and then read the body from the same
reader (reader.read(char[] / readFully) and convert to bytes using the request
charset) or (B) stop using BufferedReader entirely and implement header parsing
byte-by-byte from the InputStream, then use in.readNBytes(length) for the body;
update the code paths that reference BufferedReader and the in.readNBytes(...)
call so they consistently use the same source.
| String[] parts = requestLine.split(" "); | ||
| String method = parts[0]; | ||
| String fullPath = parts[1]; | ||
| String version = parts[2]; |
There was a problem hiding this comment.
No validation on request-line parts — ArrayIndexOutOfBoundsException on malformed input.
split(" ") may yield fewer than 3 elements for a malformed request line (e.g. "GET /path"). Accessing parts[1] or parts[2] then throws.
🛡️ Proposed fix
String[] parts = requestLine.split(" ");
+ if (parts.length < 3) {
+ throw new IOException("Malformed request line: " + requestLine);
+ }
String method = parts[0];
String fullPath = parts[1];
String version = parts[2];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String[] parts = requestLine.split(" "); | |
| String method = parts[0]; | |
| String fullPath = parts[1]; | |
| String version = parts[2]; | |
| String[] parts = requestLine.split(" "); | |
| if (parts.length < 3) { | |
| throw new IOException("Malformed request line: " + requestLine); | |
| } | |
| String method = parts[0]; | |
| String fullPath = parts[1]; | |
| String version = parts[2]; |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 20 - 23,
Validate the parsed request-line before indexing: change the split to
requestLine.split(" ", 3) and check that the resulting parts array has length >=
3 (or otherwise treat as malformed), then handle the error path instead of
directly accessing parts[1]/parts[2]; update the code around HttpParser (the
requestLine, parts, method, fullPath, version variables) to throw or return a
parse error (e.g., an HttpParseException / specific error response) when the
request-line is malformed.
| Map<String, String> headers = new HashMap<>(); | ||
| String line; | ||
| while (!(line = reader.readLine()).isEmpty()) { | ||
| int colon = line.indexOf(':'); | ||
| String key = line.substring(0, colon).trim(); | ||
| String value = line.substring(colon + 1).trim(); | ||
| headers.put(key, value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
HTTP headers are case-insensitive — HashMap lookup will miss alternate casings.
Per RFC 7230, header field names are case-insensitive. A client sending content-length (lowercase) would bypass the Content-Length check on line 48. Consider using a TreeMap with case-insensitive ordering:
♻️ Suggested change
- Map<String, String> headers = new HashMap<>();
+ Map<String, String> headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Map<String, String> headers = new HashMap<>(); | |
| String line; | |
| while (!(line = reader.readLine()).isEmpty()) { | |
| int colon = line.indexOf(':'); | |
| String key = line.substring(0, colon).trim(); | |
| String value = line.substring(colon + 1).trim(); | |
| headers.put(key, value); | |
| } | |
| Map<String, String> headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); | |
| String line; | |
| while (!(line = reader.readLine()).isEmpty()) { | |
| int colon = line.indexOf(':'); | |
| String key = line.substring(0, colon).trim(); | |
| String value = line.substring(colon + 1).trim(); | |
| headers.put(key, value); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 37 - 44,
The headers map in HttpParser is case-sensitive (headers variable) which breaks
RFC 7230; change headers to use a case-insensitive key lookup (e.g., replace new
HashMap<>() with a case-insensitive Map implementation such as new
TreeMap<>(String.CASE_INSENSITIVE_ORDER) or normalize keys to lower-case when
inserting) and ensure any subsequent lookups (e.g., the Content-Length check
that reads from headers) use that same case-insensitive map; update the
header-parsing loop to put keys into the chosen case-insensitive map (or
toLowerCase() keys) so alternate casings like "content-length" are recognized.
| while (!(line = reader.readLine()).isEmpty()) { | ||
| int colon = line.indexOf(':'); | ||
| String key = line.substring(0, colon).trim(); | ||
| String value = line.substring(colon + 1).trim(); | ||
| headers.put(key, value); | ||
| } |
There was a problem hiding this comment.
readLine() can return null — NullPointerException when the connection closes early.
If the client disconnects before sending the blank line that terminates headers, readLine() returns null and .isEmpty() throws NPE.
🐛 Proposed fix
- while (!(line = reader.readLine()).isEmpty()) {
+ while ((line = reader.readLine()) != null && !line.isEmpty()) {🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 39 - 44,
In HttpParser where headers are read (the loop calling reader.readLine()), guard
against readLine() returning null before calling isEmpty() — capture the return
into a variable, check for null (treat as EOF/early close) and break or throw an
IOException as appropriate, then proceed to parse the line; also ensure you
handle a missing ':' by validating colon != -1 before using substring to avoid
further exceptions when building the headers map.
| int colon = line.indexOf(':'); | ||
| String key = line.substring(0, colon).trim(); | ||
| String value = line.substring(colon + 1).trim(); | ||
| headers.put(key, value); |
There was a problem hiding this comment.
Missing colon guard — malformed header line crashes the parser.
If a header line contains no :, indexOf returns -1 and substring(0, -1) throws StringIndexOutOfBoundsException.
🛡️ Proposed fix
int colon = line.indexOf(':');
+ if (colon < 0) {
+ throw new IOException("Malformed header: " + line);
+ }
String key = line.substring(0, colon).trim();🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 40 - 43,
The parser currently assumes every header line contains ':' and will throw on
malformed lines; update the parsing in HttpParser (the block using 'int colon =
line.indexOf(':')', 'String key', 'String value', and 'headers.put') to guard
against colon == -1: if colon < 0 either skip the malformed line (or treat the
whole trimmed line as a header key with an empty value) and do not call
substring with -1, ensuring no StringIndexOutOfBoundsException is thrown.
| if (headers.containsKey("Content-Length")) { | ||
| int length = Integer.parseInt(headers.get("Content-Lenght")); | ||
| body = in.readNBytes(length); |
There was a problem hiding this comment.
Typo: "Content-Lenght" → "Content-Length" — body is never read.
Line 48 correctly checks "Content-Length", but line 49 fetches "Content-Lenght" (misspelled). This always returns null, so Integer.parseInt(null) throws NumberFormatException for every request that has a body.
🐛 Fix the typo
if (headers.containsKey("Content-Length")) {
- int length = Integer.parseInt(headers.get("Content-Lenght"));
+ int length = Integer.parseInt(headers.get("Content-Length"));
body = in.readNBytes(length);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (headers.containsKey("Content-Length")) { | |
| int length = Integer.parseInt(headers.get("Content-Lenght")); | |
| body = in.readNBytes(length); | |
| if (headers.containsKey("Content-Length")) { | |
| int length = Integer.parseInt(headers.get("Content-Length")); | |
| body = in.readNBytes(length); |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 48 - 50,
In HttpParser, the headers Map check uses "Content-Length" but the fetch uses
the misspelled key "Content-Lenght", causing Integer.parseInt(null) and no body
read; fix by retrieving the header with the correct key ("Content-Length")
before parsing and then call in.readNBytes(length) to populate body (refer to
headers, length, body, and in.readNBytes in HttpParser).
1 participant
Summary by CodeRabbit