-
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
feat: 개발질문 게시판의 해시태그 기능 구현 완료 #629
base: develop
Are you sure you want to change the base?
Conversation
@@ -4,6 +4,7 @@ | |||
import io.swagger.v3.oas.annotations.tags.Tag; | |||
import jakarta.validation.Valid; | |||
import lombok.RequiredArgsConstructor; | |||
import org.apache.coyote.BadRequestException; |
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.
불필요한 클래스가 import되었어요. 제거 부탁드러요.
RegisterBoardHashtagPort, RetrieveBoardHashtagPort { | ||
|
||
private final BoardHashtagRepository boardHashtagRepository; | ||
private final BoardHashtagMapper mapper; |
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.
현재 프로젝트의 PersistenceAdapter
클래스에서 변수명으로 repository
, mapper
를 사용하는 경우, 도메인 이름을 접두어로 붙이는 방식과 붙이지 않는 방식이 혼재되어 있어요. 우선 해당 클래스 내에서 하나의 방식으로 통일하여 변수명을 변경하고, 이후에 전체적인 변수명 규칙에 대해 논의하는 시간을 가질 필요가 있어보여요.
List<BoardHashtagJpaEntity> findAllByBoardId(Long boardId); | ||
|
||
@Query(value = "SELECT b.* FROM board_hashtag b WHERE b.board_id = :boardId", nativeQuery = true) | ||
List<BoardHashtagJpaEntity> findAllIncludingDeletedByBoardId(Long boardId); |
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.
@Query(
value = "SELECT b.* " +
"FROM board_hashtag b " +
"WHERE b.board_id = :boardId",
nativeQuery = true
)
위 예시처럼 적절히 개행을 추가해주면 가독성 개선에 도움이 될 것 같아요.
@RequiredArgsConstructor | ||
public class BoardHashtagRepositoryImpl implements BoardHashtagRepositoryCustom { | ||
|
||
private final JPAQueryFactory queryFactory; |
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.
기존에 사용하던 QueryDSL을 대신하여 JPQL을 선택하게 된 이유가 궁금해요. 간단히 설명 부탁드려도 될까요?
@RequiredArgsConstructor | ||
public class BoardHashtagDtoMapper { | ||
|
||
public BoardHashtag fromDto(Long boardId, Long hashtagId){ |
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.
)
와 {
사이에 공백이 누락되었어요.
|
||
private final RegisterHashtagUseCase registerHashtagUseCase; | ||
|
||
@Operation(summary = "[U] 해시태그 생성", description = "ROLE_USER 이상의 권한이 필요함") |
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.
제가 생각했을 때에는 velog
에서 해시태그를 추가할 때처럼, 사용자가 직접 본인만의 해시태그를 만들어서 추가하는 방식으로 하면 원하는 해시태그가 없을 때에도 손쉽게 추가할 수 있어 자유도가 증가할 것이라고 생각했는데,
블로그의 게시글과 커뮤니티 게시글의 특성이 다르다보니 해시태그가 악용될 수 있다는 생각도 들긴 하네요.
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 Hashtag { | ||
|
||
Long id; | ||
String name; |
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.
해시태그의 중복 등록을 방지하기 위해 유니크 제약 조건을 추가하는 것이 필요해 보여요.
import page.clab.api.domain.community.board.domain.BoardHashtag; | ||
|
||
public interface ExternalRetrieveBoardHashtagUseCase { | ||
List<BoardHashtagResponseDto> getBoardHashtagInfoByBoardId(Long boardId); |
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.
{
와 메소드 선언부 사이에 개행이 누락되었어요.
for (Long hashtagId : requestDto.getHashtagIdList()) { | ||
if (!externalRetrieveHashtagUseCase.existById(hashtagId)) { | ||
throw new NotFoundException("[Hashtag] id: " + hashtagId + "에 해당하는 해시태그가 존재하지 않습니다."); | ||
}; |
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.
해당 내용은 getById()
메소드를 추가로 작성하여, PersistenceAdapter
에서 담당하도록 하는게 책임 분리에 더욱 적합해 보여요.
import page.clab.api.domain.community.hashtag.domain.Hashtag; | ||
|
||
public interface ExternalRetrieveHashtagUseCase { | ||
Hashtag getById(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.
{
와 메소드 선언부 사이에 개행이 누락되었어요.
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.
작업량이 정말 많으셨을 것 같은데, 빠르게 진행해주신 것 같아 놀랐습니다! 고생하셨습니다.
일단, 저는 해시태그에 대해 그냥 List<String>
으로 관리하고, Board를 저장할 때에도 String
으로 받아 개발 질문 게시글에 대해서만 반환하도록 하는 걸로 간단하게 생각했었는데, Hashtag
라는 새로운 도메인을 만드실 줄은 몰랐습니다..!
HashTag
엔티티를 사용하는게 아니라 그냥 String
으로만 관리하면 간단할 것 같은데 Hashtag
도메인을 새로 구현하신 이유가 있으신지 궁금합니다.
"현재는 카테고리가 개발질문인 게시글만 해시태그가 적용되어 있어서 해당 API의 응답으로 개발질문 게시판만 반환됨") | ||
@PreAuthorize("hasRole('GUEST')") | ||
@GetMapping("/hashtag") | ||
public ApiResponse<PagedResponseDto<BoardOverviewResponseDto>> retrieveBoardsByCategory( |
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.
이부분 메서드 네임을 retrieveBoardsByHashtag
로 변경해도 괜찮을 것 같아요
} | ||
|
||
@Override | ||
public List<BoardHashtag> getAllByBoardId(Long boardId) { |
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.
이전에 코드베이스 전반 검토할 때, findBy
는 예외처리를 하지 않고 조회하는 메서드, getBy
는 예외처리까지 포함되어 조회하는 메서드에 적용하기로 했어서, 작업하신 메서드 이름들을 올바른 이름으로 바꾸는 작업이 필요할 것 같아요.
.build(); | ||
} | ||
|
||
public BoardHashtagRequestDto toDto(Long boardId, List<Long> hashtagIdList) { |
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.
List<Long> hashtagIdList
-> List<Long> hashtagIds
저희 프로젝트에서 리스트를 변수로 표현할 때 뒤에 List
를 붙이는 게 아니라 복수형으로 표현하는 것으로 알고있어서 변경 제안 드립니다.
throw new InvalidBoardCategoryHashtagException("개발질문 게시판에만 해시태그를 등록할 수 있습니다."); | ||
} | ||
if (savedBoard.isDevelopmentQna() && requestDto.getHashtagIdList() != null) { | ||
externalRegisterBoardHashtagUseCase.registerBoardHashtag(boardHashtagDtoMapper.toDto(savedBoard.getId(), requestDto.getHashtagIdList())); |
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.
제가 잘못 이해하고 있는건지는 모르겠지만, 굳이 BoardHashtag
등록을 external에서 끌고와서 해야 하는지 모르겠습니다.
Board
도메인 안에서 처리가 불가능한 것인가요?
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.
Board
도메인 내에서 BoardHashtag
의 저장 및 조회를 externalRegisterBoardHashtagUseCase
, externalRetrieveBoardHashtagUseCase
를 통해 수행하고 있는 것 같은데, Board
도메인의 서비스 계층에서 담당해도 괜찮지 않을까 생각합니다.
}); | ||
}); | ||
|
||
hashtagsToAdd.forEach(idToAdd -> addHashtag(board.getId(), idToAdd, currentBoardHashtags)); |
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.
BoardHashtag
를 업데이트하는 주요 비즈니스 로직인데, 메서드 하나에 너무 많은 로직이 들어가 있기도 하고, 인덴트도 깊어서 보기 힘들거나 이해하는데 오랜 시간이 소요될 수 있을 것 같아요.
인덴트가 깊거나 한 줄이 너무 긴 부분은 메서드로 추출해서 로직의 흐름을 한 눈에 파악할 수 있도록 하는 것도 괜찮아 보입니다.
|
||
private final RegisterHashtagUseCase registerHashtagUseCase; | ||
|
||
@Operation(summary = "[U] 해시태그 생성", description = "ROLE_USER 이상의 권한이 필요함") |
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.
제가 생각했을 때에는 velog
에서 해시태그를 추가할 때처럼, 사용자가 직접 본인만의 해시태그를 만들어서 추가하는 방식으로 하면 원하는 해시태그가 없을 때에도 손쉽게 추가할 수 있어 자유도가 증가할 것이라고 생각했는데,
블로그의 게시글과 커뮤니티 게시글의 특성이 다르다보니 해시태그가 악용될 수 있다는 생각도 들긴 하네요.
@RequiredArgsConstructor | ||
public class HashtagDtoMapper { | ||
|
||
public Hashtag fromDto(String name) { |
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.
이부분은 Dto에서 Hashtag
를 생성하는 것이 아니기 때문에 메서드 이름을 of
로 해야 맞을 것 같아요.
|
||
import page.clab.api.domain.community.hashtag.domain.Hashtag; | ||
|
||
public interface RetrieveHashtagUseCase { |
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 interface RegisterHashtagPort { | ||
Hashtag save(Hashtag hashtag); | ||
|
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.
개행을 하지 않아도 될 것 같습니다.
Summary
커뮤니티 게시판 중에서 개발질문 카테고리에 대해서 해시태그를 설정할 수 있는 기능을 개발했습니다.
Tasks
(해시태그를 먼저 등록하고, 개발질문 게시판에 해시태그id를 연결하는 형태입니다.)
(개발질문 게시판 전용으로 dto를 만드는 선택지도 있었지만, @Jeong-Ag 님과 회의 후에 결정했습니다. 모든 게시판의 요청 dto에 해시태그 id list를 담아서 전달할 수 있습니다. 이때 null이어도 괜찮습니다. 이는 현재 개발질문 게시판에서만 해시태그 기능을 사용할 수 있지만 추후 확장성을 고려한 선택이었습니다.)
(게시글을 수정할 때 해시태그 수정도 함께 할 수 있도록 했습니다. 이때는 기존에 있던 해시태그 정보와 새로운 해시태그 정보를 비교해서 소프트 딜리티를 수정하거나 새롭게 등록할 수 있도록 로직을 작성했습니다.)
(해시태그를 여러개 전달하면 페이지네이션을 적용해서 해당 해시태그를 모두 포함하는 게시글을 반환합니다.)
ETC
리뷰 해주실 때 로직에 대한 피드백이나 성능 피드백 많이 해주시면 반영하겠습니다! 다만 수정된 API 연동이 최대한 빨리 진행되어야 해서, 성능 피드백은 PR 머지 후 다른 이슈에서 진행하고자 합니다.
SQL문
hashtag
BoardHashtag
Screenshot
해시태그 생성 API
개발질문 게시판이 아닌 경우 해시태그 등록 시 예외처리. 해시태그가 없다면 정상 등록 가능.
게시글 상세조회 API에서 해시태그 정보를 함께 반환하도록 수정함. 해시태그가 없는 경우는 빈 리스트를 반환. 다른 게시글 조회 관련 API도 마찬가지로 수정완료.
게시글 수정 API에서 해시태그 정보를 받도록 수정. 이때 카테고리가 개발질문인 것만 해시태그 정보를 수정가능하도록 예외처리
해시태그가 모두 포함된 게시글 조회
[자료구조 해시태그]
[JAVA, C++ 해시태그]