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

#6 여행지 관련 기본기능 구현 #25

Merged
merged 14 commits into from
Jan 4, 2024
Merged

Conversation

junmo95
Copy link
Member

@junmo95 junmo95 commented Jan 3, 2024

🎯 목적

  • 새 기능 (New Feature)

  • 리팩토링 (Refactoring)

  • 버그 수정 (Bug Fix)

  • 테스트 (Test)

  • CI/CD

  • 설정 (Setup)

  • 간략한 설명:
    : 여행지에 대한 기본 CRUD, 예외, querydsl 적용 및 구현


🛠 작성/변경 사항

  • 여행지 관련 기본 CRUD 구현
  • 여행지 관련 예외 구현
  • 조회 필터링을 위해 querydsl 적용

🔗 관련 이슈


💡 특이 사항

  • 주목할 사항
    • AppConfig.class 의 objectMapper 설정이 Json 직렬화, 역직렬화에 영향을 미쳐 관련 에러가 발생. 임시로 주석처리

📖 문서화

  • 관련 문서 링크


🧪 테스트

  • 테스트 계획 및 결과
    • (테스트계획)
    • (테스트결과)

📚 참조

  • 참조 링크

    • (링크1)
  • 참고 자료

    • (자료1)

- 기본적인 틀을 구현하기 위한 엔티티 설계 및 구현
- 기본적인 틀을 구현하기 위한 컨트롤러 및 DTO 설계 및 구현
- AppConfig 의 objectMapper가 Dto의 직렬화, 역직렬화에 영향을 끼쳐 오류가 발생함. 임시로 주석처리
- 기본적인 틀을 구현하기 위한 예외 구현
- 기본적인 틀을 구현하기 위한 서비스 구현
- 기본적인 틀을 구현하기 위한 레포지토리 구현
- querydsl 적용 및 커스텀 레포지토리
@junmo95 junmo95 added enhancement New feature or request feat 기능을 추가합니다. labels Jan 3, 2024
@junmo95 junmo95 self-assigned this Jan 3, 2024
Copy link

github-actions bot commented Jan 3, 2024

Test Results

1 tests  ±0   1 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit ef12d1c. ± Comparison against base commit 60cf680.

♻️ This comment has been updated with latest results.


@Service
@RequiredArgsConstructor
@Slf4j
Copy link
Member

Choose a reason for hiding this comment

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

클래스 계층에 @transactional(readOnly = true) 옵션을 하고,
저장, 삭제, 수정 메소드에만 @transactional을 붙이는 건 어떨까요?

.where(booleanBuilder)
.offset(pageable.getOffset())
.limit(pageable.getPageSize())
.fetchResults();
Copy link
Member

Choose a reason for hiding this comment

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

제가 알고 있기론 fetchResults()는 deprecated 되서 사용하지 않는게 좋을것 같습니다.
fetch()를 사용하고, count는 따로 카운트 쿼리를 사용하시는 건 어떤가요?

- 테스트 용을 남겨둔 코드 삭제
- 함수명 수정(toEntity, fromEntity)
- patch => put 방식 변경
- 엔티티 함수를 이용하여 모든 값이 들어있는 Dto 데이터로 엔티티 값을 대체하고, 더티채킹을 이용하여 업데이트.
- 기존에는 @ModelAttribute 활용, 하지만 필터링 요소가 아주 많을것 같지 않기에 우선은 @RequestParam통해 구현
- 위 변경에 따라 quetdsl 코드도 변경
- 기존에는 스프링 부트가 ObjectMapper를 자동 설정해주는데, 직접 설정하는 순간 설정한 내용만 적용되기에 place에서 사용하는 LocalTime 타입을 위한 설정 추가
- 불필요한 import 제거
Copy link
Member

@meena2003 meena2003 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!! :)

public class Place extends BaseTimeEntity {

@Id @GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
Copy link
Member

Choose a reason for hiding this comment

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

@column(name = "place_id") 추가하시면 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

이부분은 모든entity에 똑같이 적용되어야 하니 동민님 말씀처럼 추가되어야 하는게 맞는거 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

적용하겠습니다.


@PostMapping
public ResponseEntity<ResponseDTO<PlaceResponseDto>> placeAdd(
@RequestBody PlaceRequestDto requestDto
Copy link
Member

Choose a reason for hiding this comment

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

PlaceResponseDto와 ResponseDTO랑 'dto'가 통일이 된 것 같지 않은데 특별한 이유가 있나요?
없다면 하나로 통일하시는 게 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dto라고 생각하고 만들고 있었는데 DTO였군요. DTO로 맞추겠습니다.


return ResponseEntity
.status(responseBody.getCode())
.body(responseBody);
Copy link
Member

Choose a reason for hiding this comment

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

현재는 등록된 장소 데이터를 response로 프론트로 보내고 있잖아요? API 명세 작성할 때 프론트랑 논의해보면 되겠지만 어차피 프론트에서 GET 요청을 다시 보낼테니 id 값만 반환하면 충분하지 않을까.. 란 생각입니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 내용은 프론트랑 논의 후 변경하겠습니다.

// 프론트의 Page 정보 필요 유무에 따라 응답 객체 List, Page 나뉨
long total = from(place).where(booleanBuilder).fetchCount();

return new PageImpl<>(result, pageable, total);
Copy link
Member

Choose a reason for hiding this comment

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

PageableExecutionUtils도 한번 고려해보세요!
PageImpl()은 count 쿼리가 무조건 나가는 반면에, PageableExecutionUtils.getPage()는 굳이 카운트 쿼리가 나가지 않아도 되는 상황에선 쿼리를 날리지 않아 성능상 이점이 있다고 하네요.

Copy link
Member Author

Choose a reason for hiding this comment

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

말씀하신대로 page에 대한 세부정보를 가져오는데에 다소 성능적인 이슈가 있는 것은 인지하고있습니다.
해당 내용은 프론트에서 단순 List를 원하거나, 세부적이 Page 정보를 원하는 지에 따라 다르다고 생각합니다. 이 부분 후에 프론트와 협의 후 수정 예정입니다.

- id 명 수정
- null 불가 속성 설정
- storedCount 초기 값 설정
@junmo95 junmo95 merged commit a148409 into develop Jan 4, 2024
4 checks passed
@junmo95 junmo95 deleted the #6-place-basic-dev branch January 4, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feat 기능을 추가합니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants