-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/admin concert place #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 괜찮아보입니다..! 이전 버전을 접근하는 use case 는 없을까요?
public ResponseEntity<ResponseDto<String>> customException(CustomException exception) { | ||
ResponseDto<String> responseDto = new ResponseDto<>(exception.getStatus().toString(), | ||
exception.getMessage()); | ||
log.warn("CustomRuntimeException 발생 : code: {}, message: {}", responseDto.getCode(), responseDto.getBody()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error 로 찍어도 괜찮을 것 같습니다.
public ResponseEntity<ResponseDto<String>> customException(MethodArgumentNotValidException exception) { | ||
HttpStatus badRequest = HttpStatus.BAD_REQUEST; | ||
ResponseDto<String> responseDto = new ResponseDto<>(badRequest.toString(), "요청 값이 잘 못 됐습니다"); | ||
log.warn("MethodArgumentNotValidException 발생 : message: {}", exception.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
같은 피드백입니다.
public class Place { | ||
@Id | ||
@GeneratedValue(strategy = GenerationType.IDENTITY) | ||
private Long id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 장소에 대한 수정이 들어가는 경우 Update 로 하실 거인거죠?
}) | ||
@DynamicInsert | ||
@EntityListeners(AuditingEntityListener.class) | ||
public class PlaceVersion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
버전을 별도 엔티티로 관리하시려는 이유가 있으실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백드렸습니다.
.address(address) | ||
.identifier(identifier) | ||
.version(beforePlace.getNextVersion()) | ||
.last(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get 에서 last 를 셋팅하는 write operation 이 같이 있는데요. 분리하면 좋을 것 같습니다.
함수는 getNextPlace 로 DB 변경이 없을 것이 기대되지만 실재로는 beforePlace 의 last 가 Update 되는 함수라 안정적이지 않아보입니다.
차라리 last 를 별도 루틴에서 셋팅하게해서 read / write 를 분리하는게 좋을것 같습니다.
public Place getLastVersion(String identifier) { | ||
return placeRepository.findTopByIdentifierOrderByVersionDesc(identifier) | ||
.orElseThrow(() -> new ResourceNotFoundException("place", identifier)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사소한 부분이지만 나중에는 Service 레벨에서는 Entity 를 직접 다루기 보다 VO 를 사용하는 연습해보시면 좋을것 같습니다.
Quality Gate passedIssues Measures |
개요
관리자 관련 장소 API 개발
상세정보