Merge pull request #25 from onlyskin/exception-handling

onlyskin · web-flow · commit 47c4ce998da8 · 2018-01-12T14:50:34.000+02:00
Exception handling
diff --git a/src/main/java/httpserver/App.java b/src/main/java/httpserver/App.java
@@ -16,7 +16,7 @@ public static void main(String[] args) throws IOException {
         PathExaminer pathExaminer = new PathExaminer();
         Path root = pathExaminer.getPath(fileDirectory);
         Path logPath = pathExaminer.concatenate(root,"logs");
-        AppConfig appConfig = new AppConfig(root, new Logger(logPath, new FileOperator()));
+        AppConfig appConfig = new AppConfig(root, new Logger(logPath, new FileOperator(), System.err));
 
         ServerFactory serverFactory = new ServerFactory(new ServerSocketFactory(), appConfig);
         SocketHandlerFactory socketHandlerFactory = new SocketHandlerFactory();
diff --git a/src/main/java/httpserver/Hasher.java b/src/main/java/httpserver/Hasher.java
@@ -14,8 +14,7 @@ public String getHash(byte[] input) {
             byte[] hash = messageDigest.digest(input);
             return bytesToHex(hash);
         } catch (NoSuchAlgorithmException e) {
-            System.out.println("Unable to get hash.");
-            return "";
+            throw new RuntimeException(e);
         }
     }
 
diff --git a/src/main/java/httpserver/Logger.java b/src/main/java/httpserver/Logger.java
@@ -3,15 +3,18 @@
 import httpserver.file.FileOperator;
 
 import java.io.IOException;
+import java.io.PrintStream;
 import java.nio.file.Path;
 
 public class Logger {
     private final FileOperator fileOperator;
     private final Path logPath;
+    private final PrintStream errorStream;
 
-    public Logger(Path logPath, FileOperator fileOperator) {
+    public Logger(Path logPath, FileOperator fileOperator, PrintStream errorStream) {
         this.fileOperator = fileOperator;
         this.logPath = logPath;
+        this.errorStream = errorStream;
         createFileIfDoesntExist(logPath);
     }
 
@@ -20,14 +23,14 @@ public void log(String logString) {
         try {
             fileOperator.appendToFile(logPath, logBytes);
         } catch (IOException e) {
-            e.printStackTrace();
+            errorStream.print(e);
         }
     }
 
     private void createFileIfDoesntExist(Path path) {
         try {
             fileOperator.createFileAtPath(path);
-        } catch (IOException e) {}
+        } catch (IOException e) { }
     }
 
     public byte[] readLog() throws IOException {
diff --git a/src/main/java/httpserver/ResponseWriter.java b/src/main/java/httpserver/ResponseWriter.java
@@ -11,9 +11,11 @@
 public class ResponseWriter {
     private final HashMap<Integer, String> statuses;
     private final OutputStream outputStream;
+    private final Logger logger;
 
-    public ResponseWriter(OutputStream outputStream) {
+    public ResponseWriter(OutputStream outputStream, Logger logger) {
         this.outputStream = outputStream;
+        this.logger = logger;
         this.statuses = new HashMap<>();
         statuses.put(200, "OK");
         statuses.put(204, "No Content");
@@ -35,7 +37,7 @@ public void write(Response response) {
             writeEmptyLine();
             writePayload(response);
         } catch (IOException e) {
-            System.out.println("Error while writing to output stream.");
+            logger.log(e.toString());
         }
     }
 
diff --git a/src/main/java/httpserver/Server.java b/src/main/java/httpserver/Server.java
@@ -14,14 +14,13 @@ public Server(ServerSocket serverSocket, AppConfig appConfig) {
         this.appConfig = appConfig;
     }
 
-    public void acceptConnection(ExecutorService executorService, SocketHandlerFactory socketHandlerFactory){
+    public void acceptConnection(ExecutorService executorService, SocketHandlerFactory socketHandlerFactory) {
         try {
             Socket clientSocket = serverSocket.accept();
             SocketHandler socketHandler = socketHandlerFactory.newSocketHandler(appConfig, clientSocket);
             executorService.submit(socketHandler);
         } catch (IOException e) {
-            System.out.println("Server socket error");
-            e.printStackTrace();
+            appConfig.getLogger().log(e.toString());
         }
     }
 }
diff --git a/src/main/java/httpserver/SocketHandler.java b/src/main/java/httpserver/SocketHandler.java
@@ -3,9 +3,9 @@
 import httpserver.request.Request;
 import httpserver.request.RequestParser;
 import httpserver.responder.Responder;
-import httpserver.response.BadRequestResponse;
 import httpserver.response.MethodNotAllowedResponse;
 import httpserver.response.Response;
+import httpserver.response.ServerErrorResponse;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -38,15 +38,19 @@ public void run() {
         } catch (IllegalArgumentException e) {
             response = new MethodNotAllowedResponse();
         } catch (Exception e) {
-            response = new BadRequestResponse();
+            response = new ServerErrorResponse();
         }
 
         responseWriter.write(response);
 
+        closeStream();
+    }
+
+    private void closeStream() {
         try {
             inputStream.close();
         } catch (IOException e) {
-            System.out.println("The inputStream could not be closed.");
+            appConfig.getLogger().log(e.toString());
         }
     }
 }
diff --git a/src/main/java/httpserver/SocketHandlerFactory.java b/src/main/java/httpserver/SocketHandlerFactory.java
@@ -14,14 +14,14 @@ public SocketHandler newSocketHandler(AppConfig appConfig, Socket clientSocket)
                 clientSocket.getInputStream(),
                 new RequestParser(appConfig),
                 new Responder(new MethodResponderSupplierFactory().makeResponderSupplier()),
-                new ResponseWriter(clientSocket.getOutputStream()));
+                new ResponseWriter(clientSocket.getOutputStream(), appConfig.getLogger()));
     }
 
     public SocketHandler newSocketHandlerFromStreams(AppConfig appConfig, InputStream inputStream, OutputStream outputStream) throws IOException {
         return new SocketHandler(appConfig,
                 inputStream,
                 new RequestParser(appConfig),
                 new Responder(new MethodResponderSupplierFactory().makeResponderSupplier()),
-                new ResponseWriter(outputStream));
+                new ResponseWriter(outputStream, appConfig.getLogger()));
     }
 }
diff --git a/src/main/java/httpserver/file/PathExaminer.java b/src/main/java/httpserver/file/PathExaminer.java
@@ -34,22 +34,15 @@ public boolean isFile(Path path) {
         return exists(path) && !isDirectory(path);
     }
 
-    public byte[] fileContents(Path file) {
-        try {
-            return readAllBytes(file);
-        } catch (IOException e) {
-            return new byte[0];
-        }
+    public byte[] fileContents(Path file) throws IOException {
+        return readAllBytes(file);
     }
 
-    public Path[] directoryContents(Path dir) {
+    public Path[] directoryContents(Path dir) throws IOException {
         List<Path> result = new ArrayList<>();
-        try (DirectoryStream<Path> stream = newDirectoryStream(dir)) {
-            for (Path entry: stream) {
-                result.add(entry);
-            }
-        } catch (IOException e) {
-            System.out.println("Couldn't retrieve directory contents for directory at " + dir.toString());
+        DirectoryStream<Path> stream = newDirectoryStream(dir);
+        for (Path entry: stream) {
+            result.add(entry);
         }
         return result.toArray(new Path[0]);
     }
diff --git a/src/main/java/httpserver/request/UrlDecoder.java b/src/main/java/httpserver/request/UrlDecoder.java
@@ -7,7 +7,7 @@ public String decode(String urlString) {
         try {
             return java.net.URLDecoder.decode(urlString, "UTF-8");
         } catch (UnsupportedEncodingException e) {
-            return urlString;
+            throw new RuntimeException(e);
         }
     }
 }
diff --git a/src/main/java/httpserver/responder/GetResponder.java b/src/main/java/httpserver/responder/GetResponder.java
@@ -56,7 +56,7 @@ public Response respond(AppConfig appConfig, Request request) throws IOException
         return responseForFile(fullPath, request);
     }
 
-    private Response responseForDir(Path root, Path path) {
+    private Response responseForDir(Path root, Path path) throws IOException {
         Path[] paths = pathExaminer.directoryContents(path);
         String result = htmlLinksForContents(root, paths);
         return new OkResponse(result.getBytes());
@@ -70,7 +70,7 @@ private String htmlLinksForContents(Path root, Path[] paths) {
         return stringBuilder.toString();
     }
 
-    private Response responseForFile(Path path, Request request) {
+    private Response responseForFile(Path path, Request request) throws IOException {
         byte[] payload = pathExaminer.fileContents(path);
 
         Response response;
diff --git a/src/main/java/httpserver/responder/PatchResponder.java b/src/main/java/httpserver/responder/PatchResponder.java
@@ -49,7 +49,7 @@ private boolean noIfMatchHeader(Request request) {
         return !request.hasHeader("If-Match");
     }
 
-    private boolean matchingHash(Path fullPath, Request request) {
+    private boolean matchingHash(Path fullPath, Request request) throws IOException {
         byte[] fileContents = pathExaminer.fileContents(fullPath);
         String ifMatchHash = request.getHeaderValue("If-Match");
         return hasher.matches(fileContents, ifMatchHash);
diff --git a/src/main/java/httpserver/route/RedirectRoute.java b/src/main/java/httpserver/route/RedirectRoute.java
@@ -5,15 +5,13 @@
 import httpserver.response.FoundResponse;
 import httpserver.response.Response;
 
-import java.io.IOException;
-
 public class RedirectRoute extends Route {
     public RedirectRoute() {
         super.routeString = "/redirect";
     }
 
     @Override
-    public Response respond(AppConfig appConfig, Request request) throws IOException {
+    public Response respond(AppConfig appConfig, Request request) {
         return new FoundResponse("/");
     }
 }
diff --git a/src/test/java/httpserver/LoggerTest.java b/src/test/java/httpserver/LoggerTest.java
@@ -4,6 +4,7 @@
 import org.junit.Test;
 
 import java.io.IOException;
+import java.io.PrintStream;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.StandardOpenOption;
@@ -13,25 +14,29 @@
 import static java.nio.file.Files.write;
 import static java.nio.file.Files.exists;
 import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
 
 public class LoggerTest {
 
     private final Path logPath;
     private final Logger logger;
+    private final PrintStream printStreamMock;
+    private final FileOperator fileOperatorMock;
 
     public LoggerTest() throws IOException {
         logPath = Paths.get(tempDir().toString(), "logs");
-        logger = new Logger(logPath, new FileOperator());
+        printStreamMock = mock(PrintStream.class);
+        logger = new Logger(logPath, new FileOperator(), printStreamMock);
+        fileOperatorMock = mock(FileOperator.class);
     }
+
     @Test
     public void createsLogFileOnConstructDoesntOverwrite() throws Exception {
         assertTrue(exists(logPath));
 
         write(logPath, "fake log line\r\n".getBytes(),
                 StandardOpenOption.APPEND);
 
-        new Logger(logPath, new FileOperator());
-
         assertEquals("fake log line\r\n", new String(readAllBytes(logPath)));
     }
 
@@ -44,4 +49,14 @@ public void writesToLogAndReadsFromLog() throws Exception {
 
         assertEquals("POST example\r\nHEAD example\r\n", log);
     }
+
+    @Test
+    public void printsErrorToStdErrIfCantWriteToLog() throws Exception {
+        doThrow(new IOException()).when(fileOperatorMock).appendToFile(any(), any());
+        Logger logger = new Logger(logPath, fileOperatorMock, printStreamMock);
+
+        logger.log("");
+
+        verify(printStreamMock).print(any(Exception.class));
+    }
 }
diff --git a/src/test/java/httpserver/MethodRouteSupplierTest.java b/src/test/java/httpserver/MethodRouteSupplierTest.java
@@ -28,13 +28,22 @@ public MethodRouteSupplierTest() {
     @Test
     public void returnsResponderForMethod() throws Exception {
         Request request = new Request(Method.GET, null, null, null);
-        when(getResponderMock.handles(any())).thenReturn(true);
+        when(getResponderMock.handles(request)).thenReturn(true);
 
         MethodResponder methodResponder = methodResponderSupplier.supplyResponder(request);
 
         assertEquals(getResponderMock, methodResponder);
     }
 
+    @Test(expected = Exception.class)
+    public void throwsErrorWhenNoResponderForRequestMethod() throws Exception {
+        Request request = new Request(Method.PUT, null, null, null);
+        when(getResponderMock.handles(request)).thenReturn(false);
+        when(postResponderMock.handles(request)).thenReturn(false);
+
+        methodResponderSupplier.supplyResponder(request);
+    }
+
     @Test
     public void returnsAllRespondersRegistered() throws Exception {
         List<MethodResponder> actual = methodResponderSupplier.allResponders();
diff --git a/src/test/java/httpserver/ResponseWriterTest.java b/src/test/java/httpserver/ResponseWriterTest.java
@@ -9,16 +9,21 @@
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.OutputStream;
+import java.io.PrintStream;
 
 import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
 
 public class ResponseWriterTest {
     private final ResponseWriter responseWriter;
     private final ByteArrayOutputStream outputStream;
+    private final Logger logger;
 
     public ResponseWriterTest() {
-        this.outputStream = new ByteArrayOutputStream();
-        this.responseWriter = new ResponseWriter(outputStream);
+        outputStream = new ByteArrayOutputStream();
+        logger = mock(Logger.class);
+        responseWriter = new ResponseWriter(outputStream, logger);
     }
 
     @Test
@@ -44,6 +49,18 @@ public void writesResponseHeaders() throws Exception {
         assertTrue(output.contains("Content-Length: 7"));
     }
 
+    @Test
+    public void logsExceptionIfWritingThrowsException() throws Exception {
+        OutputStream outputStreamMock = mock(OutputStream.class);
+        doThrow(new IOException()).when(outputStreamMock).write(any());
+        ResponseWriter responseWriter = new ResponseWriter(outputStreamMock, logger);
+        Response response = new OkResponse("example".getBytes());
+
+        responseWriter.write(response);
+
+        verify(logger).log(any());
+    }
+
     @Test
     public void itWritesTheFirstLineFor404() throws Exception {
         String output = outputForResponse(new NotFoundResponse());
diff --git a/src/test/java/httpserver/ServerTest.java b/src/test/java/httpserver/ServerTest.java
@@ -15,7 +15,7 @@ public class ServerTest {
     private final ServerSocket serverSocketMock;
     private final Socket socket;
     private final AppConfig appConfigMock;
-    private Server server;
+    private final Server server;
     private final ExecutorService executorServiceMock;
     private final SocketHandler socketHandlerMock;
 
@@ -54,4 +54,15 @@ public void callsExecutorSubmitWithSocketHandler() throws Exception {
 
         verify(executorServiceMock).submit(socketHandlerMock);
     }
+
+    @Test
+    public void acceptConnectionLogsExceptionIfThereIsAnIoException() throws Exception {
+        when(serverSocketMock.accept()).thenThrow(new IOException());
+        Logger loggerMock = mock(Logger.class);
+        when(appConfigMock.getLogger()).thenReturn(loggerMock);
+
+        server.acceptConnection(executorServiceMock, socketHandlerFactoryMock);
+
+        verify(loggerMock).log(any());
+    }
 }
diff --git a/src/test/java/httpserver/SocketHandlerTest.java b/src/test/java/httpserver/SocketHandlerTest.java

Original file line number	Diff line number	Diff line change
`@@ -14,8 +14,7 @@ public String getHash(byte[] input) {`
`14`	`14`	`byte[] hash = messageDigest.digest(input);`
`15`	`15`	`return bytesToHex(hash);`
`16`	`16`	`} catch (NoSuchAlgorithmException e) {`
`17`		`- System.out.println("Unable to get hash.");`
`18`		`- return "";`
	`17`	`+ throw new RuntimeException(e);`
`19`	`18`	`}`
`20`	`19`	`}`
`21`	`20`
Original file line number	Diff line number	Diff line change
`@@ -14,14 +14,13 @@ public Server(ServerSocket serverSocket, AppConfig appConfig) {`
`14`	`14`	`this.appConfig = appConfig;`
`15`	`15`	`}`
`16`	`16`
`17`		`- public void acceptConnection(ExecutorService executorService, SocketHandlerFactory socketHandlerFactory){`
	`17`	`+ public void acceptConnection(ExecutorService executorService, SocketHandlerFactory socketHandlerFactory) {`
`18`	`18`	`try {`
`19`	`19`	`Socket clientSocket = serverSocket.accept();`
`20`	`20`	`SocketHandler socketHandler = socketHandlerFactory.newSocketHandler(appConfig, clientSocket);`
`21`	`21`	`executorService.submit(socketHandler);`
`22`	`22`	`} catch (IOException e) {`
`23`		`- System.out.println("Server socket error");`
`24`		`- e.printStackTrace();`
	`23`	`+ appConfig.getLogger().log(e.toString());`
`25`	`24`	`}`
`26`	`25`	`}`
`27`	`26`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,14 +14,14 @@ public SocketHandler newSocketHandler(AppConfig appConfig, Socket clientSocket)`
`14`	`14`	`clientSocket.getInputStream(),`
`15`	`15`	`new RequestParser(appConfig),`
`16`	`16`	`new Responder(new MethodResponderSupplierFactory().makeResponderSupplier()),`
`17`		`- new ResponseWriter(clientSocket.getOutputStream()));`
	`17`	`+ new ResponseWriter(clientSocket.getOutputStream(), appConfig.getLogger()));`
`18`	`18`	`}`
`19`	`19`
`20`	`20`	`public SocketHandler newSocketHandlerFromStreams(AppConfig appConfig, InputStream inputStream, OutputStream outputStream) throws IOException {`
`21`	`21`	`return new SocketHandler(appConfig,`
`22`	`22`	`inputStream,`
`23`	`23`	`new RequestParser(appConfig),`
`24`	`24`	`new Responder(new MethodResponderSupplierFactory().makeResponderSupplier()),`
`25`		`- new ResponseWriter(outputStream));`
	`25`	`+ new ResponseWriter(outputStream, appConfig.getLogger()));`
`26`	`26`	`}`
`27`	`27`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@ public String decode(String urlString) {`
`7`	`7`	`try {`
`8`	`8`	`return java.net.URLDecoder.decode(urlString, "UTF-8");`
`9`	`9`	`} catch (UnsupportedEncodingException e) {`
`10`		`- return urlString;`
	`10`	`+ throw new RuntimeException(e);`
`11`	`11`	`}`
`12`	`12`	`}`
`13`	`13`	`}`
Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ public Response respond(AppConfig appConfig, Request request) throws IOException`
`56`	`56`	`return responseForFile(fullPath, request);`
`57`	`57`	`}`
`58`	`58`
`59`		`- private Response responseForDir(Path root, Path path) {`
	`59`	`+ private Response responseForDir(Path root, Path path) throws IOException {`
`60`	`60`	`Path[] paths = pathExaminer.directoryContents(path);`
`61`	`61`	`String result = htmlLinksForContents(root, paths);`
`62`	`62`	`return new OkResponse(result.getBytes());`
`@@ -70,7 +70,7 @@ private String htmlLinksForContents(Path root, Path[] paths) {`
`70`	`70`	`return stringBuilder.toString();`
`71`	`71`	`}`
`72`	`72`
`73`		`- private Response responseForFile(Path path, Request request) {`
	`73`	`+ private Response responseForFile(Path path, Request request) throws IOException {`
`74`	`74`	`byte[] payload = pathExaminer.fileContents(path);`
`75`	`75`
`76`	`76`	`Response response;`