implemented services and dao level #899

Nazar1106 · 2024-12-04T15:42:05Z

No description provided.

mateacademy-ai-mentor

Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request:

Unique Email Constraint: Ensure the email field in the User model is unique in the database to meet the task requirements.
AuthenticationService Implementation: The login method should authenticate an existing user by checking if the email exists and verifying the password. It should not create a new user if the email is not found.
Registration Logic: The register method should create a new user if the email does not already exist, rather than incorrectly calling the login method.

These issues impact the core functionality of user authentication and registration, which are critical to the task. Please address these points, and feel free to reach out if you need any help. Keep up the great work! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2024-12-04T15:44:17Z

src/main/java/mate/academy/Main.java

+    private static final String FIRST_HALL_CAPACITY = "first hall with capacity 100";
+    private static final String SECOND_HALL_CAPACITY = "second hall with capacity 200";


The variables FIRST_HALL_CAPACITY and SECOND_HALL_CAPACITY are used to store descriptions, but their names suggest they are for capacity values. Consider renaming these variables to better reflect their purpose, such as FIRST_HALL_DESCRIPTION and SECOND_HALL_DESCRIPTION.

mateacademy-ai-mentor · 2024-12-04T15:44:17Z

src/main/java/mate/academy/Main.java

+            + "street racing, heists, and spies.";
+    private static final String FIRST_HALL_CAPACITY = "first hall with capacity 100";
+    private static final String SECOND_HALL_CAPACITY = "second hall with capacity 200";
+    private static final long LONG_1 = 1L;


The constant LONG_1 is used to represent a day offset for date calculations. Consider renaming it to something more descriptive, like ONE_DAY_OFFSET, to improve code readability.

mateacademy-ai-mentor · 2024-12-04T15:44:17Z

src/main/java/mate/academy/Main.java

+    private static final long LONG_1 = 1L;
+    private static final String USER_PASSWORD = "nazar1177";
+    private static final String USER_EMAIL = "nazar@gmail.com";
+    private static final String EMAIL_NOT_EXIST = "nazarss@gmail.com";


The variable EMAIL_NOT_EXIST is declared but never used in the code. If it's not needed, consider removing it to clean up the code.

mateacademy-ai-mentor · 2024-12-04T15:44:17Z

src/main/java/mate/academy/dao/impl/MovieDaoImpl.java

            if (transaction != null) {
                transaction.rollback();


The transaction rollback is correctly handled, but consider logging the exception before throwing a DataProcessingException to aid in debugging.

mateacademy-ai-mentor · 2024-12-04T15:44:17Z

src/main/java/mate/academy/dao/impl/UserDaoImpl.java

+
+    @Override
+    public User add(User user) {
+        Session session = null;


The session variable is declared outside the try block, which is not necessary. You can declare it inside the try block to limit its scope and improve code readability.

mateacademy-ai-mentor · 2024-12-04T15:44:18Z

src/main/java/mate/academy/model/User.java

+        int result = Objects.hash(getId(), getEmail(), getPassword());
+        result = 31 * result + Arrays.hashCode(getSalt());


In the hashCode method, consider using Objects.hash(id, email, password) directly without the getter methods for consistency with the equals method.

mateacademy-ai-mentor · 2024-12-04T15:44:18Z

src/main/java/mate/academy/service/impl/AuthenticationServiceImpl.java

+    public User login(String email, String password) throws AuthenticationException {
+        User user = new User();
+
+        Optional<User> userByEmail = userService.findByEmail(email);
+        if (userByEmail.isEmpty()) {
+            user.setEmail(email);
+            user.setPassword(password);
+            return user;
+        } else {
+            throw new AuthenticationException("User with email " + email + " already exist");
+        }
+    }


The login method logic is incorrect. It should authenticate an existing user by checking if the email exists and then verifying the password. Currently, it throws an exception if the user exists, which is not the expected behavior for a login method.

mateacademy-ai-mentor · 2024-12-04T15:44:18Z

src/main/java/mate/academy/service/impl/AuthenticationServiceImpl.java

+    public User register(String email, String password) throws RegistrationException {
+        try {
+            User userLogin = login(email, password);
+            return userService.add(userLogin);
+        } catch (Exception e) {
+            throw new RegistrationException("User can't register ", e);
+        }
+    }


The register method incorrectly calls the login method. Registration should create a new user if the email does not already exist. Consider checking if the user exists first, and if not, create and add the new user.

mateacademy-ai-mentor · 2024-12-04T15:44:18Z

src/main/java/mate/academy/service/impl/UserServiceImpl.java

+        user.setSalt(HashUtil.salt());
+        String userHashPassword =
+                HashUtil.hashPassword(user.getPassword(), user.getSalt());
+        user.setPassword(userHashPassword);


The add method correctly hashes the user's password with a salt. Ensure that the HashUtil.salt() method generates a secure and random salt for each user to maintain security.

mateacademy-ai-mentor · 2024-12-04T15:44:18Z

src/main/java/mate/academy/util/HibernateUtil.java

@@ -5,6 +5,7 @@

 public class HibernateUtil {
    private static final SessionFactory sessionFactory = initSessionFactory();


Consider adding a shutdown method to close the SessionFactory when the application is stopped. This can help in releasing resources properly.

mateacademy-ai-mentor

Thank you for your submission! 🎉 Your code demonstrates a solid understanding of setting up services and models in a Java application. However, there are a few critical issues that need to be addressed to ensure the functionality and security of your application:

Unique Email Constraint: Ensure the User model has a unique constraint on the email field by adding @Column(unique = true). This will enforce email uniqueness at the database level.
AuthenticationService Implementation:
- The login method should handle cases where the user is not found by checking if the Optional is present. Use user.orElseThrow() to avoid NoSuchElementException.
- The register method should hash the password using HashUtil.hashPassword(password, salt) and generate a salt before saving the user. Ensure the email does not already exist before creating a new user.
Database Query Handling: Replace getSingleResultOrNull() with getResultList() and check if the list is empty to ensure compatibility across different environments.

These improvements are crucial for the security and reliability of your application. While the decision is to APPROVE your pull request to help you move forward with your learning, please address these issues in future iterations to enhance your code quality. Keep up the good work and continue learning from these experiences! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2024-12-04T18:11:55Z

src/main/java/mate/academy/Main.java

+        AuthenticationService authenticationService = (AuthenticationService)
+                injector.getInstance(AuthenticationService.class);
+
+        User login = authenticationService.login(USER_EMAIL, USER_PASSWORD);


The login method should authenticate an existing user by checking if the email exists and verifying the password. It should not create a new user if the email is not found. Please ensure the login method in AuthenticationService is implemented correctly according to the task requirements.

mateacademy-ai-mentor · 2024-12-04T18:11:55Z

src/main/java/mate/academy/Main.java

+
+        User login = authenticationService.login(USER_EMAIL, USER_PASSWORD);
+        System.out.println(login);
+


The register method should create a new user only if the email does not already exist. Ensure that the register method in AuthenticationService checks for the existence of the email before creating a new user.

mateacademy-ai-mentor · 2024-12-04T18:11:55Z

src/main/java/mate/academy/dao/impl/UserDaoImpl.java

+            Query<User> userQuery =
+                    session.createQuery("SELECT u FROM User u where u.email=:email", User.class);
+            userQuery.setParameter(EMAIL, email);
+            User user = userQuery.getSingleResultOrNull();


The method getSingleResultOrNull() is not a standard JPA method and might not be supported in all environments. Consider using getResultList() and checking if the list is empty to handle the case where no user is found.

mateacademy-ai-mentor · 2024-12-04T18:11:55Z

src/main/java/mate/academy/model/User.java

+    @Id
+    @GeneratedValue(strategy = GenerationType.AUTO)
+    private Long id;
+    private String email;


To ensure the email field is unique in the database, consider adding a unique constraint annotation, such as @Column(unique = true), to the email field. This will enforce the uniqueness of email addresses at the database level.

mateacademy-ai-mentor · 2024-12-04T18:11:55Z

src/main/java/mate/academy/service/impl/AuthenticationServiceImpl.java

+
+    @Override
+    public User login(String email, String password) throws AuthenticationException {
+        Optional<User> user = userService.findByEmail(email);


The login method should handle the case where the user is not found. Before accessing user.get(), check if the Optional is present to avoid NoSuchElementException. Consider using user.orElseThrow() with a custom exception message.

mateacademy-ai-mentor · 2024-12-04T18:11:55Z

src/main/java/mate/academy/service/impl/AuthenticationServiceImpl.java

+        Optional<User> userByEmail = userService.findByEmail(email);
+        if (userByEmail.isEmpty()) {
+            user.setEmail(email);
+            user.setPassword(password);


The register method should hash the password before saving the user. Use HashUtil.hashPassword(password, salt) to hash the password and set it to the user object. Ensure that a salt is generated and stored with the user.

Elena-Bruyako · 2024-12-05T13:44:09Z

src/main/java/mate/academy/dao/impl/UserDaoImpl.java

+            if (user == null) {
+                return Optional.empty();
+            }
+            return Optional.of(user);


Suggested change

if (user == null) {

return Optional.empty();

}

return Optional.of(user);

return Optional.ofNullable(user);

Elena-Bruyako · 2024-12-05T13:45:05Z

src/main/java/mate/academy/dao/impl/UserDaoImpl.java

+    private static final String CAN_T_ADD_NEW_USER_MSG = "Can't add new user";
+    private static final String SPACE = " ";


Suggested change

private static final String CAN_T_ADD_NEW_USER_MSG = "Can't add new user";

private static final String SPACE = " ";

private static final String CAN_T_ADD_NEW_USER_MSG = "Can't add new user ";

Elena-Bruyako · 2024-12-05T13:45:17Z

src/main/java/mate/academy/model/User.java

+import java.util.Arrays;
+import java.util.Objects;
+
+@Entity


add Table annotation

Elena-Bruyako · 2024-12-05T13:45:46Z

src/main/java/mate/academy/model/User.java

+    @Id
+    @GeneratedValue(strategy = GenerationType.AUTO)
+    private Long id;
+    private String email;


Let's make the email unique. We need to check the email during user registration and make the email a unique field in the database.

Elena-Bruyako · 2024-12-05T13:47:58Z

src/main/java/mate/academy/service/impl/AuthenticationServiceImpl.java

+    @Override
+    public User login(String email, String password) throws AuthenticationException {
+        Optional<User> user = userService.findByEmail(email);
+        String userHashPassword = HashUtil.hashPassword(password, user.get().getSalt());


Suggested change

String userHashPassword = HashUtil.hashPassword(password, user.get().getSalt());

implemented services and dao level

b0c7f96

mateacademy-ai-mentor suggested changes Dec 4, 2024

View reviewed changes

changed logic for register and login methods

801d620

Nazar1106 requested a review from mateacademy-ai-mentor December 4, 2024 18:09

mateacademy-ai-mentor approved these changes Dec 4, 2024

View reviewed changes

Elena-Bruyako suggested changes Dec 5, 2024

View reviewed changes

		private static final String FIRST_HALL_CAPACITY = "first hall with capacity 100";
		private static final String SECOND_HALL_CAPACITY = "second hall with capacity 200";

		int result = Objects.hash(getId(), getEmail(), getPassword());
		result = 31 * result + Arrays.hashCode(getSalt());

		@@ -5,6 +5,7 @@

		public class HibernateUtil {
		private static final SessionFactory sessionFactory = initSessionFactory();


		User login = authenticationService.login(USER_EMAIL, USER_PASSWORD);
		System.out.println(login);

		private static final String CAN_T_ADD_NEW_USER_MSG = "Can't add new user";
		private static final String SPACE = " ";

implemented services and dao level #899

Are you sure you want to change the base?

implemented services and dao level #899

Uh oh!

Conversation

Nazar1106 commented Dec 4, 2024

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!