From ee21c836a9b75b07140412bddedf88d648179715 Mon Sep 17 00:00:00 2001 From: Pekka Helenius Date: Fri, 18 Sep 2020 19:38:28 +0300 Subject: [PATCH] Set SLF4J log format; improve logs; deprecate HttpExceptionHandler class Signed-off-by: Pekka Helenius --- bookstore/.gitignore | 1 + .../bookstore/web/BookController.java | 74 ++++++--------- .../bookstore/web/HttpExceptionHandler.java | 33 ------- .../bookstore/web/HttpServerLogger.java | 95 ++++++++++++------- bookstore/src/main/resources/logback.xml | 37 ++++++++ 5 files changed, 129 insertions(+), 111 deletions(-) delete mode 100644 bookstore/src/main/java/com/fjordtek/bookstore/web/HttpExceptionHandler.java create mode 100644 bookstore/src/main/resources/logback.xml diff --git a/bookstore/.gitignore b/bookstore/.gitignore index 549e00a..cf60db2 100644 --- a/bookstore/.gitignore +++ b/bookstore/.gitignore @@ -1,5 +1,6 @@ HELP.md target/ +logs/ !.mvn/wrapper/maven-wrapper.jar !**/src/main/**/target/ !**/src/test/**/target/ diff --git a/bookstore/src/main/java/com/fjordtek/bookstore/web/BookController.java b/bookstore/src/main/java/com/fjordtek/bookstore/web/BookController.java index 8fc9d7c..4a3cd36 100644 --- a/bookstore/src/main/java/com/fjordtek/bookstore/web/BookController.java +++ b/bookstore/src/main/java/com/fjordtek/bookstore/web/BookController.java @@ -7,6 +7,7 @@ import java.util.HashMap; import java.util.Map; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import javax.validation.Valid; import org.springframework.beans.factory.annotation.Autowired; @@ -49,7 +50,6 @@ public class BookController { }}; private HttpServerLogger httpServerLogger = new HttpServerLogger(); - //private HttpExceptionHandler httpExceptionHandler = new HttpExceptionHandler(); @Autowired private BookRepository bookRepository; @@ -70,14 +70,15 @@ public class BookController { value = bookListPageView, method = { RequestMethod.GET, RequestMethod.POST } ) - public String defaultWebFormGet(HttpServletRequest requestData, Model dataModel) { + public String defaultWebFormGet( + HttpServletRequest requestData, + HttpServletResponse responseData, + Model dataModel + ) { dataModel.addAttribute("books", bookRepository.findAll()); - httpServerLogger.logMessageNormal( - requestData, - bookListPageView + ": " + "HTTPOK" - ); + httpServerLogger.log(requestData, responseData); return bookListPageView; @@ -92,6 +93,7 @@ public class BookController { ) public String webFormAddBook( HttpServletRequest requestData, + HttpServletResponse responseData, Model dataModel ) { @@ -100,10 +102,7 @@ public class BookController { dataModel.addAttribute("book", newBook); dataModel.addAttribute("categories", categoryRepository.findAll()); - httpServerLogger.logMessageNormal( - requestData, - bookAddPageView + ": " + "HTTPOK" - ); + httpServerLogger.log(requestData, responseData); return bookAddPageView; } @@ -115,7 +114,8 @@ public class BookController { public String webFormSaveNewBook( @Valid @ModelAttribute("book") Book book, BindingResult bindingResult, - HttpServletRequest requestData + HttpServletRequest requestData, + HttpServletResponse responseData ) { // TODO consider better solution. Add custom Hibernate annotation for Book class? @@ -124,14 +124,12 @@ public class BookController { } if (bindingResult.hasErrors()) { - httpServerLogger.commonError("Book add: error " + book.toString(), requestData); + responseData.setStatus(HttpServletResponse.SC_BAD_REQUEST); + httpServerLogger.log(requestData, responseData); return bookAddPageView; } - httpServerLogger.logMessageNormal( - requestData, - bookAddPageView + ": " + "HTTPOK" - ); + httpServerLogger.log(requestData, responseData); return "redirect:" + bookListPageView; } @@ -145,15 +143,13 @@ public class BookController { ) public String webFormDeleteBook( @PathVariable("id") Long bookId, - HttpServletRequest requestData + HttpServletRequest requestData, + HttpServletResponse responseData ) { bookRepository.deleteById(bookId); - httpServerLogger.logMessageNormal( - requestData, - bookDeletePageView + ": " + "HTTPOK" - ); + httpServerLogger.log(requestData, responseData); return "redirect:../" + bookListPageView; } @@ -168,17 +164,15 @@ public class BookController { public String webFormEditBook( @PathVariable("id") Long bookId, Model dataModel, - HttpServletRequest requestData + HttpServletRequest requestData, + HttpServletResponse responseData ) { Book book = bookRepository.findById(bookId).get(); dataModel.addAttribute("book", book); dataModel.addAttribute("categories", categoryRepository.findAll()); - httpServerLogger.logMessageNormal( - requestData, - bookEditPageView + ": " + "HTTPOK" - ); + httpServerLogger.log(requestData, responseData); return bookEditPageView; } @@ -198,23 +192,22 @@ public class BookController { BindingResult bindingResult, Model dataModel, @PathVariable("id") Long bookId, - HttpServletRequest requestData + HttpServletRequest requestData, + HttpServletResponse responseData ) { bookId = book.getId(); dataModel.addAttribute("categories", categoryRepository.findAll()); if (bindingResult.hasErrors()) { - httpServerLogger.commonError("Book edit: error " + book.toString(), requestData); + responseData.setStatus(HttpServletResponse.SC_BAD_REQUEST); + httpServerLogger.log(requestData, responseData); return bookEditPageView; } bookRepository.save(book); - httpServerLogger.logMessageNormal( - requestData, - bookEditPageView + ": " + "HTTPOK" - ); + httpServerLogger.log(requestData, responseData); return "redirect:../" + bookListPageView; } @@ -223,20 +216,15 @@ public class BookController { // REDIRECTS @RequestMapping( - value = { "/", landingPageView }, - method = RequestMethod.GET + value = { "*" } ) @ResponseStatus(HttpStatus.FOUND) - public String redirectToDefaultWebForm() { - return "redirect:" + bookListPageView; - } - - // Other URL requests - @RequestMapping( - value = "*" + public String redirectToDefaultWebForm( + HttpServletRequest requestData, + HttpServletResponse responseData ) - public String errorWebForm(HttpServletRequest requestData) { - //return httpExceptionHandler.notFoundErrorHandler(requestData); + { + httpServerLogger.log(requestData, responseData); return "redirect:" + bookListPageView; } diff --git a/bookstore/src/main/java/com/fjordtek/bookstore/web/HttpExceptionHandler.java b/bookstore/src/main/java/com/fjordtek/bookstore/web/HttpExceptionHandler.java deleted file mode 100644 index 3bab8be..0000000 --- a/bookstore/src/main/java/com/fjordtek/bookstore/web/HttpExceptionHandler.java +++ /dev/null @@ -1,33 +0,0 @@ -// Pekka Helenius , Fjordtek 2020 - -package com.fjordtek.bookstore.web; - -import javax.servlet.http.HttpServletRequest; - -import org.springframework.http.HttpStatus; -import org.springframework.web.bind.annotation.ControllerAdvice; -import org.springframework.web.bind.annotation.ExceptionHandler; -import org.springframework.web.bind.annotation.ResponseStatus; - -@ControllerAdvice -public class HttpExceptionHandler { - - private static final String HTTPNOTFOUND = "Invalid request"; - private HttpServerLogger httpServerLogger = new HttpServerLogger(); - - @ResponseStatus( - value = HttpStatus.NOT_FOUND, - reason = HTTPNOTFOUND - ) - // Very simple exception handler (not any sophistication) - @ExceptionHandler(Exception.class) - public String notFoundErrorHandler(HttpServletRequest requestData) { - - httpServerLogger.logMessageError( - requestData, - "HTTPNOTFOUND" - ); - return "error"; - } - -} diff --git a/bookstore/src/main/java/com/fjordtek/bookstore/web/HttpServerLogger.java b/bookstore/src/main/java/com/fjordtek/bookstore/web/HttpServerLogger.java index f708344..af01882 100644 --- a/bookstore/src/main/java/com/fjordtek/bookstore/web/HttpServerLogger.java +++ b/bookstore/src/main/java/com/fjordtek/bookstore/web/HttpServerLogger.java @@ -2,58 +2,83 @@ package com.fjordtek.bookstore.web; -import java.time.LocalDateTime; +import java.util.ArrayList; +import java.util.Enumeration; +import java.util.List; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; -public class HttpServerLogger { +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; - private LocalDateTime logTimeStamp; +public class HttpServerLogger { - private void setLogTimeStamp() { - this.logTimeStamp = LocalDateTime.now(); - } + private static final Logger serverLogger = LoggerFactory.getLogger(HttpServerLogger.class); - private LocalDateTime getLogTimeStamp() { - return this.logTimeStamp; + private static boolean httpStatusRange(int comparable, int lowLimit, int upLimit) { + return lowLimit <= comparable && comparable <= upLimit; } - public void logMessageNormal( + public void log( HttpServletRequest request, - String HttpRawStatusType + HttpServletResponse response ) { - setLogTimeStamp(); - System.out.printf( - "%s: HTTP request to '%s' from client %s (%s)\n", - getLogTimeStamp(), - request.getRequestURL(), - request.getRemoteAddr(), - HttpRawStatusType - ); + int status = response.getStatus(); + + List requestParams = new ArrayList(); + Enumeration requestParamNames = request.getParameterNames(); + + if (requestParamNames != null) { + while (requestParamNames.hasMoreElements()) { + + String paramName = requestParamNames.nextElement().toString(); + String[] paramValues = request.getParameterValues(paramName); + + requestParams.add( + paramName + " = " + + String.join(", ", paramValues) + ); + } + } + + if (httpStatusRange(status, 0, 399)) { + serverLogger.info( + "HTTP request to '{}' from client '{}' [status: {}, attributes: {}]", + request.getRequestURL(), + request.getRemoteAddr(), + response.getStatus(), + requestParams + ); + } else { + serverLogger.error( + "Invalid HTTP request to '{}' from client '{}' [status: {}, attributes: {}]", + request.getRequestURL(), + request.getRemoteAddr(), + response.getStatus(), + requestParams + ); + } + } - public void logMessageError( + public void commonError( + String errorMsg, HttpServletRequest request, - String HttpRawStatusType - ) { + HttpServletResponse response, + String ...strings) { - setLogTimeStamp(); - System.err.printf( - "%s: Invalid HTTP request to '%s' from client %s (%s)\n", - getLogTimeStamp(), - request.getRequestURL(), + String[] dataString = strings; + + serverLogger.error( + "{} - {} [status: {}]", request.getRemoteAddr(), - HttpRawStatusType + errorMsg, + response.getStatus() ); - } - - public void commonError(String errorMsg, HttpServletRequest request, String ...strings) { - - // Splitting log streams for proper server error logging - System.err.printf("%s: %s - %s\n", getLogTimeStamp(), request.getRemoteAddr(), errorMsg); - for (String dataString : strings) { - System.err.printf("%s\n", dataString); + if (dataString != null) { + serverLogger.error("{}", String.join(", ", dataString)); } } diff --git a/bookstore/src/main/resources/logback.xml b/bookstore/src/main/resources/logback.xml new file mode 100644 index 0000000..be9d2ac --- /dev/null +++ b/bookstore/src/main/resources/logback.xml @@ -0,0 +1,37 @@ + + + + + + + + + + %d{yyyy-MM-dd HH:mm:ss.SSS} - [%thread] %-5level %logger{40} - %msg%n + + + + + + ${LOG_FILE} + True + + + %d{yyyy-MM-dd HH:mm:ss.SSS} - [%thread] %-5level %logger{40} - %msg%n + + + + + + + + + + + \ No newline at end of file