[4기 김별] Shorten URL 과제 PR입니다 #52

byeolhaha · 2023-10-10T04:20:08Z

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

URL 입력폼 제공 및 결과 출력
URL Shortening Key는 8 Character 이내로 생성
단축된 URL 요청시 원래 URL로 리다이렉트
단축된 URL에 대한 요청 수 정보저장 (optional)
Shortening Key를 생성하는 알고리즘 2개 이상 제공하며 애플리케이션 실행중 동적으로 변경 가능 (optional)

✅ PR 포인트

데이터의 변경이 일어나지 않고 조회가 자주 발생하기 때문에 캐시를 통해서 속도를 향상시키는 관점에서는 캐시를 사용
따라서 요청 수가 10 이상인 경우에만 캐시에 저장되도록 하였습니다.
그러나 이 10이라는 수치는 실제 데이터 요청에 따라 향후 결정
로컬 캐시를 사용한 이유
- 이 서비스의 경우 62의 8승과 같이, 만들 수 있는 short url의 개수가 정해져 있어 다차면 서비스가 종료될 것이라고 생각했습니다. 따라서 서버 확장이 불필요할 것으로 보였습니다.
- 다른 로컬캐시 라이브러리가 있었으나 고려하지 못했습니다.(Caffeine이나 Ecache를 선택하지 않은 이유에 대해서는 명확한 이유가 없어 두 개를 비교해서 저에게 필요한 것인지 생각해보겠습니다.)
요청 수가 count 되는 것에 대한 동시성을 고려해야할까
- 그 정도로 유의미한 데이터가 아닌 것 같아 단순히 캐시의 용도이거나 혹은 향후 분석의 의미로 사용될 거 같아 동시성을 고려하지 않았습니다.
캐시의 장애 발생 요인
- 다 차서 비우는 것은 갑자기 한번에 많은 양의 요청이 DB에 전달되는 상황이라고 생각했기 때문에 주기적으로 비우는 것이 장애가 덜 발생할 요인이라고 생각해 @Scheduled를 이용해서 비우는 작업을 했습니다.
- 하지만 이 부분에서 1시간마다 비우는 것을 고려했는데 1시간의 의미는 실제 요청에 대한 데이터가 있어야 올바른 예측값이 나올 거 같습니다.
- 또한 캐시의 만료기간이 아닌 단순히 비우는 작업이어서 방금 들어온 데이터도 삭제된다는 문제점이 있습니다.
검증 어노테이션을 만들어서 Origin Url이 https://나 http://로 시작하는지 확인하는 작업을 했습니다.

- 2차 캐시 이용하여 Origin Url 찾기

devYSK · 2023-10-10T16:12:02Z

src/main/java/com/prgrms/shorturl/global/config/CacheEvictScheduler.java

+
+    @Scheduled(fixedRate = 3600000)
+    public void evictUrlsCache() {
+        log.info("캐시는 1시간 뒤에 지워집니다.");


불필요한 로그메시지 인거 같아요!

사실 테스트를 통해서 검증했어야 했는데
@Scheduled에 대한 테스트 방법을 발견하게 되어 추가하도록 하겠습니다.

중요하지 않고 불필요한 로그 메세지여서 삭제하도록 하겠습니다.

이 프로젝트에서 캐시는 불필요한것 같아요 ㅎ

devYSK · 2023-10-10T16:12:16Z

src/main/java/com/prgrms/shorturl/global/config/CacheEvictScheduler.java

+
+    private final CacheManager cacheManager;
+
+    @Autowired


@Autowired?

ㅋㅋ 태그걸때 꼭 그걸로 이름지은 놈들 있어서 조심하셔유 ㅋㅋ @Autowired

devYSK · 2023-10-10T16:14:51Z

src/main/java/com/prgrms/shorturl/global/error/handler/GlobalExceptionHandler.java

+    @ExceptionHandler(ExistedOriginUrlException.class)
+    protected ResponseEntity<ErrorResponse> handleExistedOriginUrlException(ExistedOriginUrlException e) {
+        log.warn("Handle ExistedOriginUrlException", e);
+        final ErrorResponse response = ErrorResponse.of(e.getErrorCode(), e.getMessage());


왜 지역변수에 final을 사용하셨어요?

파라미터도 final을 붙여주는것이 좋아요.
메서드 내에서 해당 파라미터의 값을 변경할 수 없게 되어 파라미터 값이 예기치 않게 변경되는 것을 방지할수 있거든요
물론 코딩스타일이나 팀의 코딩 컨벤션에 따라 다르기도 해요

메서드 내에서 변할 수 없다는 것을 내포하고, 실제로 변경에 대한 위험을 방지하기 위해서 넣게 되었습니다.

파라미터에 final도 좋은 방법인 거 같아
다음 프로젝트를 진행할 때 팀원들과 이야기해 코딩 컨벤션을 정하도록 할게요 💡

devYSK · 2023-10-10T16:15:15Z

src/main/java/com/prgrms/shorturl/global/error/handler/GlobalExceptionHandler.java

+     */
+    @ExceptionHandler(MethodArgumentNotValidException.class)
+    protected ResponseEntity<ErrorResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) {
+        log.warn("Handle MethodArgumentNotValidException", e.getMessage());


warn으로 찍은 이유가 궁금해요!

devYSK · 2023-10-10T16:15:35Z

src/main/java/com/prgrms/shorturl/global/error/handler/GlobalExceptionHandler.java

+        final ErrorResponse response = ErrorResponse.of(INTERNAL_SERVER_ERROR, e.getMessage());
+        return ResponseEntity
+                .status(HttpStatus.INTERNAL_SERVER_ERROR)
+                .body(response);
+    }


Suggested change

final ErrorResponse response = ErrorResponse.of(INTERNAL_SERVER_ERROR, e.getMessage());

return ResponseEntity

.status(HttpStatus.INTERNAL_SERVER_ERROR)

.body(response);

}

final ErrorResponse response = ErrorResponse.of(INTERNAL_SERVER_ERROR, e.getMessage());

return ResponseEntity

.status(HttpStatus.INTERNAL_SERVER_ERROR)

.body(response);

}

개행에 의미를 부여해서 가독성을 높이도록 하겠습니다 🔦

e.getMessage도 좋지만 Throwable을 파라미터로 받는것도 괜찮을듯 합니다ㅎ

devYSK · 2023-10-10T16:35:09Z

src/test/java/com/prgrms/shorturl/url/repository/ShortUrlRepositoryImplTest.java

+        shortUrlService.creatShortUrl(naverUrlToSave.getOriginUrl(), "BASE62");
+        savedNaverUrl = shortUrlRepository.findByOriginUrl(naverUrlToSave.getOriginUrl()).get();
+
+        for (int i = 0; i < 20; i++) {


왜 20번인지 의도가 궁금해져요!

10번 이상일 때 캐시에 저장된 객체를 가져오는가를 테스트하기 위함이었지만
실제로 테스트를 보는 사람은 저 20의 의미를 알기 어렵다고 생각합니다.
그리고 저 20이라는 숫자는 바뀔 수 있는 수이기 때문에
항상 같은 테스트 결과가 나오도록 고치겠습니다.

devYSK · 2023-10-10T16:35:27Z

src/test/java/com/prgrms/shorturl/url/repository/ShortUrlRepositoryImplTest.java

+
+    private Optional<Urls> getCacheUrl(String shortUrl) {
+        return Optional.ofNullable(cacheManager.getCache("urls")).map(c -> c.get(shortUrl, Urls.class));
+    }
+
+    @Test
+    @DisplayName("요청 횟수가 정해진 limit 수를 넘어가면 2차 캐시에 저장하여 다음 요청에서 2차 캐시에 있는 origin url을 반환한다.")
+    void getUrlByShortUrl_overLimitedBoundCount_returnCache() {
+        //when
+        Urls urlByShortUrl = shortUrlRepository.getUrlByShortUrl(savedNaverUrl.getShortUrl());
+
+        //then
+        assertThat(urlByShortUrl).isEqualTo(getCacheUrl(savedNaverUrl.getShortUrl()).get());
+    }


테스트가 위로 오면 좋을것 같아요.

devYSK · 2023-10-10T16:35:37Z

src/test/java/com/prgrms/shorturl/url/service/ShortUrlServiceImplTest.java

+class ShortUrlServiceImplTest {
+
+    private final Urls NAVERURL = Fixtures.naverUrlInOriginUrl();
+    ;


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

수염 제거하도록 하겠습니다. 🧔‍♀️

devYSK · 2023-10-10T16:36:53Z

src/test/java/com/prgrms/shorturl/url/service/ShortUrlServiceImplTest.java

+        shortUrlService.creatShortUrl(NAVERURL.getOriginUrl(), STRATEGY_TYPE);
+
+        //when_then
+        assertThrows(ExistedOriginUrlException.class, () -> shortUrlService.creatShortUrl(NAVERURL.getOriginUrl(), STRATEGY_TYPE));


Suggested change

assertThrows(ExistedOriginUrlException.class, () -> shortUrlService.creatShortUrl(NAVERURL.getOriginUrl(), STRATEGY_TYPE));

assertThrows(ExistedOriginUrlException.class,

() -> shortUrlService.creatShortUrl(NAVERURL.getOriginUrl(), STRATEGY_TYPE));

저는 너무 길어지면 이렇게 쓰기도 해요.

devYSK · 2023-10-10T16:37:43Z

src/test/java/com/prgrms/shorturl/url/service/ShortUrlServiceImplTest.java

+
+    private final Urls NAVERURL = Fixtures.naverUrlInOriginUrl();
+    ;
+    private final String STRATEGY_TYPE = "BASE62";


중복 발견! 오타나면 영문도 모르겠어요. StragteType을 어떻게 관리하면 좋을까요

Enum으로 관리하도록 하겠습니다

WooSungHwan

캐시에다가 shortenurl을 저장하는 부분을 제외하고는 괜찮은 것 같습니다. 영수님이 이미 좋은 리뷰를 많이 주셔서 첨언할 부분은 cache 의 hit정도가 이 프로젝트의 그 기능이 적절한지에 대해서 고민해보면 좋을듯요. sql이나 jpa에도 캐시를 활용하는 부분이 있다는 점을 생각해보면 좋을 듯해요.

WooSungHwan · 2023-10-15T14:08:10Z

src/main/java/com/prgrms/shorturl/global/config/CacheEvictScheduler.java

+
+    private final CacheManager cacheManager;
+
+    @Autowired


ㅋㅋ 태그걸때 꼭 그걸로 이름지은 놈들 있어서 조심하셔유 ㅋㅋ @Autowired

WooSungHwan · 2023-10-15T14:09:06Z

src/main/java/com/prgrms/shorturl/global/config/CacheEvictScheduler.java

+
+    @Scheduled(fixedRate = 3600000)
+    public void evictUrlsCache() {
+        log.info("캐시는 1시간 뒤에 지워집니다.");


이 프로젝트에서 캐시는 불필요한것 같아요 ㅎ

WooSungHwan · 2023-10-15T14:09:58Z

src/main/java/com/prgrms/shorturl/global/error/handler/GlobalExceptionHandler.java

+        final ErrorResponse response = ErrorResponse.of(INTERNAL_SERVER_ERROR, e.getMessage());
+        return ResponseEntity
+                .status(HttpStatus.INTERNAL_SERVER_ERROR)
+                .body(response);
+    }


e.getMessage도 좋지만 Throwable을 파라미터로 받는것도 괜찮을듯 합니다ㅎ

WooSungHwan · 2023-10-15T14:14:12Z

src/main/java/com/prgrms/shorturl/url/model/Urls.java

+    @Column
+    @Embedded
+    private ShortUrl shortUrl;


unique 조건 생략된것 같네요. 비즈니스적 insert 동시성을 해결하는 이슈로 db의 unique를 활용할 수 있습니다.

WooSungHwan · 2023-10-15T14:14:58Z

src/main/java/com/prgrms/shorturl/url/repository/ShortUrlRepository.java

+
+public interface ShortUrlRepository {
+
+    void isExistedOriginUrl(String originUrl);


프로젝트 전반적으로 is로 시작하는 void가 많이보이는데 is는 플래그를 상징하는 키워드임. getXX랑 같이 뭔가 boolean을 리턴할것같은 느낌을 주니 반드시 고치세요.

WooSungHwan · 2023-10-15T14:15:29Z

src/main/java/com/prgrms/shorturl/url/repository/ShortUrlRepository.java

+
+public interface ShortUrlRepository {
+
+    void isExistedOriginUrl(String originUrl);


코드를 고치라는 부분이 아니라 이렇게 작성되지 않도록 고쳐보세요. 재발되면 안될것 같아요.

byeolhaha added 18 commits October 10, 2023 12:10

[Init] : 세팅

6404187

[Feat] : Short Url 알고리즘 전략 패턴으로 기능 구현

f7143d6

[Feat] : 인메모리 캐시 및 스케줄링 통해 캐시 비우기 구현

0318e44

[Feat] : 레포지토리 구현

a92134f

- 2차 캐시 이용하여 Origin Url 찾기

[Test] : 캐시 관련 테스트 구현

d168c30

[Feat] : Urls 엔티티 구현

2e85ba2

[Test] : Urls 엔티티 관련 테스트 구현

d5bdc8a

[Feat] : 서비스 구현

6902a2f

[Test] : 서비스 테스트 구현

94df087

[Feat] : 커스텀 예외 및 관련 Handler 구현

f42ba3b

[Feat] : short Url 생성 및 리다이렉트 controller 구현

0e908e9

[Feat] : Short Url Create Dto 및 custom 검증 어노테이션 구현

3b704d5

[Feat] : Controller dto 및 mapper 구현

2fcef74

[Feat] : index.html 구현

11235dd

[Style] : 불필요한 파일 삭제

d8f8667

[Chore] : 캐시 관련 의존성 추가

e2b45ac

[Test] : 공통으로 자주 사용하는 Urls를 Fixture로 분리

d537538

[Style] : 코드 정렬 및 개행 정리

b835dac

byeolhaha requested review from WooSungHwan and devYSK October 10, 2023 04:21

byeolhaha added Team-Blackdog 🔥 매운맛 labels Oct 10, 2023

byeolhaha self-assigned this Oct 10, 2023

devYSK reviewed Oct 10, 2023

View reviewed changes

WooSungHwan approved these changes Oct 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4기 김별] Shorten URL 과제 PR입니다 #52

[4기 김별] Shorten URL 과제 PR입니다 #52

byeolhaha commented Oct 10, 2023 •

edited

devYSK Oct 10, 2023

byeolhaha Oct 11, 2023 •

edited

WooSungHwan Oct 15, 2023

devYSK Oct 10, 2023

WooSungHwan Oct 15, 2023

devYSK Oct 10, 2023

byeolhaha Oct 11, 2023

devYSK Oct 10, 2023

devYSK Oct 10, 2023

byeolhaha Oct 11, 2023

WooSungHwan Oct 15, 2023

devYSK Oct 10, 2023

byeolhaha Oct 11, 2023

devYSK Oct 10, 2023

devYSK Oct 10, 2023

byeolhaha Oct 11, 2023

devYSK Oct 10, 2023

devYSK Oct 10, 2023

byeolhaha Oct 11, 2023

WooSungHwan left a comment

WooSungHwan Oct 15, 2023

WooSungHwan Oct 15, 2023

WooSungHwan Oct 15, 2023

WooSungHwan Oct 15, 2023

WooSungHwan Oct 15, 2023

WooSungHwan Oct 15, 2023

	assertThrows(ExistedOriginUrlException.class, () -> shortUrlService.creatShortUrl(NAVERURL.getOriginUrl(), STRATEGY_TYPE));
	assertThrows(ExistedOriginUrlException.class,
	() -> shortUrlService.creatShortUrl(NAVERURL.getOriginUrl(), STRATEGY_TYPE));


		public interface ShortUrlRepository {

		void isExistedOriginUrl(String originUrl);

[4기 김별] Shorten URL 과제 PR입니다 #52

Are you sure you want to change the base?

[4기 김별] Shorten URL 과제 PR입니다 #52

Conversation

byeolhaha commented Oct 10, 2023 • edited

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

✅ PR 포인트

Choose a reason for hiding this comment

byeolhaha Oct 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WooSungHwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byeolhaha commented Oct 10, 2023 •

edited

byeolhaha Oct 11, 2023 •

edited