Conversation
- seller의 일부 컬럼 store로 이동 - 임시 Oauth2Seller 테이블 삭제 - Seller/Seller/Controller에 존재하던 (판매자) 등록 API를 Store/Seller/Controller의 (판매자) 가게 등록 API로 이동 - API 이동에 의해 facade, service, repository 리팩토링
- OAuth2ResponseCreateCommand를 기존의 SellerCreateCommand로 변경 - CustomAnnotaion @WithMockAuthenticationPrincipal을 추가하여 @WithMockUser로는 @AuthenticationPrincipal을 테스트할 수 없는 문제 해결
WalkthroughOAuth2Seller 엔티티를 제거하고 Seller로 통합했으며, 연락처·이메일·주소 관련 필드를 Seller에서 Store로 이동하는 도메인·리포지토리·서비스·컨트롤러 전반 리팩토링이 적용되었습니다. 신규 SellerStoreFacade 기반의 스토어 등록 흐름(S3 업로드, createStore, registerStore, 롤백), Flyway 마이그레이션 및 테스트 추가/수정이 포함됩니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as SellerStoreController
participant Facade as SellerStoreFacade
participant S3 as S3Service
participant SellerSvc as SellerService
participant StoreSvc as SellerStoreService
participant Repo as SellerRepository
participant DB as Database
Client->>Controller: POST /registerStore (multipart/form-data)
Controller->>Facade: registerStoreForSeller(sellerId, request, profileImage)
Facade->>SellerSvc: getSellerById(sellerId)
SellerSvc->>Repo: findById(sellerId)
Repo-->>SellerSvc: Seller
SellerSvc-->>Facade: Seller
Facade->>Facade: validate certificationStatus == NEW
alt not NEW
Facade-->>Controller: throw ALREADY_REGISTER_STORE
else
Facade->>S3: uploadFile(seller-images/{sellerId}, profileImage)
S3-->>Facade: profileImagePath
Facade->>StoreSvc: createStore(request, profileImagePath)
StoreSvc->>DB: save(store)
DB-->>StoreSvc: Store
StoreSvc-->>Facade: Store
Facade->>Facade: validate store.status == NONE
alt not NONE
Facade->>S3: delete(profileImagePath) [rollback]
Facade-->>Controller: throw ALREADY_RESERVED_STORE
else
Facade->>StoreSvc: registerStore(seller, store)
StoreSvc->>DB: persist association
DB-->>StoreSvc: OK
StoreSvc-->>Facade: OK
Facade-->>Controller: StoreRegisterResult
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- @nested를 사용하여 테스트할 메서드 및 의미 단위로 테스트 코드를 묶음 - SellerFixture 머지 충돌에 의해 임시로 메서드명을 변경
![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
🤖 Fix all issues with AI agents
In `@src/main/java/com/bbangle/bbangle/seller/domain/Seller.java`:
- Around line 88-92: registerStore currently allows null and overwrites an
existing store; add defensive checks at the start of the method in
Seller.registerStore(Store store): validate store != null and throw an
IllegalArgumentException (or a domain-specific exception) if null, and check if
this.store != null (or !this.store.equals(store) depending on desired semantics)
and throw an IllegalStateException to prevent duplicate/overwrite; only after
those checks assign this.store = store, call
store.changeStatus(StoreStatus.RESERVED) and set certificationStatus =
CertificationStatus.PENDING so existing state is preserved and NPEs / unintended
overwrites are avoided.
- Around line 34-42: The current unique constraint is only on providerId causing
cross-provider collisions; update the Seller entity (class Seller) to declare a
composite unique constraint on (provider, providerId) instead of marking
providerId alone unique (remove unique=true on providerId and add a
`@Table`(uniqueConstraints = `@UniqueConstraint`(columnNames =
{"provider","provider_id"})) at the class level) so provider and providerId are
enforced as a pair, and update the Flyway migration V28__app.sql to drop the old
single-column unique index on provider_id and create a new composite unique
index/constraint on (provider, provider_id) to match the entity change.
In
`@src/main/java/com/bbangle/bbangle/seller/seller/service/command/SellerCreateCommand.java`:
- Around line 36-38: SellerService.createOAuth2Seller() currently uses
SellerCreateCommand.resolvedName() which can return null if both name and
nickname are null; add a defensive null check in createOAuth2Seller() to
validate the result of SellerCreateCommand.resolvedName() and handle it (e.g.,
throw an IllegalArgumentException or supply a sensible default) before
proceeding; update any null-unsafe usages in createOAuth2Seller() (references to
resolvedName()) so the method never assumes a non-null seller name and add a
clear error message mentioning the missing name/nickname.
In `@src/main/java/com/bbangle/bbangle/store/domain/Store.java`:
- Around line 34-35: DEFAULT_IDENTIFIER is currently declared as a static final
and computed once at class load, causing every Store to share the same
identifier; change it so a new identifier is generated per instance by removing
the static final constant and generating the value on construction (or via an
instance getter like getDefaultIdentifier()) using the same UUID-based
expression ((Math.abs(UUID.randomUUID().getLeastSignificantBits()) % 90000) +
10000) converted to String; also update the other occurrence referenced (lines
105-108) to the same instance-level generation approach so each Store gets a
unique identifier.
In
`@src/main/java/com/bbangle/bbangle/store/seller/controller/dto/StoreRequest.java`:
- Around line 23-31: The validation message for phoneNumber and subPhoneNumber
conflicts with the Pattern; update either the regex or the message on the
`@Pattern` annotations for fields phoneNumber and subPhoneNumber so they match:
either change the regex from ^[0-9]{11}$ to allow up to 11 digits (e.g.
^[0-9]{1,11}$) if the message should permit "11자리 이하", or keep the exact-11
regex and change the message to state exactly 11 digits (e.g. "연락처는 11자리 숫자만 입력
가능합니다."); ensure both fields use the same consistent annotation change.
In
`@src/main/java/com/bbangle/bbangle/store/seller/controller/SellerStoreController.java`:
- Around line 37-47: 메서드 registryStore의 profileImage 파라미터를 선택사항으로 변경하세요:
SellerStoreController에서 method registryStore의 `@RequestPart`("profileImage")
MultipartFile profileImage를 `@RequestPart`(value = "profileImage", required =
false) MultipartFile profileImage로 변경해 null 허용하도록 하고, 컨트롤러에서 null 전달 시
SellerStoreFacade.registerStoreForSeller(sellerId, request, profileImage)가 그대로
호출되도록 유지하세요; 또한 OpenAPI/Swagger 스펙(해당 컨트롤러의 API 문서 어노테이션)이 파일 파트가 optional임을
반영하도록 업데이트하세요 (StoreCreateRequest의 profile 필드 기반 URL 처리와 일치하도록).
In
`@src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java`:
- Around line 52-58: The method registerStoreForSeller() has a TOCTOU race
between createStore() and registerStore()—wrap the whole flow in a transaction
by adding `@Transactional` to registerStoreForSeller() so createStore(), the Store
status check (Store.getStatus()/StoreStatus.NONE) and
sellerStoreService.registerStore(...) execute in one transaction; alternatively,
if you prefer entity-level locking, add optimistic locking (`@Version` field) to
the Store entity or apply a pessimistic lock (e.g.,
`@Lock`(LockModeType.PESSIMISTIC_WRITE)) when loading the Store to prevent
concurrent registration.
In
`@src/main/java/com/bbangle/bbangle/store/seller/service/SellerStoreService.java`:
- Around line 32-43: The current newProfile assignment uses request.profile()
when profileImagePath is null, which can overwrite the existing profile with
null/empty; change the logic in SellerStoreService so newProfile is
profileImagePath if non-null, otherwise use request.profile() only if it's
non-null/non-blank, and as a final fallback use the store's current profile
(e.g., call store.getProfile() or equivalent) before calling
store.updateDetail(...); update the newProfile computation and then pass that
variable into store.updateDetail(newProfile, request.shortDescription(),
request.phoneNumber(), request.subPhoneNumber(), request.email(),
request.originAddress(), request.originAddressDetail()) to preserve the existing
profile when client sends no profile.
In
`@src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java`:
- Around line 334-336: The `@DisplayName` on the nested test class is misleading:
change the annotation on the Fail_CreateStoreTest nested class so it reflects
failure cases (e.g., "스토어 생성 및 조회에 실패한다.") to match the class name and the tests
it contains; update the `@DisplayName` value decorating the Fail_CreateStoreTest
class accordingly to keep test names consistent and clear.
🧹 Nitpick comments (11)
src/main/java/com/bbangle/bbangle/seller/seller/controller/SellerController.java (1)
47-59: TODO 주석은 이슈/티켓 링크로 추적하세요.v3에서 API 이동 예정이라면, TODO 주석만 두기보다 이슈/티켓 번호를 함께 남겨 추적 가능하게 해 주세요.
필요하면 티켓 템플릿이나 체크리스트 초안 만들어 드릴까요?
src/test/java/com/bbangle/bbangle/common/client/annotation/WithMockAuthenticationPrincipal.java (1)
66-68: @target과@Documented를추가하여 테스트 어노테이션의 적용 범위를 명시하세요.Spring Security의
@WithMockUser등 표준 테스트 어노테이션들은@Target({METHOD, TYPE}),@Documented메타 어노테이션을 적용합니다. 이를 통해 테스트 메서드뿐만 아니라 테스트 클래스 수준에서도 사용할 수 있으며, IDE 지원과 문서화가 개선됩니다.제안 변경
import com.bbangle.bbangle.common.client.factory.WithMockUserSecurityContextFactory; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; import org.springframework.security.test.context.support.WithSecurityContext; /** * {`@code` `@AuthenticationPrincipal`}을 사용하는 Controller 테스트를 위한 커스텀 Mock 인증 어노테이션.-@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.METHOD, ElementType.TYPE}) +@Retention(RetentionPolicy.RUNTIME) +@Documented `@WithSecurityContext`(factory = WithMockUserSecurityContextFactory.class) public `@interface` WithMockAuthenticationPrincipal {src/test/java/com/bbangle/bbangle/common/client/factory/WithMockUserSecurityContextFactory.java (1)
18-23:role값에 대한 정규화 로직을 추가하여 접두사 중복을 방지해 주세요.annotation의 기본값(
"USER")과 문서에서 "ROLE_ 접두사는 자동으로 처리된다"고 명시하고 있지만, 누군가 실수로role="ROLE_USER"형태로 넘기면ROLE_ROLE_USER가 되어 권한 매칭이 깨질 수 있습니다. 다음과 같이 정규화하는 것이 안전합니다:String role = annotation.role(); String normalizedRole = role.startsWith("ROLE_") ? role : "ROLE_" + role; Authentication authentication = new UsernamePasswordAuthenticationToken( annotation.userId(), annotation.credentials(), List.of(new SimpleGrantedAuthority(normalizedRole)) );src/main/java/com/bbangle/bbangle/store/domain/model/EmailVO.java (1)
21-22: 사소한 포매팅 이슈가 있습니다.
- Line 21:
columnDefinition = "VARCHAR(100)"앞에 공백이 두 개 있습니다.- Line 22:
private String email에 공백이 두 개 있습니다.🔧 제안된 수정
- `@Column`(name = "email", columnDefinition = "VARCHAR(100)") - private String email; + `@Column`(name = "email", columnDefinition = "VARCHAR(100)") + private String email;src/test/java/com/bbangle/bbangle/fixture/store/domain/StoreFixture.java (1)
10-21: 테스트 픽스처가 새로운 Store 생성 방식에 맞게 업데이트되었습니다.
Store.createForSeller()팩토리 메서드를 사용하여 리팩토링된 도메인 모델과 일치합니다. 다만, 8개의 위치 기반 파라미터는 가독성과 유지보수성 측면에서 개선의 여지가 있습니다.향후 리팩토링 시 빌더 패턴이나 DTO를 활용하면 파라미터 순서 실수를 방지하고 가독성을 높일 수 있습니다. 현재 TODO 주석에서 언급한 대로 나중에 개선해도 됩니다.
src/main/java/com/bbangle/bbangle/seller/seller/service/command/SellerCreateCommand.java (1)
14-25: Record에서 중복된@Builder생성자입니다.Java record는 이미 canonical constructor를 자동 생성합니다.
@Builder를 record 선언부에 직접 적용하면 별도의 생성자 정의 없이도 빌더를 사용할 수 있습니다.♻️ 간소화된 코드 제안
+@Builder public record SellerCreateCommand( String name, String nickname, OauthServerType provider, String providerId ) { - - `@Builder` - public SellerCreateCommand( - String name, - String nickname, - OauthServerType provider, - String providerId - ) { - this.name = name; - this.nickname = nickname; - this.provider = provider; - this.providerId = providerId; - } public static SellerCreateCommand from(OAuth2Response response) {src/test/java/com/bbangle/bbangle/seller/domain/SellerUnitTest.java (1)
11-12: 불필요한 MockitoExtension 사용테스트에서 Mock 객체를 사용하지 않으므로
@ExtendWith(MockitoExtension.class)를 제거할 수 있습니다.♻️ 제안된 수정
-import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.junit.jupiter.MockitoExtension; `@DisplayName`("[단위 테스트] Seller") -@ExtendWith(MockitoExtension.class) public class SellerUnitTest {src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java (1)
120-125: Thread.sleep 사용은 테스트 안정성에 영향을 줄 수 있습니다.
Thread.sleep(1)은 타임스탬프 차이를 보장하기 위해 사용된 것으로 보이지만, 이는 테스트를 느리게 하고 CI 환경에서 불안정할 수 있습니다. 가능하다면 테스트 데이터에 명시적인 순서를 부여하거나, 커서 기반 정렬 기준을 ID로 변경하는 것을 고려해 주세요.src/test/java/com/bbangle/bbangle/fixture/seller/domain/SellerFixture.java (1)
8-17: 유틸리티 클래스에 private 생성자 추가 권장Fixture 클래스는 인스턴스화할 필요가 없으므로 private 생성자를 추가하는 것이 좋습니다.
StoreFixture에서도 이 패턴이 사용되고 있습니다.♻️ 제안된 수정
public class SellerFixture { + private SellerFixture() { + } + // dev 브랜치에 defaultSeller를 사용하고 있어서 임시로 설정하였습니다. public static Seller defaultSellerMergeConflict() {src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java (1)
65-72: 예외 재발생 시 원본 스택 트레이스 손실
throw new BbangleException(e.getBbangleErrorCode())는 원본 예외의 스택 트레이스를 잃습니다. 디버깅을 위해 원인(cause)을 포함하는 것이 좋습니다.♻️ 제안된 수정
- throw new BbangleException(e.getBbangleErrorCode()); + throw e; // 원본 BbangleException을 그대로 재발생또는 BbangleException이 cause를 지원한다면:
throw new BbangleException(e.getBbangleErrorCode(), e);src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeUnitTest.java (1)
57-164: 테스트 케이스 보완 권장테스트 커버리지가 좋지만, 몇 가지 엣지 케이스가 누락되어 있습니다:
APPROVED 상태 판매자 거부 테스트:
fail_registerStoreForSeller_already_registered_seller는 PENDING 상태만 테스트합니다. 파사드에서 APPROVED도 거부하므로 이에 대한 테스트가 필요합니다.INVALID_PROFILE 예외 테스트:
profileImage와request.profile()둘 다 null/blank인 경우INVALID_PROFILE예외가 발생해야 합니다.♻️ 추가 테스트 제안
`@Test` `@DisplayName`("이미 승인된 판매자는 예외가 발생한다.") void fail_registerStoreForSeller_approved_seller() { // given Seller seller = mock(Seller.class); given(sellerService.getSellerById(1L)).willReturn(seller); given(seller.getCertificationStatus()).willReturn(CertificationStatus.APPROVED); // when & then Assertions.assertThatThrownBy(() -> sellerStoreFacade.registerStoreForSeller( 1L, mock(StoreRequest.StoreCreateRequest.class), multipartFile )) .isInstanceOf(BbangleException.class) .satisfies(e -> { BbangleException ex = (BbangleException) e; Assertions.assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.ALREADY_REGISTER_STORE); }); } `@Test` `@DisplayName`("프로필 이미지와 프로필 경로가 모두 없으면 예외가 발생한다.") void fail_registerStoreForSeller_without_profile() { // given StoreRequest.StoreCreateRequest request = mock(StoreRequest.StoreCreateRequest.class); given(request.profile()).willReturn(null); // when & then Assertions.assertThatThrownBy(() -> sellerStoreFacade.registerStoreForSeller( 1L, request, null )) .isInstanceOf(BbangleException.class) .satisfies(e -> { BbangleException ex = (BbangleException) e; Assertions.assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.INVALID_PROFILE); }); }
src/main/java/com/bbangle/bbangle/seller/seller/service/command/SellerCreateCommand.java
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/store/seller/controller/SellerStoreController.java
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/store/seller/service/SellerStoreService.java
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/main/java/com/bbangle/bbangle/exception/BbangleErrorCode.java`:
- Around line 95-109: You changed numeric values for error constants in the
BbangleErrorCode enum (e.g., INVALID_CERTIFICATION_STATUS,
SELLER_CREATION_FAILED, …, ALREADY_REGISTER_STORE); because these codes are an
API contract, either revert to the original numeric values so external clients
and monitoring are unaffected, or if renumbering is intentional add a clear
migration: bump the API version/release, add a documented mapping (old->new) and
a helper mapping method in BbangleErrorCode to translate legacy codes, and
update client docs, changelog and monitoring alerts accordingly before merging.
In `@src/main/java/com/bbangle/bbangle/seller/repository/SellerRepository.java`:
- Line 15: The repository method findByProviderAndProviderId relies on the
uniqueness of the (provider, providerId) pair but the DB currently has
provider_id UNIQUE only; update the schema to enforce a composite unique
constraint for (provider, provider_id). Fix by either adding a DB migration
altering the seller table to add UNIQUE(provider, provider_id) in V28__app.sql
(or a new migration) or annotate the Seller entity with `@Table`(uniqueConstraints
= `@UniqueConstraint`(columnNames = {"provider", "provider_id"})) so the JPA
mapping and the findByProviderAndProviderId lookup are consistent.
In
`@src/main/java/com/bbangle/bbangle/store/seller/controller/swagger/SellerStoreApi.java`:
- Around line 36-47: Update the Swagger response annotation in SellerStoreApi so
the documented schema and example match the actual return type
SingleResult<StoreRegisterResult>: change the `@Schema` from CommonResult.class to
SingleResult.class (or a proper container type that represents
SingleResult<StoreRegisterResult>) and update the `@ExampleObject` value to
include the full payload with "success", "code", "message" and a "data" object
containing "sellerId" and a "store" object (matching StoreRegisterResult
fields). Edit the annotation block (the `@ApiResponse/`@ExampleObject around the
method that returns SingleResult<StoreRegisterResult>) to reflect these fields
and ensure keys/types mirror StoreRegisterResult's properties.
- Around line 58-62: The method name registryStore is a typo — rename the
interface method to registerStore and update all usages and implementations
accordingly: change the declaration
SingleResult<StoreResponse.StoreRegisterResult> registryStore(...) to
registerStore(...) in SellerStoreApi, then update any implementing class methods
(e.g., SellerStoreController or classes implementing SellerStoreApi), unit
tests, Swagger/OpenAPI references, and any callers to use registerStore; also
search and update related JavaDoc/comments and method references to ensure
compilation and API docs regenerate correctly.
🧹 Nitpick comments (11)
src/main/java/com/bbangle/bbangle/seller/seller/controller/SellerController.java (1)
47-58: TODO는 이슈/티켓 링크로 추적되도록 정리해주세요.
v3 이관 계획이 명확해지도록 TODO 옆에 이슈 번호나 일정 링크를 붙이면 방치 위험을 줄일 수 있습니다.src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java (2)
44-45: 조건문에 중괄호 사용을 권장합니다.복합 조건문은 가독성과 유지보수를 위해 중괄호를 사용하는 것이 좋습니다.
♻️ 제안된 수정
- if (seller.getCertificationStatus() == CertificationStatus.APPROVED || seller.getCertificationStatus() == CertificationStatus.PENDING) - throw new BbangleException(BbangleErrorCode.ALREADY_REGISTER_STORE); + if (seller.getCertificationStatus() == CertificationStatus.APPROVED + || seller.getCertificationStatus() == CertificationStatus.PENDING) { + throw new BbangleException(BbangleErrorCode.ALREADY_REGISTER_STORE); + }
65-72: 로그 레벨 일관성 검토가 필요합니다.Line 68에서는
log.warn()을, Line 78에서는log.error()를 사용하고 있습니다.BbangleException도 비즈니스 로직 실패를 의미하므로, 동일한 로그 레벨 사용을 고려해주세요.src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeUnitTest.java (1)
3-4: 중복 import 정리가 필요합니다.Line 3-4에서
assertThat과assertThatThrownBy를 static import하고 있지만, Line 26에서org.assertj.core.api.Assertions를 다시 import하여Assertions.assertThatThrownBy()형태로 사용하고 있습니다(Lines 105, 133, 158). 일관성을 위해 하나의 방식으로 통일하세요.Also applies to: 26-26
src/test/java/com/bbangle/bbangle/fixture/seller/domain/SellerFixture.java (1)
10-17: 임시 메서드명 정리가 필요합니다.주석에서 언급된 대로, dev 브랜치 병합 충돌을 피하기 위한 임시 이름입니다. 병합 후
defaultSeller()로 이름을 변경하거나, 기존 fixture와 통합하는 작업이 필요합니다.이 작업을 추적하기 위한 이슈를 생성해드릴까요?
src/test/java/com/bbangle/bbangle/fixture/store/domain/StoreFixture.java (1)
13-41: 중복 코드 제거를 권장합니다.
defaultStore()와defaultStore(StoreStatus status)메서드에서 builder 로직이 중복됩니다. 하나의 메서드가 다른 메서드를 호출하도록 리팩토링하세요.♻️ 제안된 수정
public static Store defaultStore() { - return Store.builder() - .name("빵그리의 오븐") - .profile("test.png") - .introduce("비건 베이커리") - .phoneNumberVO( - PhoneNumberVO.of("01012345678", "01098765432") - ) - .emailVO(EmailVO.of("123@test.com")) - .originAddressLine("서울") - .originAddressDetail("123동") - .status(StoreStatus.NONE) - .build(); + return defaultStore(StoreStatus.NONE); } public static Store defaultStore(StoreStatus status) { return Store.builder() .name("빵그리의 오븐") .profile("test.png") .introduce("비건 베이커리") .phoneNumberVO( PhoneNumberVO.of("01012345678", "01098765432") ) .emailVO(EmailVO.of("123@test.com")) .originAddressLine("서울") .originAddressDetail("123동") .status(status) .build(); }src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeIntegrationTest.java (2)
98-98: Store 조회 방식이 취약합니다.
storeRepository.findAll().get(0)은 테스트 데이터가 여러 개일 경우 실패할 수 있습니다.result에서 반환된storeId를 사용하여 조회하는 것이 더 안전합니다.♻️ 제안된 수정 (Line 98 예시)
- Store store = storeRepository.findAll().get(0); + Store store = storeRepository.findById(result.store().storeId()).orElseThrow();Also applies to: 139-139
183-185: Fixture 메서드를 활용하세요.
StoreFixture.defaultStore()로 생성 후changeStatus()를 호출하는 대신, 이미 정의된defaultStore(StoreStatus status)메서드를 사용하세요.♻️ 제안된 수정
- Store store = StoreFixture.defaultStore(); - store.changeStatus(StoreStatus.RESERVED); - Store newStore = storeRepository.saveAndFlush(store); + Store store = StoreFixture.defaultStore(StoreStatus.RESERVED); + Store newStore = storeRepository.saveAndFlush(store);src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java (2)
113-125: Thread.sleep 기반 시퀀싱은 테스트 플래키 위험이 있습니다.
ID 정렬이나 createdAt 명시 설정 등 결정적 방법으로 순서를 보장하는 쪽이 더 안정적입니다.
237-259: 기존 스토어 조회 케이스의 profile 값은 실제 계약과 맞추는 게 좋습니다.
이미지 파일이 없을 때 기존 프로필 경로 유지 시나리오를 명확히 하도록 request.profile을 store.getProfile()로 맞추는 것을 권장합니다.Based on learnings, In the bbangle store seller service (Java/Spring), when updating store details via StoreRequest.StoreCreateRequest, the request.profile() field contains the existing profile path from the database and cannot be directly modified by the client. The profile path only changes when a new image file is uploaded, which populates the profileImagePath parameter. Therefore, the pattern `profileImagePath == null ? request.profile() : profileImagePath` correctly preserves the existing profile.✅ 제안 수정
- StoreRequest.StoreCreateRequest request = new StoreRequest.StoreCreateRequest( - "store", - "newProfile.png", + StoreRequest.StoreCreateRequest request = new StoreRequest.StoreCreateRequest( + "store", + store.getProfile(), "newIntroduce", "01011112222", "01099998888", "temp@test.com", "서울", "123동", store.getId() );src/main/java/com/bbangle/bbangle/auth/seller/facade/OAuth2SellerFacade.java (1)
27-39: DataIntegrityViolationException 처리 범위를 좁혀야 합니다.현재 코드는 모든 데이터 무결성 위반(FK, NOT NULL, CHECK, UNIQUE 등)을 동일하게 처리하므로 UNIQUE 충돌이 아닌 다른 제약 위반이 가려질 수 있습니다. UNIQUE 위반만 재조회하도록 제약을 확인 후 처리하세요.
권장 방법:
- Hibernate의
ConstraintViolationException사용:getConstraintName()또는getSQLState()로 제약 유형 확인
- PostgreSQL: SQLState
23505= UNIQUE 위반- MySQL: SQLState
23000(다른 위반과 구분 필요)- 대안: 데이터베이스 기본 upsert 사용 (PostgreSQL의
INSERT ... ON CONFLICT)로 예외 기반 제어 흐름 제거
src/main/java/com/bbangle/bbangle/seller/repository/SellerRepository.java
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/store/seller/controller/swagger/SellerStoreApi.java
Outdated
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/store/seller/controller/swagger/SellerStoreApi.java
Outdated
Show resolved
Hide resolved
* [docs]: erd.json 추가(erd editor) * [docs]: comment 추가, pk -> fk -> 일반필드 정렬
- StoreRequest.StoreCreateRequest DTO 필드명 수정 - StoreFixture 수정
- seller의 일부 컬럼 store로 이동 - 임시 Oauth2Seller 테이블 삭제 - Seller/Seller/Controller에 존재하던 (판매자) 등록 API를 Store/Seller/Controller의 (판매자) 가게 등록 API로 이동 - API 이동에 의해 facade, service, repository 리팩토링
- OAuth2ResponseCreateCommand를 기존의 SellerCreateCommand로 변경 - CustomAnnotaion @WithMockAuthenticationPrincipal을 추가하여 @WithMockUser로는 @AuthenticationPrincipal을 테스트할 수 없는 문제 해결
- @nested를 사용하여 테스트할 메서드 및 의미 단위로 테스트 코드를 묶음 - SellerFixture 머지 충돌에 의해 임시로 메서드명을 변경
- StoreRequest.StoreCreateRequest DTO 필드명 수정 - StoreFixture 수정
…efactoring' into refactoring/seller-store-table-refactoring # Conflicts: # src/test/java/com/bbangle/bbangle/fixture/seller/domain/SellerFixture.java # src/test/java/com/bbangle/bbangle/seller/seller/service/SellerServiceIntegrationTest.java # src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeIntegrationTest.java # src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java
mono0801
changed the title
- flyway 파일 추가 - erd.json 설정 변경 - sellers 테이블의 memberId 삭제 및 연관 관계 제거 - Swagger 수정
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@src/main/java/com/bbangle/bbangle/seller/domain/Seller.java`:
- Around line 48-49: Seller declares a duplicate boolean field isDeleted mapped
to "is_deleted" which conflicts with the inherited field from
SoftDeleteCreatedAtBaseEntity; remove the isDeleted declaration in the Seller
class (the two lines with `@Column`(name = "is_deleted", columnDefinition =
"tinyint") private boolean isDeleted;) so the entity uses the inherited field
from SoftDeleteCreatedAtBaseEntity, and clean up any now-unused imports or
explicit getters/setters in Seller that duplicate the parent implementations or
reference the removed field.
In `@src/main/java/com/bbangle/bbangle/store/domain/Store.java`:
- Around line 119-134: updateDetail에서 PhoneNumberVO.of 또는 EmailVO.of 호출 중 예외가
발생하면 profile/introduce 등이 부분적으로 반영되는 문제가 있어, 먼저 모든 유효성/VO 생성 작업을 수행한 뒤 한 번에 필드에
대입하도록 수정하세요: updateDetail 메서드 내부에서 PhoneNumberVO.of(phone, subPhone)와
EmailVO.of(email)을 로컬 변수(phoneVO, emailVO)로 먼저 생성/검증하고, 모든 생성이 성공하면
this.profile, this.introduce, this.phoneNumberVO, this.emailVO,
this.originAddressLine, this.originAddressDetail 순으로 한 번에 대입하도록 변경하세요.
In
`@src/main/java/com/bbangle/bbangle/store/seller/controller/dto/StoreRequest.java`:
- Around line 23-31: The phone number validation in StoreRequest (fields
phoneNumber and subPhoneNumber) allows 9–11 digits but PhoneNumberVO only
permits exactly 11 digits; update the DTO to match the VO by changing the
`@Pattern` regexp on both phoneNumber and subPhoneNumber to require exactly 11
digits (e.g. "^[0-9]{11}$") and adjust the validation messages accordingly so
DTO and PhoneNumberVO are consistent.
In `@src/main/resources/flyway/V34__app.sql`:
- Around line 3-7: Migration defines sub_phone as varchar(20) which mismatches
the VO PhoneNumberVO.subPhoneNumber (VARCHAR(100)); update the migration to make
the sub_phone column use varchar(100) to match the VO (and prevent
truncation/schema drift), verify the referenced columns phone/sub_phone in
V34__app.sql and any other migrations/DDL that touch PhoneNumberVO fields are
consistent, and re-run schema validation/migrations to ensure no drift.
- Around line 64-65: 현재 ALTER TABLE sellers ADD CONSTRAINT uk_seller_provider_id
UNIQUE (provider_id);는 다른 OAuth 제공자 간에 동일한 provider_id가 충돌할 수 있으니, 제약조건을
provider와 provider_id의 복합 유니크로 변경하세요: 찾아볼 식별자는 ALTER TABLE sellers와 제약명
uk_seller_provider_id 및 컬럼 provider_id이며, 이를 복합 유니크 제약(예: UNIQUE(provider,
provider_id))으로 수정하거나 기존 단독 유니크를 제거한 뒤 새로운 복합 유니크 제약을 추가하도록 변경합니다.
- Around line 55-61: The migration adds NOT NULL columns provider and
provider_id to the sellers table without defaults which will fail if sellers
contains rows; update the V34__app.sql migration (ALTER TABLE sellers) to either
provide safe DEFAULT values for provider and provider_id (e.g., DEFAULT '' or a
sentinel like 'UNKNOWN'), make those columns nullable temporarily, or add a
prior backfill step that populates existing rows before changing them to NOT
NULL; reference the ALTER TABLE statement and the provider/provider_id column
names when implementing the chosen fix so the migration will succeed on
non-empty tables.
In `@src/test/java/com/bbangle/bbangle/fixture/seller/domain/SellerFixture.java`:
- Around line 15-20: defaultSeller() in SellerFixture uses a fixed providerId
which can violate the provider_id UNIQUE constraint across tests; update
SellerFixture.defaultSeller to pass a unique providerId to Seller.create (e.g.,
generate a UUID or append a timestamp) so each test instance has a distinct
providerId, and ensure any other helper methods that create sellers follow the
same unique-id pattern (reference: SellerFixture.defaultSeller and
Seller.create).
In
`@src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeIntegrationTest.java`:
- Around line 68-103: The test currently relies on
storeRepository.findAll().get(0) which is order-dependent; instead use the
storeId returned from sellerStoreFacade.registerStoreForSeller
(StoreRegisterResult.result.storeId()) to load the created Store
deterministically. Replace the lookup using storeRepository.findAll().get(0)
with storeRepository.findById(result.storeId()).orElseThrow() and update
assertions to use that loaded Store; keep other checks (updatedSeller,
certification status, store status) unchanged.
🧹 Nitpick comments (3)
src/main/java/com/bbangle/bbangle/seller/seller/controller/SellerController.java (2)
47-47: TODO는 이슈로 추적하는 편이 안전합니다.릴리즈 이후 잔존 위험이 있으니 이 TODO를 이슈/티켓으로 연결해 추적하는 것을 권장합니다. 필요하면 v3 이전에 사용할 임시 라우팅/디프리케이션 플랜 정리도 도와드릴게요.
58-58: TODO 관리 방식 정리 추천.해당 TODO도 이슈/티켓으로 분리해 관리하면 잔존 리스크를 줄일 수 있습니다. 원하시면 v3 이전 임시 정책(예:
@Deprecated표기, 문서 주석) 정리안을 제안드릴게요.src/test/java/com/bbangle/bbangle/fixture/store/domain/StoreFixture.java (1)
36-36: TODO는 이슈로 분리해 추적하는 편이 좋습니다.
리팩토링 범위가 정해지면 이슈로 분리해 관리해 주세요. 필요하면 정리 도와드릴게요.
src/main/java/com/bbangle/bbangle/store/seller/controller/dto/StoreRequest.java
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/fixture/seller/domain/SellerFixture.java
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeIntegrationTest.java
Show resolved
Hide resolved
- Seller 엔티티의 중복된 is_deleted 컬럼 제거 - Store 업데이트 시 Email, Phone 검증 후 업데이트하도록 수정 - PhoneNumberVO 정규표현식 수정 - SellerFixture에서 Seller 객체 생성 시 providerId를 UUID로 생성하도록 변경
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/com/bbangle/bbangle/seller/seller/facade/SellerFacade.java (1)
46-51:⚠️ Potential issue | 🟡 MinorS3 업로드 성공 후 DB 저장 실패 시 트랜잭션 경계 확인 필요.
@Transactional덕분에 DB는 롤백되고, catch 블록에서 S3 파일도 삭제하는 구조는 적절합니다. 다만doc.file().getOriginalFilename()(Line 49)은null을 반환할 수 있으므로, 파일명이 필수 값이라면 사전 검증을 고려해 주세요.src/main/java/com/bbangle/bbangle/exception/BbangleErrorCode.java (1)
128-130:⚠️ Potential issue | 🟡 Minor
INVALID_ADMIN_ID코드 번호 순서 불일치
ADMIN_NOT_FOUND(-741)이INVALID_ADMIN_ID(-740)위에 선언되어 있어 코드 번호 순서가 역전됩니다. AUTH 섹션 주석은(741~760)이지만-740이 포함되어 있습니다.🔧 순서 수정 제안
// AUTH (740~ 760) + INVALID_ADMIN_ID(-740, "유효하지 않은 관리자 아이디 입니다.", BAD_REQUEST), ADMIN_NOT_FOUND(-741, "존재하지 않는 관리자입니다.", NOT_FOUND), - INVALID_ADMIN_ID(-740, "유효하지 않은 관리자 아이디 입니다.", BAD_REQUEST), ADMIN_INVALID_PASSWORD(-742, "비밀번호가 일치하지 않습니다.", BAD_REQUEST),src/test/java/com/bbangle/bbangle/auth/seller/facade/OAuth2SellerFacadeIntegrationTest.java (1)
132-176:⚠️ Potential issue | 🟠 Major
@Transactional과 동시성 테스트의 격리 문제
@Transactional어노테이션은 테스트 메서드의 스레드에만 적용됩니다.ExecutorService로 생성된 스레드들은 별도의 트랜잭션에서 실행되므로, 이 스레드들이 생성한 Seller 데이터는 테스트 종료 시 롤백되지 않습니다. 이로 인해 후속 테스트에서 데이터 오염이 발생할 수 있습니다.🔧 해결 방안
@AfterEach에서 수동으로 정리하거나, 이 테스트 메서드에@Transactional을 비활성화하고@AfterEach를 활용하는 방식을 고려해 주세요:`@AfterEach` void tearDown() { sellerRepository.deleteAll(); em.flush(); }
🤖 Fix all issues with AI agents
In `@src/main/java/com/bbangle/bbangle/seller/seller/service/SellerService.java`:
- Around line 48-59: createOAuth2Seller currently assigns name =
command.resolvedName() without validating it, allowing a null name to be
persisted; update createOAuth2Seller to validate the resolvedName() result (from
SellerCreateCommand) before calling Seller.create and sellerRepository.save: if
resolvedName() is null or empty, either throw a clear runtime exception (e.g.,
IllegalArgumentException with context) or substitute a safe default, and ensure
the exception message references the command/provider/providerId for debugging;
keep references to createOAuth2Seller, SellerCreateCommand.resolvedName,
Seller.create, and sellerRepository.save so reviewers can find the change.
In `@src/main/java/com/bbangle/bbangle/store/domain/Store.java`:
- Around line 115-117: The generateIdentifier method can produce a negative
value when Math.abs is given Long.MIN_VALUE; update generateIdentifier to avoid
Math.abs and use a non-negative modulo such as
Math.floorMod(UUID.randomUUID().getLeastSignificantBits(), 90000) (or otherwise
handle Long.MIN_VALUE) so the remainder is always non-negative before adding
10000, ensuring a 5-digit identifier; change the expression in
generateIdentifier that currently uses
Math.abs(UUID.randomUUID().getLeastSignificantBits()) % 90000 to use a safe
non-negative modulo approach.
In
`@src/main/java/com/bbangle/bbangle/store/seller/controller/dto/StoreRequest.java`:
- Around line 13-43: Remove the leftover review-note comments "// 주석 반영" from
the annotation lines in the StoreRequest DTO: delete the inline comments on the
fields storeName (Size annotation), phoneNumber (Pattern annotation),
subPhoneNumber (Pattern annotation) and the Size annotation for the detailed
address so only the validation annotations and messages remain; ensure no other
code or annotations are altered and run formatter/compile to confirm no trailing
comment tokens remain.
In
`@src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java`:
- Around line 74-83: The catch block in SellerStoreFacade currently logs only
the exception message via log.error(e.getMessage()), losing the stack trace;
update the catch to pass the exception object to the logger (e.g.,
log.error("Unexpected error while creating seller", e)) so full stack trace is
recorded, keep the existing S3 rollback call to
s3Service.deleteImage(profileImagePath) and still throw new
BbangleException(BbangleErrorCode.SELLER_CREATION_FAILED) afterwards.
In `@src/main/resources/flyway/V34__app.sql`:
- Line 60: The is_deleted column is added allowing NULLs which breaks
soft-delete queries; change the DDL to create it as NOT NULL DEFAULT 0 (use "ADD
COLUMN IF NOT EXISTS is_deleted TINYINT(1) NOT NULL DEFAULT 0"), and if the
column might already exist with NULL values, add a migration step to UPDATE the
table setting is_deleted = 0 for rows where is_deleted IS NULL before running an
ALTER TABLE ... MODIFY/CHANGE to enforce NOT NULL on the existing column (refer
to the is_deleted column and the migration script/V34__app.sql when
implementing).
In
`@src/test/java/com/bbangle/bbangle/auth/seller/facade/OAuth2SellerFacadeIntegrationTest.java`:
- Line 168: 테스트 주석의 오타를 수정하세요: 현재 assertion 라인 주석 "DB에는 판매자 게정이 1개만 존재해야한다." 에서
"게정"을 "계정"으로 바꿔 문장을 올바르게 만드십시오; 수정 대상은 해당 라인의
assertThat(sellerRepository.count()).isEqualTo(1); 주석입니다.
In
`@src/test/java/com/bbangle/bbangle/auth/seller/facade/OAuth2SellerFacadeUnitTest.java`:
- Line 74: In OAuth2SellerFacadeUnitTest update the Korean comment typo "게정" to
"계정" in the two places referencing seller account creation: the inline comment
near the given(sellerService.createOAuth2Seller(command)).willReturn(seller)
statement and the verification comment that mentions seller account creation
(the comment verifying that createOAuth2Seller wasn't called); search for "게정"
within OAuth2SellerFacadeUnitTest and replace with "계정" to correct both
occurrences.
In `@src/test/java/com/bbangle/bbangle/fixture/store/domain/StoreFixture.java`:
- Around line 13-26: The fixture uses Store.builder() without setting the
identifier, causing test Stores to have identifier=null; update basebuilder to
call and set the generated identifier before build (e.g., add
.identifier(Store.generateIdentifier()) to the builder chain) so the Store
instances created by basebuilder match the production behavior of
Store.createForSeller() which generates an identifier.
🧹 Nitpick comments (19)
src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceUnitTest.java (2)
69-92:checkStoreName_storeExists_available테스트 —Store.createForSeller로 생성된 Store의 기본 status가NONE이라는 암묵적 가정에 의존합니다.이 테스트는
Store.createForSeller가 기본적으로StoreStatus.NONE상태의 Store를 생성한다고 가정하고 있습니다. 만약createForSeller의 기본 status가 변경되면 이 테스트가 의도와 다르게 동작할 수 있습니다. 명시적으로 status를 검증하거나 주석을 추가하면 의도가 더 명확해집니다.
94-112:checkStoreName_storeExists_Unavailable— 메서드명이 다른 테스트와 네이밍 컨벤션이 다릅니다.다른 테스트 메서드는
_available,_storeNotExists처럼 소문자를 사용하는데, 이 메서드만_Unavailable로 대문자로 시작합니다. 통일하면 가독성이 좋아집니다.제안
- void checkStoreName_storeExists_Unavailable() { + void checkStoreName_storeExists_unavailable() {src/main/java/com/bbangle/bbangle/seller/seller/facade/SellerFacade.java (1)
66-73: S3 롤백 중 예외 발생 시 나머지 파일이 정리되지 않을 수 있습니다.
s3Service.deleteImage()가 예외를 던지면forEach가 중단되어 나머지 업로드된 파일들이 S3에 남게 됩니다. 각 삭제 호출을 개별적으로 try-catch로 감싸는 것을 권장합니다.♻️ 제안된 수정
// 5. 롤백: 업로드된 모든 파일 삭제 uploadedDocuments.forEach(doc -> { - log.error("Document 생성 실패로 인한 S3 문서 롤백: {}", doc.filePath()); - s3Service.deleteImage(doc.filePath()); + try { + log.error("Document 생성 실패로 인한 S3 문서 롤백: {}", doc.filePath()); + s3Service.deleteImage(doc.filePath()); + } catch (Exception rollbackEx) { + log.error("S3 롤백 실패 - 수동 정리 필요: {}", doc.filePath(), rollbackEx); + } });src/test/java/com/bbangle/bbangle/seller/seller/service/SellerDocumentServiceIntegrationTest.java (1)
56-65:Store.createForSeller의null파라미터 의미가 불명확합니다.8개의 위치 파라미터 중 3번째에
null이 전달되고 있어, 코드만으로는 어떤 필드가null인지 파악하기 어렵습니다. 테스트 가독성을 위해 인라인 주석을 추가하거나, 향후 테스트 픽스처/빌더 패턴 도입 시 명시적으로 표현하는 것을 권장합니다.예시
Store store = Store.createForSeller( "테스트 스토어", "https://example.com/profile.jpg", - null, + null, // shortDescription "01087654321", "01087654321", "test@example.com", "서울특별시 강남구 테헤란로 123", "456호" );src/main/java/com/bbangle/bbangle/seller/seller/controller/SellerController.java (1)
47-67: TODO 항목이 명확하게 표시되어 있습니다.
updateSeller와updateStoreName이 v3에서 Store 컨트롤러로 이동 예정임을 나타내는 TODO가 적절합니다. 이 작업을 추적하기 위한 이슈가 이미 생성되어 있는지 확인해 주세요.해당 TODO 항목을 추적하기 위한 GitHub 이슈를 생성해 드릴까요?
src/main/java/com/bbangle/bbangle/store/domain/Store.java (1)
34-35:boards필드에private접근 제한자가 누락되었습니다.다른 필드들은 모두
private으로 선언되어 있지만,boards만 package-private입니다.@Getter를 통해 접근을 제공하고 있으므로 필드 자체는private으로 선언하는 것이 일관성 있습니다.제안 수정안
`@OneToMany`(mappedBy = "store", fetch = FetchType.LAZY) - List<Board> boards = new ArrayList<>(); + private List<Board> boards = new ArrayList<>();src/test/java/com/bbangle/bbangle/fixture/store/domain/StoreFixture.java (1)
13-13: 메서드 이름basebuilder→baseBuilder로 camelCase 적용 권장.Java 네이밍 컨벤션에 따라 메서드 이름은 camelCase를 사용합니다.
src/main/java/com/bbangle/bbangle/seller/seller/service/command/SellerCreateCommand.java (1)
14-25: Record에@Builder적용 시 불필요한 생성자 중복Java record는 이미 canonical constructor를 자동 생성합니다.
@Builder를 record 선언부에 직접 적용하면 별도 생성자 없이도 빌더 패턴을 사용할 수 있어 보일러플레이트를 줄일 수 있습니다.♻️ 제안 수정안
+@Builder public record SellerCreateCommand( String name, String nickname, OauthServerType provider, String providerId ) { - - `@Builder` - public SellerCreateCommand( - String name, - String nickname, - OauthServerType provider, - String providerId - ) { - this.name = name; - this.nickname = nickname; - this.provider = provider; - this.providerId = providerId; - }src/main/java/com/bbangle/bbangle/seller/seller/service/SellerService.java (1)
39-46: 읽기 전용 메서드에@Transactional(readOnly = true)적용을 권장합니다.
getSellerById와findByProviderAndProviderId는 데이터를 조회만 하므로,@Transactional(readOnly = true)를 명시하면 JPA flush 모드 최적화와 의도 명확화에 도움이 됩니다.♻️ 제안 수정안
+ `@Transactional`(readOnly = true) public Seller getSellerById(Long sellerId) { return sellerRepository.findById(sellerId) .orElseThrow(() -> new BbangleException(BbangleErrorCode.SELLER_NOT_FOUND)); } + `@Transactional`(readOnly = true) public Optional<Seller> findByProviderAndProviderId(OauthServerType provider, String providerId) { return sellerRepository.findByProviderAndProviderId(provider, providerId); }src/test/java/com/bbangle/bbangle/seller/seller/service/SellerServiceIntegrationTest.java (1)
105-131:sellerRepository.findAll().get(0)사용 시 주의
@Transactional덕분에 현재는 안전하지만,findAll().get(0)은 테스트 간 데이터 격리가 깨지면 의도치 않은 엔티티를 조회할 수 있습니다.findByProviderAndProviderId를 사용하면 더 명시적입니다.♻️ 리팩토링 제안
// then - Seller savedSeller = sellerRepository.findAll().get(0); + Seller savedSeller = sellerRepository.findByProviderAndProviderId( + command.provider(), command.providerId() + ).orElseThrow();src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeUnitTest.java (1)
3-4:AssertionsForClassTypes대신Assertions사용 권장이 파일에서는
org.assertj.core.api.AssertionsForClassTypes를 import하고 있지만, 같은 PR의 다른 테스트 파일들(SellerServiceIntegrationTest,SellerUnitTest등)은org.assertj.core.api.Assertions를 사용합니다.AssertionsForClassTypes는 컬렉션 관련 assertion을 지원하지 않으며, 프로젝트 내 일관성을 위해Assertions로 통일하는 것이 좋습니다.♻️ import 통일 제안
-import static org.assertj.core.api.AssertionsForClassTypes.assertThat; -import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy;src/test/java/com/bbangle/bbangle/store/domain/StoreUnitTest.java (1)
73-75: 불필요한 주석 정리Lines 74-75와 134에
// arrange주석이// given또는// act & assert와 중복됩니다.♻️ 중복 주석 제거
void fail_create_store_with_invalid_sub_phone(String invalidPhone) { - // arrange // act & assertvoid fail_update_store_with_invalid_sub_phone(String invalidPhone) { // given Store store = createDefaultStore(); - // arrange // act & assertAlso applies to: 133-135
src/test/java/com/bbangle/bbangle/store/seller/controller/SellerStoreControllerTest.java (1)
129-132:store가null인 응답으로 테스트하면 실제 등록 결과 검증이 부족합니다.성공 케이스에서
store(null)로 설정하면 실제 응답 구조(스토어 상세 필드)가 올바르게 직렬화되는지 검증하지 못합니다.SellerStoreDetail을 포함한 응답을 반환하도록 설정하고,$.result.store.*필드도 검증하는 것이 좋습니다.src/main/java/com/bbangle/bbangle/store/seller/controller/swagger/SellerStoreApi.java (1)
25-39:registerStore에@ApiResponses어노테이션이 누락되었습니다.다른 엔드포인트(
checkStoreNameDuplicate)에는 에러 응답 문서가 있지만,registerStore에는 없습니다. 400(유효성 검사 실패), 409(이미 등록된 스토어) 등의 에러 응답을 문서화하면 API 소비자에게 도움이 됩니다.src/test/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacadeIntegrationTest.java (1)
156-173:mock(StoreCreateRequest.class)사용 시 주의가 필요합니다.통합 테스트에서
mock()을 사용하면 실제 객체와 다르게 동작할 수 있습니다. 현재는 조기 실패(인증 상태 검증)로 request 필드에 접근하지 않아 동작하지만, facade 내부 로직 순서가 변경되면NullPointerException이 발생할 수 있습니다. 실제StoreCreateRequest인스턴스를 사용하는 것이 더 안전합니다.src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java (2)
46-61:registerStore테스트에서 엔티티가 영속화되지 않습니다.
SellerFixture.defaultSeller()와StoreFixture.defaultStore()로 생성한 엔티티가 DB에 저장되지 않은 상태에서sellerStoreService.registerStore()를 호출합니다.@Transactional서비스 메서드 내에서 비영속 엔티티의 필드만 변경하므로 실질적으로 인메모리 유닛 테스트와 동일합니다.통합 테스트 목적이라면
em.persist()또는 repository를 통해 저장 후 테스트하는 것이 더 적합합니다.
120-125:Thread.sleep(1)은 정렬 순서 보장에 취약합니다.생성 시간 기반 정렬을 위해
Thread.sleep(1)을 사용하고 있지만, 시스템 타이머 해상도에 따라 동일 밀리초로 기록될 수 있습니다. ID 기반 정렬(auto-increment)이 가능하다면 sleep 없이도 순서가 보장됩니다.src/main/java/com/bbangle/bbangle/store/seller/controller/dto/StoreRequest.java (1)
20-21:introduce필드에 길이 제한이 없습니다.
storeName과originAddressDetail에는@Size가 적용되어 있지만,introduce(한줄소개)에는 길이 제한이 없습니다. DB 컬럼 길이에 따라 예기치 않은 truncation이나 에러가 발생할 수 있으므로@Size(max = ...)추가를 권장합니다.src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java (1)
66-84: S3 이미지 롤백 로직이 두 catch 블록에서 중복됩니다.
BbangleException과 일반Exceptioncatch 블록에 동일한 S3 삭제 로직이 반복됩니다.finally블록이나 헬퍼 메서드로 추출하면 유지보수성이 향상됩니다.♻️ 헬퍼 메서드 추출 예시
+ private void rollbackUploadedImage(String profileImagePath) { + if (profileImagePath != null) { + log.warn("Seller 생성 실패로 인한 S3 이미지 롤백: {}", profileImagePath); + s3Service.deleteImage(profileImagePath); + } + }catch 블록에서:
} catch (BbangleException e) { - if (profileImagePath != null) { - log.warn("Seller 생성 실패로 인한 S3 이미지 롤백: {}", profileImagePath); - s3Service.deleteImage(profileImagePath); - } + rollbackUploadedImage(profileImagePath); throw e; } catch (Exception e) { - log.error(e.getMessage()); - if (profileImagePath != null) { - log.error("Seller 생성 실패로 인한 S3 이미지 롤백: {}", profileImagePath); - s3Service.deleteImage(profileImagePath); - } + log.error("스토어 등록 중 예상치 못한 오류 발생", e); + rollbackUploadedImage(profileImagePath); throw new BbangleException(BbangleErrorCode.SELLER_CREATION_FAILED); }
src/main/java/com/bbangle/bbangle/seller/seller/service/SellerService.java
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/store/seller/controller/dto/StoreRequest.java
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/auth/seller/facade/OAuth2SellerFacadeIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/auth/seller/facade/OAuth2SellerFacadeUnitTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/fixture/store/domain/StoreFixture.java
Outdated
Show resolved
Hide resolved
- BbangleErrorCode ADMIN code 번호 수정 - Store identifier 컬럼 오버플로우 문제 해결 - Store boards 연관관계에 private 접근제어자 추가 - V34_app.sql에서 Store의 is_deleted 컬럼을 NOT NULL로 수정
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/bbangle/bbangle/auth/seller/facade/OAuth2SellerFacadeIntegrationTest.java (1)
132-177:⚠️ Potential issue | 🟡 Minor동시성 테스트에서
@Transactional롤백이 스레드에 적용되지 않습니다.
@Transactional은 테스트 메서드의 메인 스레드에만 적용됩니다.ExecutorService로 생성된 스레드들은 별도의 트랜잭션에서 실행되므로 테스트 종료 시 자동 롤백되지 않습니다. 이로 인해 테스트 DB에 데이터가 잔류하여 다른 테스트에 영향을 줄 수 있습니다.
@AfterEach에서 수동 정리하거나, 이 테스트만@Transactional을 제외하고 별도로 데이터를 정리하는 방식을 고려해 주세요.
🤖 Fix all issues with AI agents
In
`@src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java`:
- Around line 66-84: In SellerStoreFacade's exception handlers (the catch for
BbangleException and the generic catch that throws new BbangleException(
BbangleErrorCode.SELLER_CREATION_FAILED )), protect calls to
s3Service.deleteImage(profileImagePath) by wrapping each deleteImage invocation
in its own try-catch so any exception from s3Service.deleteImage is caught and
logged (e.g., log.error with the deletion exception) but not rethrown, ensuring
the original exception (the caught BbangleException or the newly created
BbangleException) continues to propagate unchanged.
🧹 Nitpick comments (7)
src/test/java/com/bbangle/bbangle/store/domain/StoreUnitTest.java (3)
60-88:null입력에 대한 검증 테스트 누락 여부를 확인하세요.
@ValueSource는null을 지원하지 않습니다.phone,subPhone,null이 전달될 때의 동작도 검증이 필요하다면@NullSource를 추가하거나 별도 테스트를 작성하는 것을 권장합니다. 만약subPhone이 선택 필드라면null허용 케이스도 테스트하면 좋겠습니다.
90-108:updateDetail후name이 변경되지 않았는지 검증 추가를 고려하세요.
updateDetail메서드가name을 파라미터로 받지 않으므로, 호출 후에도store.getName()이 원래 값(name)과 동일한지 확인하는 assertion을 추가하면 의도치 않은 사이드 이펙트를 방지할 수 있습니다.제안
// then + assertThat(store.getName()).isEqualTo(name); assertThat(store.getProfile()).isEqualTo(newProfile);
15-15: 테스트 클래스의 접근 제어자를 package-private으로 변경할 수 있습니다.JUnit 5에서는 테스트 클래스에
public접근 제어자가 필요하지 않습니다. 다른 테스트 파일(예:SellerStoreFacadeUnitTest)에서는 package-private을 사용하고 있으므로 일관성을 위해 맞추는 것을 고려해 보세요.제안
-public class StoreUnitTest { +class StoreUnitTest {src/main/resources/flyway/V34__app.sql (1)
50-52: 컬럼 삭제와 인덱스 삭제 순서가 역순입니다.Line 51에서
member_id컬럼을 먼저 삭제하면 MySQL이 해당 컬럼만 참조하는 인덱스를 자동으로 삭제합니다. 따라서 Line 52의DROP INDEX는 이미 존재하지 않는 인덱스에 대해 실행됩니다.IF EXISTS덕분에 실패하지는 않지만, 의도를 명확히 하려면 순서를 바꾸는 것이 좋습니다.제안 수정안
-ALTER TABLE sellers DROP COLUMN IF EXISTS member_id; DROP INDEX IF EXISTS member_id ON sellers; +ALTER TABLE sellers DROP COLUMN IF EXISTS member_id;src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java (2)
124-129:Thread.sleep(1)을 이용한 순서 보장은 불안정합니다.
Thread.sleep(1)은 OS 스케줄링에 따라 동일 타임스탬프가 나올 수 있어 테스트가 간헐적으로 실패할 수 있습니다. 생성 순서가 중요하다면 DB에 명시적 순서 컬럼을 두거나, 테스트에서 순서에 의존하지 않는 방식으로 검증하는 것이 더 안정적입니다.
360-366: Assertion 스타일이 혼재되어 있습니다.파일 상단(Line 4)에서
assertThat을 static import하고 있지만, 실패 케이스 테스트에서는Assertions.assertThatThrownBy와Assertions.assertThat을 직접 참조하고 있습니다. 일관성을 위해 static import로 통일하는 것이 좋습니다.♻️ 수정 제안
import부분 수정:import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy;테스트 코드 수정 (예:
storeId_notFound):- Assertions.assertThatThrownBy(() -> sellerStoreService.createStore(request, null) - ) + assertThatThrownBy(() -> sellerStoreService.createStore(request, null)) .isInstanceOf(BbangleException.class) .satisfies(e -> { BbangleException ex = (BbangleException) e; - Assertions.assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.STORE_NOT_FOUND); + assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.STORE_NOT_FOUND); });Also applies to: 387-393
src/test/java/com/bbangle/bbangle/seller/seller/service/SellerServiceIntegrationTest.java (1)
105-132:success_create_seller_withName과success_create_seller_withNickname간 조회 방식 불일치Line 123에서는
findByProviderAndProviderId를 사용하고, Line 152에서는findAll().get(0)을 사용합니다. 일관성을 위해 nickname 테스트에서도findByProviderAndProviderId를 사용하는 것이 좋습니다.findAll().get(0)은 테스트 격리가 깨질 경우 다른 Seller를 반환할 수 있습니다.♻️ 수정 제안
// then - Seller savedSeller = sellerRepository.findAll().get(0); + Seller savedSeller = sellerRepository.findByProviderAndProviderId(command.provider(), command.providerId()).orElseThrow();
src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java`:
- Around line 224-226: In SellerStoreServiceIntegrationTest update the
cursor-pagination assertion to ensure the next page excludes the cursor item:
replace the loose check using secondPage.getContent().allMatch(info -> info.id()
>= cursorId) with a strict greater-than check (info.id() > cursorId) so the
secondPage (from secondPage.getContent()) does not include the item identified
by cursorId; adjust any related assertions or variables in the test if necessary
to reflect that nextCursor represents the last-seen ID and should not be
included in subsequent pages.
- Around line 74-77: In SellerStoreServiceIntegrationTest.java replace the raw
native TRUNCATE/SET FOREIGN_KEY_CHECKS calls that use em.createNativeQuery(...)
with a transaction-safe delete: use the EntityManager (em) to run a JPQL bulk
delete like em.createQuery("DELETE FROM Store s").executeUpdate() (or annotate
the test with `@Sql` to prepare schema/data), remove MySQL-specific SET
FOREIGN_KEY_CHECKS calls, and keep em.clear() afterward; this preserves
`@Transactional` rollback and avoids DB-specific DDL/autocommit behavior.
🧹 Nitpick comments (4)
src/test/java/com/bbangle/bbangle/seller/seller/service/SellerServiceIntegrationTest.java (2)
84-91:OauthServerType.KAKAO하드코딩 대신seller.getProvider()사용 권장Line 85에서
OauthServerType.KAKAO를 직접 하드코딩하고 있습니다.SellerFixture.defaultSeller()의 provider 설정이 변경되면 이 테스트가 의도와 다르게 실패할 수 있습니다. fixture와의 결합도를 낮추기 위해seller.getProvider()를 사용하는 것이 좋습니다.♻️ 제안
- Optional<Seller> result = sellerService.findByProviderAndProviderId(OauthServerType.KAKAO, seller.getProviderId()); + Optional<Seller> result = sellerService.findByProviderAndProviderId(seller.getProvider(), seller.getProviderId());
105-132: OAuth2 판매자 생성 테스트:name과nickname이 모두null인 경우의 엣지 케이스 고려
withName과withNickname두 가지 경로가 잘 테스트되어 있습니다. 다만name과nickname이 모두null인 경우 어떤 동작을 기대하는지(예외 발생 등) 검증하는 테스트가 추가되면 방어적 커버리지가 강화될 수 있습니다. 필요 시 추후 추가를 고려해 주세요.Also applies to: 134-161
src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java (2)
4-5: Assertions import가 혼용되고 있습니다.
assertThat은 static import(Line 4)로,Assertions.assertThat은 full-qualified(Line 23, 사용처 Line 366/393)로 혼용 중입니다. 일관성을 위해 static import로 통일하는 것을 권장합니다.♻️ 수정 제안
-import org.assertj.core.api.Assertions;Line 366, 393:
- Assertions.assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.STORE_NOT_FOUND); + assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.STORE_NOT_FOUND);- Assertions.assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.INVALID_PROFILE); + assertThat(ex.getBbangleErrorCode()).isEqualTo(BbangleErrorCode.INVALID_PROFILE);Also applies to: 23-23
125-129:Thread.sleep(1)을 통한 정렬 보장은 불안정합니다.
Thread.sleep(1)로 생성 시간 차이를 만들어 정렬 순서를 보장하려는 의도로 보이나, 시스템 타이머 해상도에 따라 동일한 타임스탬프가 생성될 수 있어 테스트가 간헐적으로 실패할 수 있습니다. DB auto-increment ID 기반 정렬이라면 sleep이 불필요하고, 타임스탬프 기반이라면 명시적으로 타임스탬프를 설정하는 것이 더 안정적입니다.
src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/store/seller/service/SellerStoreServiceIntegrationTest.java
Show resolved
Hide resolved
|
수고하셨습니다! |
src/main/java/com/bbangle/bbangle/store/seller/facade/SellerStoreFacade.java
Outdated
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/common/client/annotation/WithMockAuthenticationPrincipal.java
Show resolved
Hide resolved
3 participants
Reviewer
1팀
History
🚀 Major Changes & Explanations
테이블 리팩토링
테이블 리팩토링에 따른 flyway 파일 추가
Seller에 존재하던 컬럼들(연락처, 추가 연락처, 이메일, 출고지 주소, 출고지 상세 주소, 프로필)을 Store로 이동
임시로 만든 OAuth2Seller를 삭제하고 해당 컬럼들(이름, provider, providerId, isDeleted)을 Seller로 이동
Seller 테이블에 존재하던 memberId 컬럼을 제거함으로써 memeber 테이블과의 연관 관계 삭제
코드 리팩토링
OAuth2Seller, Seller, Store와 연관된 Controller, Facade, Service, Repository, 테스트 코드 전부 리팩토링
Seller에 존재하던 일부 에러 코드를 Store로 이동
@AuthenticationPrincipal이 붙어있는 Controller를 테스트할 시@WithMockUser로는 제대로 된 테스트 시나리오 작성이 불가능하여@WithMockAuthenticationPrincipal커스텀 어노테이션을 생성테스트 코드가 많아지면서 가독성이 떨어져
@Nested를 사용해 테스트 코드를 테스트하고자 하는 메서드별로 분류📷 Test Image
💡 ETC
@WithMockAuthenticationPrincipal커스텀 어노테이션을 생성한 이유이 Controller를 테스트하기 위해
이렇게
Long, StoreCreateRequest, MultipartFile을 받으면response를 반환하도록 stubbing을 하였습니다.하지만
@WithMockUser로는@AuthenticationPrincipal Long sellerId여기에 데이터를 넣을 수 없는 문제가 발생합니다.따라서
null, StoreCreateRequest, MultipartFile을 입력받으므로given에서 설정한 stubbing 조건을 만족하지 못하여response가 아닌null을 반환하게 됩니다.이러한 문제를 해결하기 위해 커스텀 어노테이션을 따로 만들게 되었습니다.
Summary by CodeRabbit