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

[FIX] 포트폴리오 삭제 및 핀 포트폴리오 관련 오류 해결 #340

Closed
wants to merge 3 commits into from

Conversation

Goder-0
Copy link
Member

@Goder-0 Goder-0 commented May 8, 2024

📝 PR 타입

  • 기능 추가
  • 기능 수정
  • 기능 삭제
  • 리팩토링
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트

📝 반영 브랜치

📝 변경 사항

  • 포트폴리오 삭제 시, 핀 포트폴리오 자체는 남아있는 오류를 수정하였습니다
  • 프로필 편집 시, 핀 포트폴리오의 순서가 보장되지 않는 부분을 수정하였습니다.

📝 테스트 결과

📝 To Reviewer

  • 핀 포트폴리오 순서 관련하여, mysql의 field를 이용하였습니다. 이에 따라, 부득이하게 test db가 h2인 경우에는 해당 api에 관하여 오류가 날 수 있을 것 같습니다. 민규님의 생각이 궁금합니다! (관련 API : 프로필 편집)

@Goder-0 Goder-0 requested a review from mikekks May 8, 2024 18:47
@Goder-0 Goder-0 self-assigned this May 8, 2024
@Goder-0 Goder-0 linked an issue May 8, 2024 that may be closed by this pull request
2 tasks
Copy link
Contributor

@mikekks mikekks left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 조금 꼼꼼히 보면서 궁금한 부분 질문 남겼습니다 :)

Comment on lines -99 to 107
NumberExpression<Integer> orderByPin(List<Long> ids) {
// 포트폴리오 ID의 순서를 기준으로 순서를 정의합니다.
CaseBuilder caseBuilder = new CaseBuilder();
private OrderSpecifier<?> orderByPin(List<Long> ids) {
String template = "FIELD({0}, " + StringUtils.join(", ",
ids.stream().map(id -> "{" + (ids.indexOf(id) + 1) + "}").toArray()) + ")";

return caseBuilder
.when(portfolio.id.in(ids)).then(ids.indexOf(portfolio.id))
.otherwise(ids.size());
return Expressions.stringTemplate(template, Stream.concat(Stream.of(portfolio.id), ids.stream()).toArray())
.asc();
}
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
Member Author

@Goder-0 Goder-0 May 9, 2024

Choose a reason for hiding this comment

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

프로필 편집 시에, 핀 포트폴리오들의 순서를 지정할 수 있습니다.
이때 portfolios 라는 리스트 형태로 포트폴리오의 id값들을 받습니다 (ex : [3,2,1])

기존 코드에서는 해당 순서가 보장되지 않더라구요! 그래서 이를 mysql의 field를 이용하여 해결하였습니다.

관련 자료

위의 블로그의 글을 참고하였으나 잘 동작하지 않더라구요

관련 자료2
이 방법의 경우에는 저희 로직자체가 단순한 로직은 아니여서 네이티브 쿼리를 지양하고 싶었습니다!

  • 좀 더 자세히 말하자면, 사실 query 자체가 orderby field (portfolio.id,(2,1)) 이런 식으로 나가더라구요. 그래서 여러 방법 시도해보다가 저렇게 되었습니다.

yml에 아래 코드 추가하니 sql 파라미터 추적되더라고요 이도 이용하였습니다.

logging:
  level:
    org.hibernate.descriptor.sql: debug
    org.hibernate.orm.jdbc.bind: trace

Copy link
Contributor

@mikekks mikekks May 10, 2024

Choose a reason for hiding this comment

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

예전에 유튜브 개발바닥 리뷰 내용 중에 기억나는게 네이티브 쿼리는 최대한 지양하고, 만약 네이티브 쿼리로만 가능한 경우라면 설계가 잘못되었다는 얘기가 생각이 나는데요! 그런 측면에서 해당 부분을 네이티브 쿼리를 사용하지 않고, 구현해보면 좋을 것 같아요! 또한, h2 db와 호환이 되지 않는 이슈도 작은 이슈는 아니라고 생각합니다! 근데 이 부분은 당장 해결하긴 힘들 수도 있겠네요.

그래서 제 의견을 정리해보겠습니다!

  1. 성능(쿼리 횟수)를 좀 포기하더라도 가독성이 높고, 컴파일 레벨에서 에러 검출이 가능하도록(네이티브 쿼리를 사용하지 않고) 바꾸는 것이 가능할까요??
  2. h2 db와 호환될 수 있는 방법은 없을까요??

그래서 제가 대충 제안만 하는게 아니라 고민을 좀 해보려고 했는데 지금 다른 일 때문에 리뷰 자체가 조금 늦어질 것 같아서 일단 제 생각을 먼저 말씀드립니다! 저도 한 번 고민해보도록 하겠습니다!

하지만 현재 이 PR이 급하게 머지 되야 하는 상황 && 더 좋은 방법을 찾는데 시간이 많이 필요 하다면 일단 이렇게 진행하는게 최선일거 같네요! 이거에 대해서 의견 궁금합니당

@Goder-0
Copy link
Member Author

Goder-0 commented Oct 17, 2024

코드가 많이 변경되어 새로 이슈를 파서 진행하겠습니다.

@Goder-0 Goder-0 closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FIX] 포트폴리오 삭제시, 순서 재정렬 오류 해결
2 participants