develop 변경 내용 main으로 머지#41
Conversation
update(global): ci의 자바 버전 21에서 17로 변경
user: 프론트 이메일 인증 리다이렉트 주소 변경
(refactor)image: minio yml값 수정
user: 권한 로직 수정
jwt: 권한이 필요없는 endpoint는 필터 안거치도록 수정
Summary of ChangesHello @ZaMan-O, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
develop 브랜치의 변경 내용을 main으로 병합하는 PR이군요. 전반적으로 코드의 일관성을 높이고 도메인 모델을 개선하는 좋은 리팩토링이 많이 포함되어 있습니다.
- Spring Data JPA 쿼리 메서드 명명 규칙에 맞게
findByIdAndUser_Id등으로 수정한 점이 좋습니다. - 댓글 수 증감 로직을 Repository의
@Modifying쿼리에서 도메인 엔티티의 메서드로 옮겨, 객체지향적인 설계를 강화한 점도 훌륭합니다. - Minio 관련 설정 프로퍼티 키를
spring.minio.*로 통일하여 일관성을 확보했습니다.
다만, 몇 가지 중요한 개선점을 발견하여 리뷰 코멘트를 남겼습니다.
PortfolioService의delete메서드는 트랜잭션 처리와 권한 검사 순서에 심각한 문제가 있어 데이터 유실 위험이 있습니다. (critical)BlogService의delete메서드는 트랜잭션 내에서 외부 서비스를 호출하여 데이터 불일치를 유발할 수 있습니다. (high)JwtHeaderFilter에 추가된 경로는SecurityConfig와 중복 관리될 수 있고,startsWith를 사용한 경로 비교는 잠재적인 위험을 가질 수 있어 개선이 필요합니다. (medium)
자세한 내용은 각 파일의 코멘트를 확인해주세요.
| public void delete(Long userId, Long portfolioId) { | ||
| List<PortfolioImageJpaEntity> images = | ||
| portfolioImageRepository.findByPortfolioId(portfolioId); | ||
| portfolioImageRepository.findByPortfolio_Id(portfolioId); | ||
|
|
||
| for (PortfolioImageJpaEntity img : images) { | ||
| imageService.deleteImage(img.getImageUrl()); | ||
| } | ||
| PortfolioJpaEntity portfolio = portfolioRepository.findByIdAndUserId(portfolioId, userId) | ||
| PortfolioJpaEntity portfolio = portfolioRepository.findByIdAndUser_Id(portfolioId, userId) | ||
| .orElseThrow(() -> new ExpectedException(HttpStatus.NOT_FOUND, "존재하지 않는 포트폴리오입니다.")); | ||
|
|
||
| portfolioRepository.delete(portfolio); |
There was a problem hiding this comment.
이 delete 메서드에는 몇 가지 심각한 문제가 있습니다.
- 트랜잭션 부재: 메서드에
@Transactional어노테이션이 없어, 이미지 삭제와 포트폴리오 삭제 작업이 원자적으로 처리되지 않습니다. 중간에 오류가 발생하면 데이터가 불일치 상태에 빠질 수 있습니다. - 잘못된 작업 순서 및 권한 검사: 포트폴리오 삭제 권한을 확인하기 전에 외부 저장소의 이미지를 먼저 삭제하고 있습니다. 만약 사용자가 권한이 없거나 포트폴리오가 존재하지 않아
findByIdAndUser_Id에서 예외가 발생하면, 이미지는 삭제되었지만 포트폴리오는 그대로 남아있는 심각한 데이터 불일치가 발생합니다.
아래와 같이 수정하여 문제를 해결하는 것을 강력히 권장합니다.
@Transactional어노테이션을 추가하여 모든 DB 작업을 하나의 트랜잭션으로 묶습니다.- 가장 먼저 포트폴리오 존재 여부와 사용자 권한을 확인합니다.
| public void delete(Long userId, Long portfolioId) { | |
| List<PortfolioImageJpaEntity> images = | |
| portfolioImageRepository.findByPortfolioId(portfolioId); | |
| portfolioImageRepository.findByPortfolio_Id(portfolioId); | |
| for (PortfolioImageJpaEntity img : images) { | |
| imageService.deleteImage(img.getImageUrl()); | |
| } | |
| PortfolioJpaEntity portfolio = portfolioRepository.findByIdAndUserId(portfolioId, userId) | |
| PortfolioJpaEntity portfolio = portfolioRepository.findByIdAndUser_Id(portfolioId, userId) | |
| .orElseThrow(() -> new ExpectedException(HttpStatus.NOT_FOUND, "존재하지 않는 포트폴리오입니다.")); | |
| portfolioRepository.delete(portfolio); | |
| @Transactional | |
| public void delete(Long userId, Long portfolioId) { | |
| PortfolioJpaEntity portfolio = portfolioRepository.findByIdAndUser_Id(portfolioId, userId) | |
| .orElseThrow(() -> new ExpectedException(HttpStatus.NOT_FOUND, "존재하지 않는 포트폴리오입니다.")); | |
| List<PortfolioImageJpaEntity> images = | |
| portfolioImageRepository.findByPortfolio_Id(portfolioId); | |
| for (PortfolioImageJpaEntity img : images) { | |
| imageService.deleteImage(img.getImageUrl()); | |
| } | |
| portfolioRepository.delete(portfolio); | |
| } |
| public void delete(Long userId, Long blogId) { | ||
| List<BlogImageJpaEntity> images = | ||
| blogImageRepository.findByBlogId(blogId); | ||
| blogImageRepository.findByBlog_Id(blogId); | ||
|
|
||
| for (BlogImageJpaEntity img : images) { | ||
| imageService.deleteImage(img.getImageUrl()); | ||
| } | ||
|
|
||
| BlogJpaEntity blog = blogRepository.findByIdAndUserId(blogId, userId) | ||
| BlogJpaEntity blog = blogRepository.findByIdAndUser_Id(blogId, userId) | ||
| .orElseThrow(() -> new ExpectedException(HttpStatus.NOT_FOUND, "존재하지 않는 블로그입니다.")); | ||
| if (blog.getThumbnail() != null && !blog.getThumbnail().isBlank()) { | ||
| imageService.deleteImage(blog.getThumbnail()); | ||
| } | ||
|
|
||
| for (BlogImageJpaEntity img : images) { | ||
| imageService.deleteImage(img.getImageUrl()); | ||
| } | ||
|
|
||
| blogRepository.delete(blog); | ||
| } |
There was a problem hiding this comment.
이 delete 메서드는 @Transactional로 관리되고 있지만, 데이터베이스 트랜잭션 내에서 외부 시스템(Minio)의 파일을 삭제하는 로직이 있어 데이터 불일치 위험이 있습니다.
imageService.deleteImage() 호출이 반복문 중간에 실패할 경우,
- 데이터베이스 트랜잭션은 롤백되어 블로그와 이미지 정보는 DB에서 삭제되지 않습니다.
- 하지만, 예외 발생 이전에
imageService.deleteImage()로 성공적으로 삭제된 파일들은 외부 저장소에서 영구적으로 사라지며 복구되지 않습니다.
이로 인해 DB에는 존재하지만 실제 파일은 없는 '깨진 데이터'가 발생할 수 있습니다.
개선 제안:
데이터베이스 작업과 외부 시스템 호출을 분리하는 것을 고려해 보세요. 예를 들어, 트랜잭션이 성공적으로 커밋된 후에만 파일을 삭제하도록 @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)를 사용하는 방법이 있습니다. 이렇게 하면 DB 작업의 원자성을 보장하면서 외부 시스템과의 상호작용으로 인한 부작용을 최소화할 수 있습니다.
| protected boolean shouldNotFilter(HttpServletRequest request) { | ||
| String uri = request.getRequestURI(); | ||
|
|
||
| if (uri.startsWith("/api/v1/auth/sign") | ||
| || uri.startsWith("/api/v1/auth/login") | ||
| || uri.startsWith("/api/v1/auth/verify") | ||
| || uri.startsWith("/api/v1/auth/google") | ||
| || uri.startsWith("/api/v1/auth/kakao")) { | ||
| return true; | ||
| } else if (request.getMethod().equals("GET") && ( | ||
| uri.startsWith("/api/v1/main/") | ||
| || uri.startsWith("/api/v1/profile/") | ||
| || uri.startsWith("/api/v1/portfolio/view/") | ||
| || uri.startsWith("/api/v1/portfolio/comment/") | ||
| || uri.startsWith("/api/v1/blog/view/") | ||
| || uri.startsWith("/api/v1/blog/comment/"))) { | ||
| return true; | ||
| } | ||
|
|
||
| if(request.getMethod().equals("OPTIONS")) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
새로 추가된 shouldNotFilter 메서드는 JWT 필터를 건너뛸 경로를 지정하여 최적화에 도움이 됩니다. 하지만 현재 구현에는 유지보수성 측면에서 몇 가지 개선점이 있습니다.
- 하드코딩된 URL 패턴: URL 경로들이 문자열로 하드코딩되어 있어, 향후 경로 변경 시 여러 곳을 수정해야 할 수 있고 오류 발생 가능성이 높습니다.
startsWith사용의 위험성:startsWith는AntPathMatcher(예:/**)에 비해 정교하지 않습니다. 예를 들어,uri.startsWith("/api/v1/auth/sign")는 의도치 않게/api/v1/auth/signup과 같은 다른 경로와도 일치할 수 있습니다.SecurityConfig와의 중복: 필터링 예외 경로는SecurityConfig에 정의된permitAll경로와 거의 동일합니다. 두 곳에서 경로를 관리하면 동기화가 누락될 위험이 있습니다.
개선 제안:
- 예외 경로 목록을 별도의
static final리스트나 배열로 추출하여 한 곳에서 관리하고,SecurityConfig와JwtHeaderFilter에서 함께 사용하도록 리팩토링하는 것을 권장합니다. startsWith대신 Spring에서 널리 사용되는AntPathMatcher를 사용하여SecurityConfig와 동일한 방식으로 경로를 검사하면 일관성과 안정성을 높일 수 있습니다.
2 participants
개요
develop의 최신 변경 내용을 main 브랜치로 머지합니다