[Refactor] Presigned URL 캐싱 #89
Conversation
Walkthrough이 변경 사항은 이미지 프리사인드 URL의 S3 요청을 줄이기 위해 프리사인드 URL 캐싱 기능을 도입하고, 관련 서비스와 테스트 코드 전반에서 이미지 저장소 의존성을 ImageService에서 ImageStorage로 일괄 교체합니다. 또한, 캐시 설정, 캐시 구성, 캐시 저장소, 그리고 이를 활용하는 테스트 코드가 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ImageStorage
participant CachePreSignedUrlStorage
participant ExternalImageStorage
Client->>ImageStorage: getPresignedUrl(imageKey)
alt imageKey is null or empty
ImageStorage-->>Client: null
else cache hit
ImageStorage->>CachePreSignedUrlStorage: get(imageKey)
CachePreSignedUrlStorage-->>ImageStorage: Optional.of(url)
ImageStorage-->>Client: url
else cache miss
ImageStorage->>CachePreSignedUrlStorage: get(imageKey)
CachePreSignedUrlStorage-->>ImageStorage: Optional.empty
ImageStorage->>ExternalImageStorage: getPresignedUrl(imageKey)
ExternalImageStorage-->>ImageStorage: url
ImageStorage->>CachePreSignedUrlStorage: put(imageKey, url)
ImageStorage-->>Client: url
end
sequenceDiagram
participant Client
participant ImageStorage
participant ExternalImageStorage
participant CachePreSignedUrlStorage
Client->>ImageStorage: upload(file, domain)
ImageStorage->>ExternalImageStorage: upload(file, domain)
ExternalImageStorage-->>ImageStorage: imageKey
ImageStorage->>ExternalImageStorage: getPresignedUrl(imageKey)
ExternalImageStorage-->>ImageStorage: url
ImageStorage->>CachePreSignedUrlStorage: put(imageKey, url)
ImageStorage-->>Client: imageKey
Estimated code review effort4 (~80분) Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/test/java/eatda/document/BaseDocumentTest.java (1)
56-58: 불필요한 공백 라인 표시(~) 제거 권장라인 56에 변경 표시만 남아 있어 추후 diff 노이즈가 될 수 있습니다. 깔끔하게 정리해 주세요.
src/main/java/eatda/repository/CacheSetting.java (1)
16-17: 생성자 매개변수명을 일반적인 이름으로 수정하세요.생성자의 첫 번째 매개변수가 "image"로 하드코딩되어 있습니다. 이는 다른 캐시 타입 추가 시 혼란을 야기할 수 있습니다.
- CacheSetting(String image, Duration timeToLive, int maximumSize) { - this.name = image; + CacheSetting(String name, Duration timeToLive, int maximumSize) { + this.name = name;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
build.gradle(2 hunks)src/main/java/eatda/config/CacheConfig.java(1 hunks)src/main/java/eatda/repository/CacheSetting.java(1 hunks)src/main/java/eatda/repository/image/CachePreSignedUrlRepository.java(1 hunks)src/main/java/eatda/repository/image/ImageDomain.java(1 hunks)src/main/java/eatda/repository/image/ImageRepository.java(1 hunks)src/main/java/eatda/repository/image/S3ImageRepository.java(3 hunks)src/main/java/eatda/service/store/CheerService.java(3 hunks)src/main/java/eatda/service/store/StoreService.java(3 hunks)src/main/java/eatda/service/story/StoryService.java(4 hunks)src/test/java/eatda/controller/BaseControllerTest.java(3 hunks)src/test/java/eatda/controller/story/StoryControllerTest.java(1 hunks)src/test/java/eatda/document/BaseDocumentTest.java(1 hunks)src/test/java/eatda/document/story/StoryDocumentTest.java(3 hunks)src/test/java/eatda/repository/BaseCacheRepositoryTest.java(1 hunks)src/test/java/eatda/repository/BaseJpaRepositoryTest.java(1 hunks)src/test/java/eatda/repository/image/ImageRepositoryTest.java(1 hunks)src/test/java/eatda/repository/image/S3ImageRepositoryTest.java(6 hunks)src/test/java/eatda/repository/store/CheerRepositoryTest.java(1 hunks)src/test/java/eatda/service/BaseServiceTest.java(3 hunks)src/test/java/eatda/service/story/StoryServiceTest.java(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/test/java/eatda/controller/story/StoryControllerTest.java (1)
Learnt from: leegwichan
PR: YAPP-Github/26th-Web-Team-1-BE#60
File: src/test/java/eatda/controller/store/StoreControllerTest.java:10-32
Timestamp: 2025-07-09T07:56:50.612Z
Learning: 컨트롤러 테스트에서 MockitoBean으로 의존성을 모킹한 경우, 상세한 비즈니스 로직 검증보다는 컨트롤러 계층의 동작(라우팅, 파라미터 처리, 응답 구조 등)을 검증하는 것이 더 적절합니다. 모킹된 데이터에 대한 상세 검증은 의미가 없기 때문입니다.
src/test/java/eatda/controller/BaseControllerTest.java (1)
Learnt from: leegwichan
PR: YAPP-Github/26th-Web-Team-1-BE#60
File: src/test/java/eatda/controller/store/StoreControllerTest.java:10-32
Timestamp: 2025-07-09T07:56:50.612Z
Learning: 컨트롤러 테스트에서 MockitoBean으로 의존성을 모킹한 경우, 상세한 비즈니스 로직 검증보다는 컨트롤러 계층의 동작(라우팅, 파라미터 처리, 응답 구조 등)을 검증하는 것이 더 적절합니다. 모킹된 데이터에 대한 상세 검증은 의미가 없기 때문입니다.
🧬 Code Graph Analysis (2)
src/main/java/eatda/repository/image/S3ImageRepository.java (2)
src/main/java/eatda/repository/image/ImageRepository.java (1)
Component(9-39)src/main/java/eatda/repository/image/CachePreSignedUrlRepository.java (1)
Component(9-27)
src/main/java/eatda/repository/image/CachePreSignedUrlRepository.java (2)
src/main/java/eatda/repository/image/ImageRepository.java (1)
Component(9-39)src/main/java/eatda/repository/image/S3ImageRepository.java (1)
Component(20-97)
🔇 Additional comments (29)
src/main/java/eatda/repository/image/ImageDomain.java (1)
1-1: 패키지 이동 후 전체 의존성 업데이트 확인 필요
eatda.service.common→eatda.repository.image로 패키지가 이동되었습니다. IDE 자동 리팩터링으로 누락된 import / FQN 참조가 없는지 전역 검색을 통해 한 번 더 확인해 주세요.src/test/java/eatda/repository/BaseJpaRepositoryTest.java (1)
16-16:BaseJpaRepositoryTest네이밍 변경 승인네이밍이 JPA 테스트 성격을 더 명확히 드러냅니다. 다른 테스트 클래스의 상속 선언도 모두 업데이트된 것 확인했습니다.
src/test/java/eatda/controller/story/StoryControllerTest.java (1)
43-44:multiPart가독성 개선 👍파라미터가 한눈에 들어와 유지보수가 용이해졌습니다. 기능-행위에는 영향 없습니다.
src/test/java/eatda/document/story/StoryDocumentTest.java (1)
64-66: 코드 포맷팅 개선이 적절합니다.multiPart 메서드 호출을 여러 줄로 분할하여 가독성이 향상되었습니다.
build.gradle (2)
60-60: 캐싱 기능을 위한 적절한 의존성 추가Spring Boot의 캐시 추상화를 위한 starter 의존성이 추가되었습니다.
71-71: 고성능 인메모리 캐시 구현체 추가Caffeine은 높은 성능과 메모리 효율성을 제공하는 캐시 라이브러리로 25분 TTL 요구사항에 적합합니다.
src/main/java/eatda/service/store/StoreService.java (3)
12-12: ImageService에서 ImageRepository로의 리팩터링이미지 관련 로직이 서비스 계층에서 리포지토리 계층으로 이동되어 아키텍처가 개선되었습니다.
29-29: 의존성 필드명 업데이트ImageRepository로의 마이그레이션에 맞춰 필드명이 적절히 변경되었습니다.
41-41: 메서드 호출 업데이트ImageRepository의 getPresignedUrl 메서드 호출로 변경되어 캐싱 기능이 활용됩니다.
src/test/java/eatda/repository/BaseCacheRepositoryTest.java (1)
1-13: BaseCacheRepositoryTest의 CacheConfig 직접 호출 검증 완료 및 승인CacheConfig.cacheManager() 메서드는 SimpleCacheManager를 생성·설정·초기화까지 자체 처리하므로, 스프링 컨텍스트 없이도 프로덕션 환경과 동일한 캐시 설정을 재현합니다.
- src/main/java/eatda/config/CacheConfig.java:
- @bean으로 정의된 cacheManager() 내부에서
- SimpleCacheManager 생성
- setCaches(…) 호출
- afterPropertiesSet() 직접 호출
- 외부 의존성 없이 완전한 초기화 로직 수행
- BaseCacheRepositoryTest에서 new CacheConfig().cacheManager() 사용 시 설정 불일치 우려 없음
따라서 해당 테스트 기반 클래스 설계를 그대로 승인합니다.
src/test/java/eatda/controller/BaseControllerTest.java (3)
18-18: ImageRepository로의 일관된 마이그레이션ImageService에서 ImageRepository로의 리팩터링이 테스트 코드에도 일관성 있게 적용되었습니다.
80-80: MockitoBean 의존성 업데이트ImageRepository를 모킹하도록 변경되어 새로운 아키텍처와 일치합니다.
112-113: 모킹 설정 업데이트가 적절합니다.ImageRepository의 메서드들에 대한 모킹 설정이 올바르게 업데이트되었으며, 컨트롤러 테스트에서는 상세한 비즈니스 로직보다는 컨트롤러 계층의 동작을 검증하는 것이 적절합니다.
src/main/java/eatda/service/store/CheerService.java (1)
6-6: 의존성 변경이 깔끔하게 적용되었습니다.ImageService에서 ImageRepository로의 리팩터링이 일관성 있게 적용되었습니다. 기존 로직은 그대로 유지하면서 캐싱 기능이 추가된 새로운 구조로 전환되었습니다.
Also applies to: 19-19, 30-30
src/test/java/eatda/service/BaseServiceTest.java (1)
13-13: 테스트 인프라가 새로운 아키텍처에 맞게 업데이트되었습니다.ImageRepository로의 변경사항과 StoryRepository 추가가 적절히 반영되었습니다. 모킹 설정도 새로운 구조에 맞게 올바르게 수정되었습니다.
Also applies to: 38-38, 58-64
src/test/java/eatda/repository/image/S3ImageRepositoryTest.java (1)
1-1: 테스트 클래스가 새로운 구조에 맞게 성공적으로 마이그레이션되었습니다.ImageServiceTest에서 S3ImageRepositoryTest로의 변경이 올바르게 적용되었습니다. 패키지 이동, 클래스명 변경, 그리고 테스트 로직 업데이트가 일관성 있게 처리되었으며, 기존 테스트 커버리지가 유지되었습니다.
Also applies to: 30-30, 36-36, 40-42, 58-58, 81-81, 102-102, 125-125
src/test/java/eatda/service/story/StoryServiceTest.java (1)
16-16: 테스트 코드가 새로운 아키텍처에 맞게 정확히 업데이트되었습니다.ImageRepository로의 변경사항이 일관성 있게 적용되었고, 불필요한 의존성도 정리되었습니다. 테스트 로직과 검증 내용은 그대로 유지되어 기능 테스트 커버리지가 보장됩니다.
Also applies to: 45-45, 93-94
src/main/java/eatda/repository/CacheSetting.java (1)
9-10: 캐시 설정이 PR 목표에 부합하게 구성되었습니다.25분 TTL 설정이 30분보다 명확히 짧게 설정되어 PR 목표에 부합하며, 1000개 최대 크기도 이미지 URL 캐싱에 적절해 보입니다.
src/main/java/eatda/config/CacheConfig.java (2)
16-26: 캐시 매니저 구성이 올바르게 구현되었습니다.CacheSetting enum을 기반으로 동적으로 캐시를 생성하는 방식은 유지보수성이 뛰어나며,
afterPropertiesSet()호출도 적절합니다.
28-35: Caffeine 캐시 설정이 적절합니다.TTL과 최대 크기 설정이 CacheSetting에서 올바르게 가져와지고 있으며, expireAfterWrite 정책이 적절합니다.
src/main/java/eatda/repository/image/CachePreSignedUrlRepository.java (1)
9-27: 캐시 저장소 구현이 깔끔하고 안전합니다.
- Optional을 사용한 null-safe 반환
- 타입 안전한 캐시 조회
- 생성자 주입을 통한 의존성 관리
모든 부분이 적절하게 구현되었습니다.
src/main/java/eatda/service/story/StoryService.java (1)
11-12: ImageRepository로의 의존성 변경이 올바르게 적용되었습니다.서비스 레이어에서 ImageService를 ImageRepository로 교체하여 캐싱 레이어를 투명하게 활용할 수 있게 되었습니다. 모든 메서드 호출이 일관되게 업데이트되었습니다.
Also applies to: 33-33, 42-42, 79-79
src/main/java/eatda/repository/image/ImageRepository.java (2)
17-23: 업로드 시 캐시 예열 전략이 우수합니다.업로드 후 즉시 presigned URL을 생성하여 캐시에 저장하는 방식은, 사용자가 업로드한 이미지가 포함된 페이지로 이동할 가능성이 높다는 PR 목표와 잘 일치합니다.
25-38: Cache-aside 패턴이 올바르게 구현되었습니다.
- 캐시 우선 조회
- 캐시 미스 시 S3에서 조회 후 캐시 저장
- null/empty 키에 대한 적절한 처리
구현이 안전하고 효율적입니다.
src/main/java/eatda/repository/image/S3ImageRepository.java (3)
1-1: 서비스에서 리포지토리로의 아키텍처 개선이 적절합니다.
- 패키지 이동 및 어노테이션 변경이 레이어 책임에 맞게 수정됨
- @component 사용이 리포지토리 레이어에 적합함
Also applies to: 11-11, 20-21
27-28: Presigned URL 유효기간 설정이 매우 적절합니다.캐시 TTL(25분)에 5분을 추가하여 30분으로 설정한 것은 캐시된 URL이 만료되기 전까지 항상 유효함을 보장하는 훌륭한 전략입니다.
80-80: null 체크 제거가 아키텍처 개선과 일치합니다.ImageRepository에서 null 처리를 담당하므로 이 레이어에서는 null 체크가 불필요해졌습니다. 책임 분리가 잘 되었습니다.
src/test/java/eatda/repository/image/ImageRepositoryTest.java (2)
16-27: 테스트 구조가 잘 설계되었습니다S3ImageRepository는 모킹하고 CachePreSignedUrlRepository는 실제 인스턴스를 사용하는 접근 방식이 적절합니다. 이를 통해 캐시 동작을 실제로 검증하면서 외부 의존성은 격리할 수 있습니다.
16-92: 전반적으로 잘 구성된 테스트입니다테스트 구조와 시나리오 커버리지가 우수합니다. 다음과 같은 장점들이 있습니다:
- Nested 클래스를 활용한 명확한 테스트 구조
- 캐시 히트/미스 시나리오의 포괄적인 커버리지
- MockMultipartFile과 ParameterizedTest의 적절한 활용
- 한국어 테스트 메서드명으로 가독성 향상
제안된 Mock 검증 추가로 테스트의 완성도를 더욱 높일 수 있을 것입니다.
# Conflicts: # src/test/java/eatda/controller/story/StoryControllerTest.java # src/test/java/eatda/service/story/StoryServiceTest.java
lvalentine6
left a comment
lvalentine6
left a comment
There was a problem hiding this comment.
이번 PR도 고생하셨습니다! 🎉
이번 기능의 추가로 불필요한 S3 요청이 많이 줄어들겠네요
관련해 코멘트 남겼으니 확인해주세요
| @Getter | ||
| public enum CacheSetting { | ||
|
|
||
| IMAGE("image", Duration.ofMinutes(25), 1_000), |
There was a problem hiding this comment.
[제안]
현재 개발, 프로덕션 인스턴스 메모리가 여유가 거의 없기에 1000개는 많을수도 있을것 같아요
우선 100개정도로만 조정해보는것은 어떨까요?
There was a problem hiding this comment.
캐시가 차지하는 전반적인 용량을 이번 기회에 고려해 보았습니다.
- 저희 서비스 자체가 이미지가 주가 되는 서비스다 보니, 개수를 널널하게 잡은 것 같아요. 저희 홈 화면에 조회되는 스토리/응원/상점 이미지만 약 20~30 장 정도 되고, 각 상점들에도 꽤 이미지가 나올 것 같아서 100장은 부족할 것 같다고 생각했습니다.
- 만들어지는 imageKey와 S3에서 주는 ImageUrl 길이를 측정해 보았을 때, 1000 개 정도 저장하면 4.5-5MB 정도 차지한다고 하더라구요. 참고 자료(ChatGPT)
위 두 조건을 고려했을 때, 크기는 어느 정도가 좋을까요?
There was a problem hiding this comment.
회의에서 논의되었던 내용으로 5MB정도라면 1000개도 적절하다고 보여집니다!
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class ImageRepository { |
There was a problem hiding this comment.
[질문]
음.. 현재 ImageRepository는 실질적으로 S3 업로드, Presigned URL 생성, 캐시 저장/탐색 등
도메인 수준의 정책 판단과 흐름 제어를 포함하고 있어 보이는데요
이런 역할은 보통 Service 계층에서 수행하는 책임으로 보고,
지금 구조는 이름만 Repository이지 실제로는 완전히 Service의 역할을 하고 있다고 느껴지는데요.
왜 계층을 바꾸신 건지 잘 이해가 되지 않아요 😿
혹시 단순히 기술적 의존성을 줄이려는 의도라면, Service 내부에서 cache + S3를 위임하는 구조로도 충분히 가능한데,
이걸 Repository로 옮긴 이유가 궁금합니다.
There was a problem hiding this comment.
저는 서비스의 상호 참조를 막기 위해서는 다른 서비스를 참조하는 것을 완전히 막아야 한다고 생각해요. 그래서 '해당 서비스는 다른 서비스에서 사용할 작업을 내포한다는 점'에서 서비스로 보기 힘들다고 생각했어요. 그래서 그나마 적합한 것이 Repository라고 생각했습니다. (너무 여러 고민을 하기 보다는 빠르게 구현하기 위해 일단 Repository로 배치했습니다)
저도 해당 부분이 "여러가지 검증을 한다는 점"에서 레포지토리라고 보기 힘들다고 생각합니다. 기존의 "ImageService"가 서비스 치고는 여러 책임들(MultipartFile 유효성 검사, 이미지 key 이름 지정, S3Client를 통한 업로드,...)이 다 같이 응집되어 있어 어느 네이밍을 붙여도 어색한 포지션이 되지 않았나 싶습니다.
해당 부분을 Repository로 판단하기 보다는, 리팩토링을 나누어 작업하기 위해 일단 임시적으로 ImageRepository로 두었다고 생각해주시면 감사드리겠습니다. 그래서 앞으로 리팩토링을 진행하면서 아래처럼 역할을 나누어 관리하려고 합니다. 아래와 같은 코드에 대해 편하게 의견 주시면 감사드리겠습니다.
public class Image {
private final MultipartFile multipartFile;
private final ImageDomain domain;
public Image(MultipartFile multipartFile, ImageDomain domain) {
// multipartFile 이미지 유효성 검사 (파일 확장자 검사, )
}
}
@Getter
@Embeddable
public ImageKey {
private final String imageKey;
....
}public class ImageRepository {
public ImageKey save(Image image) {
// 현재 PR의 S3ImageRepository, CachePreSignedUrlRepository를 이용한 로직
}
}
public class S3ImageRepository {
public ImageKey save(Image image) {
// 이미지 key 생성 로직
// FileClient 를 통한 업로드
}
}
public class FileClient { // 이름은 고민 중입니다...
public String upload(MultipartFile file, String key) {
// AWS SDK 의 s3Client를 이용해 파일 업로드
}
}There was a problem hiding this comment.
public class ImageStorage {
public ImageKey upload(Image image) {
// 현재 PR의 S3ImageStorage, CachePreSignedUrlStorage를 이용한 로직
}
}
public class S3ImageStorage {
public ImageKey upload(Image image) {
// 이미지 key 생성 로직
// FileClient 를 통한 업로드
}
}
public class FileClient { // 이름은 고민 중입니다...
public String upload(MultipartFile file, String key) {
// AWS SDK 의 s3Client를 이용해 파일 업로드
}
}There was a problem hiding this comment.
- S3ImageStorage 대신 ExternalImageStorage 로 이름을 바꾸었습니다. (FileClient 가 S3에 종속되는 것이고, ExternalImageStorage는 S3에 종속되지 않기 때문입니다.)
- (해당 PR이 무거워지기도 하고, 리팩토링 과정을 전반적으로 리뷰 받아야 할 것 같아)
ImageStorage으로 이름,패키지를 바꾸는 작업까지만 완료했습니다. - Image, ImageKey 도메인 객체 추가, 각 역할을 분리하는 작업은 새로운 이슈([PRODUCT-195] [Refactor] 이미지 도메인 객체 도입 #96) 에서 작업하도록 하겠습니다!
| @RequiredArgsConstructor | ||
| public class ImageRepository { | ||
|
|
||
| private final S3ImageRepository s3Repository; |
There was a problem hiding this comment.
[질문]
클래스명과 변수명이 다른건 의도하신건가요?
There was a problem hiding this comment.
클래스 이름이 ImageRepository 이니까, 굳이 해당 레포지토리가 image임을 명시할 필요는 없다고 생각했습니다.
관련해서 승로님의 의견도 궁금합니다!
There was a problem hiding this comment.
음.. 저는 보통 특별한 의미가 있지않으면 클래스명을 그대로 쓰는편인것 같아요.
S3ImageRepository ->s3Repository 가 되었을때 의미 변화가 크지는 않은것 같아서
저는 변수명이 클래스명을 그대로 따라가는게 좋을것 같아요!
There was a problem hiding this comment.
승로님 의견 반영하여, 클래스 이름과 변수명을 일치시켰습니다!
| Optional<String> cachedUrl = cachePreSignedUrlRepository.get(imageKey); | ||
| if (cachedUrl.isPresent()) { | ||
| return cachedUrl.get(); | ||
| } |
There was a problem hiding this comment.
저희가 이전에 합의했던 컨벤션 중 "의미없는 빈 줄은 넣지 않는다" 가 있었습니다. 그래서 공백을 추가할까 고민하다가 일단 추가 안했습니다. 해당 메서드의 공백을 어디에 넣어야 가독성이 좋을까요?
There was a problem hiding this comment.
제가 생각했던 공백은 34번째 라인과 35번째 사이에 공백이 있는게 자연스럽지 않나? 라는 생각이였어요
if (cachedUrl.isPresent()) { ... } 부분에서 캐시에서 값을 찾았을 때의 분기 처리가 끝나고
그 바로 아래에 캐시 값이 없을 때의 로직이 공백 없이 이어지니 흐름이 잘 안 보였습니다.
if문 다음에 한 줄을 띄워 분기 처리와 주 로직을 분리하는게 더 자연스럽지 않을까 해서요
There was a problem hiding this comment.
승로님 의견 반영하여 34번째 라인과 35번째 사이에 공백 추가했습니다!
| private static final String PATH_DELIMITER = "/"; | ||
| private static final String EXTENSION_DELIMITER = "."; | ||
| private static final Duration PRESIGNED_URL_DURATION = Duration.ofMinutes(30); | ||
| private static final Duration PRESIGNED_URL_DURATION = CacheSetting.IMAGE.getTimeToLive() |
There was a problem hiding this comment.
[수정]
현재 PRESIGNED_URL_DURATION 설정이 CacheSetting의 TTL에 의존하게 바뀌었는데,
정책 상으로는 오히려 Presigned URL의 유효시간이 기준이 되고,
캐시 TTL은 그보다 짧게 설정되어야 하지 않을까요?
Duration.ofMinutes(30) 등으로 기준을 고정하고
캐시는 PRESIGNED_URL_DURATION 을 기준으로 바뀌는게 좋을것 같아요
There was a problem hiding this comment.
저도 애매했는데..., 그냥 이전처럼 Duration.ofMinutes(30)으로 바꿔놓겠습니다!
| .containsExactlyInAnyOrder( | ||
| "https://s3.bucket.com/story/dummy/2.jpg", | ||
| "https://s3.bucket.com/story/dummy/1.jpg" | ||
| ); |
There was a problem hiding this comment.
위에 when과 extracting 관련 검증이 사라진 이유는 무엇인가요?
There was a problem hiding this comment.
ImageRepsitory를 Mocking해서 사용하고 있어 동일한 값을 반환합니다. (BaseServiceTest에서 Mocking을 진행합니다) 그래서 해당 부분을 삭제했습니다. 이미지 대신 storyId 값을 가지고 있는지 확인하도록 변경하겠습니다!
| Optional<String> cachedUrl = cachePreSignedUrlRepository.get(imageKey); | ||
| if (cachedUrl.isPresent()) { | ||
| return cachedUrl.get(); | ||
| } |
There was a problem hiding this comment.
제가 생각했던 공백은 34번째 라인과 35번째 사이에 공백이 있는게 자연스럽지 않나? 라는 생각이였어요
if (cachedUrl.isPresent()) { ... } 부분에서 캐시에서 값을 찾았을 때의 분기 처리가 끝나고
그 바로 아래에 캐시 값이 없을 때의 로직이 공백 없이 이어지니 흐름이 잘 안 보였습니다.
if문 다음에 한 줄을 띄워 분기 처리와 주 로직을 분리하는게 더 자연스럽지 않을까 해서요
# Conflicts: # src/main/java/eatda/service/store/CheerService.java # src/main/java/eatda/service/story/StoryService.java # src/test/java/eatda/controller/BaseControllerTest.java # src/test/java/eatda/controller/story/StoryControllerTest.java # src/test/java/eatda/document/BaseDocumentTest.java # src/test/java/eatda/document/story/StoryDocumentTest.java # src/test/java/eatda/service/BaseServiceTest.java
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/eatda/storage/image/ImageStorage.java (1)
18-24: 업로드 후 즉시 캐싱하는 전략이 적절합니다.업로드 후 사용자가 해당 페이지로 이동할 가능성이 높다는 PR 목표에 부합하는 좋은 설계입니다. 다만 외부 스토리지에 두 번의 호출이 발생하는 점을 고려해볼 수 있습니다.
성능 최적화를 고려한다면 업로드와 프리사인드 URL 생성을 하나의 메서드로 통합하는 방안을 검토해볼 수 있습니다:
public String upload(MultipartFile file, ImageDomain domain) { String imageKey = externalImageStorage.upload(file, domain); // 업로드 직후에만 캐싱하고, 필요시에만 프리사인드 URL 생성 // 하지만 현재 접근법도 UX 관점에서는 합리적입니다 String preSignedUrl = externalImageStorage.getPresignedUrl(imageKey); cachePreSignedUrlStorage.put(imageKey, preSignedUrl); return imageKey; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/eatda/domain/ImageDomain.java(1 hunks)src/main/java/eatda/service/article/ArticleService.java(3 hunks)src/main/java/eatda/service/store/CheerService.java(5 hunks)src/main/java/eatda/service/store/StoreService.java(3 hunks)src/main/java/eatda/service/story/StoryService.java(5 hunks)src/main/java/eatda/storage/image/CachePreSignedUrlStorage.java(1 hunks)src/main/java/eatda/storage/image/ExternalImageStorage.java(4 hunks)src/main/java/eatda/storage/image/ImageStorage.java(1 hunks)src/test/java/eatda/controller/BaseControllerTest.java(3 hunks)src/test/java/eatda/document/BaseDocumentTest.java(0 hunks)src/test/java/eatda/service/BaseServiceTest.java(3 hunks)src/test/java/eatda/service/story/StoryServiceTest.java(4 hunks)src/test/java/eatda/storage/BaseStorageTest.java(1 hunks)src/test/java/eatda/storage/image/ExternalImageStorageTest.java(7 hunks)src/test/java/eatda/storage/image/ImageStorageTest.java(1 hunks)
🧬 Code Graph Analysis (1)
src/main/java/eatda/storage/image/ImageStorage.java (2)
src/main/java/eatda/storage/image/CachePreSignedUrlStorage.java (1)
Component(9-27)src/main/java/eatda/storage/image/ExternalImageStorage.java (1)
Component(20-96)
💤 Files with no reviewable changes (1)
- src/test/java/eatda/document/BaseDocumentTest.java
✅ Files skipped from review due to trivial changes (3)
- src/test/java/eatda/storage/BaseStorageTest.java
- src/main/java/eatda/storage/image/CachePreSignedUrlStorage.java
- src/main/java/eatda/domain/ImageDomain.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/test/java/eatda/controller/BaseControllerTest.java
- src/main/java/eatda/service/store/CheerService.java
- src/test/java/eatda/service/BaseServiceTest.java
- src/main/java/eatda/service/story/StoryService.java
- src/main/java/eatda/service/store/StoreService.java
- src/test/java/eatda/service/story/StoryServiceTest.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/eatda/storage/image/ImageStorage.java (2)
src/main/java/eatda/storage/image/CachePreSignedUrlStorage.java (1)
Component(9-27)src/main/java/eatda/storage/image/ExternalImageStorage.java (1)
Component(20-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (14)
src/main/java/eatda/service/article/ArticleService.java (1)
6-6: 의존성 교체가 올바르게 적용되었습니다.
ImageService에서ImageStorage로의 의존성 교체가 일관되게 적용되어 캐싱 기능을 활용할 수 있게 되었습니다.Also applies to: 17-17, 27-27
src/test/java/eatda/storage/image/ExternalImageStorageTest.java (2)
1-1: 테스트 클래스 리팩토링이 일관성 있게 적용되었습니다.클래스명 변경, 패키지 이동, 그리고 의존성 업데이트가 모두 올바르게 처리되어 기존 테스트 로직을 그대로 유지하고 있습니다.
Also applies to: 11-11, 31-31, 37-37, 41-43
59-59: 메서드 호출이 새로운 클래스명으로 정확히 업데이트되었습니다.모든 테스트 메서드에서
externalImageStorage사용이 일관되게 적용되었습니다.Also applies to: 82-82, 103-103, 126-126
src/main/java/eatda/storage/image/ExternalImageStorage.java (2)
1-1: 클래스 리팩토링이 적절히 수행되었습니다.패키지 이동, 클래스명 변경, 그리고
@Service에서@Component로의 변경이 새로운 아키텍처에 적합하게 적용되었습니다.Also applies to: 3-3, 11-11, 20-21
79-95: null 안전성 검증 완료ExternalImageStorage.getPresignedUrl은 ImageStorage를 통해서만 호출되며, ImageStorage에서는 @nullable 애노테이션과 함께 입력값(imageKey)에 대한 null 검사를 수행하고 있습니다. 따라서 외부 스토리지 레이어에서 추가적인 null 체크는 필요하지 않습니다.
src/test/java/eatda/storage/image/ImageStorageTest.java (6)
20-31: 테스트 설정이 잘 구성되었습니다.
ExternalImageStorage는 모킹하고CachePreSignedUrlStorage는 실제 인스턴스를 사용하는 접근법이 캐싱 통합 테스트에 적합합니다.
36-46: 업로드 기능 테스트가 적절합니다.외부 스토리지로의 위임이 올바르게 검증되고 있습니다.
48-59: 업로드 시 캐싱 동작이 올바르게 테스트되었습니다.이미지 업로드 후 프리사인드 URL이 캐시에 저장되는 것을 확인하는 중요한 테스트입니다.
65-71: null/empty 입력 처리가 적절히 테스트되었습니다.엣지 케이스에 대한 방어적 프로그래밍이 잘 검증되고 있습니다.
73-84: 캐시 히트 시나리오가 올바르게 테스트되었습니다.캐시된 URL이 있을 때 외부 스토리지를 호출하지 않는 것을
never()검증으로 확인하는 것이 좋습니다.
86-97: 캐시 미스 시나리오가 완전히 테스트되었습니다.외부 스토리지 호출과 캐시 저장이 모두 올바르게 검증되고 있습니다.
src/main/java/eatda/storage/image/ImageStorage.java (3)
10-15: 의존성 주입과 클래스 구조가 잘 설계되었습니다.
ExternalImageStorage와CachePreSignedUrlStorage를 조합하여 캐싱 기능을 제공하는 파사드 패턴이 적절히 적용되었습니다.
26-40: 프리사인드 URL 조회 로직이 올바르게 구현되었습니다.null 안전성 검사, 캐시 우선 조회, 캐시 미스 시 외부 스토리지 호출 및 캐시 저장이 모두 적절히 처리되었습니다.
32-35: Optional 사용법이 적절합니다.캐시에서 값을 조회할 때
Optional을 사용하여 안전하게 처리하고 있습니다.
|
🎉 This PR is included in version 1.4.0-develop.28 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
✨ 개요
🧾 관련 이슈
closed #82
🔍 참고 사항 (선택)
캐싱 정책
구현 방향
Summary by CodeRabbit
신규 기능
버그 수정
테스트