Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Expand Up @@ -16,7 +16,7 @@ public interface AuthController {
ResponseEntity<String> signIn(SignInFormRequest request, HttpServletRequest httpRequest,
HttpServletResponse response);
@Operation(summary = "회원 로그아웃")
ResponseEntity<Void> logout(HttpServletRequest request);
ResponseEntity<Void> logout(User user,HttpServletRequest request);
@Operation(summary = "로그인 상태 체크")
ResponseEntity<String> checkLogin(User user);
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,13 @@ public ResponseEntity<String> signIn(@RequestBody @Valid SignInFormRequest reque
}

@GetMapping("/logout")
public ResponseEntity<Void> logout(HttpServletRequest request) {
public ResponseEntity<Void> logout(@CurrentUser User user, HttpServletRequest request) {
HttpSession session = request.getSession(false);
if (session != null) {
// redis의 세션도 삭제
session.invalidate();
}
service.logout(user);
return ResponseEntity.ok().build();
Comment on lines +48 to 55
Copy link
Contributor

Choose a reason for hiding this comment

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

회원 정보 수정 기능을 개선하는데, 로그아웃 요청 시에 HttpServletRequest가 아닌 User로 수정하는 것과 어떤 연관이 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

기존 로그아웃은 해당 회원의 세션만 삭제해서 로그인 여부를 판단했습니다 하지만 CurrentUser를 이용해 redis에 사용자를 캐싱하게되므로써 캐싱된 회원 객체도 삭제시켜줘야한다는 필요성이 생겼습니다 그래서 서비스 로직에서 redis에 저장된 해당 회원객체를 삭제해야했습니다!
추후 인가부분의 재정리가 필요해 정리할 예정입니다!

}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package com.jishop.member.dto.request;

import com.jishop.member.domain.User;

public record UserAdEmailRequest(
boolean adEmailAgree
) {
public void update(User user){
user.updateAdEmailAgree(adEmailAgree);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package com.jishop.member.dto.request;

import com.jishop.member.domain.User;

public record UserAdSMSRequest(
boolean adSMSAgree
) {
public void update(User user){
user.updateAdSMSAgree(adSMSAgree);
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package com.jishop.member.dto.request;

import com.jishop.member.domain.User;
import jakarta.validation.constraints.NotBlank;

public record UserNameRequest(
@NotBlank
String name
) {
public void update(User user){
user.updateName(name);
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package com.jishop.member.dto.request;

import com.jishop.member.domain.User;
import jakarta.validation.constraints.NotBlank;

public record UserPhoneRequest(
@NotBlank
String phone
) {
public void update(User user){
user.updatePhone(phone);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.jishop.member.repository;

import com.jishop.common.exception.DomainException;
import com.jishop.common.exception.ErrorType;
import com.jishop.member.domain.LoginType;
import com.jishop.member.domain.User;
import org.springframework.data.jpa.repository.JpaRepository;
Expand All @@ -15,4 +17,8 @@ public interface UserRepository extends JpaRepository<User, Long> {
Optional<User> findByPhone(String phone);

boolean existsByLoginId(String loginId);

default User findPersistUser(User user) {
return findById(user.getId()).orElseThrow(() -> new DomainException(ErrorType.USER_NOT_FOUND));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ public interface AuthService {
Long checkLogin(User user);
void updateAdSMSAgree(User user, UserAdSMSRequest request);
void updateAdEmailAgree(User user, UserAdEmailRequest request);
void logout(User user);
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.springframework.transaction.annotation.Transactional;

import java.time.Duration;
import java.util.function.Consumer;

@Service
@Transactional
Expand Down Expand Up @@ -67,13 +68,15 @@ public boolean checkPW(RecoveryPWRequest request){
}

public void updatePW(User user, UserNewPasswordRequest request){
Copy link
Contributor

Choose a reason for hiding this comment

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

아래에 사용자 정보 업데이트 메서드들이 동일한 패턴을 반복하고 있는데 패턴을 추상화해서 중복을 제거하는것 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

업데이트에 대한 각각의 구현체로 만들라는 말씀이실까요오??

Copy link
Contributor

Choose a reason for hiding this comment

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

밑의 코드들이

  • getPersistUser(user) 호출
  • 유저 정보 업데이트
  • updateUserCache(persistUser) 호출
    의 정형화된 패턴을 띄고 있는데 이것들을 하나로 묶어서

public void update메서드 (User user, UserRequest request) {
업데이트 실행 메서드 (user, persistUser -> persistUser.update (request.업데이트할 것()));
}
이런식으로 할 수 있지 않을까 해서요.

Copy link
Contributor

Choose a reason for hiding this comment

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

진호님 말 처럼 개별적으로 수정이 들어가는데
전화 번호, 이름, 비밀번호, 이메일 동의 이렇게 4개를 한번에 바꾸면 4개 메소드가 불려오는 방식인건가요 controller 4번 호출 같이 되는 건가요?

개별적으로 로직 처리가 있어서 개별적인 메소드 처리라면 방법이 떠오르진 않지만
통합해서 방법을 생각해봐야 할 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

지금 현재 피그마 상으로 각각 개별적으로 수정이 이루어져 해당 수정시마다 개별적인 컨트롤러가 호출 되고 있습니다
그래서 개별적인 서비스로직이 존재하게 되었습니다,,

중복되는 코드에 대해선 개선하고 알려드리겠습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

개선 완료 했습니다!

if(passwordEncoder.matches(request.password(), user.getPassword())){
User persistUser = getPersistUser(user);

if(passwordEncoder.matches(request.password(), persistUser.getPassword())){
throw new DomainException(ErrorType.PASSWORD_EXISTS);
}

String password = passwordEncoder.encode(request.password());
user.updatePassword(password);
updateUserCache(user);
persistUser.updatePassword(password);
updateUserCache(persistUser);
}

// todo: 회원 정보 조회
Expand All @@ -83,38 +86,53 @@ public UserResponse getUser(User user){

// todo: 회원 정보 수정 (이름, 전화번호)
public void updateUserName(User user, UserNameRequest request) {
user.updateName(request.name());
updateUserCache(user);
applyAndCache(user, request::update);
}

public void updatePhone(User user, UserPhoneRequest request) {
user.updatePhone(request.phone());
updateUserCache(user);
applyAndCache(user, request::update);
}

public void deleteUser(User user) {
user.delete();
User persistUser = getPersistUser(user);
persistUser.delete();
}

public Long checkLogin(User user) {
return user.getId();
}

public void updateAdSMSAgree(User user, UserAdSMSRequest request){
user.updateAdSMSAgree(request.adSMSAgree());
updateUserCache(user);
applyAndCache(user, request::update);
}

public void updateAdEmailAgree(User user, UserAdEmailRequest request){
user.updateAdEmailAgree(request.adEmailAgree());
updateUserCache(user);
applyAndCache(user, request::update);
}

private void updateUserCache(User user) {
String cacheKey = "user::" + user.getId();

// 캐시 업데이트 (기존 캐시 삭제 후 최신 정보로 재설정)
redisTemplate.delete(cacheKey);
redisTemplate.opsForValue().set(cacheKey, user, Duration.ofMinutes(30));
}

private User getPersistUser(User user){
Copy link
Contributor

Choose a reason for hiding this comment

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

이 메소드를 userRepository 쪽으로 옮기면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 메서드를 userRepository에 옮긴다는게 무슨 말인지 잘 모르겠습니다...
영속적인 객체를 사용하기 위해 findbyId로 찾았고 모든 업데이트 메서드에 존재하고 없는 회원에 대한 예외때문에 따로 메서드로 빼서 사용중이었습니다

구체적인 예시를 알려주실수 있으실까요,,?🥲

Copy link
Contributor

Choose a reason for hiding this comment

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

userRepository에 default 메소드로 빼면 좋을 것같아요

Copy link
Contributor Author

@JangBJ JangBJ Apr 16, 2025

Choose a reason for hiding this comment

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

별 차이가 없다고 느껴집니다
혹시 서비스 로직에서 사용되는 것과 repository에서 default 메서드로 선언되는 것이랑 무슨 차이가 있을까요??
repository에서는 데이터에 접근 할뿐 이에 대한 예외처리는 어짜피 서비스 로직에서 해야한다고 생각됩니다!

왜 default 메서드로 빼면 좋을지! 설명 부탁드립니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

다른 곳에서도 유저를 사용해서 레포지토리에 있다면 반복 코드를 쓰지 않을 것 같아서 입니다.
누군가는 elseThrow를 할거니 그것의 책임을 아예 repository로 가져가는거라고 생각했습니다.
기존에 findById는 optional로 감싸서 만들었기때문에 이부분을 interface 부분에서 처리를 하는 거죠!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 부분은 @currentuser로 인해 다른 도메인의 서비스 로직에서 User를 직접 조회해서 사용하는 경우는 없다고 생각했습니다!
근데 다른 도메인의 코드를 살펴보니 review 서비스 175번줄에서 userRepository의 findById를 사용한다는 것을 인지하게 되었습니다!
만약 해당 default 메서드를 생성한다면 review 서비스 로직에서 사용하실 의향이 있으신가요오??

Copy link
Contributor

Choose a reason for hiding this comment

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

사용할 것 같습니다.! 추후에 내 자신이 아닌 다른 사용자를 확인 할때 필요할 수 도 있을 것 같습니다.

User persistUser = userRepository.findPersistUser(user);

return persistUser;
}

private void applyAndCache(User user, Consumer<User> update) {
User persistUser = getPersistUser(user);
update.accept(persistUser);
updateUserCache(persistUser);
}

public void logout(User user) {
String cacheKey = "user::" + user.getId();
redisTemplate.delete(cacheKey);
}
}