[4기 윤영운] URL Shortener 과제 PR입니다. #47
Conversation
+ 인코딩 로직 도메인에서 애플리케이션 레이어로 이동
Refactor: UrlRestController createUrl()에서 responseUrl에 관련된 중복 제거
| String time) { | ||
| public static ErrorResponse of( |
| import java.util.Optional; | ||
|
|
||
| @Repository | ||
| public class UrlRepositoryImpl implements UrlRepository { |
There was a problem hiding this comment.
이거 이렇게하면 서비스에서 urlJpaRepository를 직접주입해서 쓰지 않는 컨벤션이라 행위자체가 제한되는건 좋은 방향인 것 같습니다. 하지만 save와 findBy가 분리된 인터페이스 설계 기준을 도입해보는 것도 좋은 방향이라 생각합니다.
서비스 자체도 UrlQueryService 이런식으로 조회용 서비스를 도입할수도 있거든요. 그럼 레포지토리 layer(reader, writer)를 의존하는 서비스 클래스들이 정해질거구요.
| public EncodedUrl(String encodedUrl, EncodingType encodingType) { | ||
| Assert.notNull(encodedUrl, "변환된 url은 null이 될 수 없습니다."); | ||
| Assert.notNull(encodingType, "encodingType은 null이 될 수 없습니다."); | ||
|
|
||
| if (encodedUrl.length() > MAX_URL_LENGTH) { | ||
| throw new IllegalArgumentException("변환된 url의 길이는 8이상 될 수 없습니다."); | ||
| } | ||
|
|
||
| this.encodedUrl = encodedUrl; | ||
| this.encodingType = encodingType; | ||
| } |
There was a problem hiding this comment.
encodingType 안받고 encodedUrl만 받는다고 생각하고 해보세요. 그리고 unique = true한 부분 테이블 erd 확인해보시고 unique key 이름 한번 확인해보세요.
중복이 발생해서 유니크 관련 디비 에러가 난다면 수많은 테이블중에 저런거 알아볼 수 있을까요? ㅎ
유니크키를 정의하는 방법에 대해서 좀더 고도화해보세요.
| BASE_64_V1(Base64EncoderV1::new), | ||
| BASE_64_V2(Base64EncoderV2::new); |
There was a problem hiding this comment.
이렇게 알고리즘이 두개로 나눠진다면.. 개인적으로는 테이블이 두개로 나눠져야 좀더 유연하다고 봅니다. 두 알고리즘중에 하나 뺀다고 생각하고 기존 데이터 이제 제공안할꺼니까 모두 지워야한다고 하면
테이블을 나눈 경우는 하나의 테이블을 코드레벨에서 하나의 알고리즘만 내비두고 배포해서 일정시간이 지나고 테이블 drop 치면 그만이지만 테이블이 붙어서 타입으로 구분되는 경우는 반드시 레거시가 존재하게 됩니다.
레거시는 반드시 누군가에게 혼란을 줍니다.
| Assert.notNull(encodingType, "encodingType은 null이 될 수 없습니다."); | ||
|
|
||
| if (encodedUrl.length() > MAX_URL_LENGTH) { | ||
| throw new IllegalArgumentException("변환된 url의 길이는 8이상 될 수 없습니다."); |
There was a problem hiding this comment.
이거 변환된 길이가 64진법기준이라면 분명 8자리를 넘어서는 기준도 있을텐데 그건 어떻게 하실예정인가요? ㅎ
There was a problem hiding this comment.
요구사항이 8 Character 이내 생성이였기 때문에 일단은 변환할 키 값을 1 부터 sequence하게 1씩 증가하는 값을 제공하여 8자리 이내에 가능한 경우의 수를 꽉 채우게 구현하였습니다.
하지만 분명 Url이 64^8개 이상 생성되면 위와같은 제약으로 인해 더이상 url이 생성되지 않을것 같습니다.
이거에 대해서는 좀 더 고민해봐야 할 것 같습니다...
There was a problem hiding this comment.
@young970 그걸 해결해야 이 서비스가 제한없이 유지할 수 있을겁니다 ㅎ
| @Component | ||
| public class Base64EncoderV1 extends Encoder { | ||
| private static final int BASE64 = 64; | ||
| private static final String BASE64_CHAR = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_-"; |
There was a problem hiding this comment.
언더바나 파이픈은 url 구성시에 불안함을 url에 대한 밸런스(미적)의 균형을 깨보여서 차라리 제거하고 base 62로 가는게 좀더 깔끔할거같아요.
근데 base 64선택한 이유가 뭔가요?
There was a problem hiding this comment.
먼저 첫번째는 생성 가능한 가지수가 많아서 였습니다.
두번째 이유로는 요구사항에 "애플리케이션 실행 중 동적으로 변경 가능한 변환 알고리즘 2가지를 제공" 이 있었기 때문에
두가지의 알고리즘을 제공하며 두 알고리즘간의 반환값의 충돌을 없애고 싶어서 였습니다.
아무래도 base62 와 base64를 제공하게 되면 비슷한 문자열 구성과 서로 다른 진법으로 인해 충돌이 예상되어
(ex. Base62.encode(63) 와 Base64.encode(65)의 반환값이 둘 다 "AA")
이와 같은 이유로 두 알고리즘 다 base64를 살짝 변형시킨 알고리즘을 제공하였습니다.
하지만 멘토님께서 말씀해주신대로 테이블울 분리하게 되면 해결될 문제일 것 같습니다!!
| return new ErrorResponse( | ||
| detail, | ||
| instance, | ||
| LocalDateTime.now().toString()); | ||
| } |
| public record ErrorResponse( | ||
| String detail, | ||
| String instance, | ||
| String time) { |
There was a problem hiding this comment.
detail,instance,time 보다 일반적으로 자주 쓰이고 명확하게 내용을 알아볼수있는 필드명이 있을것 같아요
errorMessage, timestamp 등
There was a problem hiding this comment.
맞습니다. 특히 instance는 다른 개발자가 보기에 추측하기 힘든 필드명인 것 같습니다...
수정하도록 하겠습니다!!
|
|
||
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<Void> handleException(HttpServletRequest request, Exception e) { | ||
| log.error("Sever Exception Request URI {}: ", request.getRequestURI(), e); | ||
|
|
||
| return ResponseEntity.internalServerError().build(); | ||
| } |
There was a problem hiding this comment.
알수없는 예외에 대해서 로깅 error까지 한부분 좋습니다.
여기서 한가지, 500응답을 클라이언트에게 보여줘야 할까를 고민해볼 수 있을것같아요
500 응답과 응답에 에 포함된 상세한 정보는 악의적인 유저에게 서버의 취약점에 대한 힌트를 제공할 수 있기 때문이죠
There was a problem hiding this comment.
현재는 알수 없는 예외에 대해서는 상세정보는 알려주지 않고 단순히 서버측 에러임만을 알려주고 있습니다.
하지만 500 status를 반환하는 방식 자체가 어떤 요청이 서버에 에러를 유발하는지 힌트를 주게 되는 것이므로 500status 조차도 숨겨야 한다고 이해 하였는데 올바르게 이해한게 맞을까요??...
| private static StringBuilder getDetailMessage(BindException e) { | ||
| BindingResult bindingResult = e.getBindingResult(); | ||
| StringBuilder stringBuilder = new StringBuilder(); | ||
|
|
||
| for (FieldError fieldError : bindingResult.getFieldErrors()) { | ||
| stringBuilder.append(fieldError.getField()).append(":"); | ||
| stringBuilder.append(fieldError.getDefaultMessage()); | ||
| stringBuilder.append(", "); | ||
| } | ||
| return stringBuilder; | ||
| } |
There was a problem hiding this comment.
StringBuilder 대신 toString으로 반환해도 괜찮을것 같아요.
아니면 에러 필드와, 에러 메시지에 대한 객체를 만들고 List로 반환하는것도 고려할수있구요
| @Table(name = "sequence") | ||
| @Getter | ||
| @NoArgsConstructor | ||
| public class Sequence { |
There was a problem hiding this comment.
sequence는 sql 예약어라 많은 제약이 들어갈 수 있어요.
또한 헷갈림을 줄 수 있는 테이블명이라 생각되네요. 의미가 불명확하거든요. 어떤 sequence인가?
다른 이름을 사용하는것이 좋습니다.
| import java.net.URI; | ||
|
|
||
| @RestController | ||
| @RequestMapping("/api/urls") |
There was a problem hiding this comment.
현재 api라는 prefix가 꼭 붙어야 하는지 고민해보시면 좋을거같아요!
| } | ||
|
|
||
| @PostMapping(consumes = MediaType.APPLICATION_JSON_VALUE) | ||
| public ResponseEntity<UrlCreateApiResponse> createUrl(@RequestBody @Valid UrlCreateApiRequest request) { |
There was a problem hiding this comment.
너무 길어지면 내리는 것도 좋습니다~
| public ResponseEntity<UrlCreateApiResponse> createUrl(@RequestBody @Valid UrlCreateApiRequest request) { | |
| public ResponseEntity<UrlCreateApiResponse> createUrl( | |
| @RequestBody @Valid UrlCreateApiRequest request | |
| ) { |
| import jakarta.validation.constraints.NotNull; | ||
|
|
||
| public record UrlCreateApiRequest( | ||
| @NotBlank(message = "originUrl은 빈값이 들어올 수 없습니다.") String originUrl, |
| package com.young.shortenerurl.url.util; | ||
|
|
||
| public abstract class Encoder { | ||
| protected final Integer MAX_LENGTH = 8; |
| @Component | ||
| public class Base64EncoderV2 extends Encoder { | ||
| private static final int BASE62 = 64; | ||
| private static final String BASE62_CHAR = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789.~"; | ||
|
|
||
| @Override | ||
| public String encode(long index) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| while (index > 0) { | ||
| sb.append(BASE62_CHAR.charAt((int) (index % BASE62))); | ||
| index /= BASE62; | ||
| } | ||
|
|
||
| if (sb.length() > MAX_LENGTH) { | ||
| throw new IllegalArgumentException("변환된 url은 8자리를 넘어갈 수 없습니다."); | ||
| } | ||
| return sb.toString(); | ||
| } | ||
|
|
||
| } |
Optional의 orElseGet()메서드는 디폴트 값을 반환하는 용도가 알맞다고 생각하기에 방식 변경함.
📌 과제 설명
_ , -으로 대체한 알고리즘~ , .으로 대체한 알고리즘👩💻 요구 사항과 구현 내용