Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

밸런스 게임 임시저장 시 파일 중복 오류 해결 #775

Merged
merged 14 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@

import balancetalk.file.domain.File;
import balancetalk.file.domain.FileType;
import java.util.List;
import org.springframework.data.jpa.repository.JpaRepository;

public interface FileRepository extends JpaRepository<File, Long>, FileRepositoryCustom {

void deleteByResourceIdAndFileType(Long tempTalkPickId, FileType fileType);

void deleteByResourceIdInAndFileType(List<Long> ids, FileType fileType);
}
115 changes: 111 additions & 4 deletions src/main/java/balancetalk/game/application/TempGameService.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static balancetalk.file.domain.FileType.TEMP_GAME_OPTION;
import static balancetalk.game.dto.TempGameDto.CreateTempGameRequest;

import balancetalk.file.domain.File;
import balancetalk.file.domain.FileHandler;
import balancetalk.file.domain.repository.FileRepository;
import balancetalk.game.domain.MainTag;
Expand All @@ -21,7 +22,10 @@
import balancetalk.member.dto.ApiMember;
import java.util.ArrayList;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
Expand All @@ -44,14 +48,23 @@ public void createTempGame(CreateTempGameSetRequest request, ApiMember apiMember

List<CreateTempGameRequest> tempGames = request.getTempGames();

List<TempGame> newTempGames = tempGames.stream()
.map(game -> game.toEntity(fileRepository))
.toList();
List<TempGame> newTempGames = request.getTempGames().stream()
.map(game -> game.toEntity(fileRepository))
.toList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왜 수정했는지 알 수 있을까요??
위에 request.getTempGamse()를 이미 호출했는데 또 호출하는 방식으로 변경돼서 궁금합니다


if (member.hasTempGameSet()) { // 기존 임시저장이 존재하는 경우
TempGameSet existGame = member.getTempGameSet();

if (request.isNewRequest()) { // 새롭게 임시저장 하는 경우, 파일 모두 삭제
List<Long> oldFileIds = existGame.getAllFileIds();
if (!oldFileIds.isEmpty()) {
fileRepository.deleteByResourceIdInAndFileType(oldFileIds, TEMP_GAME_OPTION);
}
}

// 새롭게 불러온 경우, 파일만 재배치 (isLoaded: true)
relocateFiles(request, existGame);
existGame.updateTempGameSet(request.getTitle(), request.getSubTag(), mainTag, newTempGames);
processTempGameFiles(tempGames, existGame.getTempGames());
return;
}

Expand All @@ -67,6 +80,100 @@ public void createTempGame(CreateTempGameSetRequest request, ApiMember apiMember
processTempGameFiles(tempGames, tempGameSet.getTempGames());
}

private void relocateFiles(CreateTempGameSetRequest request, TempGameSet tempGameSet) {
if (request.getAllFileIds().isEmpty()) {
return;
}

List<Long> deletedFileIds = deleteFiles(request, tempGameSet);

List<Long> newFileIds = getNewNonDuplicateFileIds(request, deletedFileIds, tempGameSet);

relocateFilesToGameOption(request, newFileIds, tempGameSet);
}

private void relocateFilesToGameOption(
CreateTempGameSetRequest request,
List<Long> newFileIds,
TempGameSet tempGameSet
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

) 위치가 이상합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sonarqube 라인 120이 넘어서 저렇게 설정했는데

)
   {

형태가 되어야 할까요??

if (!newFileIds.isEmpty()) {
List<TempGame> tempGames = tempGameSet.getTempGames();
Map<Long, Long> fileToOptionMap = getFileToOptionMap(tempGames, request, newFileIds);

List<File> files = fileRepository.findAllById(newFileIds);
Map<Long, File> fileMap = files.stream()
.collect(Collectors.toMap(File::getId, file -> file));

Comment on lines +99 to +102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Map 생성 시 NPE 방지 필요

fileMap 생성 시 null 값에 대한 처리가 없어 NPE가 발생할 수 있습니다.

다음과 같이 수정을 제안합니다:

Map<Long, File> fileMap = files.stream()
-        .collect(Collectors.toMap(File::getId, file -> file));
+        .filter(file -> file != null)
+        .collect(Collectors.toMap(
+            File::getId,
+            file -> file,
+            (existing, replacement) -> existing,
+            LinkedHashMap::new
+        ));
📝 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.

Suggested change
List<File> files = fileRepository.findAllById(newFileIds);
Map<Long, File> fileMap = files.stream()
.collect(Collectors.toMap(File::getId, file -> file));
List<File> files = fileRepository.findAllById(newFileIds);
Map<Long, File> fileMap = files.stream()
.filter(file -> file != null)
.collect(Collectors.toMap(
File::getId,
file -> file,
(existing, replacement) -> existing,
LinkedHashMap::new
));

for (Map.Entry<Long, Long> entry : fileToOptionMap.entrySet()) {
Long fileId = entry.getKey();
Long tempGameOptionId = entry.getValue();

File file = fileMap.get(fileId);
if (file == null) {
throw new BalanceTalkException(ErrorCode.NOT_FOUND_FILE);
}
fileHandler.relocateFile(file, tempGameOptionId, TEMP_GAME_OPTION);
}
}
}

private Map<Long, Long> getFileToOptionMap(
List<TempGame> tempGames,
CreateTempGameSetRequest request,
List<Long> newFileIds
) {
Map<Long, Long> fileToOptionMap = new LinkedHashMap<>();
List<CreateTempGameRequest> tempGameRequests = request.getTempGames();

for (int i = 0; i < tempGameRequests.size(); i++) {
CreateTempGameRequest gameRequest = tempGameRequests.get(i);

List<TempGameOptionDto> tempGameOptions = gameRequest.getTempGameOptions();
for (TempGameOptionDto optionDto : tempGameOptions) {
Long fileId = optionDto.getFileId();

if (fileId != null && newFileIds.contains(fileId)) {
TempGameOption tempGameOption = tempGames.get(i).getTempGameOptions()
.stream()
.filter(option -> option.getOptionType().equals(optionDto.getOptionType()))
.findFirst()
.orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH));
fileToOptionMap.put(fileId, tempGameOption.getId());
}
}
}
return fileToOptionMap;
}

private List<Long> getNewNonDuplicateFileIds(
CreateTempGameSetRequest request,
List<Long> deletedFileIds,
TempGameSet tempGameSet
) {
List<Long> existingFileIds = tempGameSet.getAllFileIds();
return request.getAllFileIds().stream()
.filter(fileId -> !deletedFileIds.contains(fileId))
.filter(fileId -> !existingFileIds.contains(fileId))
.toList();
}

private List<Long> deleteFiles(CreateTempGameSetRequest request, TempGameSet tempGameSet) {
List<Long> existingFileIds = tempGameSet.getAllFileIds();

List<Long> newFileIds = request.getAllFileIds();

List<Long> filesIdsToDelete = existingFileIds.stream()
.filter(existingFileId -> !newFileIds.contains(existingFileId))
.toList();

if (!filesIdsToDelete.isEmpty()) {
List<File> filesToDelete = fileRepository.findAllById(filesIdsToDelete);
fileHandler.deleteFiles(filesToDelete);
}
Comment on lines +165 to +168
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

파일 삭제 실패 시 예외 처리 필요

파일 삭제 작업이 실패할 경우에 대한 예외 처리가 없습니다.

다음과 같이 수정을 제안합니다:

if (!filesIdsToDelete.isEmpty()) {
    List<File> filesToDelete = fileRepository.findAllById(filesIdsToDelete);
-    fileHandler.deleteFiles(filesToDelete);
+    try {
+        fileHandler.deleteFiles(filesToDelete);
+    } catch (Exception e) {
+        throw new BalanceTalkException(ErrorCode.FILE_DELETE_ERROR,
+            String.format("파일 삭제 중 오류가 발생했습니다. 파일 ID: %s", filesIdsToDelete));
+    }
}

Committable suggestion skipped: line range outside the PR's diff.

return filesIdsToDelete;
}

private void processTempGameFiles(List<CreateTempGameRequest> requests, List<TempGame> tempGames) {

for (int i = 0; i < requests.size(); i++) {
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/balancetalk/game/domain/TempGameOption.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public class TempGameOption {
@Enumerated(value = EnumType.STRING)
private VoteOption optionType;

private Long imgId;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GameOption과 일관성을 위해 imgUrl도 추가 부탁드립니다

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에 Game 엔티티에서 imgUrl이 불필요한 값이다보니 TempGame에도 불필요하다고 생각해서 제외했습니다.

근데 다시 생각해보니 임시게임 불러오기의 응답값을 밸런스 게임 생성할 때 쓰기 때문에 추가하는게 맞는것 같네요

@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "temp_game_id")
private TempGame tempGame;
Expand All @@ -52,5 +54,6 @@ public void assignTempGame(TempGame tempGame) {
public void updateTempGameOption(TempGameOption newTempGameOption) {
this.name = newTempGameOption.getName();
this.description = newTempGameOption.getDescription();
this.imgId = newTempGameOption.getImgId();
}
}
9 changes: 9 additions & 0 deletions src/main/java/balancetalk/game/domain/TempGameSet.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import jakarta.validation.constraints.Size;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.stream.IntStream;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
Expand Down Expand Up @@ -72,4 +73,12 @@ public void updateTempGameSet(String title, String subTag, MainTag mainTag, List
existingGame.updateTempGame(newGame);
});
}

public List<Long> getAllFileIds() {
return tempGames.stream()
.flatMap(game -> game.getTempGameOptions().stream())
.map(TempGameOption::getImgId)
.filter(Objects::nonNull)
.toList();
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package balancetalk.game.dto;

import balancetalk.file.domain.File;
import balancetalk.file.domain.repository.FileRepository;
import balancetalk.game.domain.TempGameOption;
import balancetalk.global.exception.BalanceTalkException;
Expand Down Expand Up @@ -37,6 +36,7 @@ public TempGameOption toEntity(FileRepository fileRepository) {

return TempGameOption.builder()
.name(name)
.imgId(fileId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

응답할 때도 TempGameOptionDto를 사용하는 것 같은데, imgUrl도 추가해야 하는거 아닌가요?

.description(description)
.optionType(optionType)
.build();
Expand Down
18 changes: 18 additions & 0 deletions src/main/java/balancetalk/game/dto/TempGameSetDto.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
import balancetalk.global.exception.ErrorCode;
import balancetalk.member.domain.Member;
import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Size;
import java.util.List;
import java.util.Objects;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
Expand All @@ -33,6 +35,10 @@ public static class CreateTempGameSetRequest {
@Size(max = 10, message = "서브 태그는 최대 10자까지 입력 가능합니다")
private String subTag;

@Schema(description = "최근 임시저장된 밸런스 게임 불러오기 여부", example = "true")
@NotNull(message = "isLoaded 필드는 NULL을 허용하지 않습니다.")
private Boolean isLoaded;

private List<CreateTempGameRequest> tempGames;

public TempGameSet toEntity(MainTag mainTag, Member member) {
Expand All @@ -46,6 +52,18 @@ public TempGameSet toEntity(MainTag mainTag, Member member) {
.member(member)
.build();
}

public List<Long> getAllFileIds() {
return tempGames.stream()
.flatMap(game -> game.getTempGameOptions().stream())
.map(TempGameOptionDto::getFileId)
.filter(Objects::nonNull)
.toList();
}
Comment on lines +46 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

tempGames가 null일 경우에 대한 방어 로직이 필요합니다.

현재 구현은 tempGames가 null일 경우 NullPointerException이 발생할 수 있습니다.

다음과 같이 수정하는 것을 제안드립니다:

 public List<Long> getAllFileIds() {
+    if (tempGames == null) {
+        return List.of();
+    }
     return tempGames.stream()
             .flatMap(game -> game.getTempGameOptions().stream())
             .map(TempGameOptionDto::getFileId)
             .filter(Objects::nonNull)
             .toList();
 }
📝 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.

Suggested change
public List<Long> getAllFileIds() {
return tempGames.stream()
.flatMap(game -> game.getTempGameOptions().stream())
.map(TempGameOptionDto::getFileId)
.filter(Objects::nonNull)
.toList();
}
public List<Long> getAllFileIds() {
if (tempGames == null) {
return List.of();
}
return tempGames.stream()
.flatMap(game -> game.getTempGameOptions().stream())
.map(TempGameOptionDto::getFileId)
.filter(Objects::nonNull)
.toList();
}


public boolean isNewRequest() {
return !isLoaded;
}
}

@Data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public enum ErrorCode {
EXCEEDED_IMAGES_SIZE(BAD_REQUEST, "첨부 가능한 이미지 개수를 초과했습니다."),
NOT_SUPPORTED_MIME_TYPE(BAD_REQUEST, "지원하지 않는 MIME 타입입니다."),
MISSING_MIME_TYPE(BAD_REQUEST, "MIME 타입이 존재하지 않습니다."),
FILE_ID_GAME_OPTION_ID_MISMATCH(BAD_REQUEST, "File id에 매핑되는 GameOption Id가 없습니다."),
REPORT_MY_COMMENT(BAD_REQUEST, "본인의 댓글을 신고할 수 없습니다"),
INVALID_REPORT_REASON(BAD_REQUEST, "신고 사유가 올바르지 않습니다."),
INVALID_REPORT_TYPE(BAD_REQUEST, "신고 타입이 올바르지 않습니다."),
Expand Down