[4기 안재영] Shorten URL 과제 PR입니다. #54

JaeyoungAhn · 2023-10-11T08:25:24Z

요구사항

각 요구사항을 모두 충족할 수 있도록 노력해봅시다.

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

✅ API 명세

POST: /v1/url => 링크 줄이기
GET: /v1/url/{hash} => 단축 링크를 통해 접속하기

✅ View 명세

/url-shortner => 링크 줄이는 화면으로 이동
/display-result => 링크를 줄인 결과 페이지로 이동

👩‍💻 시연

궁금한 점

리다이렉션 성공시 조회수를 1 증가 시키는 경우, 최초 시도 시에는 1이 증가하지만,
이후에 다시 시도할 경우 조회수가 오르지 않는 것을 목격하였습니다. (Postman이 아닌 브라우저)
캐시랑 관련이 있다고 생각이 들지만, 리다이렉션을 위해 서버에 접근해야하는 과정이 반드시 포함되어있음에도
받아오는 과정을 생략하게 되는건지 잘 모르겠습니다. 어떤 부분을 학습해보면 좋을까요?

WooSungHwan

코멘트 몇자 남겼습니다 ㅎ 고민의 흔적이 많이 보이네요ㅎㅎ 수고하셨어요!

WooSungHwan · 2023-10-13T01:51:38Z

src/main/java/com/prgrmrs/urlshortener/business/UrlService.java

+    private final UrlStore store;
+    private final UrlReader reader;


WooSungHwan · 2023-10-13T01:52:51Z

src/main/java/com/prgrmrs/urlshortener/business/UrlService.java

+    }
+
+    @Transactional
+    public OriginalUrl redirectUrl(String hash) {


hash라는 변수명보단 key가 맞을듯 합니다. key역할이 될수 있는게 hash인데 파라미터로 받는 순간 이건 hash라기 보다는 key로 넘어왔다고 생각해야 덜 헷갈릴 것 같네요.

말씀하신대로 hash는 너무 구체적인 것 같다고 생각합니다. key로 바꿔보겠습니다!

WooSungHwan · 2023-10-13T01:55:42Z

src/main/java/com/prgrmrs/urlshortener/global/GlobalExceptionHandler.java

+import org.springframework.web.bind.annotation.RestControllerAdvice;
+
+@RestControllerAdvice
+public class GlobalExceptionHandler {


단순한 기능 exception 예외처리뿐만 아니라 스프링을 사용하는 환경에서 발생할 수 있는 부분의 특징도 고려해서 정의해보도록 하세요.

예를 들어, 405가 발생하는 exception이 터졌는데 그럼 @ExceptionHandler(Exception.class) 여기 탈거고 500 으로 리턴될텐데 이런 부분은 고려가 안됐다고 생각이 들게되는 것 같아요.

귀찮아도 이런게 쌓이고 쌓여야 기본기가 될겁니다.

스프링 환경 내에 발생하는 더 많은 익셉션들을 처리해보는 습관을 들이도록 하겠습니다!!

WooSungHwan · 2023-10-13T01:56:04Z

src/main/java/com/prgrmrs/urlshortener/implementation/UrlReader.java

+        this.repository = repository;
+    }
+
+    public UrlMapping retrieveUrlMapping(String hash) {


retrive 좋네요 명칭

WooSungHwan · 2023-10-13T01:56:50Z

src/main/java/com/prgrmrs/urlshortener/model/UrlMapping.java

+    @Column(name = "hash")
+    private String hash;


얘는 그냥 이렇게 두면 안될것같은데요? unique 조건은 걸어줘야하지 않을까요?
-> 해쉬충돌

WooSungHwan · 2023-10-13T01:58:34Z

src/main/java/com/prgrmrs/urlshortener/presentation/UrlRestController.java

+            BindingResult bindingResult) {
+        if (bindingResult.hasErrors()) {
+            throw new UrlShortenerException(BLANK_ORIGINAL_URL);
+        }


이런데서 BindingResult 예외처리는 빼봅시다.

커스텀 익셉션를 new할 때 BLANK_ORIGINAL_URL과 같은 Enum을 통해 별도의 정형화된 에러 출력 코드라든지 메세지를 조작할 수 있었지만 @Valid를 통해 생기는 validation 관련 익셉션은 커스텀 익셉션이 아니다보니 정형화된 처리가 어려울 것 같아 이런 방법을 생각했던 것 같습니다. 조금 더 좋은 방법은 없을지 고민해보겠습니다.

WooSungHwan · 2023-10-13T01:58:59Z

src/main/java/com/prgrmrs/urlshortener/presentation/UrlThymeleafController.java

+import org.springframework.web.bind.annotation.PostMapping;
+
+@Controller
+public class UrlThymeleafController {


redirect 해주는 로직은 없네요?

주소창으로 입력하는 것만 떠올랐었는데 thymeleaf로도 리다이렉션해줄 수 있을 것 같습니다.

WooSungHwan · 2023-10-13T02:00:43Z

src/main/java/com/prgrmrs/urlshortener/model/vo/OriginalUrl.java

+import jakarta.validation.constraints.Pattern;
+
+@Embeddable
+public class OriginalUrl {


이 클래스는 요청 파라미터 바인딩도 하는거같은데 embeddable이기도 하네요? ㅎ

작은 프로그램의 특성상 별도의 DTO를 사용을 하지 않으려고 해당 클래스를 통해 파라미터 바인딩을 하는 방법을 택했지만 다른 측면으로 생각해보니 @embeddable이라는 어노테이션 자체가 JPA에 관련된 코드기도 해서 책임의 분리가 일어나지 않은 것 같습니다. 서비스 클래스 안쪽에서 해당 클래스를 사용하는 것이 바람직할 것 같습니다.

devYSK · 2023-10-14T11:28:12Z

src/main/java/com/prgrmrs/urlshortener/global/GlobalExceptionHandler.java

+        ErrorCode errorCode = exception.getErrorCode();
+        ErrorResponse errorResponse = ErrorResponse.of(errorCode);
+
+        logger.error("UrlShortenerException: {}", errorResponse);


이 로그의 레벨이 error가 맞는지 고민해보면 좋을것 같아요.
각 로그 레벨을 어떤 기준으로 나누고 찍을지 정해보시지요

관련해서 검색을 해보니 이미 개발자가 인지하고있는 구체적인 커스텀 익셉션인 경우 로그 레벨이 error인 것보다, info 수준에서 다뤄주는게 좋을 것 같아서 이 경우 info를 사용하고, Exception 같이 상위 레벨이기에 구체적으로 어떤 익셉션인지 개발자가 인지하지 못하고 있을 수도 있는 경우에는 error를 사용하는 방식으로 수정해보겠습니다.

devYSK · 2023-10-14T11:31:23Z

src/main/java/com/prgrmrs/urlshortener/presentation/UrlRestController.java

+
+    @GetMapping("/{hash}")
+    public ResponseEntity<Void> redirectURL(@PathVariable String hash) {
+        OriginalUrl originalUrl = service.redirectUrl(hash);
+
+        return ResponseEntity
+                .status(HttpStatus.MOVED_PERMANENTLY)
+                .header(HttpHeaders.LOCATION, originalUrl.getUrl())
+                .build();
+    }


이게 hash가 shorturl에서 여러 구현 방법중에 하나인데
이걸 path값과 paratmer로까지 보여줘야하는지 의문이 드네요.

hash가 아닌 다른 방식으로 사용하거나, 알고리즘이 바뀐다면 모두 바꿔야 되지 않을까요?

말씀하신 내용이 맞는 것 같습니다. 좀 더 일반화된 방법을 고안해보겠습니다.

devYSK · 2023-10-14T11:31:56Z

src/main/java/com/prgrmrs/urlshortener/presentation/dto/ShortenUrlResponse.java

+        return new ShortenUrlResponse(urlMapping.getHash(), urlMapping.getOriginalUrl().getUrl());
+    }


get.get 디미터의 법칙에 대해 알아보세요

해당 엔티티의 내부 사정을 너무 구체적으로 알지 못하도록 연속되는 get은 해당 엔티티 안에서 해결할 수 있도록 하겠습니다.

devYSK · 2023-10-14T11:33:09Z

src/main/java/com/prgrmrs/urlshortener/util/Base62Encoder.java

+    static final char[] BASE62 = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789".toCharArray();
+
+    private Base62Encoder() {
+        // on purpose


// on purpose 라는 주석은 코드의 의도를 완전히 설명하지 못하는거 같은데요?

의도가 있어서 지우지 않았다면 어떤 의도였는지 구체적으로 주석을 남겨보겠습니다.

devYSK · 2023-10-14T11:33:30Z

src/main/java/com/prgrmrs/urlshortener/util/Base62Encoder.java

+
+public class Base62Encoder {
+
+    static final char[] BASE62 = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789".toCharArray();


상수 명명 규칙은 대문자 + 언더바입니다 BASE_62

devYSK · 2023-10-14T11:37:41Z

src/test/java/com/prgrmrs/urlshortener/model/vo/OriginalUrlTest.java

+    @ParameterizedTest
+
+    @MethodSource("provideFailOriginalUrl")
+    void OriginalUrl_FailCreation(String url) {


혹시 메소드 네이밍을 이렇게 한 의도가 있을까요?
없다면 표준을 지켜주세요

엔티티의 타입에 대한 테스트여서 표준을 간과했던 것 같습니다. 수정하겠습니다.

devYSK · 2023-10-14T11:38:18Z

src/test/java/com/prgrmrs/urlshortener/presentation/UrlControllerTest.java

+
+    @Autowired
+    MockMvc mockMvc;
+
+    @Autowired
+    UrlService service;
+
+    ObjectMapper objectMapper = new ObjectMapper();
+
+    private static final String BASE_URL = "/v1/url";
+    private static final String TEST_URL = "https://www.google.com";


누군 private 누군 default.

이유가 없다면 통일성을 맞춰주는게 어떨까요

부주의 했던 것 같습니다. private으로 통일하겠습니다.

devYSK · 2023-10-14T11:38:30Z

src/test/java/com/prgrmrs/urlshortener/presentation/UrlControllerTest.java

+
+}
+
+
+


devYSK · 2023-10-14T11:39:05Z

src/test/java/com/prgrmrs/urlshortener/util/Base62EncoderTest.java

+
+    @ParameterizedTest
+    @MethodSource("provideEncodeTestData")
+    void Encode_SuccessfullyConverted(Long seq, String hash) {


메소드 네이밍 규칙을 지켜주세요

devYSK · 2023-10-14T11:40:47Z

src/main/java/com/prgrmrs/urlshortener/model/vo/OriginalUrl.java

+
+    public void setUrl(String url) {
+        this.url = url;
+    }
+


생성자로 초기화 시키는데 set이 필요한 이유가 있을까요??

필요하지 않은 게터나 세터는 필요할때만 만드는 습관을 들이겠습니다!

JaeyoungAhn added 17 commits October 11, 2023 12:43

chore: 프로젝트 셋팅

c3bcc99

chore: 프로젝트 셋팅

5f118ce

feat: Url 컨트롤러 작성

5348021

feat: Url 서비스 작성

093158d

feat: Url 구현 작성

cbf0810

feat: Url 레포지토리 작성

b979cf8

feat: Url 모델 작성

03c4820

feat: Url 데이터 객체 작성

35f7dd0

feat: Url 에러 처리

ecb5e02

feat: Base62 로직 작성

0238f84

fix: get, post 동작하도록 에러 수정

39fa662

refactor: base62 로직 변경

1ffc49d

test: UrlController 테스트 작성

b171ec8

test: OriginalUrl 테스트 작성

036b172

rename: 이름 변경

518ea21

test: Base62Encoder 테스트 작성

a7ae70d

feat: Thymeleaf 구현

b3998b4

JaeyoungAhn added Team-Blackdog 💧 순한맛 labels Oct 11, 2023

JaeyoungAhn requested review from WooSungHwan and devYSK October 11, 2023 08:25

JaeyoungAhn self-assigned this Oct 11, 2023

WooSungHwan reviewed Oct 13, 2023

View reviewed changes

devYSK approved these changes Oct 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4기 안재영] Shorten URL 과제 PR입니다. #54

[4기 안재영] Shorten URL 과제 PR입니다. #54

JaeyoungAhn commented Oct 11, 2023

WooSungHwan left a comment

WooSungHwan Oct 13, 2023

WooSungHwan Oct 13, 2023

JaeyoungAhn Oct 13, 2023

WooSungHwan Oct 13, 2023

JaeyoungAhn Oct 13, 2023

WooSungHwan Oct 13, 2023

WooSungHwan Oct 13, 2023

WooSungHwan Oct 13, 2023

JaeyoungAhn Oct 13, 2023

WooSungHwan Oct 13, 2023

JaeyoungAhn Oct 13, 2023

WooSungHwan Oct 13, 2023

JaeyoungAhn Oct 13, 2023

devYSK Oct 14, 2023

JaeyoungAhn Oct 14, 2023 •

edited

devYSK Oct 14, 2023

JaeyoungAhn Oct 14, 2023

devYSK Oct 14, 2023

JaeyoungAhn Oct 14, 2023

devYSK Oct 14, 2023

JaeyoungAhn Oct 14, 2023

devYSK Oct 14, 2023

devYSK Oct 14, 2023

JaeyoungAhn Oct 14, 2023

devYSK Oct 14, 2023

JaeyoungAhn Oct 14, 2023

devYSK Oct 14, 2023

devYSK Oct 14, 2023

devYSK Oct 14, 2023

JaeyoungAhn Oct 14, 2023

		private final UrlStore store;
		private final UrlReader reader;

		return new ShortenUrlResponse(urlMapping.getHash(), urlMapping.getOriginalUrl().getUrl());
		}


		public class Base62Encoder {

		static final char[] BASE62 = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789".toCharArray();

[4기 안재영] Shorten URL 과제 PR입니다. #54

Are you sure you want to change the base?

[4기 안재영] Shorten URL 과제 PR입니다. #54

Conversation

JaeyoungAhn commented Oct 11, 2023

요구사항

✅ API 명세

✅ View 명세

👩‍💻 시연

궁금한 점

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

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

JaeyoungAhn Oct 14, 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

JaeyoungAhn Oct 14, 2023 •

edited