Conversation
이미지 단 건 삭제를 key 또는 url로 가능하도록 기능을 구현합니다. 해당 API는 S3 안에 있는 이미지 삭제 용도로 모임에서 사용하는 삭제가 아닙니다.
모임에서 이미지 삭제는 URL을 Request Param으로 요청받은 후에 삭제합니다.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough모임 이미지 단 건 삭제 기능이 추가되었습니다. 인증된 호스트만 URL로 지정한 그룹 이미지를 검증 후 삭제하고, 관련 에러 코드 및 저장소/이미지 서비스 메서드가 확장되었습니다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GroupController
participant GroupService
participant GroupImageRepository
participant ImageUploadService
participant Storage as S3
Client->>GroupController: DELETE /{groupId}/images/one?url={url}
GroupController->>GroupService: deleteOne(userDetails, groupId, url)
GroupService->>GroupService: findActiveGroup(groupId)
GroupService->>GroupService: verifyHost(userDetails)
alt url is blank
GroupService-->>Client: IMAGE_URL_REQUIRED (400)
else
GroupService->>GroupImageRepository: existsByGroupAndImageUrl(group, url)
alt not exists
GroupService-->>Client: GROUP_IMAGE_NOT_FOUND (404)
else
GroupService->>ImageUploadService: deleteOne(null, url)
ImageUploadService->>ImageUploadService: extractKeyFromUrl(url)
ImageUploadService->>ImageUploadService: validateKey(key)
ImageUploadService->>Storage: delete object by key
Storage-->>ImageUploadService: deletion result
ImageUploadService-->>GroupService: deletion confirmed
GroupService->>GroupImageRepository: deleteByGroupAndImageUrl(group, url)
GroupImageRepository-->>GroupService: deleted
GroupService-->>Client: 204 No Content
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 5
🧹 Nitpick comments (6)
src/main/java/team/wego/wegobackend/group/domain/repository/v1/GroupImageRepository.java (1)
11-15: 성능 최적화를 위한 인덱스 고려가 필요합니다.새로 추가된 세 메서드(
findByGroupAndImageUrl,deleteByGroupAndImageUrl,existsByGroupAndImageUrl)는 모두imageUrl을 조회 조건으로 사용합니다.imageUrl은 일반적으로 긴 문자열이며, 인덱스가 없을 경우 테이블 풀 스캔이 발생할 수 있습니다.
GroupImage엔티티의imageUrl컬럼에 복합 인덱스 추가를 고려하세요:@Table(indexes = { @Index(name = "idx_group_image_url", columnList = "group_id, image_url") })또는 개별 인덱스:
@Column(name = "image_url") @Index(name = "idx_image_url") private String imageUrl;src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java (1)
440-455: URL에서 key 추출 실패 시 에러 처리를 명확히 하세요.Line 451에서
extractKeyFromUrl(url)이null을 반환할 수 있지만, 이 경우에 대한 명시적인 에러 처리가 없습니다.validateKey에서 예외가 발생하긴 하지만, 더 명확한 에러 메시지를 제공하는 것이 좋습니다.다음과 같이 명시적인 검증을 추가하세요:
String targetKey = hasUrl ? extractKeyFromUrl(url) : key; + + if (targetKey == null || targetKey.isBlank()) { + throw new ImageException(ImageExceptionCode.INVALID_IMAGE_KEY, url != null ? url : "null"); + } validateKey(targetKey); delete(targetKey);src/main/java/team/wego/wegobackend/group/application/service/v1/GroupService.java (1)
652-658: 동시성 이슈(TOCTOU)가 발생할 수 있습니다.Lines 652와 658 사이에 Time-of-Check to Time-of-Use (TOCTOU) 경합 조건이 존재합니다. 두 개의 요청이 동시에 같은 이미지를 삭제하려 할 때:
- 요청 A:
existsByGroupAndImageUrl→ true- 요청 B:
existsByGroupAndImageUrl→ true- 요청 A:
deleteByGroupAndImageUrl→ 성공- 요청 B:
deleteByGroupAndImageUrl→ 아무것도 삭제하지 않음 (0 rows affected)현재 코드는 요청 B가 404를 반환하지 않고 성공으로 처리됩니다.
다음 중 하나를 선택하세요:
옵션 1: 비관적 락 사용
@Lock(LockModeType.PESSIMISTIC_WRITE) Optional<GroupImage> findByGroupAndImageUrl(Group group, String imageUrl);옵션 2: 삭제 결과 확인 (더 간단함)
// Repository에 추가 @Modifying @Query("DELETE FROM GroupImage gi WHERE gi.group = :group AND gi.imageUrl = :imageUrl") int deleteByGroupAndImageUrl(@Param("group") Group group, @Param("imageUrl") String imageUrl); // Service에서 int deletedCount = groupImageRepository.deleteByGroupAndImageUrl(group, url); if (deletedCount == 0) { throw new GroupException(GroupErrorCode.GROUP_IMAGE_NOT_FOUND, groupId); }옵션 2가 더 단순하고 효율적입니다.
src/test/http/group/group-image-delete.http (1)
1-172: 테스트 시나리오가 포괄적으로 잘 작성되었습니다.이 테스트 파일은 다음 시나리오를 잘 커버하고 있습니다:
- 호스트 권한 검증 (line 146-148)
- URL 파라미터 필수 검증 (line 150-152)
- 존재하지 않는 이미지 처리 (line 154-156)
- 중복 삭제 멱등성 (line 158-164)
HTTP 테스트에 명시적인 응답 검증을 추가하면 더욱 견고해집니다:
### 4-1. MEMBER가 모임 이미지 삭제 시도 -> 실패해야 함 DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img1_main}} Authorization: Bearer {{member1AccessToken}} > {% client.test("Should return 401 or 403", function() { client.assert(response.status === 401 || response.status === 403, "Expected unauthorized"); }); %}src/test/http/image/image-api.http (1)
200-203: 테스트 의도가 불명확합니다.Lines 200-203의 테스트 제목이 "key, url 모두 포함"이지만, 실제로는 url만 전달하고 있습니다.
만약
KEY_AND_URL_CONFLICT에러를 테스트하려는 의도라면:### 이미지 단 건 삭제 - key와 url 동시 전달 (에러 케이스) DELETE http://localhost:8080/api/v1/images/one ?key=test/20251216092723_0_xxx.jpeg &url=https://we-go-bucket.s3.ap-northeast-2.amazonaws.com/20251216092723_0_4ead2fbf-2109-4351-98c4-54ec2b1c8353.jpeg Authorization: Bearer {{accessToken}} > {% client.test("Should return 400 for conflict", function() { client.assert(response.status === 400, "Expected BAD_REQUEST"); }); %}만약 정상 케이스를 테스트하는 것이라면, 제목을 "이미지 단 건 삭제 (url 사용)"로 변경하세요.
src/main/java/team/wego/wegobackend/image/presentation/ImageController.java (1)
106-111: 입력 파라미터 검증을 컨트롤러 레벨에서 추가하고 문서화를 개선하세요.서비스 레이어에서 이미 null 검증 및 파라미터 충돌 검증이 구현되어 있으나, REST API 설계의 베스트 프랙티스를 따르려면 컨트롤러 레벨에서의 검증이 필요합니다:
- 빠른 실패(Fail-Fast): 컨트롤러에서 검증하면 잘못된 요청에 대해 400 Bad Request를 즉시 반환할 수 있습니다.
- 명확한 API 계약: 파라미터 사용 규칙을 문서화하면 API 사용자에게 명확한 지침을 제공할 수 있습니다.
다음과 같이 개선하는 것을 권장합니다:
+ /** + * 이미지를 단 건 삭제합니다. + * + * @param key S3 객체 키 (key 또는 url 중 정확히 하나만 제공) + * @param url 이미지 전체 URL (key 또는 url 중 정확히 하나만 제공) + * @return 204 No Content + */ @DeleteMapping("/one") public ResponseEntity<Void> deleteOne( @RequestParam(value = "key", required = false) String key, @RequestParam(value = "url", required = false) String url ) { + boolean hasKey = key != null && !key.isBlank(); + boolean hasUrl = url != null && !url.isBlank(); + + if (!hasKey && !hasUrl) { + throw new IllegalArgumentException("key 또는 url 중 하나는 반드시 제공해야 합니다."); + } + if (hasKey && hasUrl) { + throw new IllegalArgumentException("key와 url은 동시에 제공될 수 없습니다."); + } imageUploadService.deleteOne(key, url); return ResponseEntity.noContent().build(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/team/wego/wegobackend/group/application/service/v1/GroupService.java(2 hunks)src/main/java/team/wego/wegobackend/group/domain/exception/GroupErrorCode.java(1 hunks)src/main/java/team/wego/wegobackend/group/domain/repository/v1/GroupImageRepository.java(1 hunks)src/main/java/team/wego/wegobackend/group/presentation/v1/GroupController.java(1 hunks)src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java(3 hunks)src/main/java/team/wego/wegobackend/image/domain/exception/ImageExceptionCode.java(2 hunks)src/main/java/team/wego/wegobackend/image/presentation/ImageController.java(1 hunks)src/test/http/group/group-image-delete.http(1 hunks)src/test/http/image/image-api.http(2 hunks)
⏰ 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). (2)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Agent
🔇 Additional comments (1)
src/main/java/team/wego/wegobackend/group/domain/exception/GroupErrorCode.java (1)
32-33: 형식 문자열 처리가 올바르게 구현되어 있습니다. GroupService에서 Long 타입의 groupId가 전달되며, String.format()이 %d와 함께 Long 타입을 자동으로 처리하므로 추가 조치는 불필요합니다.
src/main/java/team/wego/wegobackend/group/application/service/v1/GroupService.java
Show resolved
Hide resolved
src/main/java/team/wego/wegobackend/group/presentation/v1/GroupController.java
Show resolved
Hide resolved
| public String extractKeyFromUrl(String imageUrl) { | ||
| if (imageUrl == null || imageUrl.isBlank()) { | ||
| return null; | ||
| } | ||
| if (imageUrl == null || imageUrl.isBlank()) return null; | ||
|
|
||
| String clean = imageUrl; | ||
| int qIdx = clean.indexOf('?'); | ||
| if (qIdx >= 0) clean = clean.substring(0, qIdx); | ||
|
|
||
| String endpoint = awsS3Properties.getPublicEndpoint(); | ||
| if (endpoint != null && !endpoint.isBlank()) { | ||
| String prefix = endpoint.endsWith("/") ? endpoint : endpoint + "/"; | ||
| if (imageUrl.startsWith(prefix)) { | ||
| return imageUrl.substring(prefix.length()); | ||
| if (clean.startsWith(prefix)) { | ||
| return clean.substring(prefix.length()); | ||
| } | ||
| } | ||
|
|
||
| // fallback: 마지막 "/" 이후를 key로 사용 | ||
| int idx = imageUrl.lastIndexOf('/'); | ||
| if (idx >= 0 && idx + 1 < imageUrl.length()) { | ||
| return imageUrl.substring(idx + 1); | ||
| try { | ||
| URI uri = URI.create(clean); | ||
| String path = uri.getPath(); // "/dir/xxx.webp" | ||
| if (path == null || path.isBlank()) return null; | ||
| return path.startsWith("/") ? path.substring(1) : path; | ||
| } catch (IllegalArgumentException e) { | ||
| return clean; // url이 아니면 key로 간주 | ||
| } | ||
|
|
||
| // 그래도 안 되면 전체를 key로 시도 (최악의 경우) | ||
| return imageUrl; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
URL 파싱 실패 시 입력값을 그대로 반환하는 로직을 검토하세요.
Line 410에서 URI 파싱 실패 시 정제된 입력값(clean)을 그대로 key로 간주하여 반환합니다. 이는 URL도 아니고 유효한 key 형식도 아닌 값을 그대로 통과시킬 수 있습니다.
다음과 같이 개선을 고려하세요:
try {
URI uri = URI.create(clean);
String path = uri.getPath(); // "/dir/xxx.webp"
if (path == null || path.isBlank()) return null;
return path.startsWith("/") ? path.substring(1) : path;
} catch (IllegalArgumentException e) {
- return clean; // url이 아니면 key로 간주
+ // key로 간주하되, 기본 검증 수행
+ if (clean.contains("..") || clean.startsWith("/")) {
+ return null;
+ }
+ return clean;
}| private void validateKey(String key) { | ||
| if (key == null || key.isBlank()) { | ||
| throw new ImageException(ImageExceptionCode.KEY_OR_URL_REQUIRED); | ||
| } | ||
| if (key.contains("..") || key.startsWith("/")) { | ||
| throw new ImageException(ImageExceptionCode.DIR_INVALID_TRAVERSAL); | ||
| } | ||
| if (!key.matches("[a-zA-Z0-9_\\-./]+")) { | ||
| throw new ImageException(ImageExceptionCode.INVALID_IMAGE_KEY, key); | ||
| } | ||
| } |
There was a problem hiding this comment.
잘못된 에러 코드를 사용하고 있습니다.
Line 459에서 key가 null이거나 blank일 때 KEY_OR_URL_REQUIRED를 던지고 있습니다. 하지만 이 메서드는 이미 key 또는 url이 제공된 후 호출되는 내부 검증 메서드이므로, 더 적절한 에러 코드는 KEY_REQUIRED 또는 새로운 INVALID_IMAGE_KEY입니다.
다음과 같이 수정하세요:
private void validateKey(String key) {
if (key == null || key.isBlank()) {
- throw new ImageException(ImageExceptionCode.KEY_OR_URL_REQUIRED);
+ throw new ImageException(ImageExceptionCode.KEY_REQUIRED);
}
if (key.contains("..") || key.startsWith("/")) {
throw new ImageException(ImageExceptionCode.DIR_INVALID_TRAVERSAL);
}
if (!key.matches("[a-zA-Z0-9_\\-./]+")) {
throw new ImageException(ImageExceptionCode.INVALID_IMAGE_KEY, key);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java
around lines 457 to 467, the validation currently throws
ImageExceptionCode.KEY_OR_URL_REQUIRED when key is null or blank but this
internal validator is only called after a key or URL is known to be required;
change the thrown code to ImageExceptionCode.KEY_REQUIRED (or use the existing
INVALID_IMAGE_KEY if you prefer a single code) so the error accurately reflects
that the key is missing, keeping the other checks (directory traversal and
allowed characters) unchanged.
| KEY_OR_URL_REQUIRED(HttpStatus.BAD_REQUEST, | ||
| "이미지: key 또는 url 중 하나는 필수입니다."), | ||
|
|
||
| KEY_AND_URL_CONFLICT(HttpStatus.BAD_REQUEST, | ||
| "이미지: key와 url은 동시에 보낼 수 없습니다. 둘 중 하나만 보내주세요."), | ||
|
|
||
| INVALID_IMAGE_KEY(HttpStatus.BAD_REQUEST, | ||
| "이미지: 잘못된 key 형식입니다. key=%s"), | ||
|
|
||
| KEY_REQUIRED(HttpStatus.BAD_REQUEST, | ||
| "이미지: key가 존재하지 않습니다."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for usage of KEY_OR_URL_REQUIRED and KEY_REQUIRED error codes
echo "=== KEY_OR_URL_REQUIRED 사용처 ==="
rg -n "KEY_OR_URL_REQUIRED" --type java -C3
echo ""
echo "=== KEY_REQUIRED 사용처 ==="
rg -n "KEY_REQUIRED" --type java -C3
echo ""
echo "=== ImageExceptionCode.java 전체 구조 확인 ==="
wc -l src/main/java/team/wego/wegobackend/image/domain/exception/ImageExceptionCode.javaRepository: WeGo-Together/WeGo_BackEnd
Length of output: 3793
validateKey() 메서드의 에러 코드를 수정하세요.
ImageUploadService.java 라인 459에서 KEY_OR_URL_REQUIRED 대신 KEY_REQUIRED를 사용해야 합니다. 현재는 validateKey() 메서드 내에서 key가 null이거나 비어있을 때 "key 또는 url 중 하나는 필수입니다"라는 부적절한 메시지를 반환하고 있습니다. 또한 KEY_REQUIRED는 정의되었으나 사용되지 않는 dead code이므로, 이 라인을 수정하여 정의된 에러 코드를 활용하세요.
🤖 Prompt for AI Agents
In
src/main/java/team/wego/wegobackend/image/domain/exception/ImageExceptionCode.java
lines 49-59 and in
src/main/java/team/wego/wegobackend/image/service/ImageUploadService.java at
line 459: validateKey() currently throws KEY_OR_URL_REQUIRED when the key is
null/empty; change validateKey() to throw ImageExceptionCode.KEY_REQUIRED
instead of KEY_OR_URL_REQUIRED so the defined KEY_REQUIRED enum is used (no enum
deletion needed), and ensure the thrown error includes the key value/context as
expected by KEY_REQUIRED's format.
There was a problem hiding this comment.
Pull request overview
This PR implements a single image deletion feature for groups, allowing group hosts to delete individual images by providing the full URL. The implementation includes authorization checks to ensure only the group host can delete images, and adds comprehensive validation for both key and URL-based deletion.
Key Changes:
- Added
deleteOneendpoints to bothImageControllerandGroupControllerthat accept URL parameters for image deletion - Enhanced
ImageUploadServicewith URL-to-key extraction logic and key validation - Added new error codes for missing/conflicting parameters and invalid keys
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/http/image/image-api.http |
Added authentication setup and image deletion test case |
src/test/http/group/group-image-delete.http |
Comprehensive test scenarios for group image deletion including permission checks and edge cases |
src/main/java/team/wego/wegobackend/image/presentation/ImageController.java |
Updated deleteOne endpoint to accept both key and URL parameters |
src/main/java/team/wego/wegobackend/image/domain/exception/ImageExceptionCode.java |
Added new error codes for validation (KEY_OR_URL_REQUIRED, KEY_AND_URL_CONFLICT, INVALID_IMAGE_KEY, KEY_REQUIRED) |
src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java |
Implemented deleteOne method with key/URL validation and enhanced extractKeyFromUrl to handle query parameters |
src/main/java/team/wego/wegobackend/group/presentation/v1/GroupController.java |
Added new deleteOne endpoint for group-specific image deletion with authorization |
src/main/java/team/wego/wegobackend/group/domain/repository/v1/GroupImageRepository.java |
Added repository methods for finding, checking existence, and deleting images by group and URL |
src/main/java/team/wego/wegobackend/group/domain/exception/GroupErrorCode.java |
Added error codes for missing URL and image not found scenarios |
src/main/java/team/wego/wegobackend/group/application/service/v1/GroupService.java |
Implemented deleteOne method with host permission validation and transactional deletion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public ResponseEntity<Void> deleteOne( | ||
| @AuthenticationPrincipal CustomUserDetails userDetails, | ||
| @PathVariable Long groupId, | ||
| @RequestParam(value = "url", required = false) String url | ||
| ) { | ||
| groupService.deleteOne(userDetails, groupId, url); | ||
| return ResponseEntity.noContent().build(); |
There was a problem hiding this comment.
The return type of deleteOne is inconsistent with the deleteGroup method. The deleteGroup method returns ResponseEntity<ApiResponse<Void>> (line 115) while deleteOne returns ResponseEntity<Void> (line 128). For consistency within the API, both delete endpoints should use the same response wrapper pattern. Consider changing the return type to match deleteGroup.
| public ResponseEntity<Void> deleteOne( | |
| @AuthenticationPrincipal CustomUserDetails userDetails, | |
| @PathVariable Long groupId, | |
| @RequestParam(value = "url", required = false) String url | |
| ) { | |
| groupService.deleteOne(userDetails, groupId, url); | |
| return ResponseEntity.noContent().build(); | |
| public ResponseEntity<ApiResponse<Void>> deleteOne( | |
| @AuthenticationPrincipal CustomUserDetails userDetails, | |
| @PathVariable Long groupId, | |
| @RequestParam(value = "url", required = false) String url | |
| ) { | |
| groupService.deleteOne(userDetails, groupId, url); | |
| return ResponseEntity | |
| .status(HttpStatus.NO_CONTENT) | |
| .body(ApiResponse.success(HttpStatus.NO_CONTENT.value(), null)); |
| imageUploadService.deleteOne(null, url); | ||
| groupImageRepository.deleteByGroupAndImageUrl(group, url); |
There was a problem hiding this comment.
The order of operations is problematic here. If the S3 deletion succeeds but the database deletion fails (due to a transaction rollback or database error), the system will be in an inconsistent state where the file is deleted from S3 but the database still references it. The database deletion should be performed first, then the S3 deletion should occur after the transaction commits successfully. Consider moving the S3 deletion outside the transaction or using a transactional event listener to execute it after commit.
|
|
||
| private void validateKey(String key) { | ||
| if (key == null || key.isBlank()) { | ||
| throw new ImageException(ImageExceptionCode.KEY_OR_URL_REQUIRED); |
There was a problem hiding this comment.
The error message KEY_OR_URL_REQUIRED is misleading in this context. At this point in validateKey(), we already know that either a key or URL was provided (checked in deleteOne() at line 444), but the extracted key turned out to be null or blank after URL parsing. A more appropriate error would be INVALID_IMAGE_KEY or a new error code like KEY_EXTRACTION_FAILED that clearly indicates the URL couldn't be parsed into a valid key.
| throw new ImageException(ImageExceptionCode.KEY_OR_URL_REQUIRED); | |
| throw new ImageException(ImageExceptionCode.INVALID_IMAGE_KEY, key); |
| "이미지: 잘못된 key 형식입니다. key=%s"), | ||
|
|
||
| KEY_REQUIRED(HttpStatus.BAD_REQUEST, | ||
| "이미지: key가 존재하지 않습니다."); | ||
|
|
There was a problem hiding this comment.
The error code KEY_REQUIRED appears to be unused in the codebase and its purpose is unclear given the existence of KEY_OR_URL_REQUIRED (line 49). If this error code is intended to represent a different scenario, consider renaming it to be more specific (e.g., KEY_EXTRACTION_FAILED), or remove it if it's not needed.
| "이미지: 잘못된 key 형식입니다. key=%s"), | |
| KEY_REQUIRED(HttpStatus.BAD_REQUEST, | |
| "이미지: key가 존재하지 않습니다."); | |
| "이미지: 잘못된 key 형식입니다. key=%s"); |
| throw new ImageException(ImageExceptionCode.KEY_OR_URL_REQUIRED); | ||
| } | ||
| if (key.contains("..") || key.startsWith("/")) { | ||
| throw new ImageException(ImageExceptionCode.DIR_INVALID_TRAVERSAL); |
There was a problem hiding this comment.
The error code DIR_INVALID_TRAVERSAL is semantically incorrect here. This error code was designed for directory path validation (preventing directory traversal attacks with ".." patterns), but it's being reused for validating S3 object keys. The error message "이미지: 잘못된 디렉토리 경로입니다." doesn't accurately describe the issue when a key starts with "/" or contains "..". Consider using the more appropriate INVALID_IMAGE_KEY error code or creating a new error code specific to key validation.
| throw new ImageException(ImageExceptionCode.DIR_INVALID_TRAVERSAL); | |
| throw new ImageException(ImageExceptionCode.INVALID_IMAGE_KEY, key); |
| if (path == null || path.isBlank()) return null; | ||
| return path.startsWith("/") ? path.substring(1) : path; | ||
| } catch (IllegalArgumentException e) { | ||
| return clean; // url이 아니면 key로 간주 |
There was a problem hiding this comment.
The fallback logic in the catch block (line 410) returns the cleaned URL as a key when URI parsing fails, which could lead to invalid S3 operations. If a malformed URL is provided, it will bypass validation and be used directly as a key. Instead of returning the clean string as-is, the method should return null to indicate extraction failure, allowing the validateKey method to properly handle and reject the invalid input.
| return clean; // url이 아니면 key로 간주 | |
| return null; // URI parsing failed, extraction failed |
| @@ -28,7 +28,9 @@ public enum GroupErrorCode implements ErrorCode { | |||
| "모임: 현재 참여 인원 수(%s)보다 작은 값으로 최대 인원을 줄일 수 없습니다."), | |||
| NO_PERMISSION_TO_DELETE_GROUP(HttpStatus.UNAUTHORIZED, "모임: 삭제할 수 있는 권한이 없습니다."), | |||
There was a problem hiding this comment.
The error code NO_PERMISSION_TO_DELETE_GROUP has an inconsistent message format. It doesn't include placeholders for groupId and userId like other similar error codes (e.g., NO_PERMISSION_TO_UPDATE_GROUP on line 25-26), but the code passes these parameters when throwing this exception (line 645 in GroupService). Add placeholders to the message template: "모임: 삭제할 수 있는 권한이 없습니다. 모임 ID: %s 회원 ID: %s"
| NO_PERMISSION_TO_DELETE_GROUP(HttpStatus.UNAUTHORIZED, "모임: 삭제할 수 있는 권한이 없습니다."), | |
| NO_PERMISSION_TO_DELETE_GROUP(HttpStatus.UNAUTHORIZED, "모임: 삭제할 수 있는 권한이 없습니다. 모임 ID: %s 회원 ID: %s"), |
| ### 3-1. 모임 이미지 단 건 삭제 - 440x240 (sortOrder=0) | ||
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_main}} | ||
| Authorization: Bearer {{hostAccessToken}} | ||
|
|
||
| ### 3-2. 모임 이미지 단 건 삭제 - 100x100 (sortOrder=0) | ||
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_thumb}} | ||
| Authorization: Bearer {{hostAccessToken}} | ||
|
|
||
| ### 3-3. 모임 상세 조회(삭제 후) - sortOrder=0 이미지 제거 확인 |
There was a problem hiding this comment.
The comment label is duplicated. There are two sections labeled "3-1" (lines 129 and 134). The second one should be labeled "3-2" to maintain sequential numbering.
| ### 3-1. 모임 이미지 단 건 삭제 - 440x240 (sortOrder=0) | |
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_main}} | |
| Authorization: Bearer {{hostAccessToken}} | |
| ### 3-2. 모임 이미지 단 건 삭제 - 100x100 (sortOrder=0) | |
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_thumb}} | |
| Authorization: Bearer {{hostAccessToken}} | |
| ### 3-3. 모임 상세 조회(삭제 후) - sortOrder=0 이미지 제거 확인 | |
| ### 3-2. 모임 이미지 단 건 삭제 - 440x240 (sortOrder=0) | |
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_main}} | |
| Authorization: Bearer {{hostAccessToken}} | |
| ### 3-3. 모임 이미지 단 건 삭제 - 100x100 (sortOrder=0) | |
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_thumb}} | |
| Authorization: Bearer {{hostAccessToken}} | |
| ### 3-4. 모임 상세 조회(삭제 후) - sortOrder=0 이미지 제거 확인 |
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_main}} | ||
| Authorization: Bearer {{hostAccessToken}} | ||
|
|
||
| ### 3-2. 모임 이미지 단 건 삭제 - 100x100 (sortOrder=0) |
There was a problem hiding this comment.
The comment label is incorrect. This should be labeled "3-3" instead of "3-2" to follow the sequential numbering from the previous test cases.
| ### 3-2. 모임 이미지 단 건 삭제 - 100x100 (sortOrder=0) | |
| ### 3-3. 모임 이미지 단 건 삭제 - 100x100 (sortOrder=0) |
| DELETE http://localhost:8080/api/v1/groups/{{groupId_png_jpg}}/images/one?url={{img0_thumb}} | ||
| Authorization: Bearer {{hostAccessToken}} | ||
|
|
||
| ### 3-3. 모임 상세 조회(삭제 후) - sortOrder=0 이미지 제거 확인 |
There was a problem hiding this comment.
The comment label is incorrect. This should be labeled "3-4" instead of "3-3" to follow the sequential numbering from the previous test cases.
| ### 3-3. 모임 상세 조회(삭제 후) - sortOrder=0 이미지 제거 확인 | |
| ### 3-4. 모임 상세 조회(삭제 후) - sortOrder=0 이미지 제거 확인 |
삭제 순서로 인한 데이터 불일치 위험이 있습니다. Lines 657-658에서 S3 삭제를 먼저 수행하고 DB 삭제를 나중에 수행합니다. 만약 DB 삭제(line 658)가 실패하면: S3 객체는 이미 삭제됨 (롤백 불가) DB 레코드는 여전히 존재 결과: 존재하지 않는 이미지를 가리키는 DB 레코드 발생 (dangling reference) 또한 imageUploadService.deleteOne() 내부에서 예외가 발생하면 트랜잭션이 롤백되지만, 이미 S3에서 삭제된 객체는 복구할 수 없습니다. 다음과 같이 DB 삭제를 먼저 수행합니다. 쿼리 파라미터의 required 속성이 실제 동작과 일치하지 않습니다. Line 131에서 url 파라미터를 required=false로 선언했지만, GroupService.deleteOne()은 url이 null이거나 blank이면 IMAGE_URL_REQUIRED 예외를 던집니다. 이는 API 계약과 실제 구현 사이의 불일치를 만듭니다. 다음 중 하나를 선택하세요: 옵션 1: required=true로 변경 (권장)
github-project-automation
bot
moved this from In progress
to Done
in WeGo-Together Backend
1 participant
📝 Pull Request
📌 PR 종류
해당하는 항목에 체크해주세요.
✨ 변경 내용
모임에서 이미지 단 건 삭제 기능을 구현합니다. key가 아니라 url 전체를 모두 입력받아서 삭제합니다.
🔍 관련 이슈
🧪 테스트
변경된 기능에 대한 테스트 범위 또는 테스트 결과를 작성해주세요.
🚨 확인해야 할 사항 (Checklist)
PR을 제출하기 전에 아래 항목들을 확인해주세요.
🙋 기타 참고 사항
리뷰어가 참고하면 좋을 만한 추가 설명이 있다면 적어주세요.
Summary by CodeRabbit
새로운 기능
개선 사항
테스트
✏️ Tip: You can customize this high-level summary in your review settings.