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

Bp 25 implement user registration #6

Merged
merged 13 commits into from
Jul 20, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package gdsc.konkuk.platformcore.application.member;

import gdsc.konkuk.platformcore.domain.member.entity.Member;
import gdsc.konkuk.platformcore.domain.member.entity.MemberRole;
import lombok.Builder;
import lombok.Getter;

@Getter
@Builder
Copy link
Member

Choose a reason for hiding this comment

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

builder를 여기다 선언하면 다음과 같은 코드도 생성될것 같습니다.

Member.builder()
		.memberId(request.getMemberId())
		.build();

또한 인자로 null도 전달 가능한 상황입니다. 방어적인 생성이 필요하다 봅니다.

public class MemberRegisterRequest {
private String memberId;
private String password;
private String name;
private String email;
private MemberRole memberRole;
private int batch;

public static Member toEntity(MemberRegisterRequest request) {
return Member.builder()
.memberId(request.getMemberId())
.password(request.getPassword())
.name(request.getName())
.email(request.getEmail())
.role(request.getMemberRole())
.batch(request.getBatch())
.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package gdsc.konkuk.platformcore.application.member;

import java.util.Optional;

import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import gdsc.konkuk.platformcore.application.member.exceptions.UserAlreadyExistException;
import gdsc.konkuk.platformcore.domain.member.entity.Member;
import gdsc.konkuk.platformcore.domain.member.repository.MemberRepository;
import gdsc.konkuk.platformcore.global.exceptions.ErrorCode;
import gdsc.konkuk.platformcore.global.responses.SuccessResponse;
import lombok.RequiredArgsConstructor;

@Service
@RequiredArgsConstructor
@Transactional(readOnly = true)
Copy link
Member

Choose a reason for hiding this comment

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

(readOnly = true)의 장점은 무엇일까요?

Copy link
Collaborator Author

@ekgns33 ekgns33 Jul 18, 2024

Choose a reason for hiding this comment

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

읽기 전용으로 트랜잭션을 설정하여 어느정도 성능의 이점을 가져갈 수 있습니다. 대표적으로 데이터 변경이 일어나지 않는것을 보장하므로 스냅샷을 저장하지 않죠(변경감지가 필요없어요).
다만 엔티티에 대해서 낙관적락을 사용하는 경우 쿼리한 데이터들의 수정을 리드온리에서 하는경우 일관성이 깨질 수 있습니다. 현재 우리 엔티티에는 낙관적 락을 적용하지 않았기 때문에 서비스 클래스에 넓은 범위로 적용했습니다. 위에서 언급한 내용은 데이터 수정과 관련된 메소드를 명세와 코드리뷰로 해결할 수 있다고 생각해요.

public class MemberService {

private final PasswordEncoder passwordEncoder;
private final MemberRepository memberRepository;

private boolean checkMemberAlreadyExist(String memberId) {
Copy link
Member

Choose a reason for hiding this comment

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

private 메서드는 public 메서드 보다 아래쪽에 나와야 합니다!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

private를 아래로 내리겠습니다!

Optional<Member> member = memberRepository.findByMemberId(memberId);
return member.isPresent();
}

@Transactional
public SuccessResponse register(MemberRegisterRequest registerRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

register 메서드는 SuccessResponse에 의존적이게 되었군요, 향후 다른 응답이 필요하면 해당 부분을 변경해야 할것 같은데요?
Controller의 역할을 조금더 생각해보시면 좋을것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

서비스단에서는 객체만 반환을 하고 프레젠테이션 레이어에서 응답객체를 만들어 반환하도록 하는것을 말씀하시나용?


if(checkMemberAlreadyExist(registerRequest.getMemberId())) {
throw UserAlreadyExistException.of(ErrorCode.USER_ALREADY_EXISTS);
}

Member member = Member.builder()
ekgns33 marked this conversation as resolved.
Show resolved Hide resolved
.memberId(registerRequest.getMemberId())
.password(passwordEncoder.encode(registerRequest.getPassword()))
.name(registerRequest.getName())
.email(registerRequest.getEmail())
.role(registerRequest.getMemberRole())
.batch(registerRequest.getBatch())
.build();

memberRepository.save(member);

return SuccessResponse.messageOnly();
}
}
Copy link
Contributor

@goldentrash goldentrash Jul 19, 2024

Choose a reason for hiding this comment

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

UserAlreadyExistException의 필요를 잘 모르겠습니다.

사용되는 형태를 살펴보니 UserAlreadyExistException.of(ErrorCode.USER_ALREADY_EXIST)의 형태입니다.
우리는 이미 ErrorCode Enum을 통해 Error의 유형을 구별하고 logMessage를 통한 메시지를 결정했습니다. 또한, ErrorCode/global/excpetions에 위치해서 codebase의 top-level에서 일괄 관리되고 있습니다. ErrorCode에 의존적이라면, application/member아래에서 domain 단위로 나눠 다시 관리할 필요가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음 에러코드에 의존하게된다는 말씀이군요. 하지만 에러코드의 경우 프로젝트 전역에서 공통으로 사용하는 값으로 로직이 존재하지않는 순수한 값만을 담고있는 클래스입니다. 의존이 생겨 에러코드를 사용하는 코드단에서 수정할일이 있을까요? 에러코드가 변해서 수정이 일어나야한다면 에러코드 자체를 바꾸는 경우만 생각나는데 이것은 로직의 변경으로 바라봐야하지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

예외클래스가 존재할 필요성에 대해서는 저는 이렇게 생각해요

    1. 에러코드와 무관하게 에러추적할때 어떤 비즈니스로직에서 발생했는지 파악하기 용이하다.
    1. 비즈니스 에러들을 하나로 묶어서 처리하면 에러코드 필드값으로 에러를 분류해야하는데 오히려 이게 복잡도를 높이지 않을까요? 에러코드는 같은데 예외의 이유가 다를 수 있을 것 같아요. 예외클래스 자체가 의미를 담고있다는 점에서 저는 만들었습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

BusinessException.of(ErrorCode.User_ALREADY_EXIST)의 형태로 사용해도 충분하지 않을까 싶었습니다.

아래는 /global/controller/GlobalExceptionHandler의 코드 일부분입니다.

@ExceptionHandler(UserAlreadyExistException.class)
protected ResponseEntity<ErrorResponse> handleUserAlreadyExistException(final UserAlreadyExistException e) {
	log.error("UserAlreadyExistException Caught! [{}]", e.getLogMessage());
	final ErrorResponse response = ErrorResponse.of(e.getMessage(), e.getLogMessage());
	return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST);
}

@ExceptionHandler(BusinessException.class)
protected ResponseEntity<ErrorResponse> handleBusinessException(final BusinessException e) {
	log.error("BusinessException Caught! [{}]", e.getLogMessage());
	final ErrorResponse response = ErrorResponse.of(e.getMessage(), e.getLogMessage());
	return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST);
}

BusinexxException은 정의 및 사용 scope가 확실히 Global입니다. 그런데 UserAlreadyExisitException의 경우 정의는 Domain scope에서 이뤄지지만 사용은 Global scope까지 확장되고 있습니다. 앞으로 Domain이 계속 늘어날 것이고, Domain마다 각각의 Exception을 관리하게 될 것인데 이 모든 것들을 Global scope가 control해야 하는 구조는 관리에 어려움이 따를것같습니다.

그래서 떠오른 방법에 대한 @ekgns33의 의견이 궁금합니다!

  1. MemberErrorCodeapplication/member아래에 새로 정의합니다.
  2. UserAlreadyExistException exception에 대한 controllerAddvice를 /controller/member로 옮깁니다.
  3. MemberControllerAdvice에서 BuisnessException을 다시 throw합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1번으로 가는게 어떨까요

Copy link
Contributor

Choose a reason for hiding this comment

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

인터넷 오류였을까요...? 분명 전 하나의 답글을 보고 2번째 comment를 작성했는데 이제보니 2개네요???

말씀하신것처럼 ErrorCode만 사용하는것보다는 Class를 분기하는게 더 로직을 단순하게 만든다고 생각합니다. 제가 BusinessException.of(ErrorCode.User_ALREADY_EXIST)를 언급했던 이유는 ControllerAdvice에서 BusinessExceptionUserAlreadyExistException의 처리에 약간의 log message를 제외한 나머지가 동일했기 때문입니다.

2번째 comment에서 작성한 떠오른 방법은 1, 2, 3 중에서 하나를 선택하자가 아닌 순서대로 전부라는 의미였는데, 지금 다시 읽어보니 뭔가뭔가인 느낌이 있네요. 해당 사항은 저도 조금 더 고민해 보겠습니다. 1번을 말씀하셨는데, 떠오른 좋은 구조가 있다면 편하신대로 re-factoring을 진행해주시면 감사합니다. 그런데 만약 저 3중에 하나를 골라야 한다고 생각하셨다면 전혀 아니니, 더 나은 구조가 없다면 현상 유지도 좋다고 생각합니다!

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package gdsc.konkuk.platformcore.application.member.exceptions;

import gdsc.konkuk.platformcore.global.exceptions.BusinessException;
import gdsc.konkuk.platformcore.global.exceptions.ErrorCode;

public class UserAlreadyExistException extends BusinessException {

protected UserAlreadyExistException(ErrorCode errorCode, String logMessage) {
super(errorCode, logMessage);
}

public static UserAlreadyExistException of(ErrorCode errorCode) {
return new UserAlreadyExistException(errorCode, errorCode.getLogMessage());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package gdsc.konkuk.platformcore.controller.member;

import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

import gdsc.konkuk.platformcore.application.member.MemberRegisterRequest;
import gdsc.konkuk.platformcore.application.member.MemberService;
import gdsc.konkuk.platformcore.global.responses.SuccessResponse;
import lombok.RequiredArgsConstructor;

@RestController
@RequestMapping("/api/v1/members")
@RequiredArgsConstructor
public class MemberController {

private final MemberService memberService;

@PostMapping("")
Copy link
Member

Choose a reason for hiding this comment

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

("") 이부분 없어도 됩니다!

public ResponseEntity<SuccessResponse> signup(@RequestBody MemberRegisterRequest registerRequest) {
memberService.register(registerRequest);
return ResponseEntity.status(201).body(SuccessResponse.messageOnly());
Copy link
Member

Choose a reason for hiding this comment

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

201같은 상수를 직접 사용하기 보다는, HttpStatus 에 정의된 상수값을 사용하길 권장합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

매직넘버를 제거하고 HttpStatus.Created이런 정의된 상수를 사용하는 것을 말씀하시는걸까요?

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.HttpMethod;
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer;
import org.springframework.security.config.http.SessionCreationPolicy;
import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
import org.springframework.security.web.SecurityFilterChain;
import org.springframework.security.web.authentication.www.BasicAuthenticationFilter;
Expand All @@ -22,16 +21,16 @@ public class SecurityConfig {

private final CustomAuthenticationSuccessHandler customAuthenticationSuccessHandler;
private final CustomAuthenticationFailureHandler customAuthenticationFailureHandler;

private static final String API_PREFIX = "/api/v1";
@Bean
public SecurityFilterChain securityFilterChain(HttpSecurity httpSecurity) throws Exception {
httpSecurity
.addFilterBefore(new SecurityContextPersistenceFilter(), BasicAuthenticationFilter.class)

.authorizeHttpRequests(authorize -> authorize
.requestMatchers("/docs/**").permitAll()
.requestMatchers("/admin").hasRole("ADMIN")
.requestMatchers("/member").hasRole("MEMBER")
.requestMatchers(apiPath("/docs/**")).permitAll()
.requestMatchers(HttpMethod.POST, apiPath("/members")).permitAll()
.requestMatchers(apiPath("/admin/**")).hasRole("ADMIN")
.anyRequest().authenticated())

.formLogin(login -> login
Expand All @@ -56,4 +55,8 @@ public SecurityFilterChain swaggerFilterchain(HttpSecurity httpSecurity) throws
public BCryptPasswordEncoder passwordEncoder() {
return new BCryptPasswordEncoder();
}

private String apiPath(String path) {
return API_PREFIX + path;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.RestControllerAdvice;

import gdsc.konkuk.platformcore.application.member.exceptions.UserAlreadyExistException;
import gdsc.konkuk.platformcore.global.exceptions.BusinessException;
import gdsc.konkuk.platformcore.global.exceptions.ErrorCode;
import gdsc.konkuk.platformcore.global.responses.ErrorResponse;
Expand All @@ -14,6 +15,12 @@
@RestControllerAdvice
public class GlobalExceptionHandler {

@ExceptionHandler(UserAlreadyExistException.class)
protected ResponseEntity<ErrorResponse> handleUserAlreadyExistException(final UserAlreadyExistException e) {
log.error("UserAlreadyExistException Caught! [{}]", e.getLogMessage());
Copy link
Member

Choose a reason for hiding this comment

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

log.error("UserAlreadyExistException Caught! [{}]", e.getLogMessage(), e); 끝에 e도 인자로 추가하면 stack trace도 모두 출력됩니다. 추가적인 e를 위한 {}는 지정하지 않아도 되구요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그런 방법이 있군요! 적용해보겠습니다🤗

final ErrorResponse response = ErrorResponse.of(e.getMessage(), e.getLogMessage());
return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST);
}

@ExceptionHandler(BusinessException.class)
protected ResponseEntity<ErrorResponse> handleBusinessException(final BusinessException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ public enum ErrorCode {
INTERNAL_SERVER_ERROR("서버 오류입니다. 잠시후 재시도 해주세요", "[ERROR] : 예상치못한 에러 발생", 500),
Copy link
Member

Choose a reason for hiding this comment

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

이부분도 직접 500을 사용하기 보다는 HttpStatus 사용 권장

Copy link
Contributor

@goldentrash goldentrash Jul 18, 2024

Choose a reason for hiding this comment

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

ErrorCode.statusCode는 HTTP Status가 아닌, front-end에서의 error handling을 위한 additional code입니다.

다만, 현재 ErrorCode.logMessage가 해당 역할을 홀로 완벽히 수행하고 있는 것으로 보이는데, ErrorCode.StatusCod property 삭제에 대해서는 어떻게 생각하시나요? @ekgns33

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞습니다. 응답객체를 만들어주는 부분에서 http 응답코드를 정하면 enum에 정의할 필요가 없다고 생각해요. 어떻게 생각하시나요

USER_NOT_FOUND("사용자가 존재하지 않습니다. 다시 입력해주세요", "[ERROR] : 사용자 정보를 찾을 수 없음", 404),
INVALID_USER_INFO("사용자 정보가 올바르지 않습니다. 다시 입력해주세요", "[ERROR] : 사용자 정보가 올바르지 않음", 400),
DEACTIVATED_USER("탈퇴한 사용자입니다. 다시 확인해주세요", "[ERROR] : 탈퇴한 사용자", 400);
DEACTIVATED_USER("비활성화된 사용자입니다. 다시 확인해주세요", "[ERROR] : 탈퇴한 사용자", 400),
USER_ALREADY_EXISTS("이미 존재하는 사용자입니다.", "[ERROR] : 이미 존재하는 사용자", 400);

private final String message;
private final String logMessage;
private final int statusCode;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package gdsc.konkuk.platformcore.application.member;

import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.BDDMockito.*;

import java.util.Optional;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.transaction.annotation.Transactional;

import gdsc.konkuk.platformcore.application.member.exceptions.UserAlreadyExistException;
import gdsc.konkuk.platformcore.domain.member.entity.MemberRole;
import gdsc.konkuk.platformcore.domain.member.repository.MemberRepository;
import gdsc.konkuk.platformcore.global.responses.SuccessResponse;

class MemberServiceTest {

private MemberService subject;

@Mock
private MemberRepository memberRepository;

@Mock
private PasswordEncoder passwordEncoder;


@BeforeEach
void setUp() {
MockitoAnnotations.openMocks(this);
subject = new MemberService(passwordEncoder, memberRepository);
}

@Test
@DisplayName("새로운 멤버 회원가입 성공")
@Transactional
void should_success_when_newMember_register() {
Copy link
Member

@zbqmgldjfh zbqmgldjfh Jul 18, 2024

Choose a reason for hiding this comment

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

해당 class의 이름은 MemberServiceTest 인데,

  1. Transactional을 통해 데이터를 롤백한다는 것 -> Repository까지 확인하는 테스트 인것 같은데,
  2. 상단에 보면 Mock을 선언 -> Repository 는 가짜로 지정한 행동만 반환한다? Service Layer만 테스트 하고 싶은건가?
    인데, 둘중 뭘 의도한 테스트 인지 모르겠습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

로컬에서 테스트할때 레포지토리를 살려놓은상태에서도 진행했는데 모킹하고나서도 그대로 놓은것 같습니다! 이 테스트에서는 서비스 레이어만 테스트하고자하여 삭제하겠습니다 😀

//given
MemberRegisterRequest memberRegisterRequest =
MemberRegisterRequest.builder()
.memberId("202011288")
.password("password")
.email("[email protected]")
.name("홍길동")
.memberRole(MemberRole.MEMBER)
.batch(2024)
.build();
given(memberRepository.findByMemberId(any())).willReturn(Optional.empty());
given(passwordEncoder.encode(any())).willReturn("password");
//when
SuccessResponse expected = SuccessResponse.messageOnly();
SuccessResponse actual = subject.register(memberRegisterRequest);
//then
assertEquals(expected.isSuccess(), actual.isSuccess());
assertEquals(expected.getData(), actual.getData());
}

@Test
@DisplayName("이미 존재하는 멤버 회원가입 실패")
@Transactional
void should_fail_when_already_exist_member_register() {
//given
MemberRegisterRequest memberRegisterRequest =
MemberRegisterRequest.builder()
.memberId("202011288")
.password("password")
.email("[email protected]")
.name("홍길동")
.memberRole(MemberRole.MEMBER)
.batch(2024)
.build();
given(memberRepository.findByMemberId(any()))
.willReturn(Optional.of(MemberRegisterRequest
.toEntity(memberRegisterRequest)));

//then
assertThrows(UserAlreadyExistException.class, () -> {
//when
subject.register(memberRegisterRequest);
});
}
}
Loading