Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4기 - 박세영] Url Shortener 과제 PR입니다 #45

Open
wants to merge 29 commits into
base: SY97P
Choose a base branch
from

Conversation

onetuks
Copy link

@onetuks onetuks commented Oct 6, 2023

🔑 과제 소개

SprintBoot로 URL Shortener 구현

🚀 요구사항

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

🗂️ API 명세

URL 입력 폼

Method URI
GET /

원본 URL을 단축 URL로 매핑

Method URI
POST /shortener
Request Parameter Type Description
originUrl String 원본 URL
Response Parameter Type Description
mapping UrlMappingResponse 매핑 결과

단축 URL에 대한 원본 URL로 리다이렉션

Method URI
GET /{shortUrl}
Path Variable Type Description
shortUrl String 단축 URL

URL 매핑 정보 조회

Method URI
GET /mappings
Response Parameter Type Description
mappings UrlMappingResponses 모든 매핑 정보
pageInfo PageInfo 페이징 정보

🤖 DataBase 테이블

📺 동작 화면

메인화면

메인화면

상세 페이지

모든 매핑 조회 페이지

단축 URL 요청 횟수 증가한 상태

Copy link

@WooSungHwan WooSungHwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shorten url 내용이 많이 어렵진 않을텐데. 이런 과제가 취업과제로 나오는게 이유가 있어요. 기본이라는 영역을 가장 잘 드러내보일수 있으니까 좀더 갈고닦아봤으면 좋겠어요 ㅎ

@@ -0,0 +1,40 @@
package com.tangerine.urlshortener.url.algorithm;

public class Base62BaseAlgorithm implements BaseAlgorithm {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base62BaseAlgorithm 이 클래스든 base64 알고리즘 클래스든 이건 단순 알고리즘인거지 mappingId 이런 변수명은 shorten url 에 종속된 변수명인 것 같아요.
이 클래스에 선언된 메소드는 base 62든 64든 처리할 encode value 혹은 decode value를 처리하기만 하면되지 mappingId, shortUrl 이런 변수명하고는 하등 상관관계가 없도록 구성해놔야해요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정했습니다!

@GetMapping(path = "/mappings")
public String mappingInfos(
Model model,
Pageable pageable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 Pageable 인터페이스 컨트롤러에 박아놓고 쓰면 저 스펙대로 요청날라오면 그거대로 바인딩 됩니다. 만약 저기 조회 조건에 page랑 limit말고 sort까지 전달되면 그거 그대로 repository 정렬조건으로 쓸수있는거에요. 그 정렬조건이 인덱스가 걸려있지 않으면 쿼리동작시간 백엔드 개발자가 예상할수도 없는 상황이구요.
차라리 pageable 만들 때 필요한 값들 QueryParam 클래스로 따로 구성해놓고 별도로 PageRequest 만들어서 호출하셔요.
Pageable 인스턴스 만들때 구현체를 직접 만들도록 제어하는 환경이 백엔드 본인에게 나을지 프레임워크단에 열어놓고 클라이언트 개발자 혹은 요청자(누가될지모름)가 제어할수 있도록 냅둘지 진지하게 고민해봐요 이건.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필요하다고 생각했는데 꼬집어주셔서 감사합니다

Comment on lines 16 to 19
result.originUrl().getOriginUrlText(),
result.shortUrl().getShortUrlText(),
result.algorithm().name(),
result.requestCount().getRequestCount()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게하니까 NPE 날거같은 느낌만 주게돼요. 타고타고 하지말고 result안에서 구성하면 될거에요. 그럼 그 안에서 null 처리라도 해볼수 있고.

Comment on lines +24 to +26
public static RequestCount addCount(RequestCount requestCount) {
return new RequestCount(requestCount.getRequestCount() + 1);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런거 잘못써서 STW 늘어날 수 있으니깐 한 스레드내에서 얼마나 호출될 수 있는지 체크하고 조심해서 써야되요. count만 증가되는 것처럼 보이는데 실질적으론 인스턴스를 찍어내고 있으니깐 위험해보이는 메소드에요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞습니다.
값 객체를 매번 새로 생성해서 엔티티에 갱신시키니까 GC 가능성이 높아질 것 같아요.
하지만, 저는 한 번의 요청에서 한 번만 카운트가 증가하도록 로직을 구현해서 스레드 당 카운트 증가는 1회가 최대라고 생각합니다.
때문에 이 로직으로 STW로 사용성이 줄어들 것 같지 않습니다.
무엇보다 값 객체 내부 값을 변경하는 것보다 새로이 값 객체를 생성하는 것이 값 객체를 제대로 활용하는 것 같아요.
만약에 STW 가 문제가 된다면, 엔티티에 값 객체를 두지 않고 원시값으로 필드를 변경하겠습니다.

new RequestCount(0)
)
));
if (urlMapping.getShortUrl().isEmptyShortUrl()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPE 날거같은 불안한 구조.

Comment on lines 28 to 36
UrlMapping urlMapping = urlMappingJpaRepository.findByOriginUrl(shortenParam.originUrl())
.orElseGet(() -> urlMappingJpaRepository.save(
new UrlMapping(
shortenParam.originUrl(),
new ShortUrl(),
shortenParam.algorithm(),
new RequestCount(0)
)
));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

�exist로 검색하고 boolean 결과에 따라 제어할수있도록 변경하세요.
조회해서 없으면 default 값을 내놓는게 orElseGet 쓰라고 만든거지 없으면 집어넣는 행위하라고 만든 인터페이스 아닌 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exist 를 생각 못했네요.
고민하던 문제였는데 시원해졌습니다.

*/
@PostMapping(path = "/shortener")
public String shortenOriginUrl(
@ModelAttribute ShortenRequest shortenRequest,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post인데 왜 굳이 modelattribute로 쓴거죠? body를 활용할 수 있다는게 가장 큰 장점일텐데 query parameter로 넘겼던 이유가 있나요?

Copy link
Author

@onetuks onetuks Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저번에 쿼리 파라미터로만 모든 요청 처리하는 걸 따라해보고 싶었던 마음도 있었고,
form 데이터를 post 로 받아올 때 @ RequestBody 사용하면 415 응답이 나오기에 @ ModelAttribute 를 썼습니다.
만약에 JSON을 주고 받는 REST 로 구현했다면 Body 방식을 사용했을 것 같습니다.

@PathVariable String shortUrl
) {
OriginUrl originUrl = urlService.findOriginUrl(new ShortUrl(shortUrl));
return "redirect:" + originUrl.getOriginUrlText();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originUrl.redirect(); 이렇게 리턴하고 안으로 옮기는게 더 깔끔할듯 ㅎ

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 그러네요
originUrl 필드값만 필요하니까 쓸데없는 의존성 생길 불안도 없는 좋읔 방법인 것 같습니다

Comment on lines 19 to 20
public String illegalArgumentExceptionHandler(IllegalArgumentException e) {
logger.warn("{} - {}", getClass().getSimpleName(), e);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IllegalArgumentException가 warn 레벨이여야 하는 이유가 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 예외가 같은 수준이라고 생각해서 warn 이라고 했습니다.

warn: 프로그램 실행에서 예외상황이 가능한 경우 -> 예외처리된 경우
error: 정확한 원인을 모르거나, 의도하지 않은 예외상황인 경우

이렇게 구분하여 로그레벨을 다시 설정했습니다.

Comment on lines 24 to 29
@ResponseStatus(NOT_FOUND)
@ExceptionHandler(RuntimeException.class)
public String runtimeExceptionHandler(RuntimeException e) {
logger.warn("{} - {}", getClass().getSimpleName(), e);
return "error/not-found";
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RuntimeException은 모든 언체크드 익셉션의 상위 예외라서 NotFound보다는 다른 status code가 좋을것 같아요.
사용자나 서버 개발자나 내부적으로 모르는 예외가 발생했는데 status가 404가 나오면 당황스러울수도 있을것 같네요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotFound 에 대한 예외를 추가로 만들어 붙이고, RuntimeException 은 400으로 처리했습니다.

Comment on lines 9 to 39

@Override
public String encode(Long mappingId) {
StringBuilder sb = new StringBuilder();
while (mappingId > 0) {
sb.append(tokens[(int) (mappingId % TOKEN_LENGTH_BOUND)]);
mappingId /= TOKEN_LENGTH_BOUND;
}
return padding(sb);
}

@Override
public long decode(String shortUrl) {
long decodedId = 0L;
long factor = 1L;
for (int i = 0; i < URL_LENGTH_BOUND; i++) {
decodedId += String.valueOf(tokens).indexOf(shortUrl.charAt(i)) * factor;
factor *= TOKEN_LENGTH_BOUND;
}
return decodedId;
}

private String padding(StringBuilder sb) {
int length = sb.length();
while (length < URL_LENGTH_BOUND) {
sb.append(PADDING_TOKEN);
length++;
}
return sb.toString();
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 코드가 따박따박 붙어있는것보다 문맥에 따라 개행을 해주면 가독성을 챙길 수 있어요

Suggested change
@Override
public String encode(Long mappingId) {
StringBuilder sb = new StringBuilder();
while (mappingId > 0) {
sb.append(tokens[(int) (mappingId % TOKEN_LENGTH_BOUND)]);
mappingId /= TOKEN_LENGTH_BOUND;
}
return padding(sb);
}
@Override
public long decode(String shortUrl) {
long decodedId = 0L;
long factor = 1L;
for (int i = 0; i < URL_LENGTH_BOUND; i++) {
decodedId += String.valueOf(tokens).indexOf(shortUrl.charAt(i)) * factor;
factor *= TOKEN_LENGTH_BOUND;
}
return decodedId;
}
private String padding(StringBuilder sb) {
int length = sb.length();
while (length < URL_LENGTH_BOUND) {
sb.append(PADDING_TOKEN);
length++;
}
return sb.toString();
}
@Override
public String encode(Long mappingId) {
StringBuilder sb = new StringBuilder();
while (mappingId > 0) {
sb.append(tokens[(int) (mappingId % TOKEN_LENGTH_BOUND)]);
mappingId /= TOKEN_LENGTH_BOUND;
}
return padding(sb);
}
@Override
public long decode(String shortUrl) {
long decodedId = 0L;
long factor = 1L;
for (char c : shortUrl.toCharArray()) {
decodedId += String.valueOf(tokens).indexOf(c) * factor;
factor *= TOKEN_LENGTH_BOUND;
}
return decodedId;
}
private String padding(StringBuilder sb) {
int length = sb.length();
while (length < URL_LENGTH_BOUND) {
sb.append(PADDING_TOKEN);
length++;
}
return sb.toString();
}

Comment on lines 6 to 19

public record ShortenRequest(
String originUrl,
String algorithm
) {

public static ShortenParam to(ShortenRequest request) {
return new ShortenParam(
new OriginUrl(request.originUrl()),
Algorithm.valueOf(request.algorithm())
);
}

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보니까, Service와 Controller의 DTO를 분리하신것 같아요.
Algorithm은 enum이니까 바로 받을 수 있을것 같네요

Suggested change
public record ShortenRequest(
String originUrl,
String algorithm
) {
public static ShortenParam to(ShortenRequest request) {
return new ShortenParam(
new OriginUrl(request.originUrl()),
Algorithm.valueOf(request.algorithm())
);
}
}
public record ShortenRequest(
String originUrl,
Algorithm algorithm
) {
}

또한 Service의 dto를 생성하는데 꼭 정적 팩토리 메소드 일 필요는 없을것 같아요.
자기자신의 static 메소드에 자기자신의 인스턴스를 넘긴다는 어색하네요.
무조건 ShortenRequest의 인스턴스로 ShortenParam을 만들어야 한다면 다음과 같이 사용할 수 있지 않을까요?

Suggested change
public record ShortenRequest(
String originUrl,
String algorithm
) {
public static ShortenParam to(ShortenRequest request) {
return new ShortenParam(
new OriginUrl(request.originUrl()),
Algorithm.valueOf(request.algorithm())
);
}
}
public record ShortenRequest(
String originUrl,
String algorithm
) {
public ShortenParam to() {
return new ShortenParam(
new OriginUrl(this.originUrl),
Algorithm.valueOf(this.algorithm)
);
}
}

그렇다면 컨트롤러에서는?

    @PostMapping(path = "/shortener")
    public String shortenOriginUrl(
        @RequestBody @Valid  ShortenRequest shortenRequest,
        Model model
    ) {
        UrlMappingResult mappingInfo = urlService.createShortUrl(shortenRequest.to());
        UrlMappingResponse mapping = UrlMappingResponse.of(mappingInfo);
        model.addAttribute("mapping", mapping);
        return "mapping-info";
    }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO 변환을 하나의 형태로 정하고 싶었습니다.
하지만 같은 역할을 한다면 static 보다는 인스턴스 메소드로 구현하는 편이 나은 방법이라고 생각합니다.

Comment on lines +6 to +13
public record UrlMappingResponses(
Page<UrlMappingResponse> results
) {

public static UrlMappingResponses of(UrlMappingResults mappingResults) {
return new UrlMappingResponses(mappingResults.results()
.map(UrlMappingResponse::of));
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

org.springframework.data.domain.Page에 꽤 많은 필드가 들어있어요

필요한것만 돌려주면 어떨까요?

Comment on lines +12 to +21

@NotBlank
@Column(name = "origin_url", nullable = false, unique = true)
private String originUrlText;

public OriginUrl(String originUrlText) {
Assert.isTrue(StringUtils.isNotBlank(originUrlText), "원본 url은 반드시 주어져야 합니다.");
this.originUrlText = originUrlText;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

두번 방어를 의도하신걸까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 validation 이 필요할 수 있게 될 것 같아, 넣어두었습니다.
그리고, 실수로 생성자 가드를 빼놓는 경우가 종종 있는데, validation을 적어두면 그 실수를 좀 더 빠르게 찾을 수 있기도 해서 두 번 걸어두고 있습니다.

Comment on lines 14 to 17

@Test
@DisplayName("올바른 정보로 생성한다.")
void instance_Success() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어떤것이 올바른 정보인지 궁금해지네요

new OriginUrl(originUrlText),
new ShortUrl(shortUrlText),
Algorithm.BASE62,
new RequestCount(0)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 용도의 비즈니스 규칙이라면, 처음 생성시 0을 초기화 해도 될거같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기본 생성자에서 0으로 초기화되도록 수정했습니다.

Comment on lines 34 to 40

@Test
@DisplayName("올바르지 않은 정보로 생성 실패한다.")
void instance_fail() {
// Given
String originUrlText = "";
String shortUrlText = "short";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RequestCount가 초기 생성시 무조건 0으로 초기화 된다면 이 테스트는 바뀔 수 있겠죠?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

초기값을 생각해보니까, urlMapping 엔티티가 새로이 생성될 때에는 ShortUrl도 없고, RequestCount 는 0으로 초기화되어야 했습니다.
이 내용을 반영해서 생성자를 만들었고, 그에 따라 테스트에 필요한 정보는 originUrl로 압축되었습니다.

Comment on lines 23 to 24

@SpringBootTest(properties = "spring.config.location=" + "classpath:/application-test.yaml")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TestPropertySource 라는 어노테이션에 대해 알아보세요

1. ExceptionHandler 로그 레벨 분류
2. RuntimeException 따로 처리
Copy link

@WooSungHwan WooSungHwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 ㅎㅎ 코멘트 몇자 남겼으니 한번 짚고 넘어가셔요 ㅎㅎ

import org.springframework.web.bind.annotation.ResponseStatus;

@ControllerAdvice
public class GlobalExceptionHandler {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception에 대한 500 예외처리가 없네요.

Comment on lines +36 to +45
private String padding(StringBuilder sb) {
int length = sb.length();

while (length < URL_LENGTH_BOUND) {
sb.append(PADDING_TOKEN);
length++;
}

return sb.toString();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앞으로 이런거작성할땐 util 패키지가 따로 없는지 확인부터 하셔요. 직접 개발하면 지쳐서 제때 뭐 못만들어요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 base64 유틸로는 url 안정성에 문제가 생겨서 + /를 치환할 필요가 있었습니다

long requestCount
) {

public static UrlMappingResponse of(UrlMappingResult result) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

영어를 생각해보면 of라는 건 '~의' 뭐 이렇게 쓰는거잖아요? 예컨데 kind of xxx -> xxx의 종류. 그럼 이건 xxx로 이루어진 무언가를 뜻하는건데.(이건 또 consist of 이기도 하구) 구성요소로 봐야겠죠?
영어 공부를 하자는건 아닌데 의미상 static UrlMappingResponse(Long id, String originUrl, ... long requestCount) { } 이게 맞는 것 같고.
무언가 객체로부터 인스턴스를 불러오겠다면 static UrlMappingResponse from(UrlMappingResult result) 은 어떨지
선언된 코드로 봅시다.

UrlMappingResponse.from(urlMappingResult); // urlMappingResult 데이터로부터의 인스턴스화
UrlMappingResponse.of(...) // ...로 이루어진 데이터로부터의 인스턴스화

�의미상 이런건 어떤지 ㅎ

Comment on lines +43 to +49
@Transactional
public OriginUrl findOriginUrl(ShortUrl shortUrl) {
UrlMapping urlMapping = urlMappingJpaRepository.findByShortUrl(shortUrl)
.orElseThrow(() -> new RuntimeException("해당 URL은 매핑되지 않았습니다."));
urlMapping.addRequestCount();
return urlMapping.getOriginUrl();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서비스의 목적은 조회지만 count 증가를 update하는 부분이 있는데 이걸 뭐 어떻게 못하는건가요? ㅎ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants