feat: 이전 기록 불러오기 API 구현 #374 #379
Conversation
Test Results 23 files 23 suites 5s ⏱️ Results for commit e35b81b. ♻️ This comment has been updated with latest results. |
🌻Test Coverage Report
|
…oowacourse-teams/2024-staccato into feature/#374-call-past-records
| on: | ||
| push: | ||
| pull_request: | ||
| branches: [ "develop-be", "develop" ] |
There was a problem hiding this comment.
네, flyway 테스트 중이라서 우선은 트리거를 열어놓을 예정입니다.
| if (memberRepository.existsByNickname(member.getNickname())) { | ||
| return memberRepository.findByNickname(member.getNickname()); | ||
| } | ||
| validateNickname(member.getNickname()); |
There was a problem hiding this comment.
닉네임이 아닌 UUID로 유저를 판별하는 서비스는, 설령 다른 유저와 교류가 있더라도 중복 닉네임을 허용하는 경우도 있습니다.
- 중복 닉네임 허용
- 장점: 내가 원하는 닉네임을 입력하지 못해 서비스 관심도가 떨어지는 상황을 막을 수 있다. 닉네임 중복 검사를 하지 않아 서버 부하가 줄어든다.
- 단점: 채팅 기능이 있는 경우, 사용자 간 혼란을 초래한다. 악의적인 사칭이 가능하다.
- 중복 닉네임 제한
- 장점: 유저 식별이 쉬워진다.
- 단점: (중복 닉네임 허용의 장점을 챙길 수 없음)
저희 서비스에 나중에 사용자간 교류 기능을 넣을 경우, 중복 닉네임이 허용되면 채팅 기능에서 사용자간 혼란을 초래할 수 있어 허용하지 않는 것이 좋을 것 같습니다. 리니는 어떻게 생각하시나요?
There was a problem hiding this comment.
저희는 UUID가 아닌, 닉네임으로 사용자를 판별합니다.
UUID는 사용자가 이전 기록을 불러오기 위해 제공해주는 코드로 사용하고자 추가된 값이고, 사용자가 본인을 판단할 때 사용하는 것은 닉네임이라고 생각합니다.
그렇기 때문에, 사용자가 중복 닉네임을 사용할 수 없도록 하는 것이 좋다고 생각해요.
후에, 함께하는 사람 기능이 추가된다면, UUID로 사용자를 찾는 것이 더 불편할 것 같다고 생각합니다
| public static Member create(String nickname) { | ||
| return Member.builder() | ||
| .nickname(nickname) | ||
| .code(UUID.randomUUID().toString()) | ||
| .build(); |
There was a problem hiding this comment.
UUID 생성(비즈니스 로직)과 멤버 객체 생성의 책임을 분리할 수 있을 것 같습니다.
장점 - 책임 분리
- UUID 생성 방식이 변경되더라도(ex. 중복 UUID 생성 방지를 위해 UUID 생성을 여러 번 시도) Member 클래스에는 영향을 주지 않는다.
Member.create()메서드를 테스트할 때 UUID를 실제로 생성하지 않고 주입해서, 일관된 입력에 따른 결과를 테스트할 수 있다.
public class AuthService {
private Member createMember(LoginRequest loginRequest) {
String uuid = UUID.randomUUID().toString();
Member member = loginRequest.toMember(uuid);
validateNickname(member.getNickname());
return memberRepository.save(member);
}
}public record LoginRequest() {
public Member toMember(String uuid) {
return Member.create(nickname, uuid);
}
}public class Member {
public static Member create(String nickname, String uuid) {
return Member.builder()
.nickname(nickname)
.code(uuid)
.build();
}
}There was a problem hiding this comment.
UUID가 중복될 가능성이 매우 낮지만 0은 아니므로, 아래와 같은 로직을 사용해서 5번 정도 시도해서 최대한 중복되지 않게 만드는 방법도 좋을 것 같아요!
public class AuthService {
private static final int MAX_ATTEMPTS = 5;
private Member createMember(LoginRequest loginRequest) {
String uuid = generateUniqueUuid();
Member member = loginRequest.toMember(uuid);
validateNickname(member.getNickname());
return memberRepository.save(member);
}
private String generateUniqueUuid() {
for (int attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
String uuid = UUID.randomUUID().toString();
if (!memberRepository.existsByCode(uuid)) {
return uuid;
}
}
throw new StaccatoException("회원 가입 시 문제가 발생했습니다. 잠시 후 다시 시도해 주세요.");
}
}public interface MemberRepository extends JpaRepository<Member, Long> {
boolean existsByCode(String code);
}There was a problem hiding this comment.
create()라는 정적 메서드는 멤버를 새로 생성한다는 의미에서 만든 메서드입니다.
code는 사용자가 처음 생성되는 순간에 발급될 코드입니다.
따라서, 해당 코드가 어떻게 생성될지를 결정하는 주체는 Member라고 생각합니다.
제가 느끼기에는 어떻게 코드를 발급할 것인지를 외부에서 결정하게 되면 코드가 분산되어있게 되고, 변경 지점이 많아진다고 생각하는데 폭포는 어떻게 생각하시나요?
There was a problem hiding this comment.
매 초 10억개의 uuid를 100년에 걸쳐서 생성할 때 단 하나의 uuid가 중복될 확률은 50%이다.
UUID가 중복될 가능성은 정말 희박한데요,
이 가능성을 고려하기 위해 User 테이블의 모든 레코드를 5번씩(최소 1번) 순회하는 것이 꼭 필요할까요?🤔
There was a problem hiding this comment.
UUID가 중복될 가능성은 정말 희박한데요,
이 가능성을 고려하기 위해 User 테이블의 모든 레코드를 5번씩(최소 1번) 순회하는 것이 꼭 필요할까요?🤔
생각해보니 매우 희박한 확률을 방지하기 위해 비용이 큰 쿼리를 매번 사용하는 것은 오버헤드가 너무 큰 것 같네요. 차라리 UUID를 만들 때 시스템 고유값을 섞어서 중복을 줄이는 방법이 훨씬 나은 것 같습니다. 다만, 리니가 말씀해주신대로 중복 가능성이 정말 희박하기 때문에 굳이 아래처럼 고칠 필요는 없다고 생각합니다.
public String generateTimeBasedUUID(String name) {
return UUID.nameUUIDFromBytes((name + System.currentTimeMillis()).getBytes()).toString();
}There was a problem hiding this comment.
create()라는 정적 메서드는 멤버를 새로 생성한다는 의미에서 만든 메서드입니다. code는 사용자가 처음 생성되는 순간에 발급될 코드입니다. 따라서, 해당 코드가 어떻게 생성될지를 결정하는 주체는
Member라고 생각합니다. 제가 느끼기에는 어떻게 코드를 발급할 것인지를 외부에서 결정하게 되면 코드가 분산되어있게 되고, 변경 지점이 많아진다고 생각하는데 폭포는 어떻게 생각하시나요?
처음에 Member가 본인의 필드를 직접 생성해서 대입하고 있어 어색하게 느꼈습니다. 하지만 리니 의견을 듣고 다시 생각해서, 초기값을 설정해준다는 느낌으로 코드를 조금 바꿔보니 그렇게 어색하게 느껴지지 않게 되었습니다. 저도 평소에 아래와 같은 코드를 많이 쓰는 것 같구요.
public Member(String nickname, String imageUrl) {
this.nickname = new Nickname(nickname);
this.imageUrl = imageUrl;
this.code = UUID.randomUUID().toString();
}이렇게 관점을 바꿔 생각해보니, UUID 생성 로직이 변경되더라도 그냥 Member 객체가 해당 UUID 생성 메서드를 private으로 갖고 있으면 될 것 같다는 생각이 들었습니다. 좋은 의견 감사합니다.
There was a problem hiding this comment.
생각해보니 매우 희박한 확률을 방지하기 위해 비용이 큰 쿼리를 매번 사용하는 것은 오버헤드가 너무 큰 것 같네요. 차라리 UUID를 만들 때 시스템 고유값을 섞어서 중복을 줄이는 방법이 훨씬 나은 것 같습니다. 다만, 리니가 말씀해주신대로 중복 가능성이 정말 희박하기 때문에 굳이 아래처럼 고칠 필요는 없다고 생각합니다.
이 부분은 적용하려면 현재 DB 필드의 길이 제한을 수정해야될 것 같아요! 팀원들 다같이 한 번 의논해보고 적용해보죠!
| flyway: | ||
| enabled: true | ||
| locations: classpath:db/migration | ||
| baseline-on-migrate: true |
There was a problem hiding this comment.
baseline-on-migrate
baseline-on-migrate 옵션은 데이터베이스가 이미 존재하지만 Flyway로 관리되지 않은 기존 데이터베이스에서 마이그레이션을 시작할 때 사용된다.
- 옵션 값: true
- Flyway는 데이터베이스에 flyway_schema_history 테이블이 없거나, 초기 상태의 데이터베이스에서 마이그레이션을 시도할 때, 자동으로 baseline을 수행한다.
- 지정된 baselineVersion (기본값은 1)을 기준으로 데이터베이스의 현재 상태를 초기화한다. 즉, 해당 버전을 기준으로 이후의 마이그레이션 파일들을 적용한다.
- 옵션 값: false
- Flyway는 flyway_schema_history 테이블이 없는 데이터베이스에서 마이그레이션을 시도하면 에러가 발생한다. 데이터베이스가 초기화되어 있지 않다고 판단하기 때문이다.
| SET code = UUID(); | ||
|
|
||
| ALTER TABLE member | ||
| MODIFY COLUMN code VARCHAR(36) NOT NULL UNIQUE; |
There was a problem hiding this comment.
제가 고친 부분인데 endline 추가를 깜빡했네요. 😓 혹시 한 번에 해주실 수 있나요?
| @@ -22,4 +26,10 @@ public ResponseEntity<LoginResponse> login(@Valid @RequestBody LoginRequest logi | |||
| LoginResponse loginResponse = authService.login(loginRequest); | |||
| return ResponseEntity.ok(loginResponse); | |||
| } | |||
|
|
|||
| @GetMapping("/members") | |||
There was a problem hiding this comment.
login 을 한다는 행위를 통해 토큰을 발급한다는 의미에서 POST 는 어떨까요?
| <include resource="console-appender.xml"/> | ||
| <root level="info"> | ||
| <appender-ref ref="STDOUT"/> | ||
| </root> |
There was a problem hiding this comment.
Dev 환경에서 기록되는 로그들을 모니터링 도구를 통해 확인하는 것과는 별개로
추가적으로 콘솔에도 로그가 남게하신 이유가 있을까요?
There was a problem hiding this comment.
flyway를 적용하는 과정에서 잘못된 방법으로 db 변경 시도 시 애플리케이션이 실행되는 시점에 에러가 발생해서 다운되었습니다. 해당 시점까지는 어떤 문제가 있었는지 확인하기 위해서는 콘솔 로그가 필요했어요!
(비슷한 예시로, 지난번에도 validate ddl 옵션으로 테이블이 엔티티 구조와 맞지 않아서 애플리케이션 실행에 실패한 적이 있었죠. 그 당시에도 콘솔 로그가 찍히지 않아서 에러 핸들링에 꽤나 시간이 걸렸던게 기억나네요😭)
| private Member getMemberByCode(String code) { | ||
| return memberRepository.findByCode(code) | ||
| .orElseThrow(() -> new StaccatoException("유효하지 않은 코드입니다. 올바른 코드인지 확인해주세요.")); |
There was a problem hiding this comment.
findByCode 때문에 code에 인덱스를 걸어야할 것 같기도 하면서도, 위 API가 엄청 자주 호출되는 게 아니니까 인덱스를 안 거는게 나을 것 같기도 하네요 🤔
| flyway: | ||
| enabled: false |
There was a problem hiding this comment.
prod환경은 false로 설정한 이유가 있나요?!
There was a problem hiding this comment.
운영 서버에서 flyway를 사용하는 것은 아직 더 고민해볼 필요가 있는 것 같아서 우선은 비활성화했습니다. 같이 의논해보면 좋을 것 같아요🙌
linirini
force-pushed
the
feature/#374-call-past-records
branch
from
7d57b4a to
2a3a18f
Compare
⭐️ Issue Number
🚩 Summary
code필드 추가🛠️ Technical Concerns
🙂 To Reviewer
📋 To Do