refactor: 관리자 대학 이미지 업로드 경로 추가#760
Conversation
- 관리자 대학 로고와 배경 이미지 업로드 API를 추가 - 대학 영문명을 서버에서 업로드 디렉토리명으로 변환 - S3 업로드 서비스에 동적 하위 디렉토리 경로 생성을 추가
- 대학 영문명 디렉토리명 변환 테스트를 추가 - 동적 하위 디렉토리 기반 S3 업로드 경로 테스트를 추가
Walkthrough이번 PR은 크게 두 가지 기능 변경을 담고 있어요.
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/java/com/example/solidconnection/s3/domain/UploadDirectoryNameTest.java (1)
17-49: ⚡ Quick win1) 정규화 운영 적합성 확인을 위해
null·비ASCII 케이스 테스트를 추가해 주세요.PR 목적(운영 데이터 기준 정규화 충분성 검증) 대비 현재 케이스가 조금 얕습니다.
null입력, 그리고 정규화 후 빈 문자열이 되는 입력(예: 한글/특수문자-only)을 명시적으로 고정해 두면 회귀 방지에 유리합니다.테스트 추가 예시
+ `@Test` + void null_입력이면_예외가_발생한다() { + assertThatThrownBy(() -> UploadDirectoryName.fromUniversityEnglishName(null)) + .isInstanceOf(CustomException.class); + } + + `@Test` + void 비ASCII만_있어_정규화_결과가_비면_예외가_발생한다() { + String englishName = "東京大学"; + assertThatThrownBy(() -> UploadDirectoryName.fromUniversityEnglishName(englishName)) + .isInstanceOf(CustomException.class); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/solidconnection/s3/domain/UploadDirectoryNameTest.java` around lines 17 - 49, Add additional test cases for the UploadDirectoryName.fromUniversityEnglishName method to improve test coverage of edge cases. Create test methods that cover: a null input case (which should throw a CustomException), non-ASCII characters only input such as Korean or CJK characters that would result in an empty string after normalization, and inputs containing only special characters. These tests should verify that the method handles these edge cases gracefully, either by throwing appropriate exceptions or handling the normalization in a defined way that prevents regressions in production.src/main/java/com/example/solidconnection/s3/controller/S3Controller.java (1)
83-113: ⚡ Quick win1) 중복 업로드 흐름을 헬퍼로 묶어 두 엔드포인트의 분기 오차를 줄여주세요.
현재 Line 90-96, Line 106-112의 로직이 사실상 동일해서, 다음 변경 때 한쪽만 수정되는 드리프트 위험이 있습니다.
UploadPath만 인자로 받는 private helper로 합치면 유지보수성이 좋아집니다.예시 리팩터링
`@RequireRoleAccess`(roles = Role.ADMIN) `@PostMapping`("/admin/university/logo") public ResponseEntity<UploadedFileUrlResponse> uploadAdminUniversityLogo( `@AuthorizedUser` long adminId, `@RequestParam`("file") MultipartFile imageFile, `@RequestParam`("englishName") String englishName ) { - String directoryName = UploadDirectoryName.fromUniversityEnglishName(englishName); - UploadedFileUrlResponse logoImageUrl = s3Service.uploadFile( - imageFile, - UploadPath.ADMIN_UNIVERSITY_LOGO, - directoryName - ); - return ResponseEntity.ok(logoImageUrl); + return ResponseEntity.ok(uploadAdminUniversityImage(imageFile, englishName, UploadPath.ADMIN_UNIVERSITY_LOGO)); } `@RequireRoleAccess`(roles = Role.ADMIN) `@PostMapping`("/admin/university/background") public ResponseEntity<UploadedFileUrlResponse> uploadAdminUniversityBackground( `@AuthorizedUser` long adminId, `@RequestParam`("file") MultipartFile imageFile, `@RequestParam`("englishName") String englishName ) { - String directoryName = UploadDirectoryName.fromUniversityEnglishName(englishName); - UploadedFileUrlResponse backgroundImageUrl = s3Service.uploadFile( - imageFile, - UploadPath.ADMIN_UNIVERSITY_BACKGROUND, - directoryName - ); - return ResponseEntity.ok(backgroundImageUrl); + return ResponseEntity.ok(uploadAdminUniversityImage(imageFile, englishName, UploadPath.ADMIN_UNIVERSITY_BACKGROUND)); } + +private UploadedFileUrlResponse uploadAdminUniversityImage( + MultipartFile imageFile, + String englishName, + UploadPath uploadPath +) { + String directoryName = UploadDirectoryName.fromUniversityEnglishName(englishName); + return s3Service.uploadFile(imageFile, uploadPath, directoryName); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/solidconnection/s3/controller/S3Controller.java` around lines 83 - 113, The methods uploadAdminUniversityLogo and uploadAdminUniversityBackground contain duplicate logic where both retrieve the directoryName from englishName and invoke s3Service.uploadFile with different UploadPath enum values. Create a private helper method that accepts the MultipartFile, UploadPath enum value, and englishName as parameters, then extract the common upload logic (directoryName retrieval and s3Service.uploadFile invocation) into this helper. Update both uploadAdminUniversityLogo and uploadAdminUniversityBackground to call this helper method with their respective UploadPath.ADMIN_UNIVERSITY_LOGO and UploadPath.ADMIN_UNIVERSITY_BACKGROUND values to eliminate code duplication and reduce maintenance drift.src/test/java/com/example/solidconnection/s3/service/S3ServiceDynamicPathTest.java (1)
38-55: ⚡ Quick win1) 응답 URL뿐 아니라 실제 업로드 키 인자도 함께 검증해 주세요.
지금은
response.fileUrl()만 확인해서, 내부 업로드 호출 키가 어긋나도 놓칠 수 있습니다.fileUploadService.uploadFile(...)의 path 인자에 동적 디렉터리가 포함됐는지 같이 검증하면 계약 테스트가 더 단단해집니다.검증 보강 예시
import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertAll; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.verify; + +import org.mockito.ArgumentCaptor; @@ assertAll( () -> assertThat(response.fileUrl()).startsWith("admin/logo/university_of_tokyo/"), () -> assertThat(response.fileUrl()).endsWith(".png") ); + + ArgumentCaptor<String> pathCaptor = ArgumentCaptor.forClass(String.class); + verify(fileUploadService).uploadFile(eq(null), pathCaptor.capture(), any(), eq("image/png")); + assertThat(pathCaptor.getValue()).contains("admin/logo/university_of_tokyo/"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/solidconnection/s3/service/S3ServiceDynamicPathTest.java` around lines 38 - 55, The test currently only verifies the response URL but does not validate that the internal upload call uses the correct path argument with the dynamic directory. Add mock verification to ensure that the underlying upload method within s3Service.uploadFile is being called with a path argument that correctly includes the dynamic subdirectory "university_of_tokyo" alongside the base UploadPath.ADMIN_UNIVERSITY_LOGO. Use a verification mechanism (such as Mockito's verify) to assert that the path passed to the internal upload call contains both the static upload path and the dynamic directory component to strengthen the contract test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/example/solidconnection/s3/domain/UploadDirectoryName.java`:
- Around line 11-27: The fromUniversityEnglishName method generates S3 directory
names from university English names, but since the englishName field has no
unique constraint in the database, different universities can normalize to the
same slug, causing file collisions in S3 paths. To fix this, either modify the
method to include a stable identifier like universityId as part of the directory
name path (making it unique across universities), or add a unique constraint to
the englishName field in the HostUniversity entity to prevent duplicate English
names at the database level. Choose the appropriate solution based on your
architecture and requirements.
---
Nitpick comments:
In `@src/main/java/com/example/solidconnection/s3/controller/S3Controller.java`:
- Around line 83-113: The methods uploadAdminUniversityLogo and
uploadAdminUniversityBackground contain duplicate logic where both retrieve the
directoryName from englishName and invoke s3Service.uploadFile with different
UploadPath enum values. Create a private helper method that accepts the
MultipartFile, UploadPath enum value, and englishName as parameters, then
extract the common upload logic (directoryName retrieval and
s3Service.uploadFile invocation) into this helper. Update both
uploadAdminUniversityLogo and uploadAdminUniversityBackground to call this
helper method with their respective UploadPath.ADMIN_UNIVERSITY_LOGO and
UploadPath.ADMIN_UNIVERSITY_BACKGROUND values to eliminate code duplication and
reduce maintenance drift.
In
`@src/test/java/com/example/solidconnection/s3/domain/UploadDirectoryNameTest.java`:
- Around line 17-49: Add additional test cases for the
UploadDirectoryName.fromUniversityEnglishName method to improve test coverage of
edge cases. Create test methods that cover: a null input case (which should
throw a CustomException), non-ASCII characters only input such as Korean or CJK
characters that would result in an empty string after normalization, and inputs
containing only special characters. These tests should verify that the method
handles these edge cases gracefully, either by throwing appropriate exceptions
or handling the normalization in a defined way that prevents regressions in
production.
In
`@src/test/java/com/example/solidconnection/s3/service/S3ServiceDynamicPathTest.java`:
- Around line 38-55: The test currently only verifies the response URL but does
not validate that the internal upload call uses the correct path argument with
the dynamic directory. Add mock verification to ensure that the underlying
upload method within s3Service.uploadFile is being called with a path argument
that correctly includes the dynamic subdirectory "university_of_tokyo" alongside
the base UploadPath.ADMIN_UNIVERSITY_LOGO. Use a verification mechanism (such as
Mockito's verify) to assert that the path passed to the internal upload call
contains both the static upload path and the dynamic directory component to
strengthen the contract test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf63ea4b-50d4-41a4-9fd3-c9dd412f24e6
📒 Files selected for processing (6)
src/main/java/com/example/solidconnection/s3/controller/S3Controller.javasrc/main/java/com/example/solidconnection/s3/domain/UploadDirectoryName.javasrc/main/java/com/example/solidconnection/s3/domain/UploadPath.javasrc/main/java/com/example/solidconnection/s3/service/S3Service.javasrc/test/java/com/example/solidconnection/s3/domain/UploadDirectoryNameTest.javasrc/test/java/com/example/solidconnection/s3/service/S3ServiceDynamicPathTest.java
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136df16b05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c8900fb to
136df16
Compare
- 대학 영문명 디렉토리명에 원본 영문명 해시를 추가 - 파견 대학 영문명 중복 검증과 유니크 제약을 추가 - 정규화 충돌과 영문명 중복 검증 테스트를 추가
- 업로드 경로별 이미지 전용 검증 정책을 추가 - 관리자 대학 이미지 업로드에서 문서 확장자를 차단 - 증빙 파일 업로드의 문서 확장자 허용 동작을 테스트로 보장
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/example/solidconnection/s3/service/S3ServiceTest.java (1)
114-127: ⚡ Quick win2) 신규 배경 경로(
ADMIN_UNIVERSITY_BACKGROUND)도 동일한 차단 테스트를 추가해 주세요.지금은 Line 115 기준
ADMIN_UNIVERSITY_LOGO만 문서 확장자 차단을 검증합니다. 동일하게imageOnly=true인ADMIN_UNIVERSITY_BACKGROUND케이스도 추가하면 enum 상수별 회귀를 더 빨리 잡을 수 있습니다.✅ 제안 테스트 추가
+ `@Test` + void 이미지_전용_배경_업로드_경로도_문서_확장자를_허용하지_않는다() { + // given + MockMultipartFile pdfFile = createMockFile("background.pdf", 100); + + // when & then + assertThatThrownBy(() -> s3Service.uploadFile( + pdfFile, + UploadPath.ADMIN_UNIVERSITY_BACKGROUND, + "university_of_tokyo" + )) + .isInstanceOf(CustomException.class) + .hasMessageContaining("허용된 형식"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/solidconnection/s3/service/S3ServiceTest.java` around lines 114 - 127, Add a new test method to the S3ServiceTest class that mirrors the existing test method (이미지_전용_업로드_경로는_문서_확장자를_허용하지_않는다) but uses UploadPath.ADMIN_UNIVERSITY_BACKGROUND instead of UploadPath.ADMIN_UNIVERSITY_LOGO. Create a MockMultipartFile with a PDF extension (e.g., "logo.pdf"), call s3Service.uploadFile with the background path and the PDF file, and assert that it throws a CustomException with the message containing "허용된 형식". This ensures both image-only paths properly reject document file types.src/test/java/com/example/solidconnection/admin/service/AdminHostUniversityServiceTest.java (1)
369-391: ⚡ Quick win1.
excludeId허용 분기 회귀를 막는 성공 케이스도 함께 추가해주세요.현재는 “다른 대학 영문명 충돌” 실패 경로는 잘 검증되지만, “같은 대학의 기존 영문명으로 수정” 성공 경로가 없어
validateEnglishNameNotDuplicated의 핵심 분기 회귀를 놓칠 수 있습니다. 아래처럼 성공 테스트 1개를 보강하면 안전합니다.테스트 보강 예시
@@ `@Test` void 같은_대학의_한글명으로_수정하면_성공한다() { @@ } + + `@Test` + void 같은_대학의_영문명으로_수정하면_성공한다() { + // given + HostUniversity university = universityFixture.괌_대학(); + + AdminHostUniversityUpdateRequest request = new AdminHostUniversityUpdateRequest( + "Updated Korean Name", + university.getEnglishName(), + "수정된 표시명", + null, null, null, + "https://logo.com/image.png", + "https://background.com/image.png", + null, + university.getCountry().getCode(), + university.getRegion().getCode() + ); + + // when + AdminHostUniversityDetailResponse response = adminHostUniversityService.updateHostUniversity( + university.getId(), request); + + // then + assertThat(response.englishName()).isEqualTo(university.getEnglishName()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/solidconnection/admin/service/AdminHostUniversityServiceTest.java` around lines 369 - 391, The current test validates the failure scenario where updating with another university's English name raises an exception, but lacks a success test case to verify that updating a university with its own existing English name succeeds. Add a new test method that updates university1 with its own English name to ensure the excludeId filtering logic in the validateEnglishNameNotDuplicated method correctly allows duplicate names when they belong to the same university being updated. This success case is critical to prevent regressions in the excludeId filtering branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/example/solidconnection/s3/domain/UploadPath.java`:
- Around line 42-46: The validateExtension method in the UploadPath class
currently only checks file extensions, which can be bypassed by renaming
malicious files. Enhance the validation by implementing magic byte (file
signature) based validation as the primary check and use the existing extension
validation as a secondary verification method. Add a new validation method to
verify the actual file content against expected magic bytes for each allowed
file type, and integrate this content-based check into the validateExtension
method so that both the file content signature and extension must match the
allowed types before the file is accepted.
---
Nitpick comments:
In
`@src/test/java/com/example/solidconnection/admin/service/AdminHostUniversityServiceTest.java`:
- Around line 369-391: The current test validates the failure scenario where
updating with another university's English name raises an exception, but lacks a
success test case to verify that updating a university with its own existing
English name succeeds. Add a new test method that updates university1 with its
own English name to ensure the excludeId filtering logic in the
validateEnglishNameNotDuplicated method correctly allows duplicate names when
they belong to the same university being updated. This success case is critical
to prevent regressions in the excludeId filtering branch.
In `@src/test/java/com/example/solidconnection/s3/service/S3ServiceTest.java`:
- Around line 114-127: Add a new test method to the S3ServiceTest class that
mirrors the existing test method (이미지_전용_업로드_경로는_문서_확장자를_허용하지_않는다) but uses
UploadPath.ADMIN_UNIVERSITY_BACKGROUND instead of
UploadPath.ADMIN_UNIVERSITY_LOGO. Create a MockMultipartFile with a PDF
extension (e.g., "logo.pdf"), call s3Service.uploadFile with the background path
and the PDF file, and assert that it throws a CustomException with the message
containing "허용된 형식". This ensures both image-only paths properly reject document
file types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0a4c6c4-a4fa-49d5-b75a-03ebf54a9824
📒 Files selected for processing (9)
src/main/java/com/example/solidconnection/admin/university/service/AdminHostUniversityService.javasrc/main/java/com/example/solidconnection/s3/domain/UploadDirectoryName.javasrc/main/java/com/example/solidconnection/s3/domain/UploadPath.javasrc/main/java/com/example/solidconnection/university/domain/HostUniversity.javasrc/main/java/com/example/solidconnection/university/repository/HostUniversityRepository.javasrc/main/resources/db/migration/V52__add_unique_constraint_to_host_university_english_name.sqlsrc/test/java/com/example/solidconnection/admin/service/AdminHostUniversityServiceTest.javasrc/test/java/com/example/solidconnection/s3/domain/UploadDirectoryNameTest.javasrc/test/java/com/example/solidconnection/s3/service/S3ServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/resources/db/migration/V52__add_unique_constraint_to_host_university_english_name.sql
- src/main/java/com/example/solidconnection/university/repository/HostUniversityRepository.java
- src/main/java/com/example/solidconnection/university/domain/HostUniversity.java
- src/main/java/com/example/solidconnection/s3/domain/UploadDirectoryName.java
- src/test/java/com/example/solidconnection/s3/domain/UploadDirectoryNameTest.java
- src/main/java/com/example/solidconnection/admin/university/service/AdminHostUniversityService.java
| public void validateExtension(String extension) { | ||
| if (extension == null || !FileConstants.ALL_ALLOWED_EXTENSIONS.contains(extension.toLowerCase())) { | ||
| if (extension == null || !getAllowedExtensions().contains(extension.toLowerCase())) { | ||
| throw new CustomException(ErrorCode.NOT_ALLOWED_FILE_EXTENSIONS, | ||
| "허용된 형식: " + getAllowedExtensionsMessage()); | ||
| } |
There was a problem hiding this comment.
1) 확장자-only 검증은 우회 업로드를 막지 못합니다.
Line 43 기준으로 현재 검증은 파일명 확장자만 신뢰합니다. 이 방식은 내용이 다른 파일(예: 실행파일/스크립트)을 확장자만 바꿔 통과시킬 수 있어요. 최소한 매직바이트(파일 시그니처) 기반 검증을 추가하고, 확장자 검증은 보조 수단으로 두는 편이 안전합니다.
🔧 제안 패치 (개념 예시)
public void validateExtension(String extension) {
if (extension == null || !getAllowedExtensions().contains(extension.toLowerCase())) {
throw new CustomException(ErrorCode.NOT_ALLOWED_FILE_EXTENSIONS,
"허용된 형식: " + getAllowedExtensionsMessage());
}
}
+
+// S3Service.validateFile(...)에서 확장자 검증 후 추가:
+// - imageOnly 경로: JPEG/PNG/WEBP 등 허용 포맷 시그니처 검사
+// - 비 imageOnly 경로: MIME + 시그니처 기반 최소 검증🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/example/solidconnection/s3/domain/UploadPath.java` around
lines 42 - 46, The validateExtension method in the UploadPath class currently
only checks file extensions, which can be bypassed by renaming malicious files.
Enhance the validation by implementing magic byte (file signature) based
validation as the primary check and use the existing extension validation as a
secondary verification method. Add a new validation method to verify the actual
file content against expected magic bytes for each allowed file type, and
integrate this content-based check into the validateExtension method so that
both the file content signature and extension must match the allowed types
before the file is accepted.
There was a problem hiding this comment.
@lsy1307 알겠어요! 나중에 작업하실 때 놓치지 않도록, 팔로업 이슈를 생성해 드릴까요?
- 이슈 내용 요약
UploadPath.validateExtension에 매직바이트(파일 시그니처) 기반 검증 추가- 확장자 검증은 보조 수단으로 유지
imageOnly경로와 그 외 경로에 대한 검증 전략 분리
원하시면 "이슈 만들어줘"라고 말씀해 주세요 😊
(๑˃ᵕ˂)و ✨
관련 이슈
작업 내용
특이 사항
admin/logo/{대학_영문명_slug}_{hash}와admin/background/{대학_영문명_slug}_{hash}형식입니다._치환, 특수문자 제거 규칙을 적용하고 SHA-256 기반 12자리 hex suffix를 추가합니다.host_university.english_name에 unique 제약이 추가되므로 기존 운영 데이터에 중복 영문명이 없어야 합니다.리뷰 요구사항 (선택)