Conversation
…코드에 누락된 DB의 데이터 수정하는 코드 추가
|
@dohy-eon 도현님 이번에 피드백하면서 노션에 정리되었던 폴더 구조를 확인해 보았는데 계층형 구조로 되어있는것 같은데 도메인 단위로 관련 소스가 한눈에 모여 있어서 신규 기능 개발 작업할때나 가독성 측면에서 더 효율적일것 같습니다. 근데 이번에 스프링 처음한 팀원들이 많아서 이렇게 설계하신거라면.. 괜찮을것 같습니다. 그냥 걱정이되서 한번 여쭤봤습니당.. |
|
@dohy-eon 기존 구조처럼 DTO를 한 군데 몰아서 관리하면 Controller용 DTO와 Service/Domain용 DTO가 섞일 가능성이 높고 이렇게 되면 도메인이 커질수록 관리가 어려워지고 실수도 더 많아질 것 같습니다. 클린 아키텍처 관점에서도 계층별, 목적별로 DTO를 분리하면 각 역할에 집중할 수 있고 추가, 변경, 확장에도 훨씬 유리하다고 생각합니다. 제가 생각한건 이런구조가 베스트라 생각하긴 했습니다. 진짜 완전히 같으면 공통 DTO로 합쳐도 되지만 각 레이어가 필요하면 각자 dto 만들어서 쓰는 게 맞다라고 생각이 들긴합니다. 스프링을 이번에 처음 하는 팀원이 많아서 현재 구조도 충분히 괜찮다고 생각합니다. 근데 나중에를 고려해서 한번 참고해주시면 좋을 것 같습니다. |
|
@junjinyun @dohy-eon 코드상에 api 문서화 안되어있는데 팀내에서 swagger나 REST Docs중 어떤거 도입하기로 결정이 됐나여? |
확인했습니다! 저도 개발 초기에 controller 와 service dto는 나누는게 맞지 않을까라는 고민을 많이 했는데요. 현재 dto와 entity 개념도 확실히 잡혀있는 상태가 아니라고 생각되어 통합 dto 폴더로 관리를 진행한 뒤 추후 (중간 발표 이후) 3학년 한분과 함께 리팩토링 및 고도화 작업을 진행하려고 했습니다. 코멘트 감사합니다! |
ㅜㅜ 여담이지만 도메인 별 구조로 짜여있습니다,,, |
|
@kjunh972 위에 작성드린대로 패키지는 도메인 기준이고, 그 안엔 레이어 별로 분리하도록 작업 할 예정입니다. 확인 후 코멘트 남겨주시면 감사드리겠습니다. |
Swagger 관련해서 개인별로 스터디 진행하는 중입니다. REST Docs보단 훨씬 러닝커브가 낮다고 생각되어(테스트 코드도 안짜도 되구요...) 스웨거로 진행 할 예정입니다. |
dohy-eon
left a comment
There was a problem hiding this comment.
리뷰는 일단 간단하게 남겼습니다! 오늘 오후 10시 회의 전까지 한번 쭉 '읽어보시고' 너무 어려운 부분들은 검색해서 오시면 좋을 것 같습니다. 위에 동민이형이 리뷰 다신 것도 한번 생각해오시고,
코멘트 세세하게 단 거 제외하고 전체적으로는 일단
- 도메인 폴더 아래에 User 폴더 만들어서 그 아래에 Controller, dto, entity, repo, service)넣기
- 컨트롤러마다 try-catch 반복하지 말고 @RestControllerAdvice 이 어노테이션 사용해서 전역으로 관리하기 (이건 아래 참고 링크 두개 남겨놓을게요, 시간 나시면 쭉 읽어보고 이해해오시면 될 것 같습니다.)
정도입니다. 수고하셨고 10시 회의 때 뵙겠습니다!
RestControllerAdvice로 전역에서 발생된 예외 처리하기.
Enum 타입으로 에러 메시지 및 응답 관리하기
| String token = authHeader.startsWith("Bearer ") ? authHeader.substring(7) : authHeader; | ||
| String username = JwtUtil.extractUsername(token); | ||
|
|
||
| User updatedUser = userService.updateUserInfoByMap(username, updates); |
There was a problem hiding this comment.
1-1. UserController에서 username 추출해서 updateUserInfoByMap()으로 보내는데 서비스 레이어에서 왜 String email로 받고 있고 현재 이 코드가 어떤 플로우로 동작하는지가 의문입니다... 일단 실수인 경우에 해결안으로는 jwt 발급할 때 subject에 username 말고 email 넣으면 해결될 문제라고 생각되는데 어떻게 테스트 코드가 돌아가는지를 모르겠네요😥
There was a problem hiding this comment.
- 현재 서비스 계층에서 UserRegisterRequest → User 변환을 직접 하고 있는데요, 변환 책임은 DTO에서 진행하는게 유지보수 측면에서 차이가 나기 때문에 DTO에
public User toEntity(String encodedPassword) {
return new User(username, email, encodedPassword);
}
이런 식으로 메서드를 추가하는게 좋아보입니다.
| } | ||
|
|
||
| @Transactional | ||
| public User updateUserInfoByMap(String email, Map<String, Object> updates) { |
아하 넵 이해했습니다. 노션은 users를 제가 못봤네여.. 😢 |
| @Data | ||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| public class LoginResponse { |
There was a problem hiding this comment.
현재 작성하신 dto를 확인해보았는데
현재 LoginRequest, LoginResponse를 별도 파일로 작업해주셨는데 해당 DTO는 사실상 하나의 Login에서만 사용되는 요청/응답 구조입니다. 다른 dto 파일도 다 별도 파일로 만들어주신것 같습니다.
내용이 많지도 않고 이런 경우에는 이너 클래스 로 하나의 파일에 묶는 것이 더 적절한것 같습니다.
구조적으로 나중에 API가 많아지면 domain/dto 패키지에 파일이 너무 많아지는 문제가 생기므로 요청/응답 DTO는 되도록 관련 기능 기준으로 이너클래스로 작업을 해주시는게 나을것 같습니다.
public class LoginDto {
@Data
@NoArgsConstructor
@AllArgsConstructor
public static class Request {
private String email;
private String password;
}
@Data
@NoArgsConstructor
@AllArgsConstructor
public static class Response {
private String jwt;
}
}LoginResponse의 경우 필드명은 jwt는 accessToken을 응답하는 건가요? 언제 뭘 응답하는지 네이밍이 애매한것 같습니다. accessToken이랑 refreshToken 기간산정도 고려도 되어있는 건가요?
| Optional<User> findByUsername(String username); | ||
| boolean existsByEmail(String email); | ||
| boolean existsByUsername(String username); | ||
| boolean existsByEmailAndPassword(String email, String password); |
There was a problem hiding this comment.
UserRepository에 정의된 existsByEmailAndPassword는 암호화된 비밀번호를 DB에서 직접 조회하는 방식이기 때문에 보안상 잘못된 설계입니다.
그리고 existsByEmailAndPassword 사용되지 않고 있는것 같은데 사용하지 않는 코드는 제거해주세요.
| public User login(String email, String rawPassword) { | ||
| return repository.findByEmail(email) | ||
| .filter(user -> passwordEncoder.matches(rawPassword, user.getPassword())) | ||
| .orElse(null); |
There was a problem hiding this comment.
현재는 로그인할때 유저정보가 없으면 null을 반환하고 있는데 보통 인증 로직에서는 명시적 예외를 던지는 것이 일반적입니다.
@Transactional(readOnly = true)
public User login(String email, String rawPassword) {
User user = repository.findByEmail(email)
.orElseThrow(() -> new IllegalArgumentException("이메일이 존재하지 않습니다."));
if (!passwordEncoder.matches(rawPassword, user.getPassword())) {
throw new IllegalArgumentException("비밀번호가 일치하지 않습니다.");
}
return user;
}서비스 레이어에서는 실패를 예외로 처리하고 컨트롤러에서 예외를 받아 HTTP 응답 코드로 매핑하는 것이 맞는 설계 같습니다.
| import java.util.Date; | ||
|
|
||
| public class JwtUtil { | ||
| private static final Key key = Keys.secretKeyFor(SignatureAlgorithm.HS256); |
There was a problem hiding this comment.
현재 JwtUtil에서는 아래와 같은 방식으로 키를 생성하고 있습니다.
private static final Key key = Keys.secretKeyFor(SignatureAlgorithm.HS256);이 방식은 서버 실행 시마다 랜덤한 키를 동적으로 생성하는 방식인것 같습니다.
서버를 재시작하면 이전에 발급한 JWT를 검증할 수 없습니다. 기존 토큰의 서명 검증이 실패해서 사용자 인증이 무조건 실패하게 됩니다. JWT는 발급과 검증에 동일한 Secret Key를 사용해야 합니다.
따라서 Secret Key를 외부 설정으로 고정해서 관리해주세요.
Secret Key는 절대 application.yml 코드에 하드코딩하지 말고 반드시 .env 파일에 작성 후 불러오는 방식으로 안전한 외부 설정으로 관리해주세요.
JWT_SECRET=your-secret-keyapplication.yml에서 환경변수로부터 값을 읽어서 사용해주세요
jwt:
secret: ${JWT_SECRET}그 후 코드에선 예를 들면 이런식으로 둘 중 하나로 값을 주입해 사용해서 작업 해주세요.
@ConfigurationProperties(prefix = "jwt") // 또는
@Value("${jwt.secret}") 또한 JWT 인증 설계 시에는 Access Token과 Refresh Token을 분리해서 각각의 만료 시간, 사용 목적에 맞게 발급, 관리하는 것이 보안상 안전합니다.
- Access Token: 짧은 만료시간(예: 1시간~3시간)으로 설정해 사용자 인증용으로만 사용
- Refresh Token: 더 긴 만료시간(예: 2주~한 달)으로 설정해 Access Token 재발급용으로 사용
각 토큰의 만료기간을 한번 고민해보고 고려하여 application.yml에 별도의 설정값으로 분리해서 관리해 주세요.
jwt:
secret: ${JWT_SECRET}
access-token: 3600 # 초 단위 (예: 1시간)
refresh-token: 1209600 # 초 단위 (예: 2주)| // then | ||
| User foundUser = userRepository.findByUsername(TEST_USERNAME).orElseThrow(); | ||
|
|
||
| System.out.println("서비스 반환 User 객체:"); |
There was a problem hiding this comment.
System.out.println() 기반의 테스트 출력이 너무 많습니다.
테스트 코드에서는 가급적 불필요한 출력문은 줄여주시고 검증(assert 등) 코드 위주로 작성해 주세요.
디버깅이나 로그가 꼭 필요한 경우에는 System.out.println() 대신 Logger(sl4fj 등)를 사용해 주세요.
…능을 DTO의 UserRegisterRequest 에 위임
…tControllerAdvice 와 enum을 이용해서 관리
…해 최소한의 데이터만 출력하게 변경
… 구조변경 aec9782 에서 작성한 estControllerAdvice + enum 의 구조를 전역적으로 리팩토링 및 이에따른 테스트 코드 및 컨트롤러, 서비스 등에 반영 CommonResponse 의 첫번째 필드를 error(오류 여부) 가 아닌 success(실행 성공 여부) 로 수정

[feat] 회원가입, 로그인, 유저정보 수정 기능 구현
😺 Issue
✅ 작업 리스트
⚙️ 작업 내용
📷 테스트 / 구현 내용