Conversation
모임 이미지 업데이트 및 삭제 기능을 추가합니다. S3 스토리지 관리 및 권한 확인 기능이 포함됩니다. 이미지 업데이트 요청 및 응답을 위한 DTO를 도입합니다. 이미지 관련 작업을 처리하도록 모임 서비스를 업데이트합니다.
|
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모임과 모임 이미지의 수정·삭제 기능을 추가합니다. 요청/응답 DTO, 서비스 메서드(update/delete), 컨트롤러 엔드포인트, 도메인 업데이트 메서드, S3 이미지 삭제 유틸 및 관련 HTTP 테스트 시나리오가 포함됩니다. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller as GroupController / GroupImageController
participant Service as GroupService / GroupImageService
participant Repo as GroupRepository
participant ImgUpload as ImageUploadService
participant S3 as AWS S3
User->>Controller: PATCH /api/v1/groups/{groupId} (UpdateGroupRequest)
Controller->>Service: updateGroup(userId, groupId, request)
Service->>Repo: findByIdAndDeletedAtIsNull(groupId)
Repo-->>Service: Group
Service->>Service: 권한/요청 검증, 태그 처리
Service->>Repo: save(updated Group)
Service-->>Controller: GetGroupResponse
Controller-->>User: 200 OK
User->>Controller: DELETE /api/v1/groups/{groupId}
Controller->>Service: deleteGroup(userId, groupId)
Service->>Repo: findByIdAndDeletedAtIsNull(groupId)
Repo-->>Service: Group
Service->>ImgUpload: deleteAllByUrls(imageUrls)
ImgUpload->>S3: Delete objects (batch)
S3-->>ImgUpload: ACK
Service->>Repo: delete(group)
Service-->>Controller: void
Controller-->>User: 200/204
User->>Controller: PATCH /api/v1/groups/images/{groupId} (List<RequestItem>)
Controller->>Service: updateGroupImages(userId, groupId, requests)
Service->>Repo: findByIdAndDeletedAtIsNull(groupId)
Repo-->>Service: Group
Service->>Service: 권한 검증, 요청 제한(최대 3)
Service->>ImgUpload: deleteAllByUrls(oldImageUrls)
ImgUpload->>S3: Delete objects
S3-->>ImgUpload: ACK
Service->>Repo: saveAll(new GroupImage entities)
Service-->>Controller: List<GroupImageItemResponse>
Controller-->>User: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
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: 9
🧹 Nitpick comments (7)
src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java (2)
56-57: 주석 처리된 코드 제거 필요.디버깅 또는 이전 구현 흔적으로 보이는 주석 처리된 코드가 남아 있습니다. 프로덕션 코드에서는 제거하는 것이 좋습니다.
-// String key = buildKey(dir, originalFilename, index); String key = buildKey(originalFilename, index);
410-416: 삭제 실패 시 로깅 부재.
deleteByUrl이 key가 null/blank일 때 조용히 반환합니다. 디버깅을 위해 경고 로그를 추가하는 것이 좋습니다.public void deleteByUrl(String imageUrl) { String key = extractKeyFromUrl(imageUrl); if (key == null || key.isBlank()) { + // 옵션: log.warn("이미지 URL에서 키 추출 실패: {}", imageUrl); return; } delete(key); }src/main/java/team/wego/wegobackend/group/application/service/GroupImageService.java (1)
217-225: URL 문자열 기반 이미지 타입 판별은 취약합니다.
img.getImageUrl().contains("440x240")방식은 URL 구조 변경 시 깨질 수 있습니다.GroupImage엔티티에 이미지 타입(MAIN/THUMB) 필드를 추가하는 것이 더 안정적입니다.src/test/http/group/update.http (1)
215-240: 테스트 파일이 불완전합니다.테스트 2-5 (4장 업로드 - 실패 예상)가 응답 핸들러 스크립트 없이 끝났습니다. 실패 케이스의 예상 응답을 검증하는 스크립트를 추가하거나, 최소한 테스트 파일을 올바르게 종료해 주세요.
예시:
> {% // 4장 업로드 시 실패 예상 검증 client.test("Should fail with IMAGE_UPLOAD_EXCEED", function() { client.assert(response.status === 400, "Expected 400 Bad Request"); }); %}src/main/java/team/wego/wegobackend/group/presentation/GroupImageController.java (1)
57-67: DELETE 엔드포인트에 204 No Content 응답 고려.삭제 작업 후 반환할 콘텐츠가 없으므로
HttpStatus.NO_CONTENT(204)가 RESTful 관례에 더 부합합니다. 다만 기존 API 응답 패턴과 일관성을 유지하려면 현재 구현도 허용됩니다.@DeleteMapping("/{groupId}") public ResponseEntity<ApiResponse<Void>> deleteGroupImages( @RequestParam Long userId, // TODO: 나중에 인증 정보에서 꺼내기 @PathVariable Long groupId ) { groupImageService.deleteGroupImages(userId, groupId); return ResponseEntity - .status(HttpStatus.OK) - .body(ApiResponse.success(HttpStatus.OK.value(), null)); + .status(HttpStatus.NO_CONTENT) + .body(ApiResponse.success(HttpStatus.NO_CONTENT.value(), null)); }src/main/java/team/wego/wegobackend/group/domain/exception/GroupErrorCode.java (1)
24-26: 열거형 상수 끝의,;구문 정리.Line 26의
,;는 Java에서 유효한 구문이지만 (trailing comma 허용), 코드 가독성을 위해 쉼표를 제거하는 것이 좋습니다.HOST_CANNOT_LEAVE_OWN_GROUP(HttpStatus.BAD_REQUEST, "모임: HOST는 나갈 수 없습니다. 모임 ID: %s 회원 ID: %s"), NO_PERMISSION_TO_UPDATE_GROUP(HttpStatus.FORBIDDEN, "모임: 해당 모임을 수정할 권한이 없습니다. 모임 ID: %s 회원 ID: %s"), - INVALID_MAX_PARTICIPANTS_LESS_THAN_CURRENT(HttpStatus.BAD_REQUEST, "모임: 현재 참여 인원 수(%s)보다 작은 값으로 최대 인원을 줄일 수 없습니다."),; + INVALID_MAX_PARTICIPANTS_LESS_THAN_CURRENT(HttpStatus.BAD_REQUEST, "모임: 현재 참여 인원 수(%s)보다 작은 값으로 최대 인원을 줄일 수 없습니다.");src/main/java/team/wego/wegobackend/group/application/service/GroupService.java (1)
471-477: 태그 업데이트 로직이 적절하지만, 지연 로딩 고려가 필요할 수 있습니다.
group.getGroupTags().clear()를 호출하기 전에 컬렉션이 로드되지 않았다면LazyInitializationException이 발생할 수 있습니다. 현재는@Transactional메서드 내에서 호출되므로 문제없지만, 향후 안전성을 위해 명시적으로 컬렉션을 로드하는 것을 고려해보세요.예시:
private void updateGroupTags(Group group, List<String> tagNames) { if (tagNames == null) { return; } + // Ensure collection is initialized + group.getGroupTags().size(); group.getGroupTags().clear(); saveGroupTags(group, tagNames); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/team/wego/wegobackend/group/application/dto/request/UpdateGroupImageItemRequest.java(1 hunks)src/main/java/team/wego/wegobackend/group/application/dto/request/UpdateGroupRequest.java(1 hunks)src/main/java/team/wego/wegobackend/group/application/dto/response/GroupImageItemResponse.java(1 hunks)src/main/java/team/wego/wegobackend/group/application/service/GroupImageService.java(5 hunks)src/main/java/team/wego/wegobackend/group/application/service/GroupService.java(6 hunks)src/main/java/team/wego/wegobackend/group/domain/entity/Group.java(1 hunks)src/main/java/team/wego/wegobackend/group/domain/exception/GroupErrorCode.java(1 hunks)src/main/java/team/wego/wegobackend/group/presentation/GroupController.java(3 hunks)src/main/java/team/wego/wegobackend/group/presentation/GroupImageController.java(2 hunks)src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java(2 hunks)src/test/http/group/delete.http(1 hunks)src/test/http/group/update.http(1 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 (15)
src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java (1)
418-433: 배치 삭제 로직 적절함.null-safe 스트림 처리와 빈 리스트 체크가 잘 구현되어 있습니다.
src/main/java/team/wego/wegobackend/group/application/dto/request/UpdateGroupImageItemRequest.java (1)
3-8: DTO 구조 적절함. 선택적으로 Bean Validation 추가 고려.서비스 레이어에서 null 처리를 하고 있지만, 컨트롤러 레벨에서 조기 검증을 원한다면
@NotNull,@URL등의 어노테이션 추가를 고려할 수 있습니다.src/test/http/group/delete.http (1)
1-174: 테스트 시나리오 커버리지 양호.전체 CRUD 플로우(생성, 수정, 삭제)와 삭제 후 404 확인까지 포함하여 종합적인 테스트입니다.
src/main/java/team/wego/wegobackend/group/application/service/GroupImageService.java (1)
220-220: The project is configured for Java 21 (as specified inbuild.gradle:languageVersion = JavaLanguageVersion.of(21)). Thelist.getFirst()method is fully compatible and appropriate for this project. No change needed.Likely an incorrect or invalid review comment.
src/main/java/team/wego/wegobackend/group/domain/entity/Group.java (1)
106-122: Consider nullable field handling in theupdate()method.The
update()method unconditionally assigns all parameters, which meanslocationDetailandendTimecan be overwritten withnullvalues since they lack non-null validation inUpdateGroupRequest. WhileendTimenullability appears intentional (based on the conditional time range validation that checks "if both startTime AND endTime are non-null"),locationDetailhas no such guard. If partial updates are needed in the future, consider either: (1) adding non-null validation to required fields, or (2) adopting an optional/nullable parameter pattern similar to theupdateGroupTags()method where null means "don't update this field."src/main/java/team/wego/wegobackend/group/application/dto/request/UpdateGroupRequest.java (2)
22-27:endTime의 cross-field 검증 확인 필요.
endTime은@NotNull이 없어 nullable이고,@Future는 null 값을 유효하게 처리합니다.startTime < endTime검증은 서비스 레이어에서INVALID_TIME_RANGE에러코드로 처리되는 것으로 보입니다.
endTime이 null일 때의 비즈니스 로직이 의도된 것인지 확인해 주세요. 종료 시간이 필수라면@NotNull을 추가하는 것이 좋습니다.
13-38: LGTM - 검증 어노테이션이 적절히 적용되었습니다.필수 필드에 대한
@NotBlank,@NotNull검증과maxParticipants의 범위 검증(2-12)이 잘 구성되어 있습니다. 한국어 에러 메시지도 일관성 있게 작성되었습니다.src/main/java/team/wego/wegobackend/group/application/dto/response/GroupImageItemResponse.java (2)
13-34: LGTM - null 처리가 적절합니다.
main과thumb의 null 케이스를 안전하게 처리하고 있습니다.sortOrder의 fallback 로직(main → thumb → 0)도 명확합니다.
36-38: 유용한 헬퍼 메서드.
fromMainOnly는 thumb 이미지가 없는 경우에 대한 간결한 팩토리 메서드입니다.src/main/java/team/wego/wegobackend/group/application/service/GroupService.java (4)
37-37: 이미지 삭제를 위한 의존성 추가가 적절합니다.모임 삭제 시 S3 이미지 정리를 위해
ImageUploadService의존성이 추가되었습니다. 의존성 주입이 올바르게 구성되어 있습니다.Also applies to: 55-55
479-499: 검증 로직이 적절합니다.모임 수정 요청 검증이 잘 구현되어 있습니다:
- 시간 범위 검증
- 최소 인원 검증 (2명 이상)
- 현재 참여 인원보다 적게 줄일 수 없는 검증
모든 엣지 케이스가 적절히 처리되고 있습니다.
501-541: 모임 수정 로직이 잘 구현되어 있습니다.모임 수정 플로우가 체계적으로 구현되었습니다:
- 모임 조회 및 권한 확인 (HOST만 수정 가능)
- 현재 참여 인원 계산
- 요청 검증 (시간, 인원 등)
- 엔티티 필드 업데이트 (이미지 제외)
- 태그 업데이트
- 업데이트된 응답 반환
권한 체크와 검증 로직이 적절하며, 트랜잭션 경계도 올바르게 설정되어 있습니다.
221-221: 이미지 응답 생성 로직이 새로운 DTO 구조에 맞게 업데이트되었습니다.
GroupImageItemResponse.from(main, thumb)시그니처를 사용하여 메인/썸네일 이미지를 그룹화하는 패턴으로 변경되었습니다. 이는 PR에서 변경된 DTO 구조와 일치합니다.Also applies to: 389-389
src/main/java/team/wego/wegobackend/group/presentation/GroupController.java (2)
8-8: 필요한 import 추가가 적절합니다.REST 엔드포인트 구현을 위한
DeleteMapping,PatchMapping어노테이션과UpdateGroupRequestDTO import가 추가되었습니다.Also applies to: 10-10, 19-19
100-111: 모임 수정 엔드포인트가 적절히 구현되었습니다.PATCH 메서드를 사용한 부분 업데이트 엔드포인트가 올바르게 구현되었습니다:
@Valid어노테이션으로 요청 검증 수행- 서비스 레이어로 적절히 위임
- HTTP 200 OK 상태 코드 반환
| // 4. 기존 이미지 URL 수집 -> S3 삭제 | ||
| List<String> oldUrls = group.getImages().stream() | ||
| .map(GroupImage::getImageUrl) | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
|
|
||
| imageUploadService.deleteAllByUrls(oldUrls); | ||
|
|
||
| // 5. 기존 GroupImage 엔티티들을 관계에서 제거: DB에서도 삭제됨 | ||
| group.getImages().clear(); | ||
| groupImageRepository.flush(); // DB 즉시 삭제하기 위해 추가 |
There was a problem hiding this comment.
S3 삭제와 DB 삭제 순서로 인한 데이터 불일치 위험.
S3 이미지 삭제(Line 176)가 DB 삭제(Line 179-180)보다 먼저 발생합니다. 이후 DB 작업에서 예외가 발생하면 트랜잭션이 롤백되지만, S3에서 삭제된 이미지는 복구되지 않아 데이터 불일치가 발생할 수 있습니다.
순서를 변경하거나, S3 삭제를 트랜잭션 커밋 후 수행하는 방식을 고려하세요:
- // 4. 기존 이미지 URL 수집 -> S3 삭제
List<String> oldUrls = group.getImages().stream()
.map(GroupImage::getImageUrl)
.filter(Objects::nonNull)
.toList();
- imageUploadService.deleteAllByUrls(oldUrls);
-
// 5. 기존 GroupImage 엔티티들을 관계에서 제거: DB에서도 삭제됨
group.getImages().clear();
groupImageRepository.flush();
+
+ // S3 삭제는 DB 변경 확정 후 수행 (또는 @TransactionalEventListener 사용)
+ imageUploadService.deleteAllByUrls(oldUrls);🤖 Prompt for AI Agents
In
src/main/java/team/wego/wegobackend/group/application/service/GroupImageService.java
around lines 170-180, S3 deletion is performed before removing GroupImage
entities from the DB which can cause permanent S3 data loss if the DB
transaction later fails; to fix this, collect the existing image URLs as you
already do, then perform the DB removal and flush inside the transaction first,
and schedule the S3 deletion to run only after the transaction successfully
commits (e.g., register a TransactionSynchronization post-commit callback or
invoke the deletion from code executed after transaction commit); ensure you
still pass the previously collected URL list to the post-commit task so the S3
delete happens only on successful DB commit.
| // 3. 기존 URL들 수집 후 S3 삭제 | ||
| List<String> oldUrls = group.getImages().stream() | ||
| .map(GroupImage::getImageUrl) | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
|
|
||
| imageUploadService.deleteAllByUrls(oldUrls); | ||
|
|
||
| group.getImages().clear(); | ||
| groupImageRepository.flush(); // DB 즉시 삭제하기 위해 추가 |
There was a problem hiding this comment.
deleteGroupImages도 동일한 S3/DB 순서 문제가 있습니다.
updateGroupImages와 마찬가지로 S3 삭제가 DB 삭제보다 먼저 발생하여 트랜잭션 롤백 시 데이터 불일치가 발생할 수 있습니다.
src/main/java/team/wego/wegobackend/group/application/service/GroupService.java
Show resolved
Hide resolved
|
|
||
| imageUploadService.deleteAllByUrls(imageUrls); | ||
|
|
||
| groupRepository.delete(group); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "GroupService.java" | head -5Repository: WeGo-Together/WeGo_BackEnd
Length of output: 151
🏁 Script executed:
fd "GroupService.java" --type fRepository: WeGo-Together/WeGo_BackEnd
Length of output: 149
🏁 Script executed:
# Get the line count first
wc -l "src/main/java/team/wego/wegobackend/group/application/service/GroupService.java"Repository: WeGo-Together/WeGo_BackEnd
Length of output: 153
🏁 Script executed:
sed -n '550,580p' "src/main/java/team/wego/wegobackend/group/application/service/GroupService.java"Repository: WeGo-Together/WeGo_BackEnd
Length of output: 701
🏁 Script executed:
fd "Group.java" --type f | grep -E "entity|model|domain"Repository: WeGo-Together/WeGo_BackEnd
Length of output: 136
🏁 Script executed:
# Search for soft delete patterns
rg "deletedAt|DeletedAt|findByIdAndDeletedAtIsNull" --type java -nRepository: WeGo-Together/WeGo_BackEnd
Length of output: 1591
🏁 Script executed:
# Find Group entity and examine its relationships
rg -A5 "class Group\s" --type java | head -100Repository: WeGo-Together/WeGo_BackEnd
Length of output: 665
🏁 Script executed:
# Find all cascade configurations
rg "@OneToMany|@ManyToOne|@ManyToMany|@JoinColumn|orphanRemoval|cascade" --type java -B2 -A2 | grep -A10 -B10 "Group"Repository: WeGo-Together/WeGo_BackEnd
Length of output: 4335
Replace hard delete with soft delete to maintain consistency with established soft delete pattern.
The codebase uses a soft delete pattern throughout: the Group entity has a deletedAt field, and all queries filter using findByIdAndDeletedAtIsNull. However, line 568 performs a physical delete with groupRepository.delete(group), which is inconsistent and causes problems since cascade delete will physically remove all related GroupImage, GroupTag, and GroupUser records.
Use soft delete instead:
- groupRepository.delete(group);
+ group.setDeletedAt(LocalDateTime.now());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| groupRepository.delete(group); | |
| group.setDeletedAt(LocalDateTime.now()); |
🤖 Prompt for AI Agents
In
src/main/java/team/wego/wegobackend/group/application/service/GroupService.java
around line 568, replace the hard/physical delete call with a soft-delete: set
the Group entity's deletedAt timestamp to the current time (use Instant.now() or
LocalDateTime.now() to match the field type) and persist the change via
groupRepository.save(group) instead of groupRepository.delete(group); do not
remove related records—rely on the existing deletedAt-based query filters.
There was a problem hiding this comment.
의도한대로 완전 삭제 구현이므로 해당 리뷰는 수정 없이 넘어갑니다.
There was a problem hiding this comment.
@LimdaeIl, 알겠습니다! 완전 삭제로 진행하시는 것으로 이해했습니다.
참고로, 완전 삭제 시 관련 테이블들(GroupImage, GroupTag, GroupUser 등)이 cascade 설정에 따라 함께 삭제되므로, 필요한 경우 삭제 전에 관련 데이터를 별도로 백업하거나 로깅하는 것을 고려해보실 수 있습니다.
src/main/java/team/wego/wegobackend/group/presentation/GroupController.java
Show resolved
Hide resolved
src/main/java/team/wego/wegobackend/group/presentation/GroupImageController.java
Show resolved
Hide resolved
| public String extractKeyFromUrl(String imageUrl) { | ||
| if (imageUrl == null || imageUrl.isBlank()) { | ||
| return null; | ||
| } | ||
|
|
||
| String endpoint = awsS3Properties.getPublicEndpoint(); | ||
| if (endpoint != null && !endpoint.isBlank()) { | ||
| String prefix = endpoint.endsWith("/") ? endpoint : endpoint + "/"; | ||
| if (imageUrl.startsWith(prefix)) { | ||
| return imageUrl.substring(prefix.length()); | ||
| } | ||
| } | ||
|
|
||
| // fallback: 마지막 "/" 이후를 key로 사용 | ||
| int idx = imageUrl.lastIndexOf('/'); | ||
| if (idx >= 0 && idx + 1 < imageUrl.length()) { | ||
| return imageUrl.substring(idx + 1); | ||
| } | ||
|
|
||
| // 그래도 안 되면 전체를 key로 시도 (최악의 경우) | ||
| return imageUrl; | ||
| } |
There was a problem hiding this comment.
Fallback 로직이 다중 경로 세그먼트 URL에서 잘못된 키를 추출할 수 있습니다.
lastIndexOf('/') fallback은 https://cdn.example.com/bucket/path/to/image.webp 같은 URL에서 전체 경로(bucket/path/to/image.webp) 대신 파일명(image.webp)만 반환합니다. S3 키가 경로를 포함하는 경우 삭제가 실패할 수 있습니다.
// fallback: 마지막 "/" 이후를 key로 사용
- int idx = imageUrl.lastIndexOf('/');
- if (idx >= 0 && idx + 1 < imageUrl.length()) {
- return imageUrl.substring(idx + 1);
+ // endpoint가 매칭되지 않으면 전체 URL을 key로 시도
+ // (실제 S3 키 구조에 맞게 조정 필요)
+ try {
+ java.net.URI uri = java.net.URI.create(imageUrl);
+ String path = uri.getPath();
+ if (path != null && path.startsWith("/")) {
+ return path.substring(1);
+ }
+ } catch (IllegalArgumentException e) {
+ // URI 파싱 실패 시 전체 URL 반환
}
// 그래도 안 되면 전체를 key로 시도 (최악의 경우)
return imageUrl;🤖 Prompt for AI Agents
In
src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java
around lines 387 to 408, the fallback using lastIndexOf('/') returns only the
filename for multi-segment paths and breaks S3 keys that include directories;
instead, parse the imageUrl as a URI/URL: if it contains a scheme/host, extract
the full path component (strip any leading '/' and remove query/fragment) and
return that as the key; if it is not a full URL (no scheme/host), return the
input unchanged (or the entire remaining string) so multi-segment keys are
preserved. Ensure existing null/blank checks remain and handle
URISyntaxException/ MalformedURLException by falling back to returning the
original string.
There was a problem hiding this comment.
Pull request overview
This PR adds update and delete functionality for groups (모임) and their associated images. The implementation includes permission checks (HOST-only operations), S3 storage management for image cleanup, and new DTOs for update operations.
- Adds
PATCH /api/v1/groups/{groupId}andDELETE /api/v1/groups/{groupId}endpoints for group modification - Adds
PATCH /api/v1/groups/images/{groupId}andDELETE /api/v1/groups/images/{groupId}endpoints for image management - Implements permission validation ensuring only the HOST can modify or delete groups
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/http/group/update.http | HTTP test scenarios for group image update operations with various image formats and counts |
| src/test/http/group/delete.http | HTTP test scenarios for group image and group deletion operations |
| src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java | Adds URL-to-key extraction and bulk deletion methods for S3 image cleanup |
| src/main/java/team/wego/wegobackend/group/presentation/GroupImageController.java | Adds PATCH and DELETE endpoints for group image management |
| src/main/java/team/wego/wegobackend/group/presentation/GroupController.java | Adds PATCH and DELETE endpoints for group management |
| src/main/java/team/wego/wegobackend/group/domain/exception/GroupErrorCode.java | Adds new error codes for permission denial and invalid participant count reduction |
| src/main/java/team/wego/wegobackend/group/domain/entity/Group.java | Adds update method for modifying group properties |
| src/main/java/team/wego/wegobackend/group/application/service/GroupService.java | Implements updateGroup and deleteGroup methods with validation and S3 cleanup |
| src/main/java/team/wego/wegobackend/group/application/service/GroupImageService.java | Implements updateGroupImages and deleteGroupImages methods with permission checks and S3 management |
| src/main/java/team/wego/wegobackend/group/application/dto/response/GroupImageItemResponse.java | Refactors to include separate IDs for main and thumbnail images |
| src/main/java/team/wego/wegobackend/group/application/dto/request/UpdateGroupRequest.java | New DTO for group update requests with validation constraints |
| src/main/java/team/wego/wegobackend/group/application/dto/request/UpdateGroupImageItemRequest.java | New DTO for image update requests |
Comments suppressed due to low confidence (2)
src/main/java/team/wego/wegobackend/group/domain/entity/Group.java:66
- getGroupTags exposes the internal representation stored in field groupTags. The value may be modified after this call to getGroupTags.
private List<GroupTag> groupTags = new ArrayList<>();
src/main/java/team/wego/wegobackend/group/domain/entity/Group.java:63
- getImages exposes the internal representation stored in field images. The value may be modified after this call to getImages.
getImages exposes the internal representation stored in field images. The value may be modified after this call to getImages.
private List<GroupImage> images = new ArrayList<>();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // fallback: 마지막 "/" 이후를 key로 사용 | ||
| int idx = imageUrl.lastIndexOf('/'); | ||
| if (idx >= 0 && idx + 1 < imageUrl.length()) { | ||
| return imageUrl.substring(idx + 1); | ||
| } | ||
|
|
||
| // 그래도 안 되면 전체를 key로 시도 (최악의 경우) | ||
| return imageUrl; |
There was a problem hiding this comment.
The fallback logic may not work as intended. When extractKeyFromUrl fails to extract a proper key using the endpoint prefix, it falls back to extracting everything after the last /. However, this doesn't account for URLs with query parameters or other path segments. For example, if imageUrl is https://domain.com/bucket/folder/image.jpg?v=123, this will return image.jpg?v=123 which is not a valid S3 key. Consider parsing the URL more carefully or validating the extracted key before returning it.
| // fallback: 마지막 "/" 이후를 key로 사용 | |
| int idx = imageUrl.lastIndexOf('/'); | |
| if (idx >= 0 && idx + 1 < imageUrl.length()) { | |
| return imageUrl.substring(idx + 1); | |
| } | |
| // 그래도 안 되면 전체를 key로 시도 (최악의 경우) | |
| return imageUrl; | |
| // fallback: 마지막 "/" 이후를 key로 사용, 그리고 query/fragment 제거 | |
| int idx = imageUrl.lastIndexOf('/'); | |
| if (idx >= 0 && idx + 1 < imageUrl.length()) { | |
| String key = imageUrl.substring(idx + 1); | |
| int qIdx = key.indexOf('?'); | |
| if (qIdx >= 0) { | |
| key = key.substring(0, qIdx); | |
| } | |
| int fIdx = key.indexOf('#'); | |
| if (fIdx >= 0) { | |
| key = key.substring(0, fIdx); | |
| } | |
| return key; | |
| } | |
| // 그래도 안 되면 전체를 key로 시도 (최악의 경우) | |
| String key = imageUrl; | |
| int qIdx = key.indexOf('?'); | |
| if (qIdx >= 0) { | |
| key = key.substring(0, qIdx); | |
| } | |
| int fIdx = key.indexOf('#'); | |
| if (fIdx >= 0) { | |
| key = key.substring(0, fIdx); | |
| } | |
| return key; |
| .filter(img -> img.getImageUrl().contains("440x240")) | ||
| .findFirst() | ||
| .orElse(list.getFirst()); | ||
|
|
||
| GroupImage thumb = list.stream() | ||
| .filter(img -> img.getImageUrl().contains("100x100")) |
There was a problem hiding this comment.
The logic assumes that image URLs will contain the strings "440x240" or "100x100" to differentiate between main and thumbnail images. This is fragile and relies on a specific URL naming convention. If the URL format changes or doesn't contain these strings, the logic will incorrectly identify images. Consider storing the image type/size as metadata in the GroupImage entity or using a more robust identification mechanism.
| .filter(img -> img.getImageUrl().contains("440x240")) | |
| .findFirst() | |
| .orElse(list.getFirst()); | |
| GroupImage thumb = list.stream() | |
| .filter(img -> img.getImageUrl().contains("100x100")) | |
| .filter(img -> "MAIN".equals(img.getImageType())) | |
| .findFirst() | |
| .orElse(list.getFirst()); | |
| GroupImage thumb = list.stream() | |
| .filter(img -> "THUMBNAIL".equals(img.getImageType())) |
| public GetGroupResponse updateGroup(Long userId, Long groupId, UpdateGroupRequest request) { | ||
| // 1. 모임 조회 | ||
| Group group = groupRepository.findByIdAndDeletedAtIsNull(groupId) | ||
| .orElseThrow( | ||
| () -> new GroupException(GroupErrorCode.GROUP_NOT_FOUND_BY_ID, groupId)); | ||
|
|
||
| // 2. 권한 체크: HOST만 수정 가능 | ||
| if (!group.getHost().getId().equals(userId)) { | ||
| throw new GroupException( | ||
| GroupErrorCode.NO_PERMISSION_TO_UPDATE_GROUP, | ||
| groupId, | ||
| userId | ||
| ); | ||
| } | ||
|
|
||
| // 3. 현재 ATTEND 인원 수 (HOST 포함) | ||
| int currentAttendCount = (int) group.getUsers().stream() | ||
| .filter(gu -> gu.getStatus() == ATTEND) | ||
| .count(); | ||
|
|
||
| // 4. 요청 값 검증 (시간 / 최대 인원 / 현재 인원 대비) | ||
| validateUpdateGroupRequest(request, currentAttendCount); | ||
|
|
||
| // 5. 엔티티 필드 수정 (이미지는 건드리지 않음) | ||
| group.update( | ||
| request.title(), | ||
| request.location(), | ||
| request.locationDetail(), | ||
| request.startTime(), | ||
| request.endTime(), | ||
| request.description(), | ||
| request.maxParticipants() | ||
| ); | ||
|
|
||
| // 6. 태그 수정 | ||
| updateGroupTags(group, request.tags()); | ||
|
|
||
| // 7. 수정 결과 응답 (현재 유저 기준) | ||
| return buildGetGroupResponse(group, userId); | ||
| } |
There was a problem hiding this comment.
The new updateGroup method lacks unit test coverage. Given that the repository has existing tests for GroupService, consider adding tests to cover scenarios such as:
- Successful update by HOST
- Permission denied when non-HOST tries to update
- Validation failures (invalid time range, max participants less than current count)
- Tag updates
| public void deleteGroup(Long userId, Long groupId) { | ||
| // 1. 모임 조회 (soft delete 고려 쿼리 사용 중이지만, 여기서는 실제 삭제) | ||
| Group group = groupRepository.findByIdAndDeletedAtIsNull(groupId) | ||
| .orElseThrow( | ||
| () -> new GroupException(GroupErrorCode.GROUP_NOT_FOUND_BY_ID, groupId) | ||
| ); | ||
|
|
||
| // 2. HOST 권한 체크 | ||
| if (!group.getHost().getId().equals(userId)) { | ||
| throw new GroupException( | ||
| GroupErrorCode.NO_PERMISSION_TO_UPDATE_GROUP, // 삭제도 동일 코드 재사용 | ||
| groupId, | ||
| userId | ||
| ); | ||
| } | ||
|
|
||
| // 3. S3 이미지 삭제 (이미지 URL -> key 변환은 ImageUploadService 가 담당) | ||
| List<String> imageUrls = group.getImages().stream() | ||
| .map(GroupImage::getImageUrl) | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
|
|
||
| imageUploadService.deleteAllByUrls(imageUrls); | ||
|
|
||
| groupRepository.delete(group); | ||
| } |
There was a problem hiding this comment.
The new deleteGroup method lacks unit test coverage. Given that the repository has existing tests for GroupService, consider adding tests to cover scenarios such as:
- Successful deletion by HOST
- Permission denied when non-HOST tries to delete
- S3 cleanup verification
- Cascading deletion of related entities
| public List<GroupImageItemResponse> updateGroupImages( | ||
| Long userId, | ||
| Long groupId, | ||
| List<UpdateGroupImageItemRequest> requests | ||
| ) { | ||
| // 1. 그룹 조회 (soft delete 고려) | ||
| Group group = groupRepository.findByIdAndDeletedAtIsNull(groupId) | ||
| .orElseThrow( | ||
| () -> new GroupException(GroupErrorCode.GROUP_NOT_FOUND_BY_ID, groupId) | ||
| ); | ||
|
|
||
| // 2. HOST 권한 체크 | ||
| if (!group.getHost().getId().equals(userId)) { | ||
| throw new GroupException( | ||
| GroupErrorCode.NO_PERMISSION_TO_UPDATE_GROUP, | ||
| groupId, | ||
| userId | ||
| ); | ||
| } | ||
|
|
||
| // 3. null 방어 및 최대 개수 제한: 논리 이미지 세트 기준 | ||
| List<UpdateGroupImageItemRequest> safeList = | ||
| (requests == null) ? List.of() : requests.stream() | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
|
|
||
| if (safeList.size() > 3) { | ||
| throw new GroupException(GroupErrorCode.IMAGE_UPLOAD_EXCEED, safeList.size()); | ||
| } | ||
|
|
||
| // 4. 기존 이미지 URL 수집 -> S3 삭제 | ||
| List<String> oldUrls = group.getImages().stream() | ||
| .map(GroupImage::getImageUrl) | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
|
|
||
| imageUploadService.deleteAllByUrls(oldUrls); | ||
|
|
||
| // 5. 기존 GroupImage 엔티티들을 관계에서 제거: DB에서도 삭제됨 | ||
| group.getImages().clear(); | ||
| groupImageRepository.flush(); // DB 즉시 삭제하기 위해 추가 | ||
|
|
||
| // 6. 새 GroupImage 엔티티 생성 | ||
| List<GroupImage> newEntities = new ArrayList<>(); | ||
|
|
||
| for (int i = 0; i < safeList.size(); i++) { | ||
| UpdateGroupImageItemRequest item = safeList.get(i); | ||
|
|
||
| int sortOrder = (item.sortOrder() != null) ? item.sortOrder() : i; | ||
|
|
||
| String mainUrl = item.imageUrl440x240(); | ||
| String thumbUrl = item.imageUrl100x100(); | ||
|
|
||
| if (mainUrl != null && !mainUrl.isBlank()) { | ||
| GroupImage main = GroupImage.create(group, mainUrl, sortOrder); | ||
| newEntities.add(main); | ||
| } | ||
|
|
||
| if (thumbUrl != null && !thumbUrl.isBlank()) { | ||
| GroupImage thumb = GroupImage.create(group, thumbUrl, sortOrder); | ||
| newEntities.add(thumb); | ||
| } | ||
| } | ||
|
|
||
| if (!newEntities.isEmpty()) { | ||
| groupImageRepository.saveAll(newEntities); | ||
| } | ||
|
|
||
| // 7. sortOrder 기준으로 한 세트 단위 응답 DTO 구성 | ||
| Map<Integer, List<GroupImage>> byOrder = newEntities.stream() | ||
| .collect(Collectors.groupingBy(GroupImage::getSortOrder)); | ||
|
|
||
| return byOrder.entrySet().stream() | ||
| .sorted(Map.Entry.comparingByKey()) | ||
| .map(entry -> { | ||
| List<GroupImage> list = entry.getValue(); | ||
|
|
||
| GroupImage main = list.stream() | ||
| .filter(img -> img.getImageUrl().contains("440x240")) | ||
| .findFirst() | ||
| .orElse(list.getFirst()); | ||
|
|
||
| GroupImage thumb = list.stream() | ||
| .filter(img -> img.getImageUrl().contains("100x100")) | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
||
| return GroupImageItemResponse.from(main, thumb); | ||
|
|
||
| }) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
The new updateGroupImages method lacks unit test coverage. Given that the repository has existing tests, consider adding tests to cover scenarios such as:
- Successful image update by HOST
- Permission denied when non-HOST tries to update
- Validation for maximum 3 images
- Old images properly deleted from S3
- Correct handling of null/empty requests
| public String extractKeyFromUrl(String imageUrl) { | ||
| if (imageUrl == null || imageUrl.isBlank()) { | ||
| return null; | ||
| } | ||
|
|
||
| String endpoint = awsS3Properties.getPublicEndpoint(); | ||
| if (endpoint != null && !endpoint.isBlank()) { | ||
| String prefix = endpoint.endsWith("/") ? endpoint : endpoint + "/"; | ||
| if (imageUrl.startsWith(prefix)) { | ||
| return imageUrl.substring(prefix.length()); | ||
| } | ||
| } | ||
|
|
||
| // fallback: 마지막 "/" 이후를 key로 사용 | ||
| int idx = imageUrl.lastIndexOf('/'); | ||
| if (idx >= 0 && idx + 1 < imageUrl.length()) { | ||
| return imageUrl.substring(idx + 1); | ||
| } | ||
|
|
||
| // 그래도 안 되면 전체를 key로 시도 (최악의 경우) | ||
| return imageUrl; | ||
| } | ||
|
|
||
| public void deleteByUrl(String imageUrl) { | ||
| String key = extractKeyFromUrl(imageUrl); | ||
| if (key == null || key.isBlank()) { | ||
| return; | ||
| } | ||
| delete(key); | ||
| } | ||
|
|
||
| public void deleteAllByUrls(List<String> imageUrls) { | ||
| if (imageUrls == null || imageUrls.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| List<String> keys = imageUrls.stream() | ||
| .filter(Objects::nonNull) | ||
| .map(this::extractKeyFromUrl) | ||
| .filter(Objects::nonNull) | ||
| .filter(key -> !key.isBlank()) | ||
| .toList(); | ||
|
|
||
| if (!keys.isEmpty()) { | ||
| deleteAll(keys); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new deleteByUrl, deleteAllByUrls, and extractKeyFromUrl methods lack unit test coverage. Consider adding tests to verify:
- Correct key extraction from various URL formats
- Handling of null/empty URLs
- Proper delegation to existing delete methods
- Edge cases in URL parsing
| @Future(message = "모임: 종료 시간은 현재 이후여야 합니다.") | ||
| LocalDateTime endTime, |
There was a problem hiding this comment.
[nitpick] The @Future constraint on endTime requires it to be in the future at the time of validation, but this doesn't guarantee that endTime will be after startTime. The validation in the service layer (lines 481-484) partially addresses this, but only when both values are non-null. Consider adding a custom validator or ensuring that the relationship between startTime and endTime is properly validated even when one is null.
src/test/http/group/update.http
Outdated
|
|
||
| --boundary | ||
| Content-Disposition: form-data; name="images"; filename="img1.png" | ||
| Content-Type: image/jpeg |
There was a problem hiding this comment.
The Content-Type is declared as image/jpeg but the file being uploaded is img1.png. This mismatch between the declared Content-Type and actual file type could cause issues. The Content-Type should be image/png.
| Content-Type: image/jpeg | |
| Content-Type: image/png |
| // 2. HOST 권한 체크 | ||
| if (!group.getHost().getId().equals(userId)) { | ||
| throw new GroupException( | ||
| GroupErrorCode.NO_PERMISSION_TO_UPDATE_GROUP, // 삭제도 동일 코드 재사용 |
There was a problem hiding this comment.
The error code NO_PERMISSION_TO_UPDATE_GROUP is being reused for delete operations, but the message says "수정할 권한이 없습니다" (no permission to update). Consider creating a separate error code like NO_PERMISSION_TO_DELETE_GROUP with an appropriate message, or rename the existing one to be more generic (e.g., NO_PERMISSION_TO_MODIFY_GROUP).
| GroupErrorCode.NO_PERMISSION_TO_UPDATE_GROUP, // 삭제도 동일 코드 재사용 | |
| GroupErrorCode.NO_PERMISSION_TO_DELETE_GROUP, |
| HOST_CANNOT_LEAVE_OWN_GROUP(HttpStatus.BAD_REQUEST, "모임: HOST는 나갈 수 없습니다. 모임 ID: %s 회원 ID: %s"); | ||
| HOST_CANNOT_LEAVE_OWN_GROUP(HttpStatus.BAD_REQUEST, "모임: HOST는 나갈 수 없습니다. 모임 ID: %s 회원 ID: %s"), | ||
| NO_PERMISSION_TO_UPDATE_GROUP(HttpStatus.FORBIDDEN, "모임: 해당 모임을 수정할 권한이 없습니다. 모임 ID: %s 회원 ID: %s"), | ||
| INVALID_MAX_PARTICIPANTS_LESS_THAN_CURRENT(HttpStatus.BAD_REQUEST, "모임: 현재 참여 인원 수(%s)보다 작은 값으로 최대 인원을 줄일 수 없습니다."),; |
There was a problem hiding this comment.
There's a trailing semicolon after the comma at the end of this line. This is a syntax error that should be removed.
| INVALID_MAX_PARTICIPANTS_LESS_THAN_CURRENT(HttpStatus.BAD_REQUEST, "모임: 현재 참여 인원 수(%s)보다 작은 값으로 최대 인원을 줄일 수 없습니다."),; | |
| INVALID_MAX_PARTICIPANTS_LESS_THAN_CURRENT(HttpStatus.BAD_REQUEST, "모임: 현재 참여 인원 수(%s)보다 작은 값으로 최대 인원을 줄일 수 없습니다."); |
모임 이미지 관리의 일관성을 개선하고 잠재적인 문제를 방지하기 위해 이미지 삭제 프로세스 순서를 재정렬했습니다. 모임 삭제 권한 확인 시 올바른 오류 코드를 사용하도록 업데이트했습니다. 또한 이미지 업데이트 요청의 유효성을 검사합니다.
액세스 토큰을 가져오기 위해 사용자 로그인 기능을 추가합니다. 가져온 토큰을 사용하여 그룹에 참여합니다. 이미지 콘텐츠 형식을 png로 변경합니다.
github-project-automation
bot
moved this from In progress
to Done
in WeGo-Together Backend
1 participant
📝 Pull Request
📌 PR 종류
해당하는 항목에 체크해주세요.
✨ 변경 내용
모임 이미지 업데이트 및 삭제 기능을 추가합니다. S3 스토리지 관리 및 권한 확인 기능이 포함됩니다.
이미지 업데이트 요청 및 응답을 위한 DTO를 도입합니다. 이미지 관련 작업을 처리하도록 모임 서비스를 업데이트합니다.
자세한 로직 과정은 API 문서에서 최종 정리할 예정입니다.
🔍 관련 이슈
🧪 테스트
변경된 기능에 대한 테스트 범위 또는 테스트 결과를 작성해주세요.
🚨 확인해야 할 사항 (Checklist)
PR을 제출하기 전에 아래 항목들을 확인해주세요.
🙋 기타 참고 사항
리뷰어가 참고하면 좋을 만한 추가 설명이 있다면 적어주세요.
Summary by CodeRabbit
릴리스 노트
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.