Cristian Țone

Here's my feedback

-------------------------------------------------------------------------------------------------------------------------

1. General

- It's good that you didn't put any secret in the configuration files of the app and you decided to pass it through environment variables

- On top of my review you can use chatGPT Codex app to connect chatGPT to your github repo to review code, fix problems, change things. For all of the changes it will create pull requests so you can see it before you merge.

- It's very good that you put all the entities in the same package, all dto's in the same package and all services in the same packages.

- Setup the readme.md like you did for the frontend. I like how you did it on frontend repo.

- Consider creating a constant for system user in a utility class (for instance CommonUtils - see point 10 from this doc) and use that constant everywhere you need.

-------------------------------------------------------------------------------------------------------------------------

2. Pom.xml

- Use the same version of spring boot for all spring dependencies. You used 3.1.1 only for parent and not for others. That is one of the reasons why the app doesn't start. And put the version in the properties section so will be easy to modify in the future (like you did for mapstruct.version).

- Remove the dependency to tomcat. You don't need that specific version.

-------------------------------------------------------------------------------------------------------------------------

3. Liquibase scripts

- It's good that you decided to use liquibase to make changes in the database structure. About naming of the scripts I have a suggestion. When you work in a team and each of the members can write everyday liquibase scripts then considering only the date in the filenames is not enough.Also collisions could happen in the merge process. One suggestion is to use an index instead of timestamp. For instance 000001-add-food-for-user-12.xml, 000002-add_email_to_user.xml, etc

And to have a page somewhere on github or confluence where devs can "book" a new index which they want to use. Another suggestion is to keep the date but also add the time 20240702-140142-add-food-for-user-12.xml, 20240701-071236-add_email_to_user.xml. With the second method you might have problems when your colleagues lives in different countries, different timezones.

- You didn't create the rollback section in the liquibase. Because of that then you won't be able to rollback scripts in production with liquibase rollback command and then the only choice would be to fix manually in prod problems related to structure which is a bad idea. I know it's very annoying and time consuming to create a rollback section but it's for safety and all the customers appreciate that. Imagine a scenario when you deploy a new version of your app to prod, something is wrong with the app and the customer requests from you to un-deploy and to deploy the previous version. But your latest version already made changes to the db structure so the previous version of the app won't work until you modify the structure manually. In this situation for instance liquibase rollback command would be useful. So you rollback maybe 3-5 liquibase changesets and then you deploy the previous version of the app.

- You didn't create any sql index in the liquibase changesets when you define new tables. Those indexes are really helpful to speed-up searching in the table. But do not create indexes for all the columns or column combinations. Firstly create indexes for columns where you estimate you will search from the code. And consider also searching on two columns from the same table. If that happens in the code then you have to create an sql index for both of the columns. Also please pay attention to the index names. It would be good to be like this: <tablename>_<field1>_<field2>...

- It's very good that you defined foreign keys

-------------------------------------------------------------------------------------------------------------------------

4. Useless @Query

- you don't need to use @Query for the methods if methods name and parameters are according to schema definition

- please check all the methods from ExercisesRepository

- please check all the methods from  SetTrackingRepository

- please check all the methods from UserWeightRepository

- please check findByUserIdAndDate method from LoggerRepository

- please check  findByUserId from WorkoutsRepository

-------------------------------------------------------------------------------------------------------------------------

5. Creation of services and controllers

- I observed you created constructors with parameters for all the services but there is a big inconvenience about this. For complex apps one service can have references to dozens of repositories or other services or other components. What will you do then? Will you add a constructor with 30 parameters? The solution is to use @Autowired for the fields from service class. 

- Also for controllers I observed the same thing.

-------------------------------------------------------------------------------------------------------------------------

6. DefaultExercisesConfig class

- checking if system user exists is non-sense because my expectation is that system user is added by liquibase script so it's guaranteed that it's in the database. Generally speaking you should not check in the code if something exists if that was added/set by liquibase script. And if someone modifies MANUALLY the database it's his problem and should not be allowed.

- the default exercises should be added by liquibase script and not from code.

-------------------------------------------------------------------------------------------------------------------------

7. Controllers

- I like the way you named the REST API's with dash between words and you used the proper http methods (GET, POST, DELETE, PUT)

- Consider adding the parameter of the ResponseEntity so it's easy to understand the code. Also if you do that later if you implement swagger annotations for documentation then it will be easy to see the type of response. Example:

@PostMapping("add-exercise")

@PreAuthorize("hasAuthority(T(com.gym_website.util.enums.EAppRoles).MEMBER)")

public ResponseEntity<ResponseDto> addExercise (@RequestBody ExercisesDto exercisesDto){

   return ResponseEntity.ok(exercisesService.addExercise(exercisesDto));

}

…

@GetMapping("/get-set/{id}")

@PreAuthorize("hasAuthority(T(com.gym_website.util.enums.EAppRoles).MEMBER)")

public ResponseEntity<SetTrackingDto> getSetTracking(@PathVariable("id") Long id) {

   return ResponseEntity.ok(setTrackingService.getSetTracking(id));

}

- In the SetTrackingController class remove the / before the endpoints name because it is not needed. For instance here @PostMapping("/add-set")

- In ExercisesController, add-exercise endpoint, the request body contains an ID but i guess the frontend when calling that endpoint will not pass any ID because it's the first time when it is created. So the classes which are used for request bodies should not contain fields which are not set ever by the frontend. The solution for that endpoint is to create a new class called RequestAddExercise which contains only the fields which are set by the frontend. Please do similar checks for all the endpoints of all controllers. For the response classes it's similar. Instead of using a generic DTO which also contains fields which are never set the endpoint consider creating dedicated classes for endpoints with Response prefix. I know is not convenient but in the long term the code is easy to maintain because you can easily add/remove a new field from a body of an endpoint without affecting other endpoints.

-------------------------------------------------------------------------------------------------------------------------

8. Namming

- consider renaming LoggerController, LoggerService, LoggerRepository, LoggerEntity because it's confusing with logging mechanisms. You do not store in the base the logs of your app but you store the history of user changes. Suggestions for LoggerEntity: UserTrackerEntity or UserHistoryEntity.

- consider renaming SetTrackingController because of previous suggestions

-------------------------------------------------------------------------------------------------------------------------

9. ResponseUtilService

-  in this class there is only one method and that is used only in one controller. From a design perspective this is not good. Instead I recommend you to throw an exception in the controllers (your own exception which extends RuntimeException) and then you create a handler for all exceptions of the API's and use @ControlleAdvice for that. Please do a little bit of research about this. The idea is in that class with @ControlleAdvice you can define a method for each type of exception (triggered in controllers) which you want to catch and you have the control over the response of the API so you can set a specific error code and body.

-------------------------------------------------------------------------------------------------------------------------

10. LoginUtilService

- In Spring Boot as many services you have as heavy/lazy the app becomes. For 100 services it might still be ok but why to implement a service where it's suitable for a utility class? Also in LoginUtilService class you have static methods which violates the concept of a service in spring boot. So my suggestion is to replace it with this

public final class CommonUtils {

 private CommonUtils() {

   // Prevent instantiation

 }

 public static EAppRoles getRole(UserDto userDto) {

   if (EAppRoles.MEMBER.label.matches(userDto.getRole())) {

     return EAppRoles.MEMBER;

   }

   return EAppRoles.MEMBER;

 }

 public static Cookie createJwtCookie(String jwt) {

   Cookie cookie = new Cookie("auth-cookie", jwt);

   cookie.setPath("/");

   cookie.setDomain("localhost");

   cookie.setHttpOnly(false);

   return cookie;

 }

 public static LocalDateTime getTime (){

   return LocalDateTime.now();

 }

Also remove TimeUtilService class.

-------------------------------------------------------------------------------------------------------------------------

11. Security

- In ExercisesController, delete-exercise-{id}, any user can delete any exercise from others based on id which is not good from a security perspective. The solution is to check in the code if that id belongs to the user who passed the request. Please do similar checks also in other controllers.

- Generally speaking in controllers it's good to test the parameters before using them in the service and repository levels. For instance if you expect a maximum 100 characters for a string parameter then consider checking in the code for 200 characters, so bigger than expected. But without any limitation the hacker can call the API with a generated string of 1GB which can make the app a little bit slower and also can cause a log forging attack if that parameter is logged. Or even worse, the hacker could inject huge values in the database.

-------------------------------------------------------------------------------------------------------------------------

12. Optimizations

- Such things can be optimized and perform at the end just one query for deletion. And you put that in a try-catch-exception. If exception is triggered then you catch it and you trigger your own runtime exception with specific message

If (workoutsRepository.findById(id).isPresent()) {

   workoutsRepository.deleteById(id);

Leaving the code as it is it will result in performing two queries on the database one for find, one for delete. Such optimizations you can do in many other places too. Please check.

- I saw many places where actually you don't need to return anything meaningful on the response, like this

@PutMapping("edit-exercise")

@PreAuthorize("hasAuthority(T(com.gym_website.util.enums.EAppRoles).MEMBER)")

public ResponseEntity editExercise(@RequestBody ExercisesDto exercisesDto){

   return ResponseEntity.ok(exercisesService.editExercise(exercisesDto));

}

In this case the response object is just like this

ResponseDto responseDto = new ResponseDto();

responseDto.setSuccessMessage("Exercise modified");

That's not required. Instead you can make the method from controller to return void so code is more simplified. Like this

@PutMapping("edit-exercise")

@PreAuthorize("hasAuthority(T(com.gym_website.util.enums.EAppRoles).MEMBER)")

public void editExercise(@RequestBody ExercisesDto exercisesDto){

   exercisesService.editExercise(exercisesDto);

}