지도 관련 API 3개 구현 #44
지도 관련 API 3개 구현 #44
Conversation
WalkthroughThis update introduces a new service for querying place congestion and mapping data, refactors the controller to delegate logic to this service, and adds or updates repository methods for efficient data retrieval. Data transfer objects are updated for type safety, and Redis utility methods are expanded to support time-segmented congestion caching. Entity relationships and eager fetching strategies are clarified. Swagger annotations are added for API documentation grouping. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant PlaceCongestionQueryService
participant PlaceRepository
participant ReviewRepository
participant PlaceRedisUtil
Client->>Controller: GET /map (type, latitude, longitude)
Controller->>PlaceCongestionQueryService: getMap(type, latitude, longitude)
PlaceCongestionQueryService->>PlaceRepository: findPlacesByLocationAndType(...)
PlaceCongestionQueryService->>PlaceRedisUtil: getRedisCongestion(placeId)
PlaceCongestionQueryService-->>Controller: MapListDto
Controller-->>Client: ApiResponse(MapListDto)
Client->>Controller: GET /place/{placeId}/real-time
Controller->>PlaceCongestionQueryService: getCongestion(placeId)
PlaceCongestionQueryService->>PlaceRedisUtil: getTimeCongestion(placeId, dateTime)
PlaceCongestionQueryService-->>Controller: PlaceCongestionDto
Controller-->>Client: ApiResponse(PlaceCongestionDto)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
src/main/kotlin/busanVibe/busan/domain/place/repository/PlaceRepository.kt (1)
50-58: Method name is misleading - doesn't fetch reviewsThe method name
findByIdWithReviewAndImagesuggests it fetches reviews, but the query only eagerly loadsopenTimeandplaceImages. Consider renaming tofindByIdWithOpenTimeAndImagesfor clarity.- fun findByIdWithReviewAndImage(@Param("placeId") placeId: Long): Place? + fun findByIdWithOpenTimeAndImages(@Param("placeId") placeId: Long): Place?src/main/kotlin/busanVibe/busan/domain/place/domain/Place.kt (1)
50-51: Consider nullable type or default value for reviews propertyThe
reviewsproperty is declared as non-nullableList<Review>without a default value. This could potentially cause issues if the property is accessed before JPA initializes it, especially in scenarios where the entity is created programmatically.Consider one of these approaches:
- val reviews: List<Review>, + val reviews: List<Review> = emptyList(),Or make it nullable:
- val reviews: List<Review>, + val reviews: List<Review>?,src/main/kotlin/busanVibe/busan/domain/review/domain/repository/ReviewRepository.kt (1)
21-22: Consider consolidating duplicate functionalityThe new
findByPlacemethod appears to duplicate the functionality of the existingfindForDetailsmethod (lines 12-19). Both methods:
- Take a
Placeparameter- Return
List<Review>- Eagerly fetch the
userassociationConsider whether both methods are necessary or if one approach should be preferred. The
@EntityGraphapproach is generally cleaner than JOIN FETCH queries.If the
@EntityGraphapproach is preferred, consider deprecating or removingfindForDetails:+ @Deprecated("Use findByPlace instead") fun findForDetails(@Param("place")place: Place): List<Review>src/main/kotlin/busanVibe/busan/domain/place/controller/PlaceCongestionController.kt (1)
47-51: Consider implementing or removing the distribution endpointThe
/distributionendpoint still returnsnull. Should this endpoint be:
- Implemented as part of this PR
- Removed if not needed
- Left as a placeholder for future implementation
Please clarify the intended behavior for this endpoint.
Would you like me to help implement this endpoint or should it be removed from the controller?
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt (2)
26-27: Consider making latitude/longitude ranges configurableThe hardcoded range values of 0.05 degrees (~5.5km) might not be suitable for all use cases. Consider making these configurable through application properties to allow flexibility based on different requirements.
+import org.springframework.beans.factory.annotation.Value + @Service class PlaceCongestionQueryService( private val placeRepository: PlaceRepository, private val placeRedisUtil: PlaceRedisUtil, private val reviewRepository: ReviewRepository, + @Value("\${place.search.latitude-range:0.05}") private val latitudeRange: Double, + @Value("\${place.search.longitude-range:0.05}") private val longitudeRange: Double ) { - private val latitudeRange: Double = 0.05 - private val longitudeRange: Double = 0.05
102-102: Consider using English for log messagesThe log messages are in Korean. For better maintainability and consistency, consider using English for log messages.
- log.info("현재 시간: ${current}시") + log.info("Current time: $current") - log.info("배열 담기 시작") + log.info("Starting congestion data collection") - log.info("배열 담기 끝") + log.info("Completed congestion data collection")Also applies to: 109-109, 120-120
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceRedisUtil.kt (3)
36-41: Optimize redundant Redis callWhen
setIfAbsentreturns false, the existing value is already known to exist. The additionalgetcall is unnecessary.if (success == true) { - log.info("혼잡도 기록 저장 완료: $key, 저장된 혼잡도: $congestion") + log.info("Congestion saved: $key, value: $congestion") } else { - val existing = redisTemplate.opsForValue().get(key) - log.info("이미 존재하는 혼잡도 기록: $key, 기존 혼잡도: $existing") + log.info("Congestion already exists: $key") }If you need the existing value for logging, consider using
getAndSetor accepting that the value exists without fetching it.
45-59: Potential race condition in concurrent scenariosIn high-concurrency scenarios, there's a small window where
setPlaceTimeCongestionmight not set the value (if another thread sets it first), and the subsequentgetcould still return null.Consider using a more robust approach:
fun getTimeCongestion(placeId: Long, dateTime: LocalDateTime): Float { val roundedHour = (dateTime.hour / 3) * 3 val key = "place:congestion:$placeId-${dateTime.year}-${dateTime.monthValue}-${dateTime.dayOfMonth}-$roundedHour" val value = redisTemplate.opsForValue().get(key) return if (value != null) { - log.info("이미 존재하는 혼잡도 기록: $key, 기존 혼잡도: $value") + log.info("Existing congestion found: $key, value: $value") value.toFloatOrNull() ?: 0f } else { - setPlaceTimeCongestion(placeId, dateTime.withHour(roundedHour)) - val newValue = redisTemplate.opsForValue().get(key) - newValue?.toFloatOrNull() ?: 0f + val congestion = getRandomCongestion() + val stored = redisTemplate.opsForValue().setIfAbsent(key, congestion.toString(), Duration.ofHours(24)) + if (stored == true) { + log.info("New congestion created: $key, value: $congestion") + congestion + } else { + // Another thread created it, fetch the value + redisTemplate.opsForValue().get(key)?.toFloatOrNull() ?: 0f + } } }
62-64: Consider using Kotlin's Random for better idiomatic codeWhile
Math.random()works, Kotlin'sRandomis more idiomatic and provides better control.+import kotlin.random.Random + private fun getRandomCongestion(): Float { - return (Math.random() * 4 + 1).toFloat() + return Random.nextFloat() * 4 + 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/kotlin/busanVibe/busan/domain/place/controller/PlaceCongestionController.kt(2 hunks)src/main/kotlin/busanVibe/busan/domain/place/domain/Place.kt(2 hunks)src/main/kotlin/busanVibe/busan/domain/place/dto/PlaceMapResponseDTO.kt(3 hunks)src/main/kotlin/busanVibe/busan/domain/place/repository/PlaceRepository.kt(2 hunks)src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt(1 hunks)src/main/kotlin/busanVibe/busan/domain/place/service/PlaceRedisUtil.kt(1 hunks)src/main/kotlin/busanVibe/busan/domain/review/domain/repository/ReviewRepository.kt(2 hunks)
🔇 Additional comments (13)
src/main/kotlin/busanVibe/busan/domain/place/repository/PlaceRepository.kt (2)
8-8: LGTM: BigDecimal import for coordinate precisionGood choice using
BigDecimalfor geographic coordinates to maintain precision in location-based queries.
32-48: LGTM: Well-designed location-based query with conditional type filteringThe query correctly:
- Uses
BETWEENfor efficient coordinate range filtering- Implements conditional type filtering with SpEL expression to bypass when type is "ALL"
- Eagerly fetches
openTimeassociation to avoid N+1 queries- Uses
BigDecimalfor coordinate precisionsrc/main/kotlin/busanVibe/busan/domain/place/domain/Place.kt (1)
5-5: LGTM: Review import added for relationship mappingsrc/main/kotlin/busanVibe/busan/domain/review/domain/repository/ReviewRepository.kt (1)
5-5: LGTM: EntityGraph import addedsrc/main/kotlin/busanVibe/busan/domain/place/dto/PlaceMapResponseDTO.kt (5)
6-6: LGTM: BigDecimal import for coordinate precisionConsistent with the repository layer changes for geographic coordinate handling.
18-18: LGTM: Nullable ID fields improve type safetyMaking
placeIdandidnullable (Long?) is appropriate for optional identifier fields.Also applies to: 28-28
21-21: LGTM: Kotlin primitive Int for better performanceReplacing boxed
Integerwith Kotlin primitiveIntfor congestion levels improves performance and type consistency.Also applies to: 30-30, 44-44
22-24: LGTM: BigDecimal coordinates for precisionUsing
BigDecimalfor latitude and longitude coordinates ensures precision and consistency with the repository layer's coordinate filtering.Also applies to: 33-34
32-32: LGTM: Primitive Int for count and time fieldsUsing
IntforreviewAmountandstandardTimeis appropriate for these numeric fields.Also applies to: 43-43
src/main/kotlin/busanVibe/busan/domain/place/controller/PlaceCongestionController.kt (3)
5-5: LGTM: Proper service layer separationGood practice injecting
PlaceCongestionQueryServiceand delegating business logic to the service layer.Also applies to: 16-18
27-28: LGTM: Service delegation with proper return valuesThe endpoints now properly delegate to the service layer and return actual data wrapped in
ApiResponseinstead of null values.Also applies to: 35-36, 43-44
39-39: LGTM: Improved endpoint URL namingRenaming from
/read-timeto/real-timeis more intuitive and grammatically correct.src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt (1)
124-124: Precision conversion matches DTO typeConfirmed that
PlaceCongestionDto.realTimeCongestionLevelis defined as anIntinsrc/main/kotlin/busanVibe/busan/domain/place/dto/PlaceMapResponseDTO.kt. Using.toInt()aligns with the API contract—no precision-loss issue to address.
| @RequestParam("type", required = false, defaultValue = "ALL") type: PlaceType, | ||
| @RequestParam("latitude")latitude: Double, | ||
| @RequestParam("longtitude")longtitude: Double): ApiResponse<PlaceMapResponseDTO.MapListDto>?{ | ||
| @RequestParam("longitude")longtitude: Double): ApiResponse<PlaceMapResponseDTO.MapListDto>{ |
There was a problem hiding this comment.
Fix parameter name typo
There's a typo in the parameter name: longtitude should be longitude.
- @RequestParam("longitude")longtitude: Double): ApiResponse<PlaceMapResponseDTO.MapListDto>{
+ @RequestParam("longitude")longitude: Double): ApiResponse<PlaceMapResponseDTO.MapListDto>{Also update the service call:
- val places = placeCongestionQueryService.getMap(type, latitude, longtitude)
+ val places = placeCongestionQueryService.getMap(type, latitude, longitude)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @RequestParam("type", required = false, defaultValue = "ALL") type: PlaceType, | |
| @RequestParam("latitude")latitude: Double, | |
| @RequestParam("longtitude")longtitude: Double): ApiResponse<PlaceMapResponseDTO.MapListDto>?{ | |
| @RequestParam("longitude")longtitude: Double): ApiResponse<PlaceMapResponseDTO.MapListDto>{ | |
| @RequestParam("type", required = false, defaultValue = "ALL") type: PlaceType, | |
| @RequestParam("latitude")latitude: Double, | |
| @RequestParam("longitude")longitude: Double): ApiResponse<PlaceMapResponseDTO.MapListDto>{ | |
| val places = placeCongestionQueryService.getMap(type, latitude, longitude) | |
| // … | |
| } |
🤖 Prompt for AI Agents
In
src/main/kotlin/busanVibe/busan/domain/place/controller/PlaceCongestionController.kt
around lines 23 to 25, fix the typo in the parameter name by renaming
`longtitude` to `longitude`. Also update any corresponding service calls or
references to use the corrected parameter name `longitude` to ensure consistency
and avoid errors.
| val placeDtoList :List<PlaceMapResponseDTO.PlaceMapInfoDto> = placeList.map { | ||
| PlaceMapResponseDTO.PlaceMapInfoDto( | ||
| placeId = it.id, | ||
| name = it.name, | ||
| type = it.type.capitalEnglish, | ||
| latitude = it.latitude, | ||
| longitude = it.longitude, | ||
| congestionLevel = placeRedisUtil.getRedisCongestion(it.id) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential N+1 performance issue with Redis calls
The current implementation makes individual Redis calls for each place when fetching congestion levels. For large result sets, this could impact performance.
Consider implementing a batch fetch method in PlaceRedisUtil to retrieve congestion data for multiple places in a single operation:
// In PlaceRedisUtil
fun getRedisCongestionBatch(placeIds: List<Long>): Map<Long, Int> {
val keys = placeIds.map { "place:congestion:$it" }
val values = redisTemplate.opsForValue().multiGet(keys)
return placeIds.zip(values ?: emptyList()).associate { (id, value) ->
id to (value?.toIntOrNull() ?: generateAndStoreCongestion(id))
}
}Then update the mapping:
+ val congestionMap = placeRedisUtil.getRedisCongestionBatch(placeList.map { it.id })
val placeDtoList :List<PlaceMapResponseDTO.PlaceMapInfoDto> = placeList.map {
PlaceMapResponseDTO.PlaceMapInfoDto(
placeId = it.id,
name = it.name,
type = it.type.capitalEnglish,
latitude = it.latitude,
longitude = it.longitude,
- congestionLevel = placeRedisUtil.getRedisCongestion(it.id)
+ congestionLevel = congestionMap[it.id] ?: 0
)
}🤖 Prompt for AI Agents
In
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt
between lines 47 and 56, the code currently fetches congestion levels from Redis
one-by-one for each place, causing a potential N+1 performance issue. To fix
this, implement a batch fetch method in PlaceRedisUtil that takes a list of
place IDs and retrieves all congestion data in a single Redis multiGet call.
Then, update the placeDtoList mapping to first collect all place IDs, call this
batch method once, and map the congestion levels from the returned map instead
of calling Redis individually for each place.
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt
Outdated
Show resolved
Hide resolved
| latitude = place.latitude, | ||
| longitude = place.longitude, | ||
| address = place.address, | ||
| isOpen = true, |
There was a problem hiding this comment.
Hardcoded isOpen value doesn't reflect actual status
The isOpen field is hardcoded to true, which doesn't represent the actual open/closed status of the place. The comment mentions "openTime" but it's not being used.
Consider implementing actual open/closed logic based on the place's open times:
isOpen = determineIfOpen(place.openTimes, LocalDateTime.now())🤖 Prompt for AI Agents
In
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt
at line 93, the isOpen field is currently hardcoded to true, which does not
reflect the actual open or closed status of the place. To fix this, replace the
hardcoded true with a call to a function that determines if the place is open
based on its openTimes and the current time, such as isOpen =
determineIfOpen(place.openTimes, LocalDateTime.now()). Ensure the
determineIfOpen function correctly evaluates the openTimes to return the
accurate open status.
| fun getRedisCongestion(placeId: Long?): Int{ | ||
|
|
||
| val key = "place:congestion:$placeId" | ||
| val randomCongestion: Int = (Math.random() * 5 + 1).toInt() | ||
| val randomCongestion = getRandomCongestion().toInt().toString() | ||
|
|
||
| redisTemplate.opsForValue() | ||
| .set(key, randomCongestion.toString()) | ||
| .set(key, randomCongestion) | ||
|
|
||
| return Integer.parseInt(randomCongestion) | ||
| } |
There was a problem hiding this comment.
Method always overwrites existing congestion values
The current implementation always generates a new random value and overwrites any existing congestion data. This doesn't preserve historical data and makes the congestion values inconsistent across calls.
Consider checking if a value exists before overwriting:
fun getRedisCongestion(placeId: Long?): Int{
val key = "place:congestion:$placeId"
- val randomCongestion = getRandomCongestion().toInt().toString()
-
- redisTemplate.opsForValue()
- .set(key, randomCongestion)
-
- return Integer.parseInt(randomCongestion)
+
+ val existingValue = redisTemplate.opsForValue().get(key)
+ if (existingValue != null) {
+ return existingValue.toIntOrNull() ?: 0
+ }
+
+ val randomCongestion = getRandomCongestion().toInt()
+ redisTemplate.opsForValue().set(key, randomCongestion.toString(), Duration.ofHours(1))
+ return randomCongestion
}🤖 Prompt for AI Agents
In src/main/kotlin/busanVibe/busan/domain/place/service/PlaceRedisUtil.kt
between lines 18 and 27, the getRedisCongestion method always overwrites
existing congestion values with a new random value, losing any previously stored
data. Modify the method to first check if a congestion value already exists in
Redis for the given placeId; if it does, return that existing value instead of
generating and setting a new one. Only generate and set a new random congestion
value if no existing value is found.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt (2)
47-56: N+1 Redis calls issue still present.This is the same performance issue flagged in the previous review. The code still makes individual Redis calls for each place when fetching congestion levels.
93-93: Hardcoded isOpen value still doesn't reflect actual status.This is the same issue flagged in the previous review. The
isOpenfield remains hardcoded totrue.
🧹 Nitpick comments (3)
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt (3)
26-28: Consider making latitude and longitude ranges configurable.The hardcoded range values of 0.05 degrees might need adjustment based on different use cases or regions. Consider moving these to application properties for better flexibility.
- private val latitudeRange: Double = 0.05 - private val longitudeRange: Double = 0.05 + @Value("\${place.search.latitude-range:0.05}") + private val latitudeRange: Double = 0.05 + + @Value("\${place.search.longitude-range:0.05}") + private val longitudeRange: Double = 0.05Don't forget to import
org.springframework.beans.factory.annotation.Value.
104-119: Consider extracting complex time calculation logic.The time calculation logic is complex and would benefit from being extracted into a separate method for better readability and testability.
+ private fun generateTimePoints(current: LocalDateTime, roundedBase: Int): List<LocalDateTime> { + return (6 downTo 0).map { i -> + val hour = (roundedBase - i * 3 + 24) % 24 + current.withHour(hour) + .withMinute(0).withSecond(0).withNano(0) + .let { + if (hour > current.hour) it.minusDays(1) else it + } + } + } + @Transactional(readOnly = true) fun getCongestion(placeId: Long): PlaceMapResponseDTO.PlaceCongestionDto { val current = LocalDateTime.now() log.info("현재 시간: ${current}시") val roundedBase = (current.hour / 3) * 3 - // 최근 6개 3시간 단위 시간 생성 (기준시간 포함 총 7개) - val hours = (6 downTo 0).map { i -> (roundedBase - i * 3 + 24) % 24 } - log.info("배열 담기 시작") - val byTimePercent: List<Float> = hours.map { hour -> - val adjustedDateTime = current.withHour(hour) - .withMinute(0).withSecond(0).withNano(0) - .let { - if (hour > current.hour) it.minusDays(1) else it - } - placeRedisUtil.getTimeCongestion(placeId, adjustedDateTime) - } + val timePoints = generateTimePoints(current, roundedBase) + val byTimePercent: List<Float> = timePoints.map { dateTime -> + placeRedisUtil.getTimeCongestion(placeId, dateTime) + }
122-126: Inconsistent congestion level types.The
byTimePercentlist containsFloatvalues whilerealTimeCongestionLevelis converted toInt. Consider using consistent types for congestion levels throughout the response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt(1 hunks)
🔇 Additional comments (1)
src/main/kotlin/busanVibe/busan/domain/place/service/PlaceCongestionQueryService.kt (1)
81-82: Good fix for handling empty review set.The code now properly handles the case when there are no reviews by returning 0f instead of NaN.
지도에서 명소 조회 API
명소 기본 정보 조회 API
명소 실시간 혼잡도 조회 API
Summary by CodeRabbit
New Features
Improvements
Bug Fixes