User registration - Controller, service and repository layers in Spring framework I'm writting a registration feature currently and I I'm confused how to divide logic between layers. Can you review this code and tell if it's good way to do? In try-catch block I publish event to send verification email to new User.


Should I keep in my Controller class just AuthService field? I'm using userRepository to check if username and email is available.
In AuthService should I keep just authRepository and other services fields? Currently I'm using there RoleRepository.
Is return type in my controller a good practise? ApiRespose is two-field class.


Controller:

@PostMapping("/signup")
public ResponseEntity&lt;?&gt; registerUser(@Valid @RequestBody SignUpRequest signUpRequest, HttpServletRequest httpServletRequest) {
    if (userRepository.existsByUsername(signUpRequest.getUsername())) {
        return new ResponseEntity(new ApiResponse(false, "Username is already in use!"), HttpStatus.BAD_REQUEST);
    }

    if (userRepository.existsByEmail(signUpRequest.getEmail())) {
        return new ResponseEntity(new ApiResponse(false, "Email is already in use!"), HttpStatus.BAD_REQUEST);
    }

    User user = new User(signUpRequest.getName(), signUpRequest.getUsername(), signUpRequest.getEmail(), signUpRequest.getPassword());

    User resultUser = authService.createUser(user);

    try {
        String host = httpServletRequest.getHeader("apphost");
        eventPublisher.publishEvent(new OnRegistrationCompleteEvent(resultUser, host));
    } catch (Exception e) {
        e.printStackTrace();
    }

    URI location = ServletUriComponentsBuilder
            .fromCurrentContextPath().path("/users/{username}")
            .buildAndExpand(resultUser.getUsername()).toUri();

    return ResponseEntity.created(location).body(new ApiResponse(true, "User registered successfully"));
}


Service:

@Override
public User createUser(User user) {
    user.setPassword(passwordEncoder.encode(user.getPassword()));

    Role userRole = roleRepository.findByName(RoleName.ROLE_USER)
            .orElseThrow(() -&gt; new AppException("User Role not set."));

    user.setRoles(Collections.singleton(userRole));

    return userService.saveRegisteredUser(user);
}
Should I keep in my Controller class just AuthService field? I'm using userRepository to check if username and email is available.
  


I think it is ok to use the UserRepository directly, but that is a matter of taste. You could create an additional UserService to wrap the calls to the UserRepository, or you could also create these wrapping methods in your AuthService.

Example:

public class UserService/AuthService {

   private UserRepository userRepository;

   public boolean userExistsByUsername(String username) {
       return userRepository.existsByUsername(username);
   }
}


Then you can use these UserService/AuthService methods in your controller.

For me, at this point, this looks like an unnecessary indirection, but this is the classic way of dividing layers.


  
  In AuthService should I keep just authRepository and other services
  fields? Currently I'm using there RoleRepository.
  


Same answer as for question 1. You could write a RoleService class which wraps the RoleRepository calls.


  
  Is return type in my controller a good practise? ApiRespose is two-field class.
  


I think that's totally ok, because you want to set different HTTP status codes.

Other things:

HTTP status code

new ApiResponse(false, "Username is already in use!"), HttpStatus.BAD_REQUEST);


Just a hint, the correct HTTP status code for existing entities is 409, so I would use HttpStatus.CONFLICT. But 400 is also ok.

Exception handling

This code

Role userRole = roleRepository.findByName(RoleName.ROLE_USER)
        .orElseThrow(() -&gt; new AppException("User Role not set."));


is not inside a try-catch block so if this exception is not handled in another way by a @Provider or some @ExceptionHandler, you may improve this part. If you handling it globally, everything is fine.