User registration - Controller, service and repository layers in Spring framework

Question

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.

1. Should I keep in my Controller class just AuthService field? I'm using userRepository to check if username and email is available.

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

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

Controller:

@PostMapping("/signup")
public ResponseEntity<?> 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(() -> new AppException("User Role not set."));

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

    return userService.saveRegisteredUser(user);
}

Answer 1

1. 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.

2. 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.

3. 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(() -> 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.