Conversation
Develop-KIM
added
the
refactoring
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
Walkthrough포트 기반 아키텍처에서 리포지토리 기반 아키텍처로 마이그레이션. CityCommandPort, CityQueryPort, CountryQueryPort 인터페이스 제거 후, 리포지토리 직접 사용 및 커맨드 패턴 도입으로 변경. 검색 관련 에러 코드 제거 및 도메인 모델 업데이트. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the admin location (country/city) read/write flow by removing the previous admin “required ports”, wiring admin services directly to domain repositories/ports, and extending city updates to include latitude/longitude updates.
Changes:
- Replace
CityQueryPort/CountryQueryPort/CityCommandPortusage with direct repository access (query) andCityManagementPortcommand dispatch (modify). - Extend city update command/entity behavior to update coordinates as well as names.
- Remove unused search-related
ErrorCodeentries and apply import/formatting cleanups in admin API tests/controllers.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/souzip/adapter/webapi/admin/AdminLocationApiTest.java | Import cleanups for REST Docs test. |
| src/main/java/com/souzip/global/exception/ErrorCode.java | Removes search-related error codes. |
| src/main/java/com/souzip/domain/country/application/query/CountryQueryService.java | Now implements CountryAdminPort (but currently has a compilation issue). |
| src/main/java/com/souzip/domain/city/entity/City.java | City update now also updates lat/long. |
| src/main/java/com/souzip/domain/city/application/command/UpdateCityCommand.java | Adds lat/long to update command. |
| src/main/java/com/souzip/domain/city/application/command/CityCommandService.java | Uses new city update API (but currently has formatting + null-safety concerns). |
| src/main/java/com/souzip/application/admin/required/CountryQueryPort.java | Deleted (admin required port removed). |
| src/main/java/com/souzip/application/admin/required/CityQueryPort.java | Deleted (admin required port removed). |
| src/main/java/com/souzip/application/admin/required/CityCommandPort.java | Deleted (admin required port removed). |
| src/main/java/com/souzip/application/admin/provided/AdminLocationModifier.java | Formatting-only change. |
| src/main/java/com/souzip/application/admin/provided/AdminLocationFinder.java | Formatting-only change. |
| src/main/java/com/souzip/application/admin/AdminLocationQueryService.java | Implements admin queries via CityRepository/CountryRepository. |
| src/main/java/com/souzip/application/admin/AdminLocationModifyService.java | Dispatches create/update/delete/priority via CityManagementPort + commands. |
| src/main/java/com/souzip/adapter/webapi/admin/AdminLocationApi.java | Minor whitespace cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void updateCity(UpdateCityCommand command) { | ||
| City city = findCityById(command.cityId()); | ||
| city.update( | ||
| command.nameEn(), | ||
| command.nameKr(), | ||
| BigDecimal.valueOf(command.latitude()), | ||
| BigDecimal.valueOf(command.longitude()) | ||
| ); |
There was a problem hiding this comment.
CityCommandService converts boxed Double coordinates via BigDecimal.valueOf(command.latitude()) / ...longitude(). If either value is null this will throw an NPE at runtime. Add a null-validation step (or switch the command fields to non-null types like BigDecimal / primitive double) before converting.
| package com.souzip.domain.city.application.command; | ||
|
|
||
| import com.souzip.domain.city.application.port.CityManagementPort; | ||
| import com.souzip.domain.city.entity.City; | ||
| import com.souzip.domain.city.event.CityCreatedEvent; | ||
| import com.souzip.domain.city.event.CityDeletedEvent; | ||
| import com.souzip.domain.city.event.CityPriorityUpdatedEvent; | ||
| import com.souzip.domain.city.repository.CityRepository; | ||
| import com.souzip.domain.city.service.CityPriorityDomainService; | ||
| import com.souzip.domain.country.entity.Country; | ||
| import com.souzip.domain.country.repository.CountryRepository; | ||
| import com.souzip.global.exception.BusinessException; | ||
| import com.souzip.global.exception.ErrorCode; | ||
| import java.math.BigDecimal; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.context.ApplicationEventPublisher; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.transaction.annotation.Transactional; | ||
| import com.souzip.domain.city.application.port.CityManagementPort; | ||
| import com.souzip.domain.city.entity.City; | ||
| import com.souzip.domain.city.event.CityCreatedEvent; |
There was a problem hiding this comment.
This file appears to have an extra indentation prefix on the package/import lines and the rest of the code (leading spaces at the start of each line). It’s inconsistent with the rest of the Java sources and tends to create noisy diffs and formatting issues in IDEs/linters. Please reformat so package starts at column 0 and indentation is standard.
| public Page<City> getCities(Long countryId, String keyword, Pageable pageable) { | ||
| return cityQueryPort.getCities(countryId, keyword, pageable); | ||
| if (keyword == null || keyword.isBlank()) { | ||
| return cityRepository.findByCountryIdWithPaging(countryId, pageable); | ||
| } | ||
| return cityRepository.searchByKeyword(countryId, keyword, pageable); | ||
| } |
There was a problem hiding this comment.
keyword is passed to cityRepository.searchByKeyword(...) without trimming, so inputs like " Seoul " will likely return no results. Note CityAdminQueryService trims the keyword before calling the same repository method. Consider applying keyword.trim() here as well to keep admin search behavior consistent.
| cityManagementPort.createCity(new CreateCityCommand( | ||
| request.nameEn(), | ||
| request.nameKr(), | ||
| request.coordinate().getLatitude().doubleValue(), | ||
| request.coordinate().getLongitude().doubleValue(), | ||
| request.countryId() | ||
| )); |
There was a problem hiding this comment.
Coordinates are converted BigDecimal -> double -> BigDecimal across the admin write path (Coordinate is BigDecimal, commands are Double, entity is BigDecimal). This can introduce precision loss and is unnecessary work. Prefer keeping coordinates as BigDecimal (or Coordinate) in the command layer so the value can be passed through without lossy conversion.
| cityManagementPort.updateCity(new UpdateCityCommand( | ||
| cityId, | ||
| request.nameEn(), | ||
| request.nameKr(), | ||
| request.coordinate().getLatitude().doubleValue(), | ||
| request.coordinate().getLongitude().doubleValue() | ||
| )); |
There was a problem hiding this comment.
Same issue as createCity(...): CityUpdateRequest coordinates are converted BigDecimal -> double -> BigDecimal, risking precision loss and adding avoidable conversions. Consider changing the command (and service) to accept BigDecimal/Coordinate so update passes validated values through directly.
| @Override | ||
| public List<CountryAdminResult> getCountries(String keyword) { | ||
| if (hasNoKeyword(keyword)) { | ||
| return toResults(countryRepository.findAllByOrderByNameKrAsc()); |
There was a problem hiding this comment.
CountryAdminResult is a nested type in CountryAdminPort, but it’s referenced here without an import or qualification. This won’t compile after changing the import to CountryAdminPort. Import CountryAdminPort.CountryAdminResult (as done in tests) or reference it as CountryAdminPort.CountryAdminResult in the method signature/return types.
| public void update(String nameEn, String nameKr, BigDecimal latitude, BigDecimal longitude) { | ||
| this.nameEn = nameEn; | ||
| this.nameKr = nameKr; | ||
| this.latitude = latitude; | ||
| this.longitude = longitude; | ||
| } |
There was a problem hiding this comment.
update(...) assigns latitude/longitude directly even though the columns are nullable = false. Since these parameters are nullable, this can lead to invalid entity state and a later persistence failure. Consider validating non-null (and ideally range, consistent with Coordinate) before assignment, or accept a validated value object like Coordinate instead of raw BigDecimals.
| public record UpdateCityCommand( | ||
| Long cityId, | ||
| String nameEn, | ||
| String nameKr | ||
| String nameKr, | ||
| Double latitude, | ||
| Double longitude |
There was a problem hiding this comment.
UpdateCityCommand uses boxed Double for coordinates. This makes latitude/longitude nullable (risking NPEs where BigDecimal.valueOf(...) is used) and forces lossy BigDecimal -> double -> BigDecimal conversions in the admin flow. Prefer BigDecimal (or Coordinate) for these fields, or at least use primitive double if null is not allowed.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a187f7472
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cityManagementPort.createCity(new CreateCityCommand( | ||
| request.nameEn(), | ||
| request.nameKr(), | ||
| request.coordinate().getLatitude().doubleValue(), |
There was a problem hiding this comment.
request.coordinate().getLatitude().doubleValue()를 바로 호출하면 coordinate 내부 필드가 비어 있는 요청(예: {"coordinate":{...}}에서 latitude/longitude 누락)에서 NullPointerException이 발생해 500으로 떨어집니다. CityCreateRequest/CityUpdateRequest는 coordinate 객체 존재만 검증하고 내부 숫자 null은 막지 못하므로, 여기서 null 체크를 추가하거나 Coordinate 필드에 @NotNull 제약을 걸어 4xx로 일관되게 처리해야 합니다.
Useful? React with 👍 / 👎.
| if (keyword == null || keyword.isBlank()) { | ||
| return cityRepository.findByCountryIdWithPaging(countryId, pageable); | ||
| } | ||
| return cityRepository.searchByKeyword(countryId, keyword, pageable); |
There was a problem hiding this comment.
현재는 공백 포함 키워드가 그대로 searchByKeyword로 전달되어, 사용자가 "서울 "처럼 앞뒤 공백을 포함해 입력하면 실제 데이터가 있어도 0건이 반환될 수 있습니다. 동일 도메인의 CityAdminQueryService는 조회 전에 keyword.trim()을 적용하고 있어 동작이 불일치하므로, 여기서도 trim 후 검색해야 검색 정확도 저하를 막을 수 있습니다.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
릴리스 노트