[4기 김별] Shorten URL 과제 PR입니다 by byeolhaha · Pull Request #52 · prgrms-be-devcourse/springboot-url-shortener

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

	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);

Conversation

byeolhaha commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌 과제 설명

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

✅ PR 포인트

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byeolhaha Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WooSungHwan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

byeolhaha commented Oct 10, 2023 •

edited

Loading

byeolhaha Oct 11, 2023 •

edited

Loading