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

카테고리 순서 변경 #988

Open
wants to merge 43 commits into
base: dev/be
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
2e7fd59
refactor: category에 ordinal 필드 추가
HoeSeong123 Dec 16, 2024
fc4681a
test: countByMember 테스트 코드 작성
HoeSeong123 Dec 16, 2024
2d61a2c
feat: 카테고리 조회 응답 api 변경
HoeSeong123 Dec 16, 2024
8e1037d
refactor: 중복된 클거스 MemberFixture 제
HoeSeong123 Dec 16, 2024
2b966cd
test: 카테고리 생성시 멤버별로 마지막 순서로 생성에 대한 테스트 코드 추가
HoeSeong123 Dec 16, 2024
c4f283f
feat: 카테고리 수정 요청 api 변경 및 순서 변경 로직 추가
HoeSeong123 Dec 16, 2024
d90802b
feat: 카테고리 삭제 요청 api 변경 및 순서 변경 로직 추가
HoeSeong123 Dec 17, 2024
339286a
feat: Category에 ordinal 필드 추가 sql 파일 생성
HoeSeong123 Dec 17, 2024
52810d3
fix: 카테고리를 여러개 삭제 시 순서가 제대로 정렬되지 않는 오류 수정
HoeSeong123 Dec 17, 2024
f8dfd92
test: 카테고리 삭제시 순서 정렬 테스트 코드 작성
HoeSeong123 Dec 17, 2024
ddcb834
refactor: 기본 카테고리 수정에 대한 검증 코드 추가
HoeSeong123 Dec 17, 2024
64096db
refactor: 카테고리 수정 시 순서 검증 로직 추가
HoeSeong123 Dec 17, 2024
bc199d8
refactor: 템플릿 순서 변경시 잘못된 순서로 요청할 경우 에러코드 변경
HoeSeong123 Dec 18, 2024
0102194
refactor: swagger 문서 변경사항 반영
HoeSeong123 Dec 18, 2024
fa04a1d
refactor: 매직넘버 상수화
HoeSeong123 Dec 18, 2024
d61ac47
refactor: requestDto 스키마 예시 변경
HoeSeong123 Dec 18, 2024
25e0e47
merge conflicts 해결
HoeSeong123 Dec 23, 2024
11ab595
refactor: 카테고리 편집 api 수정
HoeSeong123 Dec 23, 2024
9eea751
test: 카테고리 편집 테스트 코드 추가
HoeSeong123 Dec 23, 2024
5a2e765
refactor: 불필요한 클래스 삭제
HoeSeong123 Dec 23, 2024
a760937
docs: 스웨거 파일 수정
HoeSeong123 Dec 23, 2024
d795cbe
feat: 동일한 카테고리에 대해 수정 및 삭제 요청에 대한 예외처리
HoeSeong123 Dec 23, 2024
f94b833
feat: 카테고리의 개수가 정확하지 않은 경우에 대한 예외 처리
HoeSeong123 Dec 23, 2024
ff27d42
refactor: 개수 검증하는 시점 변경
HoeSeong123 Dec 23, 2024
ff3acb9
refactor: 불필요한 import 제거
HoeSeong123 Dec 23, 2024
92794bd
fix: 실패하는 테스트 코드 수정
HoeSeong123 Dec 23, 2024
29be344
docs: 카테고리 수정 문서 수정
HoeSeong123 Dec 27, 2024
1f93dc0
fix: 기본 카테고리 순서 0으로 고정
HoeSeong123 Dec 27, 2024
85e87cb
refactor: 생성자 체이닝
HoeSeong123 Dec 27, 2024
f96d279
feat: 카테고리 생성 dto에 최소 순서 검증 추가
HoeSeong123 Dec 27, 2024
39d0e0c
refactor: 카테고리 순서의 타입 int로 변경
HoeSeong123 Dec 27, 2024
26a0fab
feat(response): 카테고리 생성 응답 api 변경
HoeSeong123 Dec 27, 2024
c4a6ffc
refactor: 소스 코드 순서 검증 validator를 범용적으로 변경
HoeSeong123 Dec 27, 2024
ec7a539
refactor(category): OrdinalValidator를 사용하여 카테고리 순서 검증
HoeSeong123 Dec 27, 2024
74fc71c
refactor(service): for문을 forEach로 변경
HoeSeong123 Dec 27, 2024
9757b17
refactor(service): createCategories 파라미터 변경
HoeSeong123 Dec 27, 2024
771d391
docs: 카테고리 편집 요청에 대한 예외 문서 추가
HoeSeong123 Dec 27, 2024
f79c1c3
refactor: Category 테이블에 member, name 유니크 조건 삭제
HoeSeong123 Dec 27, 2024
601ff62
test: 실패하는 테스트 코드 수정
HoeSeong123 Dec 27, 2024
bb06d03
merge conflicts 해결
HoeSeong123 Dec 27, 2024
fa429ec
fix: 실패하는 테스트 코드 수정
HoeSeong123 Dec 27, 2024
6d27648
refactor(global): 순서 검증 관련 클래스 패키지 변경
HoeSeong123 Dec 27, 2024
347eb85
refactor: 카테고리 검증 로직 별 책임 분리
HoeSeong123 Dec 27, 2024
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 @@ -4,9 +4,7 @@

import org.springframework.http.ResponseEntity;
import org.springframework.validation.annotation.Validated;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestBody;
Expand All @@ -16,7 +14,7 @@

import codezap.auth.configuration.AuthenticationPrinciple;
import codezap.category.dto.request.CreateCategoryRequest;
import codezap.category.dto.request.UpdateCategoryRequest;
import codezap.category.dto.request.UpdateAllCategoriesRequest;
import codezap.category.dto.response.CreateCategoryResponse;
import codezap.category.dto.response.FindAllCategoriesResponse;
import codezap.category.service.CategoryService;
Expand Down Expand Up @@ -45,19 +43,12 @@ public ResponseEntity<FindAllCategoriesResponse> getCategories(@RequestParam Lon
return ResponseEntity.ok(categoryService.findAllByMemberId(memberId));
}

@PutMapping("/{id}")
@PutMapping
public ResponseEntity<Void> updateCategory(
@AuthenticationPrinciple Member member,
@PathVariable Long id,
@Validated(ValidationSequence.class) @RequestBody UpdateCategoryRequest updateCategoryRequest
@Validated(ValidationSequence.class) @RequestBody UpdateAllCategoriesRequest request
) {
categoryService.update(member, id, updateCategoryRequest);
categoryService.updateCategories(member, request);
return ResponseEntity.ok().build();
}

@DeleteMapping("/{id}")
public ResponseEntity<Void> deleteCategory(@AuthenticationPrinciple Member member, @PathVariable Long id) {
categoryService.deleteById(member, id);
return ResponseEntity.noContent().build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import org.springframework.http.ResponseEntity;

import codezap.category.dto.request.CreateCategoryRequest;
import codezap.category.dto.request.UpdateCategoryRequest;
import codezap.category.dto.request.UpdateAllCategoriesRequest;
import codezap.category.dto.response.CreateCategoryResponse;
import codezap.category.dto.response.FindAllCategoriesResponse;
import codezap.global.swagger.error.ApiErrorResponse;
Expand Down Expand Up @@ -49,34 +49,19 @@ ResponseEntity<CreateCategoryResponse> createCategory(
@SecurityRequirement(name = "쿠키 인증 토큰")
@Operation(summary = "카테고리 수정", description = "해당하는 식별자의 카테고리를 수정합니다.")
Copy link
Contributor

Choose a reason for hiding this comment

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

설명을 조금 수정하면 좋을 것 같아요 !
설명만 보면 업데이트 기능만 존재하는 것 같으나, 실제로는 생성, 삭제도 진행되고 있으니까요 !

카테고리를 생성, 수정, 삭제할 수 있습니다. 혹은 사용자의 카테고리들을 생성, 수정, 삭제할 수 있습니다. 어떤가요?

@ApiResponse(responseCode = "200", description = "카테고리 수정 성공")
@ApiErrorResponse(status = HttpStatus.BAD_REQUEST, instance = "/categories/1", errorCases = {
@ApiErrorResponse(status = HttpStatus.BAD_REQUEST, instance = "/categories", errorCases = {
@ErrorCase(description = "기본 카테고리를 수정한 경우", exampleMessage = "기본 카테고리는 수정 및 삭제할 수 없습니다."),
@ErrorCase(description = "카테고리 이름이 15자를 초과한 경우", exampleMessage = "카테고리 이름은 최대 15자까지 입력 가능합니다."),
@ErrorCase(description = "카테고리의 순서가 잘못된 경우", exampleMessage = "템플릿 순서가 중복됩니다.")
Copy link
Contributor

Choose a reason for hiding this comment

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

@ErrorCase(description = "모든 필드 중 null인 값이 있는 경우", exampleMessage = "카테고리 이름이 null 입니다."),
@ErrorCase(description = "카테고리 이름이 15자를 초과한 경우", exampleMessage = "카테고리 이름은 최대 15자까지 입력 가능합니다."),
@ErrorCase(description = "삭제하려는 카테고리에 템플릿이 존재하는 경우", exampleMessage = "템플릿이 존재하는 카테고리는 삭제할 수 없습니다."),

해당 에러 케이스도 추가해주세요~!

그리고 마지막 에러 케이스에도 ,를 두면 추후 또 다른 에러 케이스가 추가됬을 때 변경 사항이 안잡혀서 좋을 것 같아요 ~!

Copy link
Contributor

Choose a reason for hiding this comment

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

애플리케이션 예외 메시지와 동일하게

Suggested change
@ErrorCase(description = "카테고리의 순서가 잘못된 경우", exampleMessage = "템플릿 순서가 중복됩니다.")
@ErrorCase(description = "카테고리의 순서가 잘못된 경우", exampleMessage = "카테고리 순서가 잘못되었습니다.")

로 변경해주세요~!

})
@ApiErrorResponse(status = HttpStatus.NOT_FOUND, instance = "/categories/1", errorCases = {
@ApiErrorResponse(status = HttpStatus.NOT_FOUND, instance = "/categories", errorCases = {
@ErrorCase(description = "해당하는 id 값인 카테고리가 없는 경우", exampleMessage = "식별자 1에 해당하는 카테고리가 존재하지 않습니다."),
})
@ApiErrorResponse(status = HttpStatus.CONFLICT, instance = "/categories", errorCases = {
@ErrorCase(description = "동일한 이름의 카테고리가 존재하는 경우", exampleMessage = "이름이 Spring 인 카테고리가 이미 존재합니다."),
})
@ApiErrorResponse(status = HttpStatus.FORBIDDEN, instance = "/categories/1", errorCases = {
@ApiErrorResponse(status = HttpStatus.FORBIDDEN, instance = "/categories", errorCases = {
@ErrorCase(description = "카테고리를 수정할 권한이 없는 경우", exampleMessage = "해당 카테고리를 수정 또는 삭제할 권한이 없는 유저입니다.")
})
ResponseEntity<Void> updateCategory(Member member, Long id, UpdateCategoryRequest updateCategoryRequest);

@SecurityRequirement(name = "쿠키 인증 토큰")
@Operation(summary = "카테고리 삭제", description = "해당하는 식별자의 카테고리를 삭제합니다.")
@ApiResponse(responseCode = "204", description = "카테고리 삭제 성공")
@ApiErrorResponse(status = HttpStatus.BAD_REQUEST, instance = "/categories/1", errorCases = {
@ErrorCase(description = "삭제하려는 카테고리에 템플릿이 존재하는 경우",
exampleMessage = "템플릿이 존재하는 카테고리는 삭제할 수 없습니다."),
})
@ApiErrorResponse(status = HttpStatus.NOT_FOUND, instance = "/categories/1", errorCases = {
@ErrorCase(description = "존재하지 않는 카테고리인 경우",
exampleMessage = "식별자 1에 해당하는 카테고리가 존재하지 않습니다."),
})
@ApiErrorResponse(status = HttpStatus.FORBIDDEN, instance = "/categories/1", errorCases = {
@ErrorCase(description = "카테고리를 수정할 권한이 없는 경우",
exampleMessage = "해당 카테고리를 수정 또는 삭제할 권한이 없는 유저입니다.")
})
ResponseEntity<Void> deleteCategory(Member member, Long id);
ResponseEntity<Void> updateCategory(Member member, UpdateAllCategoriesRequest request);
}
12 changes: 9 additions & 3 deletions backend/src/main/java/codezap/category/domain/Category.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
public class Category extends BaseTimeEntity {

private static final String DEFAULT_CATEGORY_NAME = "카테고리 없음";
private static final long DEFAULT_CATEGORY_ORDINAL = 1L;
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.

넵 전달되었습니다!!
추가로 기본 카테고리의 순서는 0으로 고정하기로 했는데 수정이 안됐었네요😅


@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
Expand All @@ -50,18 +51,23 @@ public class Category extends BaseTimeEntity {
@Column(nullable = false)
private Boolean isDefault;

public Category(String name, Member member) {
@Column(nullable = false)
private long ordinal;

public Category(String name, Member member, long ordinal) {
this.name = name;
this.member = member;
this.isDefault = false;
this.ordinal = ordinal;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@AllArgsConstructor 생성자 체이닝 제안해요~

Suggested change
public Category(String name, Member member, long ordinal) {
this.name = name;
this.member = member;
this.isDefault = false;
this.ordinal = ordinal;
}
public Category(String name, Member member, long ordinal) {
this(null, member, name, false, ordinal);
}


public static Category createDefaultCategory(Member member) {
return new Category(null, member, DEFAULT_CATEGORY_NAME, true);
return new Category(null, member, DEFAULT_CATEGORY_NAME, true, DEFAULT_CATEGORY_ORDINAL);
}

public void updateName(String name) {
public void update(String name, long ordinal) {
this.name = name;
this.ordinal = ordinal;
}

public void validateAuthorization(Member member) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package codezap.category.dto.request;

import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Size;

import codezap.global.validation.ValidationGroups.NotNullGroup;
Expand All @@ -11,6 +12,10 @@ public record CreateCategoryRequest(
@Schema(description = "카테고리 이름", example = "Spring")
@NotBlank(message = "카테고리 이름이 null 입니다.", groups = NotNullGroup.class)
@Size(max = 15, message = "카테고리 이름은 최대 15자까지 입력 가능합니다.", groups = SizeCheckGroup.class)
String name
String name,

@Schema(description = "카테고리 순서", example = "1")
@NotNull(message = "카테고리 순서가 null 입니다.", groups = NotNullGroup.class)
Long ordinal
Copy link
Contributor

Choose a reason for hiding this comment

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

null 뿐만 아니라 최소 값 check도 하면 좋을 것 같아요

@Min(value = 1, message = "1 이상이어야 합니다.")

) {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package codezap.category.dto.request;

import java.util.List;

import jakarta.validation.Valid;
import jakarta.validation.constraints.NotNull;

import codezap.global.validation.ValidationGroups.NotNullGroup;
import io.swagger.v3.oas.annotations.media.Schema;

public record UpdateAllCategoriesRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

(⚡어필의 차원에서 zap을 사용합니다 ㅎㅎ )

해당 dto에서 fail fast를 위해 소스 코드 request처럼 SourceCodesOrdinalValidator를 이용해서 dto에서도 순서 검증하는 건 어떨까요?

현재 해당 로직이 템플릿 수정 로직과 매우 유사하다고 생각합니다. 그래서 현재 SourceCodesOrdinalValidator의 이름을 OrdinalValidator로 변경하여 소스코드에서만 사용하지 않고 이렇게 순서 검증이 필요한 곳에서 재사용하면 좋을 것 같아요 ~!

이렇게 한다면 ValidatedSourceCodesOrdinalRequest의
@SourceCodesOrdinal(message = "소스 코드 순서가 잘못되었습니다.", groups = SourceCodeOrdinalGroup.class)
이 메시지를 순서가 잘못되었습니다. 로 어디서든 사용될 수 있게 변경한다면 더 재사용하는데 좋을 것 같아요~

이에 대해 어떻게 생각하시나요?

만약 변경에 동의하시다면 지금 당장 클래스 이름은 변경 안하면 좋을 것 같아요 ,,! 해당 클래스를 #991 에서 사용하고 있어서 추후 둘 다 머지되고 이름 변경하면 좋을 것 같아요~

Copy link
Contributor

Choose a reason for hiding this comment

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

이미 동일한 기능을 하는 Validator가 존재하니 좋은 의견이네요~~
소스코드와 카테고리의 동작이 언제까지 동일할지 모르니 기존의 클래스를 완전 재사용하는 것이 조금은 조심스럽기도 하지만, 나중에 동작이 달라져야 할 때 상속을 통한 분리를 고려해 봐도 좋을 것 같고요~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

너무 좋습니다!!
#991 을 머지해서 이름도 변경했습니다.

@Schema(description = "생성할 카테고리 목록")
@Valid
List<CreateCategoryRequest> createCategories,

@Schema(description = "수정할 카테고리 목록")
@Valid
List<UpdateCategoryRequest> updateCategories,

@Schema(description = "삭제할 카테고리 목록")
@NotNull(message = "삭제하는 카테고리 ID 목록이 null 입니다.", groups = NotNullGroup.class)
List<Long> deleteCategoryIds
) {
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
package codezap.category.dto.request;

import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Size;

import codezap.global.validation.ValidationGroups.NotNullGroup;
import codezap.global.validation.ValidationGroups.SizeCheckGroup;
import io.swagger.v3.oas.annotations.media.Schema;

public record UpdateCategoryRequest(
@Schema(description = "카테고리 ID", example = "1")
@NotNull(message = "카테고리 ID가 null 입니다.", groups = NotNullGroup.class)
Long id,

@Schema(description = "카테고리 이름", example = "Spring")
@NotBlank(message = "카테고리 이름이 null 입니다.", groups = NotNullGroup.class)
@Size(max = 15, message = "카테고리 이름은 최대 15자까지 입력 가능합니다.", groups = SizeCheckGroup.class)
String name
String name,

@Schema(description = "카테고리 순서", example = "1")
@NotNull(message = "카테고리 순서가 null 입니다.", groups = NotNullGroup.class)
Long ordinal
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 null 뿐만 아니라 최소 값 check도 하면 좋을 것 같아요

@Min(value = 1, message = "1 이상이어야 합니다.")

) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ public record FindCategoryResponse(
@Schema(description = "카테고리 식별자", example = "1")
Long id,
@Schema(description = "카테고리 이름", example = "Spring")
String name
String name,
@Schema(description = "카테고리 순서", example = "1")
long ordinal
) {
public static FindCategoryResponse from(Category category) {
return new FindCategoryResponse(category.getId(), category.getName());
return new FindCategoryResponse(category.getId(), category.getName(), category.getOrdinal());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,6 @@ public interface CategoryJpaRepository extends JpaRepository<Category, Long> {
List<Category> findAllByMemberIdOrderById(Long memberId);

boolean existsByNameAndMember(String categoryName, Member member);

long countByMember(Member member);
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,12 @@ public boolean existsByNameAndMember(String categoryName, Member member) {
public void deleteById(Long id) {
categoryJpaRepository.deleteById(id);
}

public long countByMember(Member member) {
return categoryJpaRepository.countByMember(member);
}

public List<Category> saveAll(Iterable<Category> entities) {
return categoryJpaRepository.saveAll(entities);
}
}
121 changes: 106 additions & 15 deletions backend/src/main/java/codezap/category/service/CategoryService.java
Copy link
Contributor

Choose a reason for hiding this comment

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

코드가 너무 길어서 검증 로직을 CategoryValidationService로 분리하면 어떨까 제안해요.

@Service
@RequiredArgsConstructor
public class CategoryService {

    private final CategoryRepository categoryRepository;
    private final CategoryValidationService validationService;

    @Transactional
    public CreateCategoryResponse create(Member member, CreateCategoryRequest request) {
        validationService.validateDuplicatedCategory(request.name(), member);
        validationService.validateOrdinal(member, request);

        Category category = categoryRepository.save(createCategory(member, request));
        return CreateCategoryResponse.from(category);
    }

    @Transactional(readOnly = true)
    public FindAllCategoriesResponse findAllByMemberId(Long memberId) {
        return FindAllCategoriesResponse.from(categoryRepository.findAllByMemberIdOrderById(memberId));
    }

    @Transactional
    public void updateCategories(Member member, UpdateAllCategoriesRequest request) {
        validationService.validateDuplicateNameRequest(request);
        validationService.validateOrdinals(request);
        validationService.validateIds(request);

        createCategories(member, request);
        updateCategories(request.updateCategories(), member);
        deleteCategories(request.deleteCategoryIds(), member);

        validationService.validateCategoriesCount(member, request);
    }

    private void createCategories(Member member, UpdateAllCategoriesRequest request) {
        categoryRepository.saveAll(request.createCategories().stream()
                .map(createRequest -> new Category(createRequest.name(), member, createRequest.ordinal()))
                .toList());
    }

    private void updateCategories(List<UpdateCategoryRequest> updates, Member member) {
        updates.forEach(update -> {
            Category category = categoryRepository.fetchById(update.id());
            category.validateAuthorization(member);
            validationService.validateDefaultCategory(category);
            category.update(update.name(), update.ordinal());
        });
    }

    private void deleteCategories(List<Long> ids, Member member) {
        ids.forEach(id -> {
            Category category = categoryRepository.fetchById(id);
            category.validateAuthorization(member);
            validationService.validateDefaultCategory(category);
            validationService.validateHasTemplate(id);
            categoryRepository.deleteById(id);
        });
    }
}
package codezap.category.service;

import java.util.HashSet;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.springframework.stereotype.Service;

import codezap.category.domain.Category;
import codezap.category.dto.request.CreateCategoryRequest;
import codezap.category.dto.request.UpdateAllCategoriesRequest;
import codezap.category.dto.request.UpdateCategoryRequest;
import codezap.category.repository.CategoryRepository;
import codezap.global.exception.CodeZapException;
import codezap.global.exception.ErrorCode;
import codezap.member.domain.Member;
import codezap.template.repository.TemplateRepository;
import lombok.RequiredArgsConstructor;

@Service
@RequiredArgsConstructor
public class CategoryValidationService {

    private final CategoryRepository categoryRepository;
    private final TemplateRepository templateRepository;

    public void validateDuplicatedCategory(String categoryName, Member member) {
        if (categoryRepository.existsByNameAndMember(categoryName, member)) {
            throw new CodeZapException(ErrorCode.DUPLICATE_CATEGORY, "이름이 " + categoryName + "인 카테고리가 이미 존재합니다.");
        }
    }

    public void validateDefaultCategory(Category category) {
        if (category.isDefault()) {
            throw new CodeZapException(ErrorCode.DEFAULT_CATEGORY, "기본 카테고리는 수정 및 삭제할 수 없습니다.");
        }
    }

    public void validateHasTemplate(Long id) {
        if (templateRepository.existsByCategoryId(id)) {
            throw new CodeZapException(ErrorCode.CATEGORY_HAS_TEMPLATES, "템플릿이 존재하는 카테고리는 삭제할 수 없습니다.");
        }
    }

    public void validateDuplicateNameRequest(UpdateAllCategoriesRequest request) {
        List<String> allNames = Stream.concat(
                request.createCategories().stream().map(CreateCategoryRequest::name),
                request.updateCategories().stream().map(UpdateCategoryRequest::name)
        ).toList();

        if (hasDuplicates(allNames)) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "요청에 중복된 카테고리 이름이 존재합니다.");
        }
    }

    public void validateOrdinal(Member member, CreateCategoryRequest request) {
        long ordinal = request.ordinal();
        long count = categoryRepository.countByMember(member);
        if (ordinal != count) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리 순서가 잘못되었습니다.");
        }
    }

    public void validateOrdinals(UpdateAllCategoriesRequest request) {
        List<Long> allOrdinals = Stream.concat(
                request.createCategories().stream().map(CreateCategoryRequest::ordinal),
                request.updateCategories().stream().map(UpdateCategoryRequest::ordinal)
        ).sorted().toList();

        if (!isSequential(allOrdinals)) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리 순서가 연속적이지 않습니다.");
        }
    }

    public void validateIds(UpdateAllCategoriesRequest request) {
        List<Long> allIds = Stream.concat(
                request.updateCategories().stream().map(UpdateCategoryRequest::id),
                request.deleteCategoryIds().stream()
        ).toList();

        if (hasDuplicates(allIds)) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "요청에 중복된 id가 존재합니다.");
        }
    }

    public void validateCategoriesCount(Member member, UpdateAllCategoriesRequest request) {
        if (request.updateCategories().size() + request.createCategories().size()
                != categoryRepository.countByMember(member) - 1) {
            throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리의 개수가 일치하지 않습니다.");
        }
    }

    private boolean hasDuplicates(List<?> items) {
        return items.size() != new HashSet<>(items).size();
    }

    private boolean isSequential(List<Long> ordinals) {
        return ordinals.stream()
                .distinct()
                .sorted()
                .collect(Collectors.toList())
                .equals(ordinals);
    }
}

다른 분들 의견도 궁금하네용 동의한다면 따봉을, 다른 의견이 있다면 코멘트 부탁해요~!

Copy link
Contributor

Choose a reason for hiding this comment

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

네이밍을 단순하게 CategoryValidator로 지어도 좋을 것 같긴 한데, XXXRepository 들의 의존성을 가지고 있기도 하네요.
CategoryValidationService도 좋을 것 같기도 하구요~~

Copy link
Contributor

Choose a reason for hiding this comment

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

음 validateDuplicateNameRequest, validateOrdinal, validateOrdinals, validateIds, validateCategoriesCount 등 레포지토리를 호출하지 않는 검증들은 일급 컬렉션을 활용해서 도메인의 책임으로 주는 건 어떨까 싶은데............. 어떻게 분리하면 좋을지 고민되네요..........

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 몰리 의견대로 도메인의 책임으로 주는 것이 좋다고 생각했습니다.
repository를 사용하지 않으면서 dto를 검증하는 로직은 커스텀 어노테이션을 통해 컨트롤러 계층에서 검증되도록 변경하였습니다.
도메인에서 진행할 수 있는 검증은 도메인 쪽으로 분리하였고, 나머지는 그대로 두었습니다.

더 좋은 의견 있으면 공유해주세요!!

Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
package codezap.category.service;

import java.util.HashSet;
import java.util.List;
import java.util.stream.IntStream;
import java.util.stream.Stream;

import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import codezap.category.domain.Category;
import codezap.category.dto.request.CreateCategoryRequest;
import codezap.category.dto.request.UpdateAllCategoriesRequest;
import codezap.category.dto.request.UpdateCategoryRequest;
import codezap.category.dto.response.CreateCategoryResponse;
import codezap.category.dto.response.FindAllCategoriesResponse;
Expand All @@ -24,13 +30,17 @@ public class CategoryService {
private final TemplateRepository templateRepository;

@Transactional
public CreateCategoryResponse create(Member member, CreateCategoryRequest createCategoryRequest) {
String categoryName = createCategoryRequest.name();
validateDuplicatedCategory(categoryName, member);
Category category = categoryRepository.save(new Category(categoryName, member));
public CreateCategoryResponse create(Member member, CreateCategoryRequest request) {
validateDuplicatedCategory(request.name(), member);
validateOrdinal(member, request);
Category category = categoryRepository.save(createCategory(member, request));
return CreateCategoryResponse.from(category);
}

private Category createCategory(Member member, CreateCategoryRequest request) {
return new Category(request.name(), member, request.ordinal());
}

public FindAllCategoriesResponse findAllByMemberId(Long memberId) {
return FindAllCategoriesResponse.from(categoryRepository.findAllByMemberIdOrderById(memberId));
}
Expand All @@ -44,11 +54,43 @@ public Category fetchById(Long id) {
}

@Transactional
public void update(Member member, Long id, UpdateCategoryRequest updateCategoryRequest) {
validateDuplicatedCategory(updateCategoryRequest.name(), member);
Category category = categoryRepository.fetchById(id);
public void updateCategories(Member member, UpdateAllCategoriesRequest request) {
validateDuplicateNameRequest(request);
validateOrdinals(request);
validateIds(request);

createCategories(member, request);
for (UpdateCategoryRequest updateCategory : request.updateCategories()) {
update(member, updateCategory);
}
for (Long deleteCategoryId : request.deleteCategoryIds()) {
delete(member, deleteCategoryId);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

�List의 forEach 쓰면 어떨까요~? 한줄로 가독성이 좋아질 것 같아요!

validateCategoriesCount(member, request);
}

private void createCategories(Member member, UpdateAllCategoriesRequest request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

전달 인자를 UpdateAllCategoriesRequest가 아닌 List 로 넘기면 더 좋을 것 같아요. 사용하는 데이터는 CreateCategoryRequest 뿐이니까요

categoryRepository.saveAll(
request.createCategories().stream()
.map(createRequest -> createCategory(member, createRequest))
.toList()
);
}

private void update(Member member, UpdateCategoryRequest request) {
Category category = categoryRepository.fetchById(request.id());
category.validateAuthorization(member);
category.updateName(updateCategoryRequest.name());
validateDefaultCategory(category);
category.update(request.name(), request.ordinal());
}

private void delete(Member member, Long categoryId) {
Category category = categoryRepository.fetchById(categoryId);
category.validateAuthorization(member);

validateDefaultCategory(category);
validateHasTemplate(categoryId);
categoryRepository.deleteById(categoryId);
}

private void validateDuplicatedCategory(String categoryName, Member member) {
Expand All @@ -57,17 +99,66 @@ private void validateDuplicatedCategory(String categoryName, Member member) {
}
}

@Transactional
public void deleteById(Member member, Long id) {
Category category = categoryRepository.fetchById(id);
category.validateAuthorization(member);
private void validateDefaultCategory(Category category) {
if (category.isDefault()) {
throw new CodeZapException(ErrorCode.DEFAULT_CATEGORY, "기본 카테고리는 수정 및 삭제할 수 없습니다.");
}
}

private void validateHasTemplate(Long id) {
if (templateRepository.existsByCategoryId(id)) {
throw new CodeZapException(ErrorCode.CATEGORY_HAS_TEMPLATES, "템플릿이 존재하는 카테고리는 삭제할 수 없습니다.");
}
if (category.isDefault()) {
throw new CodeZapException(ErrorCode.DEFAULT_CATEGORY, "기본 카테고리는 삭제할 수 없습니다.");
}

private void validateDuplicateNameRequest(UpdateAllCategoriesRequest request) {
List<String> allNames = Stream.concat(
request.createCategories().stream().map(CreateCategoryRequest::name),
request.updateCategories().stream().map(UpdateCategoryRequest::name)
).toList();

if (allNames.size() != new HashSet<>(allNames).size()) {
throw new CodeZapException(ErrorCode.INVALID_REQUEST, "요청에 중복된 카테고리 이름이 존재합니다.");
Copy link
Contributor

Choose a reason for hiding this comment

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

swagger에는 "이름이 %s 인 카테고리가 이미 존재합니다."로 에러메시지로 되어있네요~ 둘 중 하나를 변경해 통일성 있게 해주세요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이게 좀 고민이 많은데 둘은 다른 에러입니다.

첫 번째 에러는 카테고리 생성 시에 발생하는 에러인데, 이 경우는 1개의 카테고리에 대해서만 생성 요청을 보내므로 DB에서 이름 중복 검증을 진행합니다.

하지만 두 번째 에러는 카테고리 편집(생성, 수정, 삭제) 시에 발생하는 에러인데, 이 경우는 DB에서 검증하면 안되고 요청 안에서 중복된 이름이 있는지 확인해야 합니다. 왜냐면 통째로 덮어 씌우는 로직이기 때문입니다.

똑같이 가져가는게 나을까요? 다른 분들의 의견도 궁금해요~

추가로 문서에 이 경우에 대한 에러도 추가해주었습니다.

}
}

private void validateOrdinal(Member member, CreateCategoryRequest request) {
long ordinal = request.ordinal();
long count = categoryRepository.countByMember(member);
if (ordinal != count) {
throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리 순서가 잘못되었습니다.");
}
}
Comment on lines +96 to +102
Copy link
Contributor

@zeus6768 zeus6768 Dec 26, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-12-26 at 8 29 41 PM

로직이 잘못된 것 같아요. 기본 카테고리와 같은 ordinal을 갖는 카테고리가 생성 가능하네요.

ordinal + 1이 count와 일치해야 할 것 같아요~

해당 내용으로 테스트코드 추가하면 좋겠네용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

치명적이네요.
기본 카테고리인 카테고리 없음을 0으로 고정하기로 해서 로직은 맞습니다! 다만 기본 카테고리의 순서를 잘못 설정해놨었네요.
추가로 로직에 대한 테스트 코드는 작성되어 있습니다!
제우스 최고👍


private void validateOrdinals(UpdateAllCategoriesRequest request) {
List<Long> allOrdinals = Stream.concat(
request.createCategories().stream().map(CreateCategoryRequest::ordinal),
request.updateCategories().stream().map(UpdateCategoryRequest::ordinal)
).sorted().toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

제가 위에 말한대로 ValidatedSourceCodesOrdinalRequest을 사용한다면 해당 코드를 request에 extractSourceCodesOrdinal() 메서드로 두고 호출해서 사용할 수 있을 것 같아요~

그래서 dto에서 1차 검증으로 fast fail의 장점을 잡고, 내부에서는 안정성을 위해 한 번 더 체크하는 장점을 잡을 때 하나의 메서드를 사용하면 추후 리펙터링 시에도 좋을 것 같아요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그래서 dto에서 1차 검증으로 fast fail의 장점을 잡고, 내부에서는 안정성을 위해 한 번 더 체크하는 장점을 잡을 때 하나의 메서드를 사용하면 추후 리펙터링 시에도 좋을 것 같아요~

이 부분이 잘 이해가 되지 않는데, 다시 설명해줄 수 있나요?

제가 이해한 바로는 위 코드를 dto에 두고 service에서도 해당 코드를 불러서 재사용하자는 것 같은데 맞나요??
만약 맞다면 똑같은 검증 로직을 dto와 service에서 두 번 실행하자는 얘기인가요?


boolean isSequential = IntStream.range(0, allOrdinals.size())
.allMatch(i -> allOrdinals.get(i) == i + 1);

if (!isSequential) {
throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리 순서가 연속적이지 않습니다.");
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 에러 메시지를 카테고리 순서가 잘못되었습니다.로 해서 순서에 대한 예외처리 메시지를 동일하게 해주면 좋을 것 같아요.
그렇지 않다면 해당 예외 메시지도 swagger에 추가해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

켬미가 얘기한 SourceCodeOrdinalValidator를 재사용하자는 피드백을 반영하면서 해당 검증 로직은 삭제하였습니다.

}
}

private void validateIds(UpdateAllCategoriesRequest request) {
List<Long> allIds = Stream.concat(
request.updateCategories().stream().map(UpdateCategoryRequest::id),
request.deleteCategoryIds().stream()
).toList();

if (allIds.size() != new HashSet<>(allIds).size()) {
throw new CodeZapException(ErrorCode.INVALID_REQUEST, "요청에 중복된 id가 존재합니다.");
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 예외도 swagger에 추가해주세요~

}
}

private void validateCategoriesCount(Member member, UpdateAllCategoriesRequest request) {
if (request.updateCategories().size() + request.createCategories().size()
!= categoryRepository.countByMember(member) - 1) {
throw new CodeZapException(ErrorCode.INVALID_REQUEST, "카테고리의 개수가 일치하지 않습니다.");
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 예외도 swagger에 추가해주세요~

Copy link
Contributor

Choose a reason for hiding this comment

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

+) 카테고리 전체 (수정되지 않는 카테고리)도 보내주어야 한다는 게 전달되면 좋을 것 같네요!
그렇지 않으면 여기서 예외가 많이 터질 것 같슴다

}
categoryRepository.deleteById(id);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE category ADD COLUMN ordinal BIGINT NOT NULL;
Copy link
Contributor

@zeus6768 zeus6768 Dec 26, 2024

Choose a reason for hiding this comment

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

(member_id, ordinal) 합성 필드에 unique 조건 걸어야 하지 않을까요?!

Suggested change
ALTER TABLE category ADD COLUMN ordinal BIGINT NOT NULL;
ALTER TABLE category ADD COLUMN ordinal BIGINT NOT NULL;
ALTER TABLE category ADD CONSTRAINT member_ordinal_unique UNIQUE (member_id, ordinal);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

추가를 했었는데 오류가 발생해서 삭제했습니다. 추가로 기존에 걸려있던 (member_id, name)에 대한 unique 조건도 삭제하였습니다.

발생한 오류는 다음과 같습니다.

현재 저희는 생성, 수정, 삭제를 한 트랜잭션 안에서 동시에 수행합니다.
그렇다보니 데이터를 삭제했다고 하더라도 실제로 데이터베이스에는 해당 데이터가 삭제되지 않은 상태입니다. 그 상태에서 생성 쿼리를 실행하다보니 유니크 제약 조건에 위배되게 됩니다.
즉 아래의 경우에서 에러가 발생하게 됩니다.

  • name1, name2 라는 카테고리가 존재함
  • name1 카테고리 삭제, 동시에 name1 이라는 카테고리 생성
  • name1이라는 카테고리가 아직 삭제되지 않았기 때문에 생성할 수 없음. (유니크 제약 조건 위배)

이 문제를 어떻게 해겷해야 할 지 잘 모르겠네요.
유니크 제약 조건이 걸려있긴 해야 할 거 같은데 현재 저희 로직에서는 flush를 강제로 진행하는 것 말고는 방법이 떠오르지 않네요..
다른 분들 생각은 어떤가요?

flush를 해서라도 제약 조거는 거는게 좋을까요?
아니면 다른 좋은 방법이 있을까요..

Loading
Loading