feat: 관리자 페이지 분실물 수정 기능 구현 by jeonseohee9 · Pull Request #126 · greedy-team/zup-zup-be

jeonseohee9 · 2026-02-07T12:53:51Z

📎 Issue 번호

closed #125

✨ 작업 내용

PUT /api/admin/lost-items/{id}
관리자가 보류 상태인 분실물의 상세 정보를 수정하고 승인되는 api
keepImageIds에 포함되지 않은 기존 이미지는 DB와 S3에서 삭제되고 추가된 파일은 S3에 저장됩니다

🎯 리뷰 포인트

놓친 부분이나 개선하면 좋을 점 피드백해주시면 감사하겠습니다!!

📝 기타

✅ 테스트

수동 테스트 완료
테스트 코드 완료

HyerimH · 2026-02-17T15:27:19Z

서희님, 이번 작업 고생 많으셨어요! 전체적으로 꼼꼼하게 잘 작성해주신 것 같습니다 👍👍 몇 가지 제안드리고 싶은 부분이 있어서 리뷰 남겨봅니다!

[이전 pr 리뷰내용](https://github.com//pull/104#discussion_r2518801006)

전에 창희님이 다음과 같은 리뷰를 남겨주신 적이 있었는데요!💡 저도 공감하는 부분이라 나중에 전체적으로 리팩토링을 진행하면 좋을 것 같아요:) 우선 admin 관련 DTO를 먼저 맞춰주셔도 좋고 나중에 한번에 리팩토링 해도 괜찮을 것 같습니다🔥

HyerimH · 2026-02-17T15:14:45Z

+    private void validateImageOwnership(Long lostItemId, List<Long> safeKeepIds) {
+        if (!safeKeepIds.isEmpty()) {
+            long count = lostItemImageRepository.countByIdInAndLostItemId(safeKeepIds, lostItemId);
+            if (count != safeKeepIds.size()) {
+                throw new ApplicationException(LostItemException.INVALID_IMAGE_ACCESS);
+            }
+        }
+    }


현재 validateImageOwnership에서 소유권 확인을 위해 DB에 count 쿼리를 날리고 있는 것 같아요! 앞서 existingImages를 조회했으니 이 리스트에 keepImageIds가 모두 포함되어 있는지를 확인하는 방식으로 변경하면 DB 쿼리를 하나 줄일 수 있을 것 같습니다🔥

HyerimH · 2026-02-17T15:16:05Z

        return AdminPendingLostItemListResponse.of(commands, page, limit, commands.size());
    }

+    @Transactional


현재 updateLostItem 메서드 전체에 @transactional이 걸려있는데, 이 안에서 S3 업로드 로직이 포함되어 있더라구요! 혹시 S3 업로드가 지연되면 그 시간만큼 DB 커넥션을 점유하게 되어 성능 이슈가 생길 수도 있을 것 같습니다.

LostItemRegisterService를 참고해보니 S3 업로드를 먼저 하고 DB 저장을 나중에 트랜잭션으로 처리했던데, 이번 수정 로직도 비슷하게 S3 업로드를 트랜잭션 밖으로 빼는 구조로 가져가면 어떤지 의견이 궁금합니다:)

HyerimH · 2026-02-17T15:20:30Z

+        CreateLostItemCommand dummyCommand = new CreateLostItemCommand(
+                request.description(),
+                request.depositArea(),
+                request.foundAreaId(),


수정 로직에서 검증을 위해 create용 커멘드 객체를 만들어서 사용하고 있는 것 같은데 만약 나중에 생성 시에는 필수지만 수정 시에는 선택인 필드가 생기거나 검증 조건이 달리자면 수정 로직에도 영향을 받을 수 있을 것 같아요! 수정용 커멘트를 만들어서 수정 전용 검증 메서드를 따로 만드는 방식은 어떨까요??

HyerimH · 2026-02-17T15:22:34Z

buildCommands 메서드를 DTO 내부 정적 메서드로 옮기면 서비스 코드도 간결해질 것 같은데 DTO로 옮기는 거 어떤가요? 요기 안에 있는 AdminLostResult도 정팩메를 쓰면 조금 더 간결해질 것 같아용

HyerimH · 2026-02-17T15:22:54Z

생성 로직에서는 LostItemStorageService에게 DB 저장 책임을 위임하고 있는데, 수정 로직은 서비스 내에서 직접 엔티티를 수정하고 있어서 코드가 약간 무거워진 느낌이 드는 것 같아요.

이 수정 부분을 LostItemStorageService에 updateLostItem 같은 메서드를 추가하면 가독성도 좋아지고 책임도 나눌 수 있을 것 같은데 어떻게 생각하시는지 궁금합니다!

jeonseohee9 · 2026-02-20T23:39:24Z

서희님, 이번 작업 고생 많으셨어요! 전체적으로 꼼꼼하게 잘 작성해주신 것 같습니다 👍👍 몇 가지 제안드리고 싶은 부분이 있어서 리뷰 남겨봅니다!

이전 pr 리뷰내용
전에 창희님이 다음과 같은 리뷰를 남겨주신 적이 있었는데요!💡 저도 공감하는 부분이라 나중에 전체적으로 리팩토링을 진행하면 좋을 것 같아요:) 우선 admin 관련 DTO를 먼저 맞춰주셔도 좋고 나중에 한번에 리팩토링 해도 괜찮을 것 같습니다🔥

리뷰 감사합니당!! 오랜만에 하다보니까 잊고있었네요.. 우선 admin관련 dto 네이밍은 맞추었습니다
DB 쿼리 제거 ,s3 업로드를 트랜잿션밖으로 이동, 수정용 커맨드 생성과 StorageService로 책임 분리하였습니다!

…jeonseohee9/feat-59-admin

chxghee

서희님 오랜만이에요 ~~~ ㅎㅎ
리뷰가 조금 늦어져서 죄송합니다 ㅠㅠ 😭

이번에는 구현의 세부 로직보다는, 현재 발생하고 있는 문제랑 설계 관점 위주로 코멘트를 남겨봤어요.
한 번 확인해보시고, 시간 괜찮으실 때 천천히 반영해주셔도 될 것 같아요 🙂

트랜잭션을 이렇게 분리해서 설계해보는 건 아마 처음이실 수도 있을 것 같은데,
한 번 제대로 정리해두시면 앞으로 비슷한 상황에서 많이 도움이 되실 거예요!

단순히 기능을 구현하는 걸 넘어서, 최적화와 설계에 관련된 것들이라서
이런 부분에서 많이 고민해보시면 분명 많은 공부가 될 것 같습니다 👍

chxghee · 2026-02-22T13:06:09Z

+            UpdateLostItemCommand command = UpdateLostItemCommand.of(updateRequest);
+
+            UpdateLostItemResult result = adminLostItemService.updateLostItem(
+                    lostItemId,
+                    command,
+                    newImages
+            );


여기서 커맨드 객체를 만들어서 서비스에 전달하고 있으니까,
lostItemId, newImages 또한 같이 커멘드에 묶에서 서비스 계층에 전달하면 더욱 깔끔할것 같네용~~

P.S. 다른 코드에 대해서도 이렇게 맞추면 좋겠지만 너무나도 대 공사기 떄문에,
지금 구현하는 관리자 분실물 업데이트 기능 부터 DTO컨벤션에 맞게 수정하면 될것같아요!!

chxghee · 2026-02-22T13:35:00Z

+    public UpdateLostItemResult updateLostItem(Long lostItemId,
+                                               UpdateLostItemCommand command,
+                                               List<MultipartFile> newImages) {
+
+        List<UploadedImageData> uploadedImages =
+                lostItemStorageService.uploadImages(newImages);
+
+        try {
+            return updateLostItemTx(
+                    lostItemId,
+                    command,
+                    uploadedImages
+            );
+        } catch (Exception e) {
+            lostItemStorageService.cleanupImages(uploadedImages);
+            throw e;
+        }
+    }
+
+    @Transactional
+    public UpdateLostItemResult updateLostItemTx(Long lostItemId,
+                                                 UpdateLostItemCommand command,
+                                                 List<UploadedImageData> uploadedImages) {


관리자 페이지 분실물 수정 기능의 첫 번째 진입점이 되는 서비스 메서드는 updateLostItem으로 파악했습니다.

이전에 혜림님께서 남겨주셨던 “Tx를 분리해보면 어떨까?”라는 리뷰를 반영해주신 것으로 보여서, 그 부분은 의도를 고민해보신 흔적이 느껴졌습니다 👍

다만, 현재 구조에서는 의도하신 대로 트랜잭션이 동작하지 않을 가능성이 있어 보여요.

updateLostItem 내부에서 같은 클래스에 있는 updateLostItemTx를 호출하고 있는데,
이 부분이 셀프 인보케이션이라는 문제를 발생하고 있어요.

아시다시피 스프링의 @Transactional은 프록시 기반으로 동작하기 때문에,
AdminLostItemService 외부(ex. 컨트롤러)에서 해당 서비스를 호출할 때는 프록시를 통해 트랜잭션이 정상적으로 적용됩니다.

하지만 서비스 내부에서 자기 자신의 메서드를 호출하게 되면,
프록시를 거치지 않고 자기 자신인 실제 객체의 메서드를 직접 호출하게 됩니다.

그 결과 updateLostItemTx에 @Transactional을 붙이더라도 해당 어노테이션이 실제로는 적용되지 않게 됩니다.

이 문제를 해결하는 가장 간단한 방법은, updateLostItemTx를 별도의 클래스로 분리하여 외부에서 호출되도록 만드는 것입니다.

그렇게 하면 자기 자신의 내부에서 호출하는것이 아니기 때문에,
프록시를 타고 트랜잭션이 정상적으로 적용될 수 있을 것 같습니다!

제가 이전에 분실물 등록 기능을 구현할 때
LostItemRegisterService와 LostItemStorageService를 분리했던 것도 같은 이유였습니다.

LostItemStorageService에서는 DB 관련 작업을 담당하고, @Transactional을 붙여 외부에서 호출될 때 트랜잭션이 적용되도록 구성했고

LostItemRegisterService에서는 S3 업로드와 같은 외부 I/O 작업을 담당하면서, DB 작업은 LostItemStorageService에 위임하도록 구성했습니다.
(지금보니 네이밍이 좀 별로네요..ㅋㅋㅋ)

이렇게 분리하면, DB 작업과 오래 걸리는 외부 I/O 작업을 분리할 수 있어서 혜림님이 남겨주신 리뷰 대로,
이 하나의 요청이서 DB 커넥션의 점유 시간을 줄이고 동시 처리할 수 있는 요청수를 늘릴 수 있는것이지용

지금 구조에서는 updateLostItem의 작업 흐름을 조금 더 명확하게 나눠서 가져가면 좋을 것 같습니다 🙂

러프하지만, 예를 들면 어드민 분실물 수정 작업은 아래와 같은 순서로 정리해볼 수 있을 것 같아요.

수정 대상 분실물의 상태 및 요청 값에 대한 유효성 검사를 먼저 수행
(보류 상태인지, 수정 가능한 상태인지, 등등 하고 계신 유효성 검사를 진행하면 될것 같아요)
유효성 검사를 S3 업로드 이전에 먼저 한다면, 불필요한 업로드/삭제가 발생하지 않도록 할 수 있겠네용

새로 등록할 사진이 있다면 S3에 먼저 업로드하고 업로드 결과(URL)는 이후 DB 반영에 사용할 수 있도록 보관
(없다면 이 단계는 패스)

트랜잭션 내에서 분실물 정보 갱신
이 단계에서는 DB 일관성만 책임지도록 가져가는 것이 좋아 보입니다.

삭제 요청된 기존 사진은 DB 반영이 정상적으로 완료된 이후 S3에서 삭제
(DB가 실패했는데 S3만 먼저 삭제되는 상황은 피하는 것이 안전할 것 같습니다.)

만약 2~3 과정에서 예외가 발생했다면...
이미 업로드된 신규 이미지가 있다면 보상 차원에서 해당 이미지를 S3에서 삭제

이렇게 정리하면

1,3번의 DB 작업은 다른 클래스의 메서드로 분리해서 각각 트랜잭션으로 일관성을 보장하고

S3 작업은 선업로드 + 보상 처리 전략으로 관리하면서

전체 흐름도 비교적 명확해질 것 같습니다.

동시에 수정이 일어난다 하면 문제가 발생할수 있긴 설계이긴 하지만,
어드민 기능이라 동시 요청은 발생하지 않을 것으로 보이기 때문에 이런식으로 Tx를 분리해도 무리가 없어 보여요

조금 러프한 가이드이긴 하지만,
이런 식으로 단계별 책임을 분리해서 메서드를 만들고 구조를 다듬어보시면 좋을 것 같아요~~

P.S. 위에서 적은 설계는 어디까지나 예시 수준으로 봐주시면 좋을 것 같고,
구체적인 설계는 서희님께서 한 번 직접 고민해보시고 정리해보시면 더 좋을 것 같습니다 👍

안녕하세요 창희님!!
자세한 리뷰 정말 감사드려요!!

유효성 먼저
신규 이미지 S3 업로드
Tx 내 DB 갱신
커밋 후 기존 이미지 S3 삭제
실패 시 신규 업로드 이미지 보상 삭제
순으로 로직을 수정하였으며
Tx 분리를 별도 클래스로 빼서 프록시 타게끔 변경하였습니다!!

chxghee

서희님 고생하셨습니다~!!

잘 동작하게끔 흐름을 수정해 주신 것 같아요~~
API 동작을 잘 이해 한 건지 모르겠어서 질문 하나 남겼어요! 확인해 주시고 천천히 답변 남겨 주세용

chxghee · 2026-03-05T02:20:58Z

+        List<ItemFeatureRequest> featureOptions,
+        List<Long> keepImageIds,
+        List<MultipartFile> newImages
+


어플리케이션 계층인 Command 에서는 입력 유효성을 해주지 않아도 될 것 같아요
이미 presentation 계층의 Reuqest 에서 유효성 검사를 하고 있으니 여기 검증은 중복 로직인 것 같아요!

chxghee · 2026-03-05T03:04:05Z

+
+        } catch (Exception e) {
+            lostItemStorageService.cleanupImages(uploadedImages);
+            throw e;


여기 예외에 대한 부분도 커스텀 예외로 감싸고 저희가 쉽게 인지할 수 있도록 ERROR 로그도 남기면 하면 좋을거 같아요!

chxghee · 2026-03-05T03:21:59Z

+        lostItemImageRepository.deleteAllInBatch(toDeleteFromDb);
+
+        saveLostItemImage(uploadedImages, lostItem);
+


지금 새롭게 추가되어 수정되는 이미지 데이터의 order (순서) 필드는 어떻게 관리 되고 있는 설명해 주실 수 있나요??

API 설계가 수정할 이미지를 새로운 이미지로 덮어 씌우는 작업을 하고 있는거 같은데,
그럼 새롭게 저장되는 이미지들의 순서는 해당 교체되는 이미지들의 순서를 이어 받아 등록이 되는 건가요?

이미지를 삭제만 할 수 있는지?
교체를 할 이미지를 등록해야만 이미지를 삭제하고 새로운 이미지로 교체를 할 수 있는 것인지가 궁금하네요!

놓친 부분 짚어주셔거 감사합니다!ㅜㅜ
재색인하는 방식으로 인덱스 로직을 추가하였고, 삭제만 원하는 경우에는 UpdateLostItemCommand 요청에서
keepImageIds에 유지하고 싶은 이미지 ID를 보내고
newImages에 빈 리스트로 보내는 방식으로 구현하였습니다

jeonseohee9 added 6 commits February 2, 2026 23:19

feat: dto 구현 [#125]

122a81a

feat: 도메인 수정 [#125]

76812a0

feat: 분실물 정보 수정 기능 구현 [#125]

cbe21f2

feat: 분실물 이미지 로직 수정 [#125]

01760f6

test: 테스트 코드 작성 [#125]

db92635

docs: 스웨거 작성 [#125]

a0f403e

jeonseohee9 self-assigned this Feb 7, 2026

jeonseohee9 added the ✨ Feature 기능 개발 label Feb 7, 2026

HyerimH approved these changes Feb 17, 2026

View reviewed changes

jeonseohee9 added 8 commits February 20, 2026 00:51

feat: dto 네이밍 [#125]

bef8bcd

fix: 수정/생성 로직 분리 [#125]

205ee79

test: 테스트 코드 수정 [#125]

59a8ca4

refactor: keepimageids request dto로 이동[#125]

9280865

refactor: keepimageids request dto로 이동[#125]

cf1b62d

test: 테스트 코드 수정[#125]

eade2e6

Merge branch 'main' into jeonseohee9/feat-59-admin

5fd376c

refactor: dto 이름 수정[#125]

3905a1a

Merge remote-tracking branch 'origin/jeonseohee9/feat-59-admin' into …

9a7065f

…jeonseohee9/feat-59-admin

chxghee reviewed Feb 22, 2026

View reviewed changes

jeonseohee9 added 5 commits February 28, 2026 15:54

refactor: command 수정[#125]

53d3117

refactor: 트랜잭션 로직 분리[#125]

bd73c5a

refactor: 트랜잭션 로직 분리[#125]

d689175

refactor: 테스트 수정[#125]

b537cf2

refactor: 스웨거 수정[#125]

723c5ae

chxghee reviewed Mar 5, 2026

View reviewed changes

jeonseohee9 added 3 commits March 7, 2026 22:18

refactor: 인덱싱 로직추가[#125]

62c5906

refactor: command 수정[#125]

73a830b

refactor: 에러 로그 추가[#125]

10dfae9

kokodak force-pushed the jeonseohee9/feat-59-admin branch 2 times, most recently from 009fc89 to 10dfae9 Compare June 13, 2026 13:08

chemistryx force-pushed the jeonseohee9/feat-59-admin branch from 10dfae9 to 38ccda1 Compare June 20, 2026 02:14

kokodak force-pushed the jeonseohee9/feat-59-admin branch from 38ccda1 to 10dfae9 Compare June 20, 2026 03:23

		lostItemImageRepository.deleteAllInBatch(toDeleteFromDb);

		saveLostItemImage(uploadedImages, lostItem);

Uh oh!

Conversation

jeonseohee9 commented Feb 7, 2026

📎 Issue 번호

✨ 작업 내용

🎯 리뷰 포인트

📝 기타

✅ 테스트

Uh oh!

HyerimH commented Feb 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyerimH Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeonseohee9 commented Feb 20, 2026

Uh oh!

chxghee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chxghee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HyerimH Feb 17, 2026 •

edited

Loading