-
Notifications
You must be signed in to change notification settings - Fork 5
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
feature :: 회원가입 기본 로직 작성 (재업) #35
Conversation
단위테스트 통과 인수테스트 통과
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! LGTM :-)
아..! 추가로 가끔 ','나 '괄호'의 위치가 코드마다 다른 경우가 있는 것 같은데요~
컨벤션 상에 크게 이슈는 없지만 Reformat code 단축키를 이용해서 정리하는 방법을 사용해도 좋을 것 같아요! (윈도우: Ctrl + Alt + L / 맥: Opt + Cmd + L)
public MemberDto login(MemberDto loginDto) { | ||
|
||
Member loginEntity = getReqeustLoginEntity(loginDto); | ||
Member memberEntity = memberRepository.findAllByIdAndPassword(loginEntity.getId(), loginEntity.getPassword()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반환값을 보면 결과가 1개만 리턴되는 걸로 보이는데 'findAll~'로 사용하신 이유가 있으신가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 제가 잘못 썼습니다 🙃 오늘 리팩터링때 바로 고쳐놓겠습니다
public void createJwt(Integer memberNo, String encryptToken) { | ||
this.memberNo = memberNo; | ||
this.jwtToken = encryptToken; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createJwt 는 실제로는 사용되지 않는 건가요?
createAuthJwt 로 사용하는 코드만 보여서요~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅠㅠ 리팩터링때 지우고 올리겠습니다
|
||
@Getter | ||
@Entity | ||
@DynamicUpdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 어노테이션은 처음 보네요~
변경된 컬럼으로만 update 쿼리를 만들다니.. 한 수 배워갑니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DynamicUpdate
주요 특징 및 용도
-
부분 업데이트 최적화:
@DynamicUpdate
를 사용하면 [[엔티티]]의 일부 속성만 업데이트될 때 관련 SQL 쿼리를 최적화 즉, 전체 레코드를 갱신하는 대신 변경된 필드만 업데이트하려고 시도합니다. 이것은 업데이트 작업의 효율성을 높일 수 있습니다. -
최적화된 쿼리 생성:
@DynamicUpdate
를 사용하면 Hibernate는 변경된 속성에 대한 업데이트 쿼리만 생성하므로 불필요한 데이터베이스 작업을 최소화할 수 있습니다. 이것은 네트워크 대역폭과 데이터베이스 부하를 감소시킬 수 있습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분이 아마 없어도 선 조회 후 엔티티 속성값 일부에 대해 바인딩 한 뒤 스냅샷과 다르게 수정 된 상태일때 save하면 영속성 컨텍스트가 부분 업데이트만 해주긴 할텐데 혹시 몰라서 넣어놨어요
|
||
MemberDto memberInfo = memberService.login(loginDto); | ||
|
||
return new ResponseEntity<>(memberInfo, HttpStatus.ACCEPTED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
202(Accepted)로 응답하는 이유가 궁금한데 간단히 설명해주실 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분은 제가 잘못 알고있었던게 접속 허가까진 맞는데 그 이후의 대한 반응은 없는거였더라구요 보통은 어떤 경우든 responce까지 성공하면 200이 맞다고 해서.. 수정해놓겠습니다
return new ResponseEntity(null,HttpStatus.CREATED); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity 반환시 빌더가 아니라 이 코드처럼 생성자 반환으로 통일하면 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 저랑 현준님 같은 경우는 저렇게 쓴걸로 기억합니다
protected Member getNewJoinMemberEntity(MemberDto requestDto){ | ||
|
||
String encodePassword = convertSHA256.convertToSHA256(requestDto.password()); | ||
|
||
return Member.createJoinMember(requestDto.id(), requestDto.email(), encodePassword); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConvertSHA256을 컴포넌트 빈으로 등록해서 주입받도록 바꾸신 점이 좋아보입니다. 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🥇
@@ -5,4 +5,6 @@ | |||
import org.springframework.data.jpa.repository.JpaRepository; | |||
|
|||
public interface MemberRepository extends JpaRepository <Member, Id> { | |||
|
|||
Member findAllByIdAndPassword(String id, String password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Member 객체 하나를 반환한다면 findAll 키워드를 사용하지 않는 것이 좋다고 생각합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩터링때 포함해서 수정하겠습니다 !
💡 Motivation and Context
여기에 왜 이 PR이 필요했는지, PR을 통해 무엇이 바뀌는지에 대해서 설명해 주세요
🔨 Modified
현준님 PR 받은 후 충돌 발생 된 코드들에 대해 다시 merge 처리 하였습니다.
로그인시 가입했던 ID / PW 확인 후 JWT 토큰 발급처리합니다.
토큰은 1가지만 발급하며, 발급 된 토큰은 한번 더 단방향 암호화 하여 인증 테이블에 저장됩니다. 토큰의 위변조가 발생시 해싱값이 불일치하게 되므로 토큰 갱신이 불가, 로그인으로 튕길 예정입니다. (이 기능이 유효성검사에 들어가야함.)
키 생성 암호화는 Keys.secretKeyFor(io.jsonwebtoken.SignatureAlgorithm.HS256)를 이용하였습니다.
토큰의 유효시간은 15분으로 설정하였습니다.
변동사항은 없으나 GPT 코드리뷰 체험해보고자 재업했습니다.
🌟 More
여기에 PR 이후 추가로 해야 할 일에 대해서 설명해 주세요
인증테이블 생성 SQL flyway update
회원 테이블의 대한 암호화 및 VIEW 복호화
회원가입시 ID 및 email 중복 조회 체크 API
globalFilter를 적용하여 사용자가 보유중인 JWT토큰 만료 2분전, 토큰 유효성 검사 로직이 필요합니다.
로그아웃, ID찾기, PW 재설정 API
테스트코드가 좀 맘에 들지않아 추후 리팩터링이 필요해보입니다.
📋 커밋 전 체크리스트
🤟🏻 PR로 완료된 이슈
closes #