Refactory of PR #13 #14
Conversation
게시글 생성 기능 추가
Article Repository - findByTitle 메서드 추가 Test code - 모든 article 받아오기 (findAll) - article id 체크 (findById) - 제목으로 검색 (findByTitle)
findByTitleLikeOrContentLike() Title과 Content를 like로 검색
test code를 통해 article 데이터 수정 구현
test code를 통해 article 삭제 구현
commentService.create를 통해 댓글 저장 기능 추가
CommentService.getComment를 통해 댓글 내용 확인 기능 추가
id를 통해 comment를 받아서 바로 결과를 테스트합니다
나중에 로그인 인증 후 CRUD을 수행할 수 있게 추가하겠습니다
comment 저장 시 로그인된 유저를 저장
Article에 user 정보가 함께 저장됩니다
article의 controller와 service에서 실행되어야 할 기능 메서드 추가
로그인 권한을 확인하기 위해 Principal 객체 추가 Principal로 로그인 권한 확인 후 수정, 삭제 기능 수행
Article과 Comment에 Swagger 어노테이션 추가
Article controller에 UserService가 제대로 DI되지 않아 발생한 오류 해결
Article을 생성할 때 발생하는 sendError() 해결 - Entity에 @JsonBackReference 추가
article에 dto를 적용하여 DB에 저장
- DTO 생성 - @PathVariable에서 @parameter 제거 - 권한 ROLE_CLIENT 변경
댓글 생성과 읽기에 대한 기능 추가
댓글 수정, 삭제 기능 구현
Article과 Comment의 Controller에 있는 mapping 경로를 HTTP Method에 맞게 수정
HttpServletRequest 파라미터를 Swagger에 노출되지 않게 hidden 옵션으로 수정
@PathVariable에 Swagger 파라미터 API인 @parameter(description) 추가
사용자에게 노출되지 않는 Service에 Swagger documention 제거
제목으로 게시글 검색 기능 추가
- url mapping 혼란을 막기 위해 /id/{id}, /title/{title}로 변경
- 검색 결과가 없을 경우를 대비하여 repository 메서드 리턴 타입을 Optional으로 수정
Optional 타입의 객체에서 if-else 대신 orElseThrow()로 사용
|
PR 한 번 하면 이때까지 브랜치에 커밋했던 모든 게 한꺼번에 올라가네요.. |
간단한 홈페이지를 추가했습니다.
- 보안 주석 처리 제거
- Permit access to the homepage ("/") for all users
- 컨트롤러 로직과 서비스 로직 분리 필요
- MapStruct를 이용한 매핑 라이브러리 적용 - MapStruct toDto TestCode 작성 - Article Entity의 createdDate, ModiftDate 클래스를 TimeStamp 로 변환
src/main/java/com/gdsc/blog/article/service/ArticleService.java
Outdated
Show resolved
Hide resolved
| List<ArticleDto> articleList = new ArrayList<>(); | ||
| articles.forEach(article -> articleList.add(articleMapper.toDto(article))); |
There was a problem hiding this comment.
이런 경우에 ArrayList는 불리할 수 있습니다.
- 길이가 계속 늘어나게 되는 추가 위주의
List일 경우(ArrayList는 Capacity에 도달 시 작업을 잠깐 멈추고 뒤에 큰 Array를 하나 붙여 늘리는 식) - 랜덤 접근보다 순차적 접근 오퍼레이션이 많을 시(불리하다기 보다는, 강점이 없는 거긴 합니다)
이 상황에서, articles가 articleList에 ArticleDto 타입으로 쌓이는 게 원하는 오퍼레이션이라면, 이 소스코드에서는 ArrayList의 한계치에 도달할 때마다 ArrayList의 길이를 늘리는 "좋지 않은" 동작이 자주 발생하게 되겠죠,
단례로, 기본적인 ArrayList의 Cap은 10이기 때문에, 50개의 게시글을 돌리게 되면 3번의 사이즈 증가 Operation과 함께 80사이즈의 ArrayList가 생성되게 될 것입니다.
따라서, 이 동작을 가능한 한 피하는 방향으로 LinkedList(순차적 추가/삭제 오퍼레이션에 유리한 리스트) 혹은 .map() 메소드를 사용하는 것이 더 나은 방향으로 보여요.
| List<ArticleDto> articleList = new ArrayList<>(); | |
| articles.forEach(article -> articleList.add(articleMapper.toDto(article))); | |
| // Map article entity list to dto list | |
| List<ArticleDto> articleList = articles.map(articleMapper::toDto); |
There was a problem hiding this comment.
private void grow(int minCapacity) {
// overflow-conscious code
int oldCapacity = elementData.length;
int newCapacity = oldCapacity + (oldCapacity >> 1);
if (newCapacity - minCapacity < 0)
newCapacity = minCapacity;
if (newCapacity - MAX_ARRAY_SIZE > 0)
newCapacity = hugeCapacity(minCapacity);
// minCapacity is usually close to size, so this is a win:
elementData = Arrays.copyOf(elementData, newCapacity);
}해당 사례에 대한 참고 자료입니다! ArrayList의 사이즈 증가 메소드에요
src/main/java/com/gdsc/blog/article/service/ArticleService.java
Outdated
Show resolved
Hide resolved
| // article list to article dto list | ||
| List<ArticleDto> articleList = new ArrayList<>(); | ||
| articles.forEach(article -> articleList.add(articleMapper.toDto(article))); |
There was a problem hiding this comment.
이 부분도, +69~+70 부분의 리뷰를 참고하여 고쳐 주시기 바랍니다.
src/main/java/com/gdsc/blog/article/service/ArticleService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Generated 코드가 깃으로 올라오면 곤란한데... 이거는 실수로 올라온 건지 확인이 필요할 거 같네요
| // Disable CSRF (cross site request forgery) | ||
| http.csrf(AbstractHttpConfigurer::disable); |
There was a problem hiding this comment.
CSRF는 Production 환경에서는 켜져 있는 게 좋을 거 같은데...스위칭이 가능하게 구현되면 좋을 거 같습니다
There was a problem hiding this comment.
JWT 토큰을 이용하여 서비스를 이용하는 부분에선 CSRF를 비활성화 하는 게 맞다고 알고 있습니다.
제가 잘못 알고있는 부분일까요?
| final User user = userRepository.findByUsername(username).get(); | ||
|
|
||
| if (user == null) { | ||
| throw new UsernameNotFoundException("User '" + username + "' not found"); | ||
| } |
There was a problem hiding this comment.
이 부분에서, optional.get()은 데이터를 반환하고 없거나 null일 경우 NoSuchElement를 throw하기 때문에, 아래의 if문이 절대로 동작하지 않게 됩니다.
https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#get--
따라서, 의도하고자 하는 동작을 위해서는 .orElse(null) 혹은 .orElseThrow() 메소드를 사용해야 합니다.
| final User user = userRepository.findByUsername(username).get(); | |
| if (user == null) { | |
| throw new UsernameNotFoundException("User '" + username + "' not found"); | |
| } | |
| // Example 1(추천 안함) | |
| final User user = userRepository.findByUsername(username).orElse(null); | |
| if (user == null) { | |
| throw new UsernameNotFoundException("User '" + username + "' not found"); | |
| } | |
| // Example 2 | |
| final User user = userRepository.findByUsername(username).orElseThrow(() -> | |
| new UsernameNotFoundException("User '" + username + "' not found")); | |
| } catch (Exception ex) { | ||
| // this is very important, since it guarantees the user is not authenticated at all | ||
| SecurityContextHolder.clearContext(); | ||
| httpServletResponse.sendError(HttpServletResponse.SC_BAD_REQUEST, ex.getMessage()); |
There was a problem hiding this comment.
필터가 실패했을 경우 bad request보다는 unauthorised 혹은 forbidden이 HTTP 코드 상 조금 더 전하고자 하는 바에 일치하지 않을까 싶습니다!
cmsong111
force-pushed
the
feature-refactory
branch
from
654fb40 to
53fcee5
Compare
#13 에서 코드 리뷰 내용 바탕으로 리펙토링 진행했습니다