Rename SocketServer to Server Move HTTP request handling logic to a dedicated ConnectionHandler. - #28
Conversation
Move HTTP request handling logic to a dedicated ConnectionHandler.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the static SocketServer flow with a composed Server that is constructed with a Logger and a DefaultConnectionHandlerFactory; adds ConnectionHandler, ConnectionHandlerFactory, and DefaultConnectionHandlerFactory; App now builds Logger, HttpParser, factory and Server, then calls server.start(). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant Factory as ConnectionHandlerFactory
participant Handler as ConnectionHandler
participant Parser as HttpParser
Client->>Server: TCP connect + send HTTP request
Server->>Factory: create(socket)
Factory-->>Handler: new ConnectionHandler(socket, httpParser, logger)
Server->>Handler: start virtual thread (Thread.ofVirtual().start)
Handler->>Parser: parse(inputStream)
Parser-->>Handler: HttpRequest(method,path,...)
Handler-->>Client: write HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/ConnectionHandler.java`:
- Around line 17-28: The run() method in ConnectionHandler parses the
HttpRequest but never writes any HTTP response back to the client; update run()
to write a valid HTTP response to socket.getOutputStream() (for example
"HTTP/1.1 200 OK" with required headers and a short body or "501 Not
Implemented" for unsupported methods), flush and close the stream, and handle
IOExceptions; reference the existing socket, httpParser.parse(...) and the
HttpRequest (request.method()/request.path()) when deciding response content, or
add a clear TODO comment if leaving this intentionally unimplemented.
In `@src/main/java/org/juv25d/Server.java`:
- Around line 8-15: The instance field httpParser and constructor
Server(HttpParser) are unused because createSocket() is static and creates its
own parser; convert createSocket() to an instance method (remove static) so it
uses this.httpParser instead of instantiating a new HttpParser, and update any
callers (e.g., start() or App.java) to call the instance createSocket() on new
Server(new HttpParser()); ensure you remove local HttpParser creation inside
createSocket() and reference the field httpParser throughout.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/ConnectionHandler.java (1)
25-26: Prefer a logger overe.printStackTrace().
printStackTrace()writes to stderr without structured context (timestamp, thread name, connection info). Consider usingjava.util.loggingor SLF4J for better observability, or at minimum log which client triggered the error.src/main/java/org/juv25d/Server.java (1)
18-18: Method namecreateSocketis misleading.This method doesn't just create a socket — it binds, listens, and runs the accept loop indefinitely. A name like
start()orrun()better communicates the blocking, long-lived nature of this call.
FionaSprinkles
left a comment
FionaSprinkles
left a comment
There was a problem hiding this comment.
This looks good to me! I only have a comment/question on the println vs logging method.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/ConnectionHandler.java`:
- Line 25: Fix the typo in the log message inside ConnectionHandler by changing
the logged string "reuest" to "request" in the logger.info call that constructs
the message using request.method() and request.path(); locate the
logger.info(...) invocation in the ConnectionHandler class and correct the
spelling so the message reads "Handling request: " + request.method() + " " +
request.path().
🧹 Nitpick comments (1)
src/main/java/org/juv25d/ConnectionHandler.java (1)
13-13: Logger should be declaredfinal.Standard Java convention for logger fields.
Proposed fix
- private static Logger logger = Logger.getLogger(ConnectionHandler.class.getName()); + private static final Logger logger = Logger.getLogger(ConnectionHandler.class.getName());
fmazmz
left a comment
fmazmz
left a comment
There was a problem hiding this comment.
Great work.
Some suggestions below that will really help us later when we write unit tests.
|
@johanbriger |
bamsemats
left a comment
bamsemats
left a comment
There was a problem hiding this comment.
Follow up on posted comments and this looks good for approval.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
simonforsberg
left a comment
simonforsberg
left a comment
There was a problem hiding this comment.
I think this PR accomplishes what Issue #20 set out to do, that is refactoring the server architecture. The logging improvements and dependency injection that have been mentioned are good points, but might be better suited for a separate issue/PR focused on improving testability, as @fmazmz also mentioned.
That way we can merge this refactoring and tackle DI as its own focused task!
Not sure if I've missed anything of the conversation, but logging has already been initiated in separate PR/branch, so focus should definitely not be on logging for this PR (this might have been said, I just dont want anyone to do double work and add logging logic which is already implemented). |
I'm sorry, I can see now that PR #22 have already resolved that, please excuse a newbie! |
- Inject Logger inside of constructor - Inject handlerFactory into the Server that handles creation of a new ConnectionHandler on each request - Remove HttpParser from Server as it is not handling the parsing of a request
d3dd7b9
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/Server.java`:
- Around line 11-16: The Server constructor and field currently depend on the
concrete DefaultConnectionHandlerFactory; change the field type handlerFactory
and the constructor parameter connectionHandlerFactory to the interface type
ConnectionHandlerFactory (replace DefaultConnectionHandlerFactory with
ConnectionHandlerFactory in the Server class), update the constructor assignment
to this.handlerFactory = connectionHandlerFactory, and ensure any imports/usages
of handlerFactory elsewhere in Server are compatible with the
ConnectionHandlerFactory interface so alternative implementations (e.g., mocks)
can be injected.
|
Some refactors to keep things decoupled and allow us to test each component as a unit later if we need to. ➜ ~ curl -v http://localhost:3000/
* Host localhost:3000 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
* Trying [::1]:3000...
* Connected to localhost (::1) port 3000
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/8.5.0
> Accept: */*
>
* Empty reply from server
* Closing connection
curl: (52) Empty reply from server
➜ ~ |
Core Implementation: - Add StaticFileHandler for serving files from /resources/static/ - Add MimeTypeResolver for Content-Type detection - Add security validation to prevent path traversal attacks Testing: - Add MimeTypeResolverTest (15 test cases) - Add StaticFileHandlerTest (20+ test cases) - All tests passing Example Files: - Add index.html demo page with gradient styling - Add styles.css for professional styling - Add app.js for JavaScript functionality demo Note: Integration with Server/ConnectionHandler will be added after PR #28 merges to avoid conflicts. Foundation work for #18
annikaholmqvist94
left a comment
annikaholmqvist94
left a comment
There was a problem hiding this comment.
Excellent refactoring that significantly improves the codebase architecture.
-
Clean separation of concerns
- Server handles transport layer (connections, threads)
- ConnectionHandler handles application layer (HTTP parsing, logging)
- Clear single responsibility for each class
-
Java best practices
- Virtual threads for high concurrency (perfect for I/O-bound operations)
- Dependency injection throughout
- Immutable fields with
final - Factory pattern for flexible handler creation
-
Testability
- Each component can be tested in isolation
- Easy to mock dependencies (HttpParser, Logger)
- Clean interfaces enable different implementations
This architecture provides the perfect foundation for static file serving!
Integration points identified:
- App.java (lines 10-17): Create and inject StaticFileHandler
- DefaultConnectionHandlerFactory: Add StaticFileHandler parameter
- ConnectionHandler fields (lines 9-12): Add StaticFileHandler field
- ConnectionHandler.run() (lines 23-25): Add response logic
Note:
Currently, requests are parsed but no response is sent back
(client will hang). My StaticFileHandler integration will complete this flow.
questions:
- Should response writing stay in ConnectionHandler,
or be abstracted to a separateResponseWriterclass
-Is the current error handling sufficient, or should
we add more specific HTTP error codes (500, 400, etc.)?
Great work!
| SocketServer.createSocket(); | ||
| Logger logger = ServerLogging.getLogger(); | ||
| HttpParser httpParser = new HttpParser(); | ||
| DefaultConnectionHandlerFactory handlerFactory = | ||
| new DefaultConnectionHandlerFactory(httpParser, logger); | ||
|
|
||
| Server server = new Server( | ||
| logger, | ||
| handlerFactory | ||
| ); | ||
|
|
||
| server.start(); |
There was a problem hiding this comment.
Great refactoring, app.java now only handles object creation and wiring, not business logic.
Testability: Each component can now be tested independently
Flexibility: Easy to swap implementations (e.g., MockLogger for testing)
When I add StaticFileHandler, should I pass it through the
ConnectionHandlerFactory, or inject it directly into
DefaultConnectionHandlerFactory? PR #36
Example:
DefaultConnectionHandlerFactory handlerFactory =
new DefaultConnectionHandlerFactory(httpParser, logger, staticFileHandler);Would this be the right approach?
| public class ConnectionHandler implements Runnable { | ||
| private final Socket socket; | ||
| private final HttpParser httpParser; | ||
| private final Logger logger; |
There was a problem hiding this comment.
Nice use of "final" - ensures immutability after construction.
Suggestion for my StaticFileHandler integration:
Would it make sense to add StaticFileHandler as a field here?
private final StaticFileHandler staticFileHandler;This would allow ConnectionHandler to delegate GET requests
to StaticFileHandler.
| public ConnectionHandler(Socket socket, HttpParser httpParser, Logger logger) { | ||
| this.socket = socket; | ||
| this.httpParser = httpParser; | ||
| this.logger = logger; |
There was a problem hiding this comment.
Constructor follows DI pattern well.
Note:This is where I'll need to add StaticFileHandler
when integrating my PR #36 :
public ConnectionHandler(Socket socket, HttpParser httpParser,
Logger logger, StaticFileHandler fileHandler) {
this.socket = socket;
this.httpParser = httpParser;
this.logger = logger;
this.staticFileHandler = fileHandler;
}| public interface ConnectionHandlerFactory { | ||
| Runnable create(Socket socket); | ||
| } |
There was a problem hiding this comment.
Good use of Factory pattern
This interface allows for different ConnectionHandler implementations:
DefaultConnectionHandlerFactory(current)MockConnectionHandlerFactory(for testing)
Suggestion:
Consider making the return type more specific:
ConnectionHandler create(Socket socket);Instead of Runnable, to expose ConnectionHandler-specific methods
if needed in the future.
Integration note (PR #36):I'll modify DefaultConnectionHandlerFactory
to pass StaticFileHandler to ConnectionHandler, rather than creating
a new factory implementation
There was a problem hiding this comment.
Factory pattern implementation looks clean
Integration question:
For StaticFileHandler integration would these options suit?:
Option 1:
Modify this factory
private final StaticFileHandler staticFileHandler;
public DefaultConnectionHandlerFactory(HttpParser httpParser,
Logger logger,
StaticFileHandler fileHandler) {
this.httpParser = httpParser;
this.logger = logger;
this.staticFileHandler = fileHandler;
}
@Override
public Runnable create(Socket socket) {
return new ConnectionHandler(socket, httpParser, logger, staticFileHandler);
}option 2:
Create a new factory
public class StaticFileConnectionHandlerFactory implements ConnectionHandlerFactory {
// ... custom implementation
}Which approach aligns better with the architecture?
| import java.util.logging.Logger; | ||
|
|
||
| public class Server { | ||
| private static final int PORT = 3000; |
There was a problem hiding this comment.
Suggestion:
Consider making PORT configurable:
private final int port;
public Server(Logger logger, ConnectionHandlerFactory factory, int port) {
this.logger = logger;
this.handlerFactory = factory;
this.port = port;
}This would allow:
- Testing on different ports
- Running multiple servers
- Environment-specific configuration
But since we are only creating one server this is only a suggestion for further improvement:)
| public void start() { | ||
| try (ServerSocket serverSocket = new ServerSocket(PORT, 64)) { | ||
| logger.info("Server started at port: " + serverSocket.getLocalPort()); | ||
|
|
||
| while (true) { | ||
| Socket socket = serverSocket.accept(); | ||
| Runnable handler = handlerFactory.create(socket); | ||
| Thread.ofVirtual().start(handler); | ||
| } | ||
|
|
||
| } catch (IOException e) { | ||
| throw new RuntimeException("Server error", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Excellent use of Virtual Threads
- No thread pool management needed
- Perfect for I/O-bound tasks like HTTP
Question:
Should we add shutdown handling?
private volatile boolean running = true;
public void stop() {
running = false;
}
while (running) {
Socket socket = serverSocket.accept();
// ...
}
…edicated ConnectionHandler. (#28) * Rename SocketServer to Server Move HTTP request handling logic to a dedicated ConnectionHandler. * Convert createSocket to an instance method named start(). * Replace console prints with Logger in ConnectionHandler * Fixed typo in ConnectionHandler * refactor Server and ConnectionHandler: - Inject Logger inside of constructor - Inject handlerFactory into the Server that handles creation of a new ConnectionHandler on each request - Remove HttpParser from Server as it is not handling the parsing of a request * accept ConnectionHandlerFactory and not a specific implementation * normalize handlerFactory name --------- Co-authored-by: WHITEROSE <firasmoussa60@gmail.com>
| try (socket) { | ||
| HttpRequest request = httpParser.parse(socket.getInputStream()); | ||
|
|
||
| logger.info("Handling request: " + request.method() + " " + request.path()); |
There was a problem hiding this comment.
CRITICAL: This is where HTTP response logic should go
Currently, the request is parsed but no response is sent back.
This will cause the client to hang.
For my PR #36 integration, I propose adding:**
HttpRequest request = httpParser.parse(socket.getInputStream());
logger.info("Handling request: " + request.method() + " " + request.path());
// NEW CODE - Handle GET requests with StaticFileHandler
if (request.method().equalsIgnoreCase("GET")) {
HttpResponse response = staticFileHandler.handle(request);
HttpResponseWriter.write(socket.getOutputStream(), response);
} else {
// Return 405 Method Not Allowed for non-GET
HttpResponse response = createMethodNotAllowedResponse();
HttpResponseWriter.write(socket.getOutputStream(), response);
}Questions:
- Should response writing be handled here, or abstracted to another class?
- Should we add a
ResponseWriterinterface for flexibility? - Is error handling sufficient, or should we return 500 on exceptions?
* Implement static file handler with security and tests Core Implementation: - Add StaticFileHandler for serving files from /resources/static/ - Add MimeTypeResolver for Content-Type detection - Add security validation to prevent path traversal attacks Testing: - Add MimeTypeResolverTest (15 test cases) - Add StaticFileHandlerTest (20+ test cases) - All tests passing Example Files: - Add index.html demo page with gradient styling - Add styles.css for professional styling - Add app.js for JavaScript functionality demo Note: Integration with Server/ConnectionHandler will be added after PR #28 merges to avoid conflicts. Foundation work for #18 * Introduce ADR structure and first ADR - Add ADR README explaining the ADR process for the team - Add TEMPLATE for writing future ADRs - Add ADR-001 documenting static file serving architecture Closes #16 * Rename SocketServer to Server Move HTTP request handling logic to a dedicated ConnectionHandler. (#28) * Rename SocketServer to Server Move HTTP request handling logic to a dedicated ConnectionHandler. * Convert createSocket to an instance method named start(). * Replace console prints with Logger in ConnectionHandler * Fixed typo in ConnectionHandler * refactor Server and ConnectionHandler: - Inject Logger inside of constructor - Inject handlerFactory into the Server that handles creation of a new ConnectionHandler on each request - Remove HttpParser from Server as it is not handling the parsing of a request * accept ConnectionHandlerFactory and not a specific implementation * normalize handlerFactory name --------- Co-authored-by: WHITEROSE <firasmoussa60@gmail.com> * Add testing for ServerLogging.java. Configure ServerLogging.java for tests. (#34) * Add testing for ServerLogging.java. Configure ServerLogging.java to improve its testability. * Update logger_shouldNotAddDuplicateHandlers test to properly test non-inclusion of duplicates. * Update ServerLogging.java to guard against invalid level string entries in configure method (logger.parse => setLevel). * feature/FilterPlugin (#17) * http parser * Bunny fixes. (only using input stream to recieve requests) * Bunny review improvements * Improved http parser ReadLine helper method to eliminate dependency on mark() and reset(). Implemented handleClient() using socket as a try-with-resources to avoid memory leakage in case of exception thrown by httpparser-methods. * NumberFormatException fix on line 53 -> 60 * Added foundation for Filters and Plugins. Added FilterChain to use created filters, and a Pipeline class to handle the workflow (Client → Filters → Plugin → Response → Client). Modified SocketServer handleClient() to use FilterChain. Added example code in App.java for Pipeline usage. TODO: Initialize HttpResponse class * Fixing build fail * add init and destroy to filter interface * add methods to pipeline class * Add servlet-style filter pipeline with lifecycle support * add documentation about temporary server impl * test: verify filters execute in order and plugin is called last * test: add coverage for filter blocking, lifecycle init, and empty pipeline * add loggingfiltertest * fix: construct HttpResponse with required arguments * fix: construct HttpResponse with required arguments * Update FilterChainImpl tests to use full HttpResponse constructor to ensure compatibility with future implementations * remove duplicated class --------- Co-authored-by: Kristina M <kristina0x7@gmail.com> Co-authored-by: WHITEROSE <firasmoussa60@gmail.com> * Add charset=utf-8 to text-based Content-Type headers * Integrate StaticFileHandler with Pipeline architecture * Refactor to use immutable fields and clean up unused setters; update with usage notes for future integration. * Enhance StaticFilesPlugin to properly handle headers with map iteration; simplify and clarify class documentation. * Update tests to verify Content-Type headers include charset=utf-8 * connect StaticFilesPlugin --------- Co-authored-by: johanbriger <johanbriger@gmail.com> Co-authored-by: WHITEROSE <firasmoussa60@gmail.com> Co-authored-by: Mats Rönnqvist <203552386+bamsemats@users.noreply.github.com> Co-authored-by: Linus Westling <141355850+LinusWestling@users.noreply.github.com> Co-authored-by: Kristina M <kristina0x7@gmail.com>
7 participants
Summary by CodeRabbit