Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
@@ -0,0 +1,58 @@
package com.example.solidconnection.admin.location.region.controller;

import com.example.solidconnection.admin.location.region.dto.AdminRegionCreateRequest;
import com.example.solidconnection.admin.location.region.dto.AdminRegionResponse;
import com.example.solidconnection.admin.location.region.dto.AdminRegionUpdateRequest;
import com.example.solidconnection.admin.location.region.service.AdminRegionService;
import jakarta.validation.Valid;
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
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;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

@RequiredArgsConstructor
@RequestMapping("/admin/regions")
@RestController
public class AdminRegionController {

private final AdminRegionService adminRegionService;

@GetMapping
public ResponseEntity<List<AdminRegionResponse>> getAllRegions() {
List<AdminRegionResponse> responses = adminRegionService.getAllRegions();
return ResponseEntity.ok(responses);
}

@PostMapping
public ResponseEntity<AdminRegionResponse> createRegion(
@Valid @RequestBody AdminRegionCreateRequest request
) {
AdminRegionResponse response = adminRegionService.createRegion(request);
return ResponseEntity.status(HttpStatus.CREATED).body(response);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

응답 코드를 지정하는 방향으로 작성해주었네요

여담으로 예전에 RESTful API 설계 원칙에 맞게 응답 코드를 지정하자고 얘기가 나왔던 것으로 기억하는데, 여유 생기면 한 번 진행해봐야겠네요

}

@PutMapping("/{code}")
public ResponseEntity<AdminRegionResponse> updateRegion(
@PathVariable String code,
@Valid @RequestBody AdminRegionUpdateRequest request
) {
AdminRegionResponse response = adminRegionService.updateRegion(code, request);
return ResponseEntity.ok(response);
}

@DeleteMapping("/{code}")
public ResponseEntity<Void> deleteRegion(
@PathVariable String code
) {
adminRegionService.deleteRegion(code);
return ResponseEntity.noContent().build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.example.solidconnection.admin.location.region.dto;

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

public record AdminRegionCreateRequest(
@NotBlank(message = "지역 코드는 필수입니다")
@Size(min = 1, max = 10, message = "지역 코드는 1자 이상 10자 이하여야 합니다")
String code,

@NotBlank(message = "한글 지역명은 필수입니다")
@Size(min = 1, max = 100, message = "한글 지역명은 1자 이상 100자 이하여야 합니다")
String koreanName
) {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.example.solidconnection.admin.location.region.dto;

import com.example.solidconnection.location.region.domain.Region;

public record AdminRegionResponse(
String code
) {

public static AdminRegionResponse from(Region region) {
return new AdminRegionResponse(
region.getCode()
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.example.solidconnection.admin.location.region.dto;

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

public record AdminRegionUpdateRequest(
@NotBlank(message = "한글 지역명은 필수입니다")
@Size(min = 1, max = 100, message = "한글 지역명은 1자 이상 100자 이하여야 합니다")
String koreanName
) {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package com.example.solidconnection.admin.location.region.service;

import com.example.solidconnection.admin.location.region.dto.AdminRegionCreateRequest;
import com.example.solidconnection.admin.location.region.dto.AdminRegionResponse;
import com.example.solidconnection.admin.location.region.dto.AdminRegionUpdateRequest;
import com.example.solidconnection.common.exception.CustomException;
import com.example.solidconnection.common.exception.ErrorCode;
import com.example.solidconnection.location.region.domain.Region;
import com.example.solidconnection.location.region.repository.RegionRepository;
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Service
@RequiredArgsConstructor
public class AdminRegionService {

private final RegionRepository regionRepository;

@Transactional(readOnly = true)
public List<AdminRegionResponse> getAllRegions() {
return regionRepository.findAll()
.stream()
.map(AdminRegionResponse::from)
.toList();
}

@Transactional
public AdminRegionResponse createRegion(AdminRegionCreateRequest request) {
regionRepository.findById(request.code())
.ifPresent(region -> {
throw new CustomException(ErrorCode.REGION_ALREADY_EXISTS);
});

regionRepository.findByKoreanName(request.koreanName())
.ifPresent(region -> {
throw new CustomException(ErrorCode.REGION_ALREADY_EXISTS);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

저희는 orElseThrow 로 예외 던졌는데 이 부분도 조금 다르네요
예외 처리를 따로 private 메서드로 분리하지 않는 것도 그렇구요 ..!


Region region = new Region(request.code(), request.koreanName());
Region savedRegion = regionRepository.save(region);

return AdminRegionResponse.from(savedRegion);
}
Comment on lines +29 to +52

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

중복 체크 로직에 경쟁 조건(race condition) 위험이 있습니다.

현재 구조의 문제점:

  1. 31-34줄: code 중복 체크
  2. 36-39줄: koreanName 중복 체크
  3. 41-42줄: Region 생성 및 저장

위 체크와 저장 사이에 다른 트랜잭션이 동일한 code나 koreanName을 가진 Region을 생성할 수 있습니다. 이는 동시성 환경에서 중복 데이터가 저장될 수 있는 위험을 초래합니다.

권장 해결 방안:

  1. Region 엔티티의 code와 koreanName 필드에 데이터베이스 레벨 unique 제약 조건 추가
  2. DataIntegrityViolationException을 catch하여 REGION_ALREADY_EXISTS 에러로 변환
 @Transactional
 public AdminRegionResponse createRegion(AdminRegionCreateRequest request) {
-    regionRepository.findById(request.code())
-            .ifPresent(region -> {
-                throw new CustomException(ErrorCode.REGION_ALREADY_EXISTS);
-            });
-
-    regionRepository.findByKoreanName(request.koreanName())
-            .ifPresent(region -> {
-                throw new CustomException(ErrorCode.REGION_ALREADY_EXISTS);
-            });
-
-    Region region = new Region(request.code(), request.koreanName());
-    Region savedRegion = regionRepository.save(region);
-
-    return AdminRegionResponse.from(savedRegion);
+    try {
+        Region region = new Region(request.code(), request.koreanName());
+        Region savedRegion = regionRepository.save(region);
+        return AdminRegionResponse.from(savedRegion);
+    } catch (DataIntegrityViolationException e) {
+        throw new CustomException(ErrorCode.REGION_ALREADY_EXISTS);
+    }
 }

이 방식은 데이터베이스의 ACID 속성을 활용하여 원자적으로 중복을 방지합니다.

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

🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/admin/location/region/service/AdminRegionService.java
around lines 29 to 45, the current pre-checks for duplicate code and koreanName
create a race condition before saving; instead add database-level UNIQUE
constraints for Region.code and Region.koreanName (update the JPA entity
annotations or DB migration/DDL), remove or keep but do not rely on the
in-memory findBy checks, and wrap the save call in a try/catch that catches
DataIntegrityViolationException (or the specific Spring/JPA persistence
exception thrown on unique violation) and rethrow new
CustomException(ErrorCode.REGION_ALREADY_EXISTS) so concurrent inserts are
handled atomically by the DB; keep the method @Transactional as-is.


@Transactional
public AdminRegionResponse updateRegion(String code, AdminRegionUpdateRequest request) {
Region region = regionRepository.findById(code)
.orElseThrow(() -> new CustomException(ErrorCode.REGION_NOT_FOUND));

regionRepository.findByKoreanName(request.koreanName())
.ifPresent(existingRegion -> {
if (!existingRegion.getCode().equals(code)) {
throw new CustomException(ErrorCode.REGION_ALREADY_EXISTS);
}
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

업데이트 시 중복 이름 체크도 경쟁 조건에 노출되어 있습니다.

createRegion과 동일한 문제로, 중복 체크와 저장 사이에 다른 트랜잭션이 동일한 koreanName을 사용할 수 있습니다. 데이터베이스 unique 제약 조건을 추가하고 예외 처리로 전환하는 것을 권장합니다.

🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/admin/location/region/service/AdminRegionService.java
around lines 52-57, the current in-memory check for duplicate koreanName during
update is racy; add a unique constraint on the korean_name column at the
DB/schema level and remove reliance on the pre-check as the sole guard, then
modify the service to catch the persistence constraint exception (e.g.,
DataIntegrityViolationException or ConstraintViolationException) thrown on save
and translate it into a CustomException(ErrorCode.REGION_ALREADY_EXISTS); ensure
the update/save is still performed inside the transactional context but rely on
DB uniqueness + exception handling rather than the non-atomic find-then-check
pattern.


Region updatedRegion = new Region(region.getCode(), request.koreanName());
Region savedRegion = regionRepository.save(updatedRegion);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

return AdminRegionResponse.from(savedRegion);
}

@Transactional
public void deleteRegion(String code) {
Region region = regionRepository.findById(code)
.orElseThrow(() -> new CustomException(ErrorCode.REGION_NOT_FOUND));

regionRepository.delete(region);
}
Comment thread
Gyuhyeok99 marked this conversation as resolved.
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ public enum ErrorCode {
APPLICATION_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "사용자의 대학 지원 정보를 찾을 수 없습니다."),
USER_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "회원을 찾을 수 없습니다."),
UNIVERSITY_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "대학교를 찾을 수 없습니다."),
REGION_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "지역을 찾을 수 없습니다."),
REGION_NOT_FOUND_BY_KOREAN_NAME(HttpStatus.NOT_FOUND.value(), "이름에 해당하는 지역을 찾을 수 없습니다."),
REGION_ALREADY_EXISTS(HttpStatus.CONFLICT.value(), "이미 존재하는 지역입니다."),
COUNTRY_NOT_FOUND_BY_KOREAN_NAME(HttpStatus.NOT_FOUND.value(), "이름에 해당하는 국가를 찾을 수 없습니다."),
GPA_SCORE_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "존재하지 않는 학점입니다."),
LANGUAGE_TEST_SCORE_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "존재하지 않는 어학성적입니다."),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import java.util.Optional;
import org.springframework.data.jpa.repository.JpaRepository;

public interface RegionRepository extends JpaRepository<Region, Long> {
public interface RegionRepository extends JpaRepository<Region, String> {

List<Region> findAllByKoreanNameIn(List<String> koreanNames);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
package com.example.solidconnection.admin.location.region.service;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;

import com.example.solidconnection.admin.location.region.dto.AdminRegionCreateRequest;
import com.example.solidconnection.admin.location.region.dto.AdminRegionResponse;
import com.example.solidconnection.admin.location.region.dto.AdminRegionUpdateRequest;
import com.example.solidconnection.common.exception.CustomException;
import com.example.solidconnection.common.exception.ErrorCode;
import com.example.solidconnection.location.region.domain.Region;
import com.example.solidconnection.location.region.fixture.RegionFixture;
import com.example.solidconnection.location.region.repository.RegionRepository;
import com.example.solidconnection.support.TestContainerSpringBootTest;
import java.util.List;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;

@TestContainerSpringBootTest
@DisplayName("지역 관련 관리자 서비스 테스트")
class AdminRegionServiceTest {

@Autowired
private AdminRegionService adminRegionService;

@Autowired
private RegionRepository regionRepository;

@Autowired
private RegionFixture regionFixture;

@Nested
class 전체_지역_조회 {

@Test
void 지역이_없으면_빈_목록을_반환한다() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

저희 서비스에서 테스트 코드에 '// given' 부분 내용이 존재하지 않다면 주석을 아예 빼버려도 되나요?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

다른 테스트 코드들 보니 안쓰고 있네요!

// when
List<AdminRegionResponse> responses = adminRegionService.getAllRegions();

// then
assertThat(responses).isEqualTo(List.of());
}

@Test
@DisplayName("저장된 모든 지역을 조회한다")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

테스트 메서드에 @DisplayName 어노테이션도 저희 암묵적으론 사용하지 않고 있고요

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

아직 손볼 곳들이 좀 많군요! 다른분들 확인하신 후에 고치려고 좀 납두긴 했습니다 ㅎㅎ.. 수정해놓겠습니다~

void 저장된_모든_지역을_조회한다() {
// given
Region region1 = regionFixture.영미권();
Region region2 = regionFixture.유럽();
Region region3 = regionFixture.아시아();

// when
List<AdminRegionResponse> responses = adminRegionService.getAllRegions();

// then
assertThat(responses)
.hasSize(3)
.extracting(AdminRegionResponse::code)
.containsExactlyInAnyOrder(
region1.getCode(),
region2.getCode(),
region3.getCode()
);
}
}

@Nested
class 지역_생성 {

@Test
void 유효한_정보로_지역을_생성하면_성공한다() {
// given
AdminRegionCreateRequest request = new AdminRegionCreateRequest("KR_SEOUL", "서울");

// when
AdminRegionResponse response = adminRegionService.createRegion(request);

// then
assertThat(response.code()).isEqualTo("KR_SEOUL");

// 데이터베이스에 저장되었는지 확인
Region savedRegion = regionRepository.findById(request.code()).orElseThrow();
assertThat(savedRegion.getKoreanName()).isEqualTo(request.koreanName());
}

@Test
void 이미_존재하는_코드로_지역을_생성하면_예외_응답을_반환한다() {
// given
regionFixture.영미권();

AdminRegionCreateRequest request = new AdminRegionCreateRequest("AMERICAS", "새로운 영미권");

// when & then
assertThatCode(() -> adminRegionService.createRegion(request))
.isInstanceOf(CustomException.class)
.hasMessage(ErrorCode.REGION_ALREADY_EXISTS.getMessage());
}

@Test
void 이미_존재하는_한글명으로_지역을_생성하면_예외_응답을_반환한다() {
// given
regionFixture.유럽();

AdminRegionCreateRequest request = new AdminRegionCreateRequest("NEW_CODE", "유럽");

// when & then
assertThatCode(() -> adminRegionService.createRegion(request))
.isInstanceOf(CustomException.class)
.hasMessage(ErrorCode.REGION_ALREADY_EXISTS.getMessage());
}
}

@Nested
class 지역_수정 {

@Test
void 유효한_정보로_지역을_수정하면_성공한다() {
// given
Region region = regionFixture.영미권();

AdminRegionUpdateRequest request = new AdminRegionUpdateRequest("미주");

// when
AdminRegionResponse response = adminRegionService.updateRegion(region.getCode(), request);

// then
assertThat(response.code()).isEqualTo(region.getCode());
Region updatedRegion = regionRepository.findById(region.getCode()).orElseThrow();
assertThat(updatedRegion.getKoreanName()).isEqualTo(request.koreanName());
}

@Test
void 존재하지_않는_지역_코드로_수정하면_예외_응답을_반환한다() {
// given
AdminRegionUpdateRequest request = new AdminRegionUpdateRequest("부산");

// when & then
assertThatCode(() -> adminRegionService.updateRegion("NOT_EXIST", request))
.isInstanceOf(CustomException.class)
.hasMessage(ErrorCode.REGION_NOT_FOUND.getMessage());
}

@Test
void 다른_지역의_한글명으로_수정하면_예외_응답을_반환한다() {
// given
Region region1 = regionFixture.영미권();
Region region2 = regionFixture.유럽();

AdminRegionUpdateRequest request = new AdminRegionUpdateRequest(region2.getKoreanName());

// when & then
assertThatCode(() -> adminRegionService.updateRegion(region1.getCode(), request))
.isInstanceOf(CustomException.class)
.hasMessage(ErrorCode.REGION_ALREADY_EXISTS.getMessage());
}

@Test
void 같은_지역의_한글명으로_수정하면_성공한다() {
// given
Region region = regionFixture.아시아();

AdminRegionUpdateRequest request = new AdminRegionUpdateRequest(region.getKoreanName());

// when
AdminRegionResponse response = adminRegionService.updateRegion(region.getCode(), request);

// then
assertThat(response.code()).isEqualTo(region.getCode());
}
}

@Nested
class 지역_삭제 {

@Test
void 존재하는_지역을_삭제하면_성공한다() {
// given
Region region = regionFixture.영미권();

// when
adminRegionService.deleteRegion(region.getCode());

// then
assertThat(regionRepository.findById(region.getCode())).isEmpty();
}

@Test
void 존재하지_않는_지역을_삭제하면_예외_응답을_반환한다() {
// when & then
assertThatCode(() -> adminRegionService.deleteRegion("NOT_EXIST"))
.isInstanceOf(CustomException.class)
.hasMessage(ErrorCode.REGION_NOT_FOUND.getMessage());
}
}
}
Loading