-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Complete task: add logger to main class #1240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| .idea/* | ||
| *.iml | ||
| target/* | ||
| logs/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,15 +6,21 @@ | |
| import mate.academy.service.AuthenticationServiceImpl; | ||
| import mate.academy.service.OrderService; | ||
| import mate.academy.service.OrderServiceImpl; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public class Main { | ||
|
|
||
| private static final Logger logger = LogManager.getLogger(Main.class); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This violates checklist guidance: "Let's make our log messages informative" (checklist). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Project is missing a Log4j2 configuration under src/main/resources that defines appenders and a root logger. This violates the requirement: "There is no Log4j2 configuration file (e.g., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Log4j2 configuration file. The task requires: "Configure the logger in this project. Add appenders for File and Console" and additionally (from the detailed requirements) you must "Add a Log4j2 configuration file (log4j2.xml or log4j2.properties) under src/main/resources that: - Defines at least a Console appender. - Defines a File appender writing into the logs/ directory. - Defines a root logger that references both the Console and File appenders at an appropriate level." There is no file under src/main/resources configuring appenders/root logger. Add the config file so logs go to console and to logs/ (the logs/ directory is already ignored in .gitignore). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an INFO-level log on successful login in AuthenticationServiceImpl.login. The task guidance asked: "In AuthenticationServiceImpl’s login method: Add an informative DEBUG-level entry log such as logger.debug("Method login was called. Params: login={}", login); Add a clear INFO-level log message on successful login." You already have a DEBUG entry (good), but there is no INFO log emitted when login succeeds—please add an informative info-level message (e.g. logger.info("User logged in. login={}, userId={}", login, user.getUserId()); ) before returning the user so successful auth events are recorded. |
||
|
|
||
| public static void main(String[] args) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This violates checklist guidance: "Let's make our log messages informative" (checklist). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This log message lacks full context required by the checklist: "Let's make our log messages informative" and you should "include key context such as |
||
| AuthenticationService authenticationService = new AuthenticationServiceImpl(); | ||
| User user; | ||
| String login = "bob"; | ||
| try { | ||
| user = authenticationService.login("bob", "1234"); | ||
| user = authenticationService.login(login, "1234"); | ||
| } catch (AuthenticationException e) { | ||
| e.printStackTrace(); | ||
| logger.error("Failed to login={}", login,e); | ||
| return; | ||
| } | ||
| OrderService orderService = new OrderServiceImpl(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,16 @@ | |
|
|
||
| import mate.academy.exception.AuthenticationException; | ||
| import mate.academy.model.User; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public class AuthenticationServiceImpl implements AuthenticationService { | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The project is missing a Log4j2 configuration file under src/main/resources (e.g. log4j2.xml or log4j2.properties). The task requires a configuration that defines at least a Console appender and a File appender that writes into the logs/ directory, and a root logger referencing both appenders at an appropriate level. Please add such a config file so logging actually routes to console and the logs/ file. |
||
| private static final Logger logger = LogManager.getLogger(AuthenticationServiceImpl.class); | ||
|
|
||
| @Override | ||
| public User login(String login, String password) throws AuthenticationException { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Project is missing a Log4j2 configuration file under src/main/resources that defines Console and File appenders (writing to the already-ignored logs/ directory) and a root logger referencing both appenders. This violates: "There is no Log4j2 configuration file (e.g., |
||
| //TODO: add corresponding log message about method login was called | ||
| logger.debug("Login attempt. Parms: login={}",login); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an INFO-level log for successful login and fix the small typo in the debug message. You already have an informative DEBUG entry which is good; also log success (for example: logger.info("User logged in successfully. login={}", login);). Also consider correcting |
||
| User user = findByLogin(login); | ||
| if (!user.getPassword().equals(password)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This violates checklist item: "Let's make our log messages informative" — There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This log is emitted before the order id is assigned and doesn't include the order identifier or product count. The task requires informative logs with key context (orderId, counts). See: "Log messages should include key context (such as |
||
| throw new AuthenticationException("Username or password are incorrect"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,16 +4,22 @@ | |
| import java.util.List; | ||
| import mate.academy.model.Order; | ||
| import mate.academy.model.Product; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no Log4j2 configuration file under src/main/resources. The task description requires: "Configure the logger in this project. Add appenders for File and Console." Add a log4j2.xml or log4j2.properties that defines at least a Console appender, a File appender (writing to logs/), and a root logger referencing both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Log4j2 configuration file under src/main/resources. The task requires adding a |
||
| public class OrderServiceImpl implements OrderService { | ||
|
|
||
| private static final Logger logger = LogManager.getLogger(OrderServiceImpl.class); | ||
|
|
||
| @Override | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The project uses Log4j loggers but I don't see a Log4j configuration that adds File and Console appenders. The task description explicitly requires: "Configure the logger in this project. Add appenders for File and Console". Add a log4j2 configuration (e.g. log4j2.xml or log4j2.properties under resources) registering a Console appender and a File appender (write files to the ignored logs/ folder). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logs only a short message without context. This violates the checklist item: "Let's make our log messages informative". Include meaningful context (for example: that login() was called and which There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The login entry log includes the login but consider using a more informative message and the appropriate level. Per checklist "Let's make our log messages informative", prefer e.g. |
||
| public Order completeOrder(Long userId) { | ||
| // TODO: add log message about method completeOrder was called | ||
| List<Product> products = getAllProductsFromShoppingCart(userId); | ||
| Order order = new Order(products, userId); | ||
| // NOTE: In production ready code this order identifier should be generated by DB | ||
| // For test purpose we simplify this and return dummy data | ||
|
Comment on lines
+13
to
19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AuthenticationServiceImpl.login does not log a successful login at INFO level. Per the requirements, add a clear INFO-level message on successful login that includes context (e.g. |
||
| order.setOrderId(1L); | ||
| logger.info("Order completed for userId={}, orderId={}, products size={}", | ||
| userId, order.getOrderId(), products.size()); | ||
| return order; | ||
| } | ||
|
|
||
|
|
@@ -24,7 +30,8 @@ private List<Product> getAllProductsFromShoppingCart(Long userId) { | |
| Product macBook = new Product("MacBook Air 2020", BigDecimal.valueOf(1399)); | ||
| Product xiaomi = new Product("Xiaomi 12", BigDecimal.valueOf(499)); | ||
| List<Product> products = List.of(iphone, macBook, xiaomi); | ||
| // TODO: add log message about successful fetched data from DB | ||
| logger.info("All products in shopping cart for userId={}, products size={}", | ||
| userId, products.size()); | ||
| return products; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <Configuration status="WARN"> | ||
|
|
||
| <Appenders> | ||
| <!-- Konsola --> | ||
| <Console name="ConsoleAppender" target="SYSTEM_OUT"> | ||
| <PatternLayout pattern="%d{HH:mm:ss} [%t] %-5level %logger{36} - %msg%n"/> | ||
| </Console> | ||
|
|
||
| <!-- Plik --> | ||
| <File name="FileAppender" fileName="logs/app.log" append="true"> | ||
| <PatternLayout pattern="%d{yyyy-MM-dd HH:mm:ss} [%t] %-5level %logger{36} - %msg%n"/> | ||
| </File> | ||
| </Appenders> | ||
|
|
||
| <Loggers> | ||
| <Root level="debug"> | ||
| <AppenderRef ref="ConsoleAppender"/> | ||
| <AppenderRef ref="FileAppender"/> | ||
| </Root> | ||
| </Loggers> | ||
|
|
||
| </Configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The task requires: "Configure the logger in this project. Add appenders for File and Console" (description). I don't see any Log4j2 configuration file (e.g. log4j2.xml or log4j2.properties) in the project — without it the appenders (File and Console) are not configured. Add a configuration file that defines Console and File appenders and appropriate loggers/levels (and ensure the log file path is inside the ignored logs/ directory).