[4기 유원우] Shorten URL 과제 PR입니다.#51
[4기 유원우] Shorten URL 과제 PR입니다.#51wonu606 wants to merge 31 commits intoprgrms-be-devcourse:wonu606from
Conversation
- SpringBoot 프로젝트 생성 - .gitignore 설정
- 테스트를 통과하지만 서비스 추가시 실패 예정 - 서비스 동작을 통해 성공하도록 구현
- request에서 간략한 검증하도록 수정 - response 반환시 json으로만 받을 수 있게 강제
- DefaultUrlShortenerService에 UrlLink를 생성하는 로직 구현 - UrlLink 도메인은 원본 Url과 Hash 값을 저장
- 잘못된 id 값이 들어올 경우 Exception 발생하도록 수정
- Hash 값을 가진다는 것을 명확하게 표현하기 위해 이름 변경
- Url에 대한 Hash를 생성하고 Repository를 통해 존재 여부를 확인하여 유일성 보장
- yaml 형식으로 작성된 내용을 properties 형식으로 수정
- `LocalUrlLinkRepository` 클래스를 통해 `UrlLink` 객체를 Local Memory에서 저장하고 관리 - `UrlLink` 객체를 관리하는 클래스명을 명확하게 표현하도록 수정
- UrlShortenerApiController와 UrlShortenerService에서 수행하는 작업이 유사하나, 함수명이 다르므로 동일한 작업을 수행하는 것처럼 보이지 않았음 두 클래스에서의 작업이 유사함을 명확히 나타내기 위해 create 함수명을 통일함
- URL 단축 생성 로직에서, 이미 단축된 URL이 존재하는 경우 해당 URL을 반환하도록 로직 수정 - 이 변경으로 인해 불필요한 리소스 사용을 줄이고, 동일한 원본 URL에 대해 항상 동일한 단축 URL을 제공함
- 중복된 테스트 클래스 제거(UrlShortenerApiControllerMockTest) - 함수명 given_when_then 사용 - private으로 선언되지 않은 필드 private 선언
- get은 못 찾을 시 에러가 발생해야 한다고 생각하지만 현재 로직은 발생하지 않는다. find가 존재하지 않더라도 오류가 발생하지 않는다고 생각하여 변경
- ShortUrl을 받아 원본 URL로 리다이렉트하는 로직 구현 - 사용자가 ShortUrl을 접근할 때, 해당 ShortUrl에 매핑된 원본 URL로 브라우저가 리다이렉트됨
- `UrlLinkRepository`에서 `UrlHash`를 사용하여 `UrlLink`를 조회한 후, 원본 URL을 반환하도록 로직 수정(이전에는 `UrlHash` 값을 반환하였으나, 이제는 원본 Url 값을 반환하도록 변경)
|
|
||
| private static final UrlValidator URL_VALIDATOR = new UrlValidator(); | ||
| public static final int URL_MAX_LENGTH = 2_000; | ||
|
|
| public Url { | ||
| if (!URL_VALIDATOR.isValid(value)) { | ||
| throw new IllegalArgumentException("잘못된 URL 주소입니다. Current URL: " + value); | ||
| } | ||
|
|
||
| if (StringUtils.length(value) > URL_MAX_LENGTH) { | ||
| throw new IllegalArgumentException( | ||
| String.format("URL의 길이는 %d를 넘어갈 수 없습니다 Current Length: %d", URL_MAX_LENGTH, value.length())); | ||
| } | ||
| } |
There was a problem hiding this comment.
정적 팩토리 메소드로 URL_VALIDATOR를 주입받아 검증하면서 생성한다면 다양한 검증 시나리오 테스트에 용이해질것 같아요
| } | ||
| if (StringUtils.length(value) > MAX_LENGTH) { |
| throw new IllegalArgumentException("ID는 0보다 크고 null이 아니어야 합니다. Input ID: " + newId); | ||
| } | ||
| if (this.id != null) { | ||
| throw new IllegalStateException("ID가 이미 할당되어 있습니다. Current ID: " + newId); |
|
|
||
| private long generateNewId() { | ||
| return idGenerator.getAndIncrement(); | ||
| } | ||
|
|
There was a problem hiding this comment.
이 메소드는 LocalUrlLinkRepository의 책임에 맞지 않는것 같아요
idGenerator가 generateNewId를 해주면 안될까요?
AtomicLong란걸 알 필요는 없는것 같아요
| } | ||
| } | ||
|
|
||
| throw new UniqueHashCreationException("유니크한 URL 해시를 생성하는 데 실패했습니다."); |
There was a problem hiding this comment.
유니크한 URL 해시를 생성하는 데 실패했단거는 여기서는 사실 서버쪽의 잘못이라고 볼 수 있어요.
사용자의 잘못이 아니라면, 우리 서버가 잘못한 이부분을 사용자한테 보여줄 필요가 있을까 고민해보면 좋을것 같아요.
재시도 하면 성공할까요?
| import com.prgrms.wonu606.shorturl.domain.UrlHash; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| /** |
|
|
||
| @Test | ||
| void whenShortUrlExists_thenRedirect() throws Exception { |
| @Autowired | ||
| MockMvc mockMvc; | ||
|
|
||
| @Autowired | ||
| ObjectMapper objectMapper; | ||
|
|
There was a problem hiding this comment.
이 친구들은 왜 default로 하셨는지 궁금합니다.
통일성을 맞춰보면 어떨까여
| @Override | ||
| public Long save(UrlLink urlLink) { | ||
| validateUrlLink(urlLink); | ||
|
|
||
| Long newId = generateNewId(); | ||
|
|
||
| urlLink.initializeId(newId); | ||
|
|
||
| storage.put(newId, urlLink); | ||
|
|
||
| hashedUrlIndex.compute(urlLink.getUrlHash(), (key, existingUrlLink) -> { | ||
| if (existingUrlLink != null) { | ||
| throw new IllegalArgumentException("UrlHash가 이미 인덱싱 되어 있습니다. Current UrlHash: " + urlLink.getUrlHash()); | ||
| } | ||
| return urlLink; | ||
| }); | ||
|
|
||
| return newId; | ||
| } |
There was a problem hiding this comment.
UrlLink가 밖에서 생성 되어서 아쉽게도 initializeId()라는 메소드를 만들었네요 .
@Override
public UrlLink save(UrlLink urlLinkToSave) {
Long newId = generateNewId();
UrlLink newUrlLink = new UrlLink(newId, urlLinkToSave.getOriginalUrl(), urlLinkToSave.getUrlHash());
storage.put(newId, newUrlLink);
hashedUrlIndex.compute(newUrlLink.getUrlHash(), (key, existingUrlLink) -> {
if (existingUrlLink != null) {
throw new IllegalArgumentException("UrlHash가 이미 인덱싱 되어 있습니다. Current UrlHash: " + newUrlLink.getUrlHash());
}
return newUrlLink;
});
return newUrlLink;
}이렇게 하면 객체의 불변성을 유지하면서 필요한 정보를 캡슐화할 수 있어요
그러나 몇가지 주의해야해요
- 메모리 사용량 증가
- 추가적인 생성자 관리 방법
- 객체 동일성
WooSungHwan
left a comment
There was a problem hiding this comment.
영수님이 이미 많은 부분을 리뷰해주셔서 따로 전달할 부분은 보이지 않네요 ㅎ 과제하시느라 고생많으셨어요 ㅎ
| ) | ||
| public interface UrlShortenerApiResponseMapper { | ||
|
|
||
| @Mapping(target = "shortenUrl", expression = "java(baseUrl + result.hashedShortUrl())") |
There was a problem hiding this comment.
추후 미션으로 expression = "java()" 를 활용하지 않는 방법은 없을까를 고민해보시길 ㅎ
|
|
||
| public record UrlHash( | ||
| String value | ||
| ) { | ||
|
|
There was a problem hiding this comment.
hash라는 방식을 활용하지만 굳이 클래스명, 변수명에 적을 필요는 없을듯. (내 알빠는 아니고 무언가 담겨있어. 난 열쇠는 모르지만 열쇠를 아는 놈이 알아서 열든지 말든지 하겄지.)
📌 과제 설명
Local Memory에 저장하는 URL Shortener
2023-10-10.5.09.52.mov
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
저는 Url이 이미 hash값으로 생성되어 있다면 재사용하고 생성되지 않았다면 생성하려고 하였습니다.
그러다 보니
UrlShortenerApiController.findOrCreateShortenedUrl와DefaultUrlShortenerService .findOrCreateShortenUrlHash같이 2가지 기능을 하는 로직이 생겨났습니다.이 로직들을 어떻게 분리하면 좋을지 궁금합니다.