Merged
Conversation
* [refactor]: hostprefix - rest도 부동산학과 조회 가능하도록 수정 * [refactor]: 학과 공지 조회 조건 null 방어 처리 * [test]: DepartmentName fromHostPrefix (rest/kure) 변환 및 예외 테스트 추가 * [refactor]: DepartmentName legacy hostPrefix 분리 * [test]: hostPrefixMap 테스트 구현 * [refactor]: 변수명 fallbackHostPrefix로 변경 * [refactor]: 변수명 fallbackHostPrefix로 변경 * [refactor]: hostPrefix 매칭 책임 캡슐화 * [refactor]: HOST_PREFIX_MAP O(n)으로 로직 수정
… 업그레이드) (#343) * fix : testcontainers 버전 2.0.2 migration * fix : apache common-lang3 3.13.0 -> 3.18.0 업그레이드
* fix : FCM 메시지 공지 id 추가 * feat : 공지사항 JpaRepository SaveAll 사용하는 Port & Adapter 메서드 추가 * fix : 업데이트 시 DB에 저장한 내용 다시 return하도록 수정 * fix : 업데이트 시 DB에 저장한 내용 다시 return하도록 수정(대학원) * fix : firebase notice 알림 보낼 시 로깅에 id 추가
* chore : AWS S3 SDK 의존성 추가 * feat : CloudStorageConfig 추가 * feat : StoragePort 추가 * feat : S3CompatibleStorageAdapter 추가 * feat : MockStorageAdapter 추가 * feat : CloudStorageException & ErrorCode 추가 * feat : application 환경설정 추가(common & test) * feat : CloudStorageProperties 추가 * refactor : ErrorCode Enum 이름 변경(STORAGE_S3_SDK_PROBLEM) * fix : MockStorageAdapter 파일 조회 시 에러 수정 * fix : StoragePort PresignedUrl String으로 반환받도록 수정 * fix : 불필요 throws 구문 수정 * test : S3CompatibleStorageAdapterTest 추가 * test : URL과 String 비교로 인한 테스트 실패하는 현상 수정 * refactor : CloudStorageConfig 중복 Builder 메서드화 * feat: dev.yml environment 추가 * fix : CloudStorageException 계층구조 변경 * feat : InfrastructureException 추가 * fix : Storage Upload간 byte 읽어오지 않고 바로 스트리밍 하도록 수정
Walkthrough이 PR은 S3 호환 클라우드 스토리지 지원(AWS S3, OCI 포함), 새로운 인프라 예외 처리 체계, 테스트 컨테이너 및 AWS SDK 의존성 업데이트, 파일 업로드 및 스토리지 구성 추가, 공지사항 저장소 로직 개선을 포함합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Port as StoragePort
participant Adapter as S3CompatibleStorageAdapter
participant S3Client as S3Client/S3Presigner
participant Exception as CloudStorageException
rect rgba(100, 150, 200, 0.5)
Note over App,Exception: 파일 업로드 흐름
App->>Port: upload(stream, key, contentType, length)
Port->>Adapter: upload(stream, key, contentType, length)
Adapter->>Adapter: PutObjectRequest 빌드
Adapter->>S3Client: putObject(request, RequestBody)
alt 성공
S3Client-->>Adapter: 응답
Adapter-->>Port: 완료
Port-->>App: 반환
else SdkClientException
S3Client-->>Adapter: 예외 발생
Adapter->>Exception: 생성 (STORAGE_S3_SDK_PROBLEM)
Exception-->>App: 예외 throw
end
end
rect rgba(150, 100, 200, 0.5)
Note over App,Exception: 사전 서명 URL 생성 흐름
App->>Port: getPresignedUrl(key)
Port->>Adapter: getPresignedUrl(key)
Adapter->>Adapter: GetObjectRequest 빌드
Adapter->>S3Client: presign(GetObjectPresignRequest, 1시간 만료)
alt 성공
S3Client-->>Adapter: PresignedGetObjectRequest
Adapter->>Adapter: URL 추출
Adapter-->>Port: URL 반환
Port-->>App: URL
else 예외
S3Client-->>Adapter: 예외 발생
Adapter->>Exception: 생성 (STORAGE_S3_SDK_PROBLEM)
Exception-->>App: 예외 throw
end
end
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/kustacks/kuring/worker/update/notice/KuisHomepageNoticeUpdater.java (1)
191-196:⚠️ Potential issue | 🔴 Critical
ConcurrentModificationException발생 가능 — for-each 루프 내에서 리스트 요소 제거Line 194에서
newNormalNotices를 for-each로 순회하면서remove(notice)를 호출하고 있습니다. 이는ConcurrentModificationException을 발생시킵니다. 이 버그는 Line 182에서 변경된saveAllCategoryNoticesAndReturn에 전달되는newNotices데이터에 직접 영향을 미칩니다.🐛 Iterator 또는 removeIf를 사용한 수정 제안
- List<String> changedImportantArticleIds = new ArrayList<>(); - for (Notice notice : newNormalNotices) { - if (Collections.binarySearch(savedImportantArticleIds, notice.getArticleId()) >= 0) { // 정렬되어있다, 이진탐색으로 O(logN)안에 수행 - changedImportantArticleIds.add(notice.getArticleId()); - newNormalNotices.remove(notice); - } - } + List<String> changedImportantArticleIds = new ArrayList<>(); + newNormalNotices.removeIf(notice -> { + if (Collections.binarySearch(savedImportantArticleIds, notice.getArticleId()) >= 0) { + changedImportantArticleIds.add(notice.getArticleId()); + return true; + } + return false; + });
🤖 Fix all issues with AI agents
In
`@src/main/java/com/kustacks/kuring/common/exception/handler/CommonExceptionHandler.java`:
- Around line 83-88: The InfrastructureExceptionHandler method can NPE if
exception.getErrorCode().getHttpStatus() is null; update
CommonExceptionHandler.InfrastructureExceptionHandler to defensively handle a
null HttpStatus by resolving a safe fallback (e.g.,
HttpStatus.INTERNAL_SERVER_ERROR) before building the ResponseEntity and log
that a default status was used, and additionally consider enforcing non-null
httpStatus in the InfrastructureException constructor or its factory to prevent
future misuse (reference: InfrastructureException, ErrorCode.getHttpStatus(),
and CommonExceptionHandler.InfrastructureExceptionHandler).
In `@src/main/java/com/kustacks/kuring/config/CloudStorageConfig.java`:
- Around line 47-52: CloudStorageConfig's oracleStorageClient() and
oracleS3Presigner() call URI.create(properties.endpoint()) which can NPE when
properties.endpoint() is null; add a null check (e.g. Objects.requireNonNull or
an explicit if and throw IllegalStateException with a clear message) at the
start of oracleStorageClient() and oracleS3Presigner() (or validate once in the
CloudStorageConfig constructor) to fail fast when properties.endpoint() is
missing for the dev/OCI profile, so you never call URI.create with a null value.
In
`@src/main/java/com/kustacks/kuring/storage/exception/CloudStorageException.java`:
- Around line 6-11: Add a constructor to CloudStorageException that accepts a
cause and forwards it to the super constructor (e.g., public
CloudStorageException(ErrorCode errorCode, Throwable cause) { super(errorCode,
cause); }), and update the S3CompatibleStorageAdapter catch sites that currently
create new CloudStorageException(errorCode) (the catch blocks around the S3 SDK
exceptions) to pass the caught exception (e) into the new constructor so the
original cause is preserved.
In
`@src/test/java/com/kustacks/kuring/storage/adapter/out/S3CompatibleStorageAdapterTest.java`:
- Around line 119-130: The test getPresignedUrlWithS3Exception currently
simulates an SdkClientException but should simulate an S3Exception to exercise
that branch; update the mock for mockS3Presigner.presignGetObject(...) in that
test to throw an S3Exception (constructed via
S3Exception.builder().message(...).build() or equivalent) instead of
SdkClientException so the S3Exception handling path in
S3CompatibleStorageAdapter.getPresignedUrl is validated.
- Around line 145-156: The test method deleteFileWithS3Exception incorrectly
throws SdkClientException instead of an S3Exception; update the mock setup in
S3CompatibleStorageAdapterTest (method deleteFileWithS3Exception) to throw an
S3Exception from mockS3Client.deleteObject(...) so the code path matching S3
errors is exercised (e.g., construct an S3Exception with a message via its
builder and use doThrow(...) on mockS3Client). Ensure the thrown exception type
aligns with the adapter's S3-specific handling so assertThatThrownBy(() ->
s3CompatibleStorageAdapter.delete(fileKey)) still expects CloudStorageException.
- Around line 82-95: The test uploadFileWithS3Exception is currently throwing
SdkClientException instead of S3Exception; change the stub on
mockS3Client.putObject to throw an S3Exception (e.g., build one with
S3Exception.builder().message("S3 error").build()) and add the import for
software.amazon.awssdk.services.s3.model.S3Exception so the S3Exception path in
S3CompatibleStorageAdapter.upload(...) is exercised; keep the existing assertion
that a CloudStorageException is thrown.
🧹 Nitpick comments (8)
src/main/java/com/kustacks/kuring/message/application/service/FirebaseNotificationService.java (1)
106-108: 기존 코드:notificationDtoList.get(0)빈 리스트 방어 코드 부재현재
sendNotifications에서 빈 리스트를 걸러주고 있어 실질적 문제는 없지만,loggingNoticeSendInfo가 직접 호출될 경우IndexOutOfBoundsException이 발생할 수 있습니다. 방어적으로 빈 리스트 체크를 추가하는 것을 고려해 보세요.🛡️ 방어 코드 제안
private void loggingNoticeSendInfo(List<NoticeMessageDto> notificationDtoList) { + if (notificationDtoList.isEmpty()) { + return; + } log.info("FCM에 {}카테고리에 {}개의 공지 메세지를 전송.", notificationDtoList.get(0).getCategory(), notificationDtoList.size());src/main/java/com/kustacks/kuring/notice/domain/DepartmentName.java (1)
108-118: HOST_PREFIX_MAP의 충돌 병합 전략(a, b) -> a는 구성 오류를 무시할 수 있습니다.현재는
hostPrefix와fallbackHostPrefix값이 모두 고유하여 충돌이 발생하지 않지만, 향후 누군가 다른 학과의hostPrefix와 동일한fallbackHostPrefix를 추가하면 기존 매핑이 조용히 유지되어 디버깅이 어려운 버그가 발생할 수 있습니다.충돌 시 예외를 던지도록 변경하면 구성 오류를 빠르게 감지할 수 있습니다.
♻️ 제안 변경
.collect(Collectors.toMap( Map.Entry::getKey, Map.Entry::getValue, - (a, b) -> a + (a, b) -> { + throw new IllegalStateException( + "Duplicate HOST_PREFIX_MAP key found: " + a + " vs " + b); + } ))src/test/java/com/kustacks/kuring/notice/domain/DepartmentNameTest.java (1)
103-124:hostPrefixMap_test는 전체 enum을 순회하며 매핑 정합성을 검증하여 좋습니다.한 가지 선택적 개선 사항: 현재 테스트는 모든 hostPrefix가 올바르게 매핑되는지 확인하지만,
HOST_PREFIX_MAP의 크기까지 검증하면 중복 키가 무시되었거나 의도치 않은 항목이 추가된 경우도 감지할 수 있습니다. 다만, 이 수준의 검증이 반드시 필요한 것은 아닙니다..github/workflows/dev.yml (1)
11-11: 환경 이름 "Test-Server"가 개발 배포 워크플로우와 불일치합니다.이 워크플로우는 "Deploy to OCI (Develop)"인데, 환경 이름은
Test-Server입니다. 향후 혼동을 방지하기 위해Dev-Server등으로 통일하는 것을 고려해 주세요.src/main/java/com/kustacks/kuring/common/exception/InfrastructureException.java (1)
10-17:super()에 메시지를 전달하지 않아getMessage()가 null을 반환합니다.첫 번째 생성자에서
super()에 메시지를 전달하지 않으면, 표준 로깅 프레임워크나catch블록에서exception.getMessage()를 호출할 때null이 반환됩니다.errorCode의 메시지를RuntimeException에도 전달하는 것이 좋습니다.♻️ 제안하는 수정
public InfrastructureException(ErrorCode errorCode) { + super(errorCode.getMessage()); this.errorCode = errorCode; } public InfrastructureException(ErrorCode errorCode, Exception e) { - super(e); + super(errorCode.getMessage(), e); this.errorCode = errorCode; }src/main/java/com/kustacks/kuring/common/exception/code/ErrorCode.java (1)
110-112: 새 에러 코드가 "DomainLogicException" 섹션 아래에 위치해 있습니다.
STORAGE_S3_SDK_PROBLEM과FILE_IO_EXCEPTION은 인프라 수준의 에러 코드인데,DomainLogicException주석 블록 하단에 추가되어 있습니다. 가독성을 위해 별도의InfrastructureException섹션 주석을 추가하거나, 위치를 재조정하는 것을 고려해 주세요.src/main/java/com/kustacks/kuring/storage/adapter/out/MockStorageAdapter.java (1)
17-20: Mockupload에서 InputStream을 소비하지 않음Mock이므로 심각한 문제는 아니지만,
upload호출 후InputStream이 소비되지 않아 실제 환경과 동작이 다릅니다. 통합 테스트에서 리소스 누수 패턴을 놓칠 수 있습니다. 필요 시inputStream.close()호출이나 drain 로직을 고려해 보세요.src/main/java/com/kustacks/kuring/storage/adapter/out/S3CompatibleStorageAdapter.java (1)
46-48: 원본 예외 원인(cause)이 유실됩니다
CloudStorageException을 던질 때 원본S3Exception/SdkClientException을 cause로 전달하지 않아, 프로덕션에서 S3 관련 장애 디버깅 시 근본 원인 추적이 불가능합니다. 이 패턴이getPresignedUrl(Line 67-68)과delete(Line 81-82)에서도 동일하게 반복됩니다.
CloudStorageException에 cause를 받는 생성자를 추가하고, 원본 예외를 전파하는 것을 권장합니다.♻️ CloudStorageException에 cause 생성자 추가 및 적용
CloudStorageException.java:public CloudStorageException(ErrorCode errorCode, Throwable cause) { super(errorCode, cause); }그 후 어댑터에서:
} catch (S3Exception | SdkClientException e) { - throw new CloudStorageException(STORAGE_S3_SDK_PROBLEM); + throw new CloudStorageException(STORAGE_S3_SDK_PROBLEM, e); }
src/main/java/com/kustacks/kuring/common/exception/handler/CommonExceptionHandler.java
Show resolved
Hide resolved
src/main/java/com/kustacks/kuring/storage/exception/CloudStorageException.java
Show resolved
Hide resolved
src/test/java/com/kustacks/kuring/storage/adapter/out/S3CompatibleStorageAdapterTest.java
Show resolved
Hide resolved
src/test/java/com/kustacks/kuring/storage/adapter/out/S3CompatibleStorageAdapterTest.java
Show resolved
Hide resolved
src/test/java/com/kustacks/kuring/storage/adapter/out/S3CompatibleStorageAdapterTest.java
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
version 2.17.0
Summary by CodeRabbit
릴리스 노트
새로운 기능
Tests
Chores