Skip to content
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

[IL-63][issue-4] Hibernate issue with authorities save and fixed issue with registration response #98

Merged
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.zufar.onlinestore.common.exception.handler;

import com.zufar.onlinestore.common.response.ApiResponse;
import com.zufar.onlinestore.user.exception.UserAlreadyRegisteredException;
import lombok.extern.slf4j.Slf4j;
import org.apache.logging.log4j.util.Strings;
import org.springframework.context.support.DefaultMessageSourceResolvable;
Expand Down Expand Up @@ -40,12 +41,19 @@ protected ApiResponse<Void> buildResponse(Exception exception, HttpStatus httpSt
}

private List<String> collectErrorMessages(Exception exception) {
return exception instanceof MethodArgumentNotValidException methodArgumentNotValidException ?
methodArgumentNotValidException
.getBindingResult()
.getAllErrors().stream()
.map(DefaultMessageSourceResolvable::getDefaultMessage)
.toList() : List.of(exception.getMessage());
if (exception instanceof UserAlreadyRegisteredException userAlreadyRegisteredException) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception relates to specific security module. Please, create new separate SecurityExceptionHandler class and create method:

    @ExceptionHandler(UserAlreadyRegisteredException.class)
    @ResponseStatus(HttpStatus.BAD_REQUEST)
    public ApiResponse<Void> handleUserAlreadyRegisteredException(final UserAlreadyRegisteredException  exception) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return userAlreadyRegisteredException.getErrors();
}

if (exception instanceof MethodArgumentNotValidException methodArgumentNotValidException) {
return methodArgumentNotValidException
.getBindingResult()
.getAllErrors().stream()
.map(DefaultMessageSourceResolvable::getDefaultMessage)
.toList();
}

return List.of(exception.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Collections.singletonList

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

private String buildErrorDescription(Exception exception) {
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/com/zufar/onlinestore/user/api/AuthorityService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.zufar.onlinestore.user.api;

import com.zufar.onlinestore.user.entity.Authority;
import com.zufar.onlinestore.user.entity.UserEntity;
import com.zufar.onlinestore.user.entity.UserGrantedAuthority;
import org.springframework.stereotype.Service;

@Service
public class AuthorityService {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultUserAuthoritySetter sound clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this service may have a greater responsibility than setting the default role


public void setDefaultAuthority(UserEntity savedUserEntity) {
UserGrantedAuthority defaultAuthority = UserGrantedAuthority
.builder()
.authority(Authority.USER)
.build();

savedUserEntity.addAuthority(defaultAuthority);
}
}
11 changes: 10 additions & 1 deletion src/main/java/com/zufar/onlinestore/user/api/UserService.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@
import com.zufar.onlinestore.user.converter.UserDtoConverter;
import com.zufar.onlinestore.user.dto.UserDto;
import com.zufar.onlinestore.user.entity.UserEntity;
import com.zufar.onlinestore.user.exception.UserAlreadyRegisteredException;
import com.zufar.onlinestore.user.exception.UserNotFoundException;
import com.zufar.onlinestore.user.repository.UserRepository;
import com.zufar.onlinestore.user.validation.UserDataValidator;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Service;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.UUID;

Expand All @@ -19,11 +22,16 @@ public class UserService implements UserApi {

private final UserRepository userCrudRepository;
private final UserDtoConverter userDtoConverter;
private final AuthorityService authorityService;
private final UserDataValidator userDataValidator;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this validator from this class
You should create A Custom Validator Annotation with Spring like that:
https://reflectoring.io/bean-validation-with-spring-boot/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


@Override
public UserDto saveUser(final UserDto userDto) {
UserEntity userEntity = userDtoConverter.toEntity(userDto);
userDataValidator.validate(userEntity);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should create A Custom Validator Annotation with Spring like that:
https://reflectoring.io/bean-validation-with-spring-boot/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

UserEntity userEntityWithId = userCrudRepository.save(userEntity);
authorityService.setDefaultAuthority(userEntityWithId);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is better to use before UserEntity userEntityWithId = userCrudRepository.save(userEntity);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


return userDtoConverter.toDto(userEntityWithId);
}

Expand All @@ -36,4 +44,5 @@ public UserDto getUserById(final UUID userId) throws UserNotFoundException {
}
return userDtoConverter.toDto(userEntity.get());
}

}
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
package com.zufar.onlinestore.user.converter;

import com.zufar.onlinestore.user.dto.UserDto;
import com.zufar.onlinestore.user.entity.Authority;
import com.zufar.onlinestore.user.entity.UserEntity;
import com.zufar.onlinestore.user.entity.UserGrantedAuthority;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.MappingConstants;
import org.mapstruct.Named;

import java.util.Collections;
import java.util.Set;

@Mapper(componentModel = MappingConstants.ComponentModel.SPRING, uses = AddressDtoConverter.class)
public interface UserDtoConverter {
Expand All @@ -23,32 +17,6 @@ public interface UserDtoConverter {
@Mapping(target = "credentialsNonExpired", constant = "true")
@Mapping(target = "enabled", constant = "true")
@Mapping(target = "address", source = "dto.address", qualifiedByName = "toAddress")
@Mapping(target = "authorities", source = "dto", qualifiedByName = "createAuthorities")
UserEntity toEntity(final UserDto dto);

@Named("createAuthorities")
@Mapping(target = "address", source = "dto.address", qualifiedByName = "toAddress")
default Set<UserGrantedAuthority> createAuthorities(UserDto dto) {
UserEntity userEntity = UserEntity.builder()
.firstName(dto.firstName())
.lastName(dto.lastName())
.email(dto.email())
.username(dto.username())
.password(dto.password())
.accountNonExpired(true)
.accountNonLocked(true)
.credentialsNonExpired(true)
.enabled(true)
.build();

Set<UserGrantedAuthority> authorities = Collections.singleton(UserGrantedAuthority
.builder()
.authority(Authority.USER)
.user(userEntity)
.build());

userEntity.setAuthorities(authorities);

return authorities;
}
}
27 changes: 24 additions & 3 deletions src/main/java/com/zufar/onlinestore/user/entity/UserEntity.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.springframework.security.core.userdetails.UserDetails;

import java.util.Iterator;
import java.util.Set;
import java.util.UUID;

Expand All @@ -44,13 +44,13 @@ public class UserEntity implements UserDetails {
@Column(name = "last_name", nullable = false)
private String lastName;

@Column(name = "user_name", nullable = false)
@Column(name = "user_name", nullable = false, unique = true)
private String username;

@Column(name = "stripe_customer_token", nullable = true, unique = true)
private String stripeCustomerToken;

@Column(name = "email", nullable = false)
@Column(name = "email", nullable = false, unique = true)
private String email;

@Column(name = "password", nullable = false)
Expand All @@ -75,6 +75,27 @@ public class UserEntity implements UserDetails {
@Column(name = "enabled", nullable = false)
private boolean enabled;

public void addAuthority(UserGrantedAuthority authority) {
this.authorities.add(authority);
authority.setUser(this);
}

public void removeAuthority(UserGrantedAuthority authority) {
authority.setUser(null);
this.authorities.remove(authority);
}

public void removeAuthorities() {
Iterator<UserGrantedAuthority> iterator = this.authorities.iterator();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use authorities.forEach(authority -> authority.setUser(null)); authorities.clear();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

while (iterator.hasNext()){
UserGrantedAuthority authority = iterator.next();

authority.setUser(null);
iterator.remove();
}
}

@Override
public boolean equals(Object o) {
if (this == o)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,14 @@
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont use wildcard imports

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

import org.springframework.security.core.GrantedAuthority;

import java.util.UUID;

@Builder
@Getter
@Setter
@AllArgsConstructor
@NoArgsConstructor
@Entity
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.zufar.onlinestore.user.exception;

import lombok.Getter;

import java.util.List;

@Getter
public class UserAlreadyRegisteredException extends RuntimeException {

private final List<String> errors;

public UserAlreadyRegisteredException(List<String> errors) {
this.errors = errors;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.zufar.onlinestore.user.exception.handler;

import com.zufar.onlinestore.common.exception.handler.GlobalExceptionHandler;
import com.zufar.onlinestore.common.response.ApiResponse;
import com.zufar.onlinestore.payment.exception.PaymentNotFoundException;
import com.zufar.onlinestore.user.exception.UserAlreadyRegisteredException;
import lombok.extern.slf4j.Slf4j;
import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.springframework.web.bind.annotation.RestControllerAdvice;

@Slf4j
@RestControllerAdvice
public class UserExceptionHandler extends GlobalExceptionHandler {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this exception is related to security model more than to user model. That is why I think you should rename this class like SecurityExceptionHandler and move this class to com.zufar.onlinestore.security.exception.handler package


@ExceptionHandler(UserAlreadyRegisteredException.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public ApiResponse<Void> handleUserAlreadyRegisteredException(final UserAlreadyRegisteredException exception) {
ApiResponse<Void> apiResponse = buildResponse(exception, HttpStatus.BAD_REQUEST);
log.error("Handle user already registered exception: failed: messages: {}, description: {}.",
apiResponse.messages(), apiResponse.description());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return apiResponse;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@
import com.zufar.onlinestore.user.entity.UserEntity;
import org.springframework.data.jpa.repository.JpaRepository;

import java.util.Optional;
import java.util.UUID;

public interface UserRepository extends JpaRepository<UserEntity, UUID> {

UserEntity findUserByUsername(String username);

Optional<UserEntity> findByEmail(String email);

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package com.zufar.onlinestore.user.validation;

import com.zufar.onlinestore.user.entity.UserEntity;
import com.zufar.onlinestore.user.exception.UserAlreadyRegisteredException;
import com.zufar.onlinestore.user.repository.UserRepository;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;

import java.util.List;
import java.util.Optional;
import java.util.UUID;

@RequiredArgsConstructor
@Component
public class UserDataValidator {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should create A Custom Validator Annotation with Spring like that:
https://reflectoring.io/bean-validation-with-spring-boot/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


private final List<String> errors;
private final UserRepository userCrudRepository;

public void validate(UserEntity user) {
if (!isEmailUnique(user.getUserId(), user.getEmail())) {
errors.add(String.format("User with email = %s is already registered", user.getEmail()));
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (!isUsernameUnique(user.getUserId(), user.getUsername())) {
errors.add(String.format("User with username = %s is already registered", user.getUsername()));
}

if (!errors.isEmpty()) {

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

throw new UserAlreadyRegisteredException(errors);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
}

public boolean isEmailUnique(UUID id, String email) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to create separate small class UniqueEmailValidator with the method
public boolean validate(String email) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Optional<UserEntity> userByEmail = userCrudRepository.findByEmail(email);

if (userByEmail.isEmpty()) {
return true;
}

return userByEmail.map(UserEntity::getUserId).filter(userId -> userId == id).isPresent();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way please:

return userByEmail.map(UserEntity::getUserId)
                  .filter(userId -> userId == id)
                  .isPresent();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need an id to perform this check? I dont think so because email is uniquely identifies the object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

public boolean isUsernameUnique(UUID id, String username) {
UserEntity userByUsername = userCrudRepository.findUserByUsername(username);

if (userByUsername == null) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


return userByUsername.getUserId().equals(id);
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ CREATE TABLE user_details
account_non_locked BOOLEAN NOT NULL,
credentials_non_expired BOOLEAN NOT NULL,
enabled BOOLEAN NOT NULL,
UNIQUE(stripe_customer_token),
UNIQUE (user_name, email, stripe_customer_token),
CONSTRAINT fk_address
FOREIGN KEY (address_id)
REFERENCES address (id)
Expand Down
Loading