refactor: 행정구역 검색 포함하고 coroutineScope 활용한 병렬 요청#88
Merged
Conversation
…tive region data handling
…nd flower spot search functionality
…aching for flower spots and landmarks
Contributor
📝 WalkthroughWalkthroughConsolidates landmark types under com.pida.place, removes the FlowerSpot-only search endpoint and related search types, and adds a unified Place search flow with District domain, persistence, caching, and an orchestration facade that concurrently queries districts, landmarks, and flower spots. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Controller as PlaceController
participant Facade as PlaceFacade
participant District as DistrictService
participant Landmark as LandmarkService
participant FlowerSpot as FlowerSpotService
participant LandmarkAPI as LandmarkSearchClient
participant Events as EventPublisher
Client->>Controller: GET /places/search?query&user
Controller->>Facade: search(query, user)
Facade->>Events: publishSearchEvent(query, user)
par Concurrent queries
Facade->>District: searchDistricts(query)
District-->>Facade: List<District]
Facade->>Landmark: searchLandmarks(query)
Landmark-->>Facade: List<Landmark]
Facade->>FlowerSpot: searchFlowerSpots(query)
FlowerSpot-->>Facade: List<FlowerSpot]
end
alt stored landmarks sufficient
Facade->>Events: publishLandmarkFetchEvent(stored)
else insufficient
Facade->>LandmarkAPI: searchByKeyword(query)
LandmarkAPI-->>Facade: List<NewLandmark]
Facade->>Events: publishLandmarkFetchEvent(external)
end
Facade-->>Controller: PlaceSearchResult(districts, landmarks, flowerSpots)
Controller-->>Client: PlaceSearchResultResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/place/PlaceController.kt`:
- Around line 32-36: PlaceController currently uses `@Valid` on the
List<AddDistrictRequest> in placeAdd but lacks class-level validation and
authorization; add the Spring `@Validated` annotation to the PlaceController class
so each AddDistrictRequest in the List is validated, and secure the endpoint by
adding a Spring Security annotation (e.g.,
`@PreAuthorize`("hasRole('ROLE_ADMIN')") or an appropriate expression) either on
the placeAdd method or the controller class to restrict access before calling
districtService.addAll(requests.map { it.toDistrict() }); ensure imports for
`@Validated` and `@PreAuthorize` are added and adjust the security expression to
match your app's roles/authorities.
In `@pida-core/core-domain/src/main/kotlin/com/pida/place/DistrictFinder.kt`:
- Around line 17-24: The cache invocation in searchByKeyword (the suspend fun
searchByKeyword) passes a lambda that calls the blocking
districtRepository.searchByKeyword() and CacheAdvice.invoke currently performs
blocking cacheRepository.get()/put() calls on the caller dispatcher; wrap the
blocking DB and cache operations in withContext(Dispatchers.IO) (either by
changing the lambda passed to cacheAdvice.invoke to call
withContext(Dispatchers.IO) { districtRepository.searchByKeyword(keyword) } or,
better, modify CacheAdvice.invoke to switch to withContext(Dispatchers.IO)
around cacheRepository.get()/put() and any repository calls) so all blocking I/O
runs on the IO dispatcher and not the main coroutine dispatcher.
In `@pida-core/core-domain/src/main/kotlin/com/pida/place/DistrictService.kt`:
- Around line 15-16: The current logic splits query into keywords and calls
districtFinder.searchByKeyword with keywords.first(), which can be an empty
string for a blank query; update DistrictService.kt to guard against blank
queries by trimming and filtering out empty tokens (e.g., val trimmed =
query.trim(); val keywords = trimmed.split("\\s+".toRegex()).filter {
it.isNotBlank() }) and if keywords is empty return an empty result (or
appropriate empty response) instead of calling districtFinder.searchByKeyword;
ensure any use of keywords.first() is only done after this validation.
- Around line 14-27: The searchDistricts function should first handle
blank/whitespace-only queries by returning an empty list (or early exit) instead
of calling districtFinder.searchByKeyword with an empty string; trim and check
query.isBlank() and also split into keywords while filtering out empty tokens so
keywords.first() is valid. For the in-memory filtering step (in searchDistricts)
replace the case-sensitive contains(keyword) checks with casefolded comparisons:
normalize both the fullText and each keyword to a common case/locale (e.g.,
lowercase with Locale/Korean-aware folding) before calling contains, and
consider normalizing known variant characters if needed; keep
districtFinder.searchByKeyword usage unchanged but ensure you pass a non-empty
first keyword.
In
`@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/district/DistrictCoreRepository.kt`:
- Around line 22-23: In DistrictCoreRepository inside the districts.map block
the expression "it.pinPoint as GeoJson.Point" is an unsafe cast; replace it with
a safe cast and validation (e.g., use "it.pinPoint as? GeoJson.Point" and check
for null) and handle non-Point cases gracefully (skip the entry, provide a
default, or log a warning) so you avoid ClassCastException when pinPoint is null
or a different GeoJson subtype; update any downstream usage that assumes a
non-null Point accordingly (e.g., in the map transformation or subsequent
functions that rely on point coordinates).
In
`@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/district/DistrictEntity.kt`:
- Around line 38-48: The toDistrict() conversion in DistrictEntity currently
uses the unsafe non-null assertion id!! which can NPE for unpersisted entities;
change this to an explicit null-check that throws a clear, descriptive exception
(e.g., use requireNotNull(id) or throw IllegalStateException with a message like
"DistrictEntity.id is null — toDistrict() must be called only for persisted
entities") and/or add a KDoc comment on DistrictEntity.toDistrict() stating it
must only be invoked for persisted entities so callers know the precondition.
🧹 Nitpick comments (8)
pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFinder.kt (1)
53-60: Caching implementation looks good; consider cache key normalization.The caching pattern is consistent with other methods in this class. However, using user input (
streetName) directly in cache keys could lead to cache fragmentation (e.g., "강남" vs "강남 " with trailing space).Since
FlowerSpotService.searchFlowerSpotsalready trims the input before calling this method, the immediate risk is mitigated. If this method is called from other paths in the future, consider normalizing the key (e.g., lowercase, trim) within this method for defensive consistency.pida-core/core-domain/src/main/kotlin/com/pida/support/geo/Region.kt (1)
25-26: Redundant conditions can be simplified.The additional
startsWithchecks are redundant since"서울특별시".startsWith("서울")and"경기도".startsWith("경기")are already true. The original shorter prefixes cover all cases.♻️ Suggested simplification
- startsWith("서울") || startsWith("서울특별시") -> Region.SEOUL - startsWith("경기") || startsWith("경기도") -> Region.GYEONGGI + startsWith("서울") -> Region.SEOUL + startsWith("경기") -> Region.GYEONGGIpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/district/DistrictCustomRepository.kt (1)
14-46: Consider adding a result limit to prevent unbounded queries.The
searchByKeywordmethod has no result limit, which could return a very large dataset for common prefixes (e.g., "서" could match thousands of districts). Additionally, ifkeywordis empty, the pattern becomes"%", effectively matching all non-deleted records.Consider:
- Adding a
LIMITclause to the query- Validating that
keywordis non-empty or has a minimum length♻️ Proposed fix to add result limit
fun searchByKeyword(keyword: String): List<DistrictEntity> { + require(keyword.isNotBlank()) { "Keyword must not be blank" } val pattern = "$keyword%" // B-tree index 활용을 위한 접두사 일치 패턴 val query = jpql { select(entity(DistrictEntity::class)) .from(entity(DistrictEntity::class)) .whereAnd( path(DistrictEntity::deletedAt).isNull(), or( path(DistrictEntity::sido).like(pattern), path(DistrictEntity::sigungu).like(pattern), path(DistrictEntity::eupmyeondonggu).like(pattern), path(DistrictEntity::eupmyeonridong).like(pattern), path(DistrictEntity::ri).like(pattern), ), ).orderBy( // ... existing ordering ... ) } - return entityManager.createQuery(query, jdslRenderContext).resultList + return entityManager.createQuery(query, jdslRenderContext) + .setMaxResults(100) // Reasonable limit for search results + .resultList }pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/place/PlaceController.kt (1)
40-44: Consider adding validation for the query parameter.The
queryparameter has no validation constraints. Consider adding minimum length requirements to prevent expensive searches with very short or empty queries.♻️ Proposed validation
suspend fun searchPlace( - `@RequestParam` `@Parameter`(name = "query", description = "검색 키워드") query: String, + `@RequestParam` `@Parameter`(name = "query", description = "검색 키워드") `@Size`(min = 1, max = 100) query: String, `@Parameter`(hidden = true, required = false) user: User?, ): PlaceSearchResultResponse {Add import:
import jakarta.validation.constraints.Sizepida-core/core-domain/src/main/kotlin/com/pida/place/PlaceFacade.kt (3)
48-53: Redundant region filter on landmarks.Line 53 applies
filter { it.region in SEARCH_REGIONS }tolandmarks, but in the else branch (Lines 46-48), the fetched landmarks are already filtered bySEARCH_REGIONS. This results in redundant filtering for externally fetched landmarks.♻️ Proposed fix to avoid redundant filtering
val landmarks = if (hasEnough(storedLandmarks)) { publishLandmarkFetchEvent(query, null) - storedLandmarks + storedLandmarks.filter { it.region in SEARCH_REGIONS } } else { val fetched = landmarkSearchClient.searchByKeyword(query) publishLandmarkFetchEvent(query, fetched) fetched.map { it.toLandmark() }.filter { it.region in SEARCH_REGIONS } } PlaceSearchResult( districts = districtsDeferred.await().take(MAX_DISTRICT_SEARCH_COUNT), - landmarks = landmarks.filter { it.region in SEARCH_REGIONS }.take(MAX_LANDMARK_SEARCH_COUNT), + landmarks = landmarks.take(MAX_LANDMARK_SEARCH_COUNT), flowerSpots = flowerSpotsDeferred.await(), )
67-77: Consider renaming the search event for clarity.
FlowerSpotSearchEventis published for all place searches, not just flower spot searches. This naming may cause confusion since the search now includes districts and landmarks. Consider renaming toPlaceSearchEventfor consistency with the new unified search API.
31-56: Consider error handling strategy for parallel coroutines.If any of the parallel async calls (districts, landmarks, flowerSpots) fails,
coroutineScopewill cancel all sibling coroutines and propagate the exception. This is fail-fast behavior. If partial results are acceptable (e.g., return landmarks even if district search fails), consider usingsupervisorScopewith individual error handling.pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/place/response/PlaceSearchResponse.kt (1)
12-17: Property naming inconsistency:districtshould be plural.The property
districtholds aList<PlaceSearchResponse>but uses singular form, whilelandmarksandflowerSpotscorrectly use plural. Consider renaming todistrictsfor consistency.♻️ Proposed fix
data class PlaceSearchResultResponse( `@field`:ArraySchema( schema = Schema(implementation = PlaceSearchResponse::class), arraySchema = Schema(description = "행정구역 목록"), ) - val district: List<PlaceSearchResponse>, + val districts: List<PlaceSearchResponse>,Note: This change would also need to be reflected in the
of()factory method and callers likePlaceController.
pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/place/PlaceController.kt
Show resolved
Hide resolved
...-storage/db-core/src/main/kotlin/com/pida/storage/db/core/district/DistrictCoreRepository.kt
Show resolved
Hide resolved
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/district/DistrictEntity.kt
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🌱 관련 이슈
📌 작업 내용 및 특이 사항
place패키지 구성📝 참고
/api/v1/place/search로 검색 API 엔드포인트 변경됨📌 체크 리스트
Summary by CodeRabbit
New Features
Refactor
Performance
✏️ Tip: You can customize this high-level summary in your review settings.