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

feat : 커리큘럼 ID로 키워드들 조회 API 구현 #1472

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package wooteco.prolog.roadmap.application;

import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import wooteco.prolog.roadmap.application.dto.KeywordsResponse;
import wooteco.prolog.roadmap.domain.Curriculum;
import wooteco.prolog.roadmap.domain.Keyword;
import wooteco.prolog.roadmap.domain.repository.CurriculumRepository;
import wooteco.prolog.roadmap.domain.repository.KeywordRepository;
import wooteco.prolog.session.domain.Session;
import wooteco.prolog.session.domain.repository.SessionRepository;

@RequiredArgsConstructor
@Transactional
@Service
public class RoadMapService {

private final CurriculumRepository curriculumRepository;
private final SessionRepository sessionRepository;
private final KeywordRepository keywordRepository;

@Transactional(readOnly = true)
public KeywordsResponse findAllKeywords(final Long curriculumId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

KeywordService에 넣지 않은 이유가 있을지 궁금합니다!

final Curriculum curriculum = curriculumRepository.findById(curriculumId)
.orElseThrow(() -> new IllegalArgumentException(
"해당 커리큘럼이 존재하지 않습니다. curriculumId = " + curriculumId));
Copy link
Contributor

Choose a reason for hiding this comment

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

프로젝트에서 커스텀 예외를 사용하고 있으니 가급적이면 커스텀 예외를 활용해주시는 게 유지보수 측면에서 좋을 것 같습니다!


final Set<Long> sessionIds = sessionRepository.findAllByCurriculumId(curriculum.getId())
.stream()
.map(Session::getId)
.collect(Collectors.toSet());

final List<Keyword> keywords = keywordRepository.findBySessionIdIn(sessionIds);

return KeywordsResponse.createResponseWithChildren(keywords);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,12 @@ public static KeywordsResponse createResponse(final List<Keyword> keywords) {
.collect(Collectors.toList());
return new KeywordsResponse(keywordsResponse);
}

public static KeywordsResponse createResponseWithChildren(final List<Keyword> keywords) {
List<KeywordResponse> keywordsResponse = keywords.stream()
.filter(it -> it.getParent() == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분을 isTopKeyword()와 같은 메서드로 분리하면 코드 가독성이 더 좋아질 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영했습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

Keyword에게 최상위 키워드인지 물어보는 메서드를 만들고 참조해도 좋을 것 같아요!

근데 한 편으로 궁금한 점이 파라미터로 받은 키워드 리스트가 최상위 키워드인지 확인하고 파싱하는 작업이 괜찮을 지 의문입니다. 외부에서 최상위 키워드만 들어오는 것보다 지금이 나은 지 궁금해요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

개인적으로
최상위 키워드
ㄴ 자식 키워드

이렇게 보여주기 위함은 UI 적인 요소라 DTO에서도 해도 되지 않을까라는 생각이 들어요.

.map(KeywordResponse::createWithAllChildResponse)
.collect(Collectors.toList());
return new KeywordsResponse(keywordsResponse);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package wooteco.prolog.roadmap.domain.repository;

import java.util.List;
import java.util.Set;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;
Expand All @@ -17,4 +18,6 @@ public interface KeywordRepository extends JpaRepository<Keyword, Long> {
@Query("SELECT k FROM Keyword k "
+ "WHERE k.sessionId = :sessionId AND k.parent IS NULL")
List<Keyword> findBySessionIdAndParentIsNull(@Param("sessionId") Long sessionId);

List<Keyword> findBySessionIdIn(final Set<Long> sessionIds);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package wooteco.prolog.roadmap.ui;

import lombok.RequiredArgsConstructor;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import wooteco.prolog.roadmap.application.RoadMapService;
import wooteco.prolog.roadmap.application.dto.KeywordsResponse;

@RequiredArgsConstructor
@RestController
public class RoadmapController {

private final RoadMapService roadMapService;

@GetMapping("/roadmaps")
public KeywordsResponse findKeywords(@RequestParam final Long curriculumId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

로드맵 대격변 패치를 위하신 거였나보군요!

public RoadmapResponse findByCurriculumId(@RequestParam final Long curriculumId)

Roadmap이라는 새로운 비즈니스 용어를 코드에서 사용하신다면 위와 같은 시그니처가 유지보수면에서 더 좋을 것 같다고 생각합니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

findRoadMapKeyword 로 수정했습니다!

return roadMapService.findAllKeywords(curriculumId);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package wooteco.prolog.roadmap.application;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.Mockito.when;

import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import wooteco.prolog.roadmap.application.dto.KeywordsResponse;
import wooteco.prolog.roadmap.domain.Curriculum;
import wooteco.prolog.roadmap.domain.Keyword;
import wooteco.prolog.roadmap.domain.repository.CurriculumRepository;
import wooteco.prolog.roadmap.domain.repository.KeywordRepository;
import wooteco.prolog.session.domain.Session;
import wooteco.prolog.session.domain.repository.SessionRepository;

@ExtendWith(MockitoExtension.class)
class RoadMapServiceTest {

@Mock
private CurriculumRepository curriculumRepository;
@Mock
private SessionRepository sessionRepository;
@Mock
private KeywordRepository keywordRepository;
@InjectMocks
private RoadMapService roadMapService;

@Test
@DisplayName("curriculumId가 주어지면 해당 커리큘럼의 키워드들을 전부 조회할 수 있다.")
void findAllKeywords() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

커리큘럼이 없는 분기도 테스트하면 좋겠어요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

커리큘럼은 controller에서 RequestParam으로 받고 있습니다.
이때 커리큘럼 ID가 존재하지 않으면 아예 서비스 쪽으로 들어오지 않을텐데 예외 테스트가 필요할까요??

Copy link
Contributor

@nuyh99 nuyh99 Aug 3, 2023

Choose a reason for hiding this comment

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

  1. API 요청 시 아무 Id를 적었을 때
  2. 해당 서비스를 다른 컨트롤러에서 사용하면 1번의 문제가 동일하게 발생
  3. 해당 서비스를 다른 서비스에서 의존할 때

위와 같은 상황에서 문제가 될 것 같아요!

        final Curriculum curriculum = curriculumRepository.findById(curriculumId);

만약 커리큘럼이 존재하지 않는 상황이 일어날 수 없다면 위와 같은 형태로 메서드 내에서 사용해야 하지 않을까요?!

//given
final Curriculum curriculum = new Curriculum(1L, "커리큘럼1");
final Session session = new Session(1L, curriculum.getId(), "세션1");
final List<Session> sessions = Arrays.asList(session);
final Keyword keyword = Keyword.createKeyword("자바1", "자바 설명1", 1, 5, session.getId(),
null);

when(curriculumRepository.findById(anyLong()))
.thenReturn(Optional.of(curriculum));

when(sessionRepository.findAllByCurriculumId(anyLong()))
.thenReturn(sessions);

when(keywordRepository.findBySessionIdIn(any()))
.thenReturn(Arrays.asList(keyword));

final KeywordsResponse expected = KeywordsResponse.createResponse(Arrays.asList(keyword));

//when
final KeywordsResponse actual =
roadMapService.findAllKeywords(curriculum.getId());

//then
assertThat(actual)
.usingRecursiveComparison()
.isEqualTo(expected);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertAll;

import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import javax.persistence.EntityManager;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import wooteco.prolog.roadmap.domain.Curriculum;
import wooteco.prolog.roadmap.domain.Keyword;
import wooteco.prolog.roadmap.domain.repository.CurriculumRepository;
import wooteco.prolog.roadmap.domain.repository.KeywordRepository;
import wooteco.prolog.session.domain.Session;
import wooteco.prolog.session.domain.repository.SessionRepository;
Expand All @@ -23,6 +27,8 @@ class KeywordRepositoryTest {
private SessionRepository sessionRepository;
@Autowired
private EntityManager em;
@Autowired
private CurriculumRepository curriculumRepository;

@Test
void 부모_키워드와_1뎁스까지의_자식_키워드를_함께_조회할_수_있다() {
Expand Down Expand Up @@ -105,7 +111,54 @@ class KeywordRepositoryTest {
sessionId);

// then
Assertions.assertThat(extractParentKeyword.size()).isEqualTo(1);
assertThat(extractParentKeyword.size()).isEqualTo(1);
}

@Test
@DisplayName("세션 ID 리스트로 키워드 리스트를 조회한다")
void findBySessionIdIn() {
//given
final Curriculum curriculum = curriculumRepository.save(new Curriculum("커리큘럼1"));

final Session session1 = sessionRepository.save(new Session(curriculum.getId(), "세션1"));
final Session session2 = sessionRepository.save(new Session(curriculum.getId(), "세션2"));
final Session session3 = sessionRepository.save(new Session(curriculum.getId(), "세션3"));
final Session session4 = sessionRepository.save(new Session(curriculum.getId(), "세션4"));
final Session session5 = sessionRepository.save(new Session(curriculum.getId(), "세션5"));

final Keyword keyword1 = keywordRepository.save(
Keyword.createKeyword("자바1", "자바 설명1", 1, 5, session1.getId(), null));
final Keyword keyword2 = keywordRepository.save(
Keyword.createKeyword("자바2", "자바 설명2", 2, 5, session1.getId(), keyword1));
final Keyword keyword3 = keywordRepository.save(
Keyword.createKeyword("자바3", "자바 설명3", 3, 5, session1.getId(), null));
final Keyword keyword4 = keywordRepository.save(
Keyword.createKeyword("자바4", "자바 설명4", 4, 5, session1.getId(), keyword3));
keywordRepository.save(
Keyword.createKeyword("자바5", "자바 설명5", 5, 5, session2.getId(), null));
keywordRepository.save(
Keyword.createKeyword("자바6", "자바 설명6", 6, 5, session2.getId(), keyword1));
keywordRepository.save(
Keyword.createKeyword("자바7", "자바 설명7", 7, 5, session2.getId(), null));
keywordRepository.save(
Keyword.createKeyword("자바8", "자바 설명8", 8, 5, session3.getId(), keyword2));
final Keyword keyword9 = keywordRepository.save(
Keyword.createKeyword("자바9", "자바 설명9", 9, 5, session4.getId(), keyword2));
final Keyword keyword10 = keywordRepository.save(
Keyword.createKeyword("자바10", "자바 설명10", 10, 5, session5.getId(), null));

final HashSet<Long> sessionIds = new HashSet<>(
Arrays.asList(session1.getId(), session4.getId(), session5.getId())
);

//when
final List<Keyword> keywords = keywordRepository.findBySessionIdIn(sessionIds);

//then
assertThat(keywords)
.usingRecursiveComparison()
.ignoringFields("id", "parent.id")
.isEqualTo(Arrays.asList(keyword1, keyword2, keyword3, keyword4, keyword9, keyword10));
}

private Keyword createKeywordParent(final Keyword keyword) {
Expand Down