refactor: 우산 대여 코드 이벤트 분리 - #502
Conversation
📝 WalkthroughWalkthroughAdds an event-driven notification flow: introduces Event base and Events publisher, wires ApplicationEventPublisher, adds UmbrellaRentedEvent and UmbrellaReturnedEvent, a RentEventHandler to persist reports and send Slack notifications, RentService now raises events (removing direct notifications), Slack notifier accepts DTO inputs, report entities use historyId and QueryDSL custom repos. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant RentSvc as "RentService"
participant Umbrella as "Umbrella"
participant Events as "Events"
participant Handler as "RentEventHandler"
participant Slack as "SlackAlarmService"
participant CondSvc as "ConditionReportService"
participant ImprSvc as "ImprovementReportService"
User->>RentSvc: addRental(request, user)
RentSvc->>Umbrella: rentUmbrella(storeIdForRent)
Umbrella-->>RentSvc: rented
RentSvc->>Events: raise(UmbrellaRentedEvent(...))
Events->>Handler: publish UmbrellaRentedEvent
Handler->>Slack: notifyRent(NotifyRentInput)
alt conditionReport present
Handler->>CondSvc: saveConditionReport(ConditionReport(historyId,...))
Handler->>Slack: notifyConditionReport(NotifyConditionReportInput)
end
Note right of RentSvc: Return flow (later)
RentSvc->>Events: raise(UmbrellaReturnedEvent(...))
Events->>Handler: publish UmbrellaReturnedEvent
Handler->>Slack: notifyReturn(NotifyReturnInput)
alt improvementReport present
Handler->>ImprSvc: saveImprovementReport(ImprovementReport(historyId,...))
Handler->>Slack: notifyImprovementReport(NotifyImprovementReportInput)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
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)
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 |
|
EOL github 버그로 안보이는것 같네요 |
| if (umbrella.storeMeta.id != rentUmbrellaByUserRequest.storeId) { | ||
| throw UmbrellaStoreMissMatchException("[ERROR] 해당 우산은 해당 매장에 존재하지 않습니다.") | ||
| } | ||
| if (umbrella.missed) { | ||
| throw MissingUmbrellaException("[ERROR] 해당 우산은 분실되었습니다.") | ||
| } | ||
| if (!umbrella.rentable) { | ||
| throw NotAvailableUmbrellaException("[ERROR] 해당 우산은 대여중입니다.") | ||
| } | ||
| umbrella.rentUmbrella() | ||
|
|
||
| umbrella.rentUmbrella(rentUmbrellaByUserRequest.storeId) | ||
|
|
There was a problem hiding this comment.
검증 로직 umbrella 객체가 처리하도록 변경
| val rentStoreName: String, | ||
| val conditionReportContent: String? = null, | ||
| val umbrellaId: Long, | ||
| val history: History, |
There was a problem hiding this comment.
이벤트가 domain에 의존하지 않게 하려고 했는데 condition report 만들려면 필요해서 추가했습니다.
History를 id만으로 생성할 수 있으면 history id 만 받도록 해도 될 것 같은데,
그럼 도메인이 id 만 가지는 생성자를 추가해야 해서 어떤게 좋은지 모르겠네요.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
src/main/kotlin/upbrella/be/util/event/Event.kt (1)
4-4: Consider using a time type or more explicit naming for clarityOptional:
- Use Instant instead of Long to avoid unit ambiguity and improve readability.
- If keeping Long, consider a more explicit name like occurredAtEpochMillis and add brief KDoc.
src/main/kotlin/upbrella/be/slack/dto/service/input/SlackAlarmInput.kt (1)
3-13: Add lightweight validation to fail fast on bad inputsConsider Bean Validation annotations (e.g., NotBlank for userName/rentStoreName, Positive for IDs) or a simple init { require(...) } to protect downstream consumers and keep logs clean.
src/main/kotlin/upbrella/be/rent/event/UmbrellaRentedEvent.kt (1)
7-11: PII considerations for loggingThis event contains userName and rentStoreName. If events are logged, consider redaction/sanitization policies to avoid leaking PII in logs.
src/main/kotlin/upbrella/be/util/event/Events.kt (1)
12-15: After-commit publishing helper (optional)If you frequently need after-commit semantics without remembering annotations, consider adding a helper that enforces AFTER_COMMIT via @TransactionalEventListener on a dedicated internal dispatcher or by documenting a standard. Alternatively, adopt TransactionalEventPublisher in producers.
src/main/kotlin/upbrella/be/config/event/EventConfiguration.kt (1)
3-17: Prefer injecting ApplicationEventPublisher directly for clarityInjecting the narrower type communicates intent and avoids passing the whole context.
Apply:
import org.springframework.beans.factory.InitializingBean -import org.springframework.context.ApplicationContext +import org.springframework.context.ApplicationEventPublisher import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import upbrella.be.util.event.Events @Configuration class EventsConfiguration( - private val applicationContext: ApplicationContext + private val publisher: ApplicationEventPublisher ) { @Bean fun eventsInitializer(): InitializingBean { - return InitializingBean { Events.setPublisher(applicationContext) } + return InitializingBean { Events.setPublisher(publisher) } } }src/main/kotlin/upbrella/be/umbrella/entity/Umbrella.kt (2)
56-58: Guard against nullable store ID on storeMeta.storeMeta.id is typically nullable (Long?) on JPA entities. If it's null, the current comparison implicitly treats it as mismatch. Make the intent explicit to avoid accidental NPEs and improve readability.
- if (this.storeMeta.id != storeIdForRent) { + if (this.storeMeta.id == null || this.storeMeta.id != storeIdForRent) { throw UmbrellaStoreMissMatchException("[ERROR] 해당 우산은 해당 매장에 존재하지 않습니다.") }
55-65: Unit tests for entity-level invariants.Add tests covering: store mismatch, missed umbrella, deleted umbrella, and already rented umbrella to ensure rentUmbrella enforces all invariants.
I can generate focused unit tests for Umbrella.rentUmbrella if helpful.
src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt (1)
30-33: Avoid passing JPA entities through events; pass IDs or value objects.UmbrellaRentedEvent carries a History entity. This couples listeners to the persistence context and risks LazyInitialization issues if listeners become async. Prefer IDs/value fields and let handlers load references as needed.
src/main/kotlin/upbrella/be/slack/SlackAlarmService.kt (1)
20-26: PII note: verify Slack channel policy for user identifiers.Messages include userId and userName. Ensure this complies with your internal PII handling policy for Slack.
Also applies to: 43-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/kotlin/upbrella/be/config/event/EventConfiguration.kt(1 hunks)src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt(1 hunks)src/main/kotlin/upbrella/be/rent/event/UmbrellaRentedEvent.kt(1 hunks)src/main/kotlin/upbrella/be/rent/service/RentService.kt(3 hunks)src/main/kotlin/upbrella/be/slack/SlackAlarmService.kt(4 hunks)src/main/kotlin/upbrella/be/slack/dto/service/input/SlackAlarmInput.kt(1 hunks)src/main/kotlin/upbrella/be/umbrella/entity/Umbrella.kt(2 hunks)src/main/kotlin/upbrella/be/util/event/Event.kt(1 hunks)src/main/kotlin/upbrella/be/util/event/Events.kt(1 hunks)
🔇 Additional comments (6)
src/main/kotlin/upbrella/be/util/event/Event.kt (1)
3-5: Base event abstraction looks goodSimple, immutable timestamp on creation. Fits Spring’s ability to publish arbitrary objects.
src/main/kotlin/upbrella/be/slack/dto/service/input/SlackAlarmInput.kt (1)
3-13: DTOs are minimal and purpose-fitClean separation between rent and condition report inputs. Names are clear.
src/main/kotlin/upbrella/be/util/event/Events.kt (1)
8-10: No early Events.raise usage detectedA global search (
rg "Events\.raise\(") found a single call site inRentService.kt:79, which occurs inside a runtime service method. There are noEvents.raiseinvocations during application startup or bean initialization.src/main/kotlin/upbrella/be/config/event/EventConfiguration.kt (1)
9-18: Initialization approach is fineSimple and reliable way to wire the static publisher.
src/main/kotlin/upbrella/be/slack/SlackAlarmService.kt (2)
20-26: LGTM: DTO-based notifyRent decouples Slack from domain entities.Nice interface boundary; simpler testing and fewer JPA concerns.
43-49: LGTM: DTO-based notifyConditionReport aligns with the refactor.Consistent with event-driven flow.
| @EventListener(UmbrellaRentedEvent::class) | ||
| fun handleUmbrellaRentedEvent(event: UmbrellaRentedEvent) { | ||
| slackAlarmService.notifyRent( | ||
| NotifyRentInput( | ||
| userId = event.userId, | ||
| userName = event.userName, | ||
| rentStoreName = event.rentStoreName | ||
| ) | ||
| ) | ||
|
|
||
| event.conditionReportContent | ||
| ?.takeIf { it.isNotBlank() } | ||
| ?.let { content -> | ||
| ConditionReport( | ||
| history = event.history, | ||
| content = content | ||
| ).also { conditionReport -> | ||
| conditionReportService.saveConditionReport(conditionReport) | ||
| slackAlarmService.notifyConditionReport( | ||
| NotifyConditionReportInput( | ||
| umbrellaId = event.umbrellaId, | ||
| rentStoreName = event.rentStoreName, | ||
| content = content | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid notifying Slack inside the main transaction; fire AFTER_COMMIT.
Current @eventlistener runs synchronously within the publishing transaction. If Slack fails, it can roll back the rental; if the transaction rolls back, Slack can be sent for a failed rental. Split persistence and side-effects and use @TransactionalEventListener phases.
Apply this refactor:
- @EventListener(UmbrellaRentedEvent::class)
- fun handleUmbrellaRentedEvent(event: UmbrellaRentedEvent) {
- slackAlarmService.notifyRent(
- NotifyRentInput(
- userId = event.userId,
- userName = event.userName,
- rentStoreName = event.rentStoreName
- )
- )
-
- event.conditionReportContent
- ?.takeIf { it.isNotBlank() }
- ?.let { content ->
- ConditionReport(
- history = event.history,
- content = content
- ).also { conditionReport ->
- conditionReportService.saveConditionReport(conditionReport)
- slackAlarmService.notifyConditionReport(
- NotifyConditionReportInput(
- umbrellaId = event.umbrellaId,
- rentStoreName = event.rentStoreName,
- content = content
- )
- )
- }
- }
- }
+ // Persist before commit to keep DB changes atomic with the rental.
+ @TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT)
+ fun persistConditionReport(event: UmbrellaRentedEvent) {
+ event.conditionReportContent
+ ?.takeIf { it.isNotBlank() }
+ ?.let { content ->
+ val conditionReport = ConditionReport(
+ history = event.history,
+ content = content
+ )
+ conditionReportService.saveConditionReport(conditionReport)
+ }
+ }
+
+ // Notify Slack after commit to avoid rollbacks and inconsistencies.
+ @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)
+ fun notifyAfterCommit(event: UmbrellaRentedEvent) {
+ slackAlarmService.notifyRent(
+ NotifyRentInput(
+ userId = event.userId,
+ userName = event.userName,
+ rentStoreName = event.rentStoreName
+ )
+ )
+ event.conditionReportContent
+ ?.takeIf { it.isNotBlank() }
+ ?.let { content ->
+ slackAlarmService.notifyConditionReport(
+ NotifyConditionReportInput(
+ umbrellaId = event.umbrellaId,
+ rentStoreName = event.rentStoreName,
+ content = content
+ )
+ )
+ }
+ }Additional imports required:
- import org.springframework.transaction.event.TransactionalEventListener
- import org.springframework.transaction.event.TransactionPhase
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt lines 17 to 44,
the Slack notification is currently sent synchronously within the main
transaction, risking rollback issues. Refactor the event listener to use
@TransactionalEventListener with phase = TransactionPhase.AFTER_COMMIT to ensure
Slack notifications occur only after the transaction successfully commits.
Separate persistence logic from side-effect notifications by splitting the
method or creating distinct handlers, and add the necessary imports for
TransactionalEventListener and TransactionPhase.
| umbrella.rentUmbrella(rentUmbrellaByUserRequest.storeId) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Concurrent rentals need locking/optimistic concurrency.
umbrella.rentUmbrella only flips a boolean. Without locking, concurrent rentals can both pass checks and set rentable=false with only one History saved correctly, or worse, double-rent. Fetch the umbrella with a write lock or add @Version to Umbrella for optimistic locking.
Options:
- Repository method with PESSIMISTIC_WRITE when loading the umbrella.
- Add optimistic locking to Umbrella:
- Add a version: @Version var version: Long? = null
- Handle OptimisticLockingFailureException on contention.
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/rent/service/RentService.kt around lines 71-72,
the umbrella.rentUmbrella call flips a boolean without concurrency control,
risking double rentals. Fix this by either fetching the umbrella entity with a
PESSIMISTIC_WRITE lock in the repository method or by adding an optimistic
locking mechanism: add a @Version annotated version field to the Umbrella entity
and handle OptimisticLockingFailureException during updates to ensure safe
concurrent access.
| // umbrellaRentedEvent | ||
| Events.raise( | ||
| UmbrellaRentedEvent( | ||
| userId = userToRent.id, | ||
| userName = userToRent.name, | ||
| rentStoreName = rentalStore.name, | ||
| conditionReportContent = rentUmbrellaByUserRequest.conditionReport, | ||
| umbrellaId = umbrella.id!!, | ||
| history = history, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Publish notifications after commit; keep events side-effect-safe.
Events.raise is invoked inside @transactional addRental. With the current @eventlistener, Slack calls run in-tx and can roll back the rental. Align with AFTER_COMMIT listeners as suggested in RentEventHandler.
To verify current event publisher semantics and listeners:
If Events is synchronous/in-tx, proceed with @TransactionalEventListener(phase = AFTER_COMMIT) in handlers (and/or provide a separate Events.raiseAfterCommit if you maintain an event bus wrapper).
🏁 Script executed:
#!/bin/bash
# Inspect event infra and listeners
fd -a "Event*.kt" | sed 's|^|# |'
rg -n "object\\s+Events|class\\s+Events|ApplicationEventPublisher|raise\\(" -A3 -B2
rg -n "@TransactionalEventListener|TransactionPhase|@EventListener\\(" -A2 -B2Length of output: 3100
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/main/kotlin/upbrella/be/config/event/EventConfiguration.ktLength of output: 617
Switch Slack notification listener to AFTER_COMMIT
The Events.raise() call in your addRental transaction publishes events synchronously by default. Any exception in the Slack notification handler will roll back the rental. To avoid side-effects impacting your transaction, handle UmbrellaRentedEvent after commit:
• File: src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt
- @EventListener(UmbrellaRentedEvent::class)
- fun handleUmbrellaRentedEvent(event: UmbrellaRentedEvent) {
+ import org.springframework.transaction.event.TransactionalEventListener
+ import org.springframework.transaction.event.TransactionPhase
+
+ @TransactionalEventListener(
+ phase = TransactionPhase.AFTER_COMMIT,
+ classes = [UmbrellaRentedEvent::class]
+ )
+ fun handleUmbrellaRentedEvent(event: UmbrellaRentedEvent) {
slackAlarmService.notifyRent(
event.userId,
event.userName,
event.rentStoreName,
event.umbrellaId,
event.conditionReportContent,
event.history
)
}This ensures notifications are sent only once the rental transaction has successfully committed.
📝 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.
| // umbrellaRentedEvent | |
| Events.raise( | |
| UmbrellaRentedEvent( | |
| userId = userToRent.id, | |
| userName = userToRent.name, | |
| rentStoreName = rentalStore.name, | |
| conditionReportContent = rentUmbrellaByUserRequest.conditionReport, | |
| umbrellaId = umbrella.id!!, | |
| history = history, | |
| ) | |
| ) | |
| // File: src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt | |
| import org.springframework.transaction.event.TransactionalEventListener | |
| import org.springframework.transaction.event.TransactionPhase | |
| @Component | |
| class RentEventHandler( | |
| private val slackAlarmService: SlackAlarmService | |
| ) { | |
| @TransactionalEventListener( | |
| phase = TransactionPhase.AFTER_COMMIT, | |
| classes = [UmbrellaRentedEvent::class] | |
| ) | |
| fun handleUmbrellaRentedEvent(event: UmbrellaRentedEvent) { | |
| slackAlarmService.notifyRent( | |
| event.userId, | |
| event.userName, | |
| event.rentStoreName, | |
| event.umbrellaId, | |
| event.conditionReportContent, | |
| event.history | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/rent/service/RentService.kt around lines 78 to
88, the Events.raise() call triggers the UmbrellaRentedEvent synchronously
within the transaction, causing Slack notification failures to roll back the
rental. To fix this, modify the event listener in
src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt to handle
UmbrellaRentedEvent with an AFTER_COMMIT transaction phase, ensuring
notifications are sent only after the rental transaction commits successfully.
| private fun send(message: String) { | ||
| val request = mutableMapOf<String, Any>( | ||
| "text" to message | ||
| ) | ||
| val entity = HttpEntity(request) | ||
| restTemplate.exchange(slackBotConfig.webHookUrl, POST, entity, String::class.java) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden Slack sending: don't fail business Tx on webhook errors.
Wrap the HTTP call and log failures so Slack outages don’t cascade into rental failures. This is especially important if listeners run in-tx.
- private fun send(message: String) {
- val request = mutableMapOf<String, Any>(
- "text" to message
- )
- val entity = HttpEntity(request)
- restTemplate.exchange(slackBotConfig.webHookUrl, POST, entity, String::class.java)
- }
+ private fun send(message: String) {
+ val request = mutableMapOf<String, Any>("text" to message)
+ val entity = HttpEntity(request)
+ try {
+ restTemplate.exchange(slackBotConfig.webHookUrl, POST, entity, String::class.java)
+ } catch (ex: Exception) {
+ // Do not propagate; Slack outage should not break rentals.
+ logger.warn("Slack webhook send failed: ${ex.message}", ex)
+ }
+ }Additional code required outside the selected range:
-
Add a logger:
import org.slf4j.LoggerFactoryprivate val logger = LoggerFactory.getLogger(SlackAlarmService::class.java)
📝 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.
| private fun send(message: String) { | |
| val request = mutableMapOf<String, Any>( | |
| "text" to message | |
| ) | |
| val entity = HttpEntity(request) | |
| restTemplate.exchange(slackBotConfig.webHookUrl, POST, entity, String::class.java) | |
| } | |
| private fun send(message: String) { | |
| val request = mutableMapOf<String, Any>("text" to message) | |
| val entity = HttpEntity(request) | |
| try { | |
| restTemplate.exchange(slackBotConfig.webHookUrl, POST, entity, String::class.java) | |
| } catch (ex: Exception) { | |
| // Do not propagate; Slack outage should not break rentals. | |
| logger.warn("Slack webhook send failed: ${ex.message}", ex) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/slack/SlackAlarmService.kt around lines 66 to 72,
the send function currently makes an HTTP call to Slack without handling
exceptions, which can cause business transactions to fail if the webhook is
down. To fix this, wrap the restTemplate.exchange call in a try-catch block,
catch any exceptions, and log the error using a logger instead of letting the
exception propagate. Also, add a logger instance to the class by importing
org.slf4j.LoggerFactory and initializing a private val logger for
SlackAlarmService.
| fun rentUmbrella(storeIdForRent: Long) { | ||
| if (this.storeMeta.id != storeIdForRent) { | ||
| throw UmbrellaStoreMissMatchException("[ERROR] 해당 우산은 해당 매장에 존재하지 않습니다.") | ||
| } | ||
| if (this.missed) { | ||
| throw MissingUmbrellaException("[ERROR] 해당 우산은 분실되었습니다.") | ||
| } | ||
| if (!this.rentable) { | ||
| throw NotAvailableUmbrellaException("[ERROR] 해당 우산은 대여중입니다.") | ||
| } | ||
| this.rentable = false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Block rentals of deleted umbrellas inside the entity (consistency with cannotBeRented).
rentUmbrella currently checks store mismatch, missed, and rentable, but not deleted. This allows renting a logically deleted umbrella if rentUmbrella is called directly. Add a deleted check (or reuse cannotBeRented) to keep invariants consistent.
Apply within this method:
fun rentUmbrella(storeIdForRent: Long) {
if (this.storeMeta.id != storeIdForRent) {
throw UmbrellaStoreMissMatchException("[ERROR] 해당 우산은 해당 매장에 존재하지 않습니다.")
}
if (this.missed) {
throw MissingUmbrellaException("[ERROR] 해당 우산은 분실되었습니다.")
}
+ if (this.deleted) {
+ throw CannotBeRentedException("[ERROR] 해당 우산은 대여 불가능한 우산입니다.")
+ }
if (!this.rentable) {
throw NotAvailableUmbrellaException("[ERROR] 해당 우산은 대여중입니다.")
}
this.rentable = false
}Additional change required outside the selected range:
- Add the import:
import upbrella.be.rent.exception.CannotBeRentedException
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/umbrella/entity/Umbrella.kt around lines 55 to
65, the rentUmbrella method lacks a check for deleted umbrellas, allowing
logically deleted umbrellas to be rented. Add a condition to check if the
umbrella is deleted, and if so, throw a CannotBeRentedException to maintain
consistency with the cannotBeRented logic. Also, add the import statement for
CannotBeRentedException at the top of the file as indicated.
| import org.springframework.context.ApplicationEventPublisher | ||
|
|
||
| object Events { | ||
| private var publisher: ApplicationEventPublisher? = null |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the publisher and narrow the API surface
- Make publisher volatile for safe publication across threads.
- Narrow raise to accept Event to avoid publishing arbitrary objects by mistake.
Apply:
object Events {
- private var publisher: ApplicationEventPublisher? = null
+ @Volatile
+ private var publisher: ApplicationEventPublisher? = null
- fun raise(event: Any) {
+ fun raise(event: Event) {
publisher?.publishEvent(event)
?: throw IllegalStateException("ApplicationEventPublisher가 설정되지 않았습니다.")
}
}Also applies to: 12-15
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/util/event/Events.kt around lines 6 and 12-15,
make the publisher variable volatile to ensure thread-safe publication. Also,
modify the raise function to accept only Event type parameters instead of any
object, narrowing the API surface and preventing accidental publishing of
arbitrary objects. Update the variable declaration and method signature
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/kotlin/upbrella/be/rent/service/RentService.kt (2)
78-88: Publish side effects after commit; ensure handler uses AFTER_COMMITRaising the event inside the @transactional method is fine. Ensure the UmbrellaRentedEvent handler is annotated with @TransactionalEventListener(phase = AFTER_COMMIT) so failures in Slack/condition-report flows cannot roll back the rental.
Run this to confirm AFTER_COMMIT usage:
#!/bin/bash set -euo pipefail echo "== Find listeners for UmbrellaRentedEvent and check their transaction phase ==" rg -n "@TransactionalEventListener|@EventListener\\(" -A3 -B2 rg -n "UmbrellaRentedEvent" -A3 -B3
71-72: Domain validation moved into Umbrella.rentUmbrella — good, but add concurrency control to prevent double-rentThe call umbrella.rentUmbrella(storeId) centralizes validation, but without locking/optimistic concurrency, two concurrent rentals can both pass checks and flip rentable=false (or otherwise corrupt state).
Prefer fetching the umbrella with a write lock or using optimistic locking:
Option A (pessimistic lock): load with SELECT ... FOR UPDATE in your service/repository.
- Add a repository method with @lock(PESSIMISTIC_WRITE) (or JPA hint for Hibernate).
- Update this call site to use a for-update fetch.
Apply this minimal diff here (requires adding findUmbrellaByIdForUpdate in UmbrellaService):
- val umbrella = umbrellaService.findUmbrellaById(rentUmbrellaByUserRequest.umbrellaId) + val umbrella = umbrellaService.findUmbrellaByIdForUpdate(rentUmbrellaByUserRequest.umbrellaId) umbrella.rentUmbrella(rentUmbrellaByUserRequest.storeId)Option B (optimistic lock): add @Version var version: Long? to Umbrella, and handle OptimisticLockingFailureException with a retry (bounded).
🧹 Nitpick comments (7)
src/test/kotlin/upbrella/be/rent/service/RentServiceTest.kt (1)
155-156: Prevent global state leakage from Events publisher in testsSetting a global publisher affects other tests. Ensure it’s consistently set for each test class or reset after tests to avoid cross-test interference.
You can add an @AfterEach to restore the previous publisher (if the API exposes it), or at minimum ensure every test class interacting with Events sets NoOp in its setup. Example outside this hunk:
// Example: ensure isolation (if Events exposes getter) private lateinit var previousPublisher: Any @BeforeEach fun setUp() { previousPublisher = Events.getPublisher() // if available Events.setPublisher(NoOpPublisher) } @AfterEach fun tearDown() { Events.setPublisher(previousPublisher) // if available }If a getter isn’t available, consider adding one to the Events utility or providing a TestEventsRule/extension to encapsulate setup/teardown.
src/main/kotlin/upbrella/be/rent/service/ConditionReportService.kt (1)
11-17: Add transactional semantics to service methodsReads should be marked read-only for performance; saves should run within a write transaction. This also aligns with Spring best practices and prevents lazy-init oddities.
Apply this diff:
import org.springframework.stereotype.Service +import org.springframework.transaction.annotation.Transactional import upbrella.be.rent.dto.response.ConditionReportPageResponse import upbrella.be.rent.entity.ConditionReport import upbrella.be.rent.repository.ConditionReportRepository import upbrella.be.rent.repository.CustomConditionReportRepository @Service class ConditionReportService( private val conditionReportRepository: ConditionReportRepository, private val customConditionReportRepository: CustomConditionReportRepository, ) { - fun findAll(): ConditionReportPageResponse = + @Transactional(readOnly = true) + fun findAll(): ConditionReportPageResponse = ConditionReportPageResponse.of(customConditionReportRepository.findAllConditionReport()) - fun saveConditionReport(conditionReport: ConditionReport) { + @Transactional + fun saveConditionReport(conditionReport: ConditionReport) { conditionReportRepository.save(conditionReport) } }src/main/kotlin/upbrella/be/rent/dto/response/ConditionReportResponse.kt (1)
3-8: Clarify the meaning of id (historyId vs conditionReportId)The repository now selects either conditionReport.id or historyId depending on the chosen fix. To prevent confusion and accidental API regressions, consider renaming the field to
historyIdif that’s the intended identity exposed to clients; otherwise, ensure all call sites and tests expectconditionReportId.If you decide to change the name, update the DTO and its usages:
data class ConditionReportResponse( val historyId: Long, val umbrellaUuid: Long, val content: String?, val etc: String? )src/test/kotlin/upbrella/be/rent/service/ConditionReportServiceTest.kt (2)
80-85: Update to historyId is correct, but the local entity setup is now unusedConditionReport(historyId = history.id!!) matches the entity API change; however, the conditionReport instance created here is not used in this test path anymore. Consider removing this setup to keep the test lean.
Apply this diff to remove the unused setup:
- conditionReport = ConditionReport( - id = 1L, - content = "content", - historyId = history.id!!, - etc = "etc", - )
107-115: Stub uses DTO list — good. Add interaction verification to guard the new pathThe stub is correct. Strengthen the test by verifying that:
- customConditionReportRepository.findAllConditionReport() is called once
- conditionReportRepository is not touched
Apply this diff to add interaction verifications:
// then assertAll( { assertThat(allConditionReports) .usingRecursiveComparison() .isEqualTo(conditionReportsResponse) }, { assertThat(allConditionReports.conditionReports.size).isEqualTo(1) } - ) + ) + // verify repository interactions + then(customConditionReportRepository).should(times(1)).findAllConditionReport() + org.mockito.Mockito.verifyNoInteractions(conditionReportRepository)Note: verifyNoInteractions requires import of org.mockito.Mockito.verifyNoInteractions or the fully qualified reference as shown above.
src/main/kotlin/upbrella/be/rent/service/RentService.kt (2)
78-88: Include stable identifiers in the event payloadrentStoreName is useful for human-readable notifications, but handlers may also need a stable key. Consider including rentStoreId in UmbrellaRentedEvent to avoid extra lookups later.
Example change at this call site (requires adding rentStoreId: Long to the event class):
Events.raise( UmbrellaRentedEvent( userId = userToRent.id, userName = userToRent.name, - rentStoreName = rentalStore.name, + rentStoreName = rentalStore.name, + rentStoreId = rentalStore.id!!, conditionReportContent = rentUmbrellaByUserRequest.conditionReport, umbrellaId = umbrella.id!!, historyId = history.id!!, ) )
78-88: Normalize optional condition report contentIf conditionReportContent can be null/blank, ensure the handler treats blank as “no report.” Optionally normalize here to reduce branching downstream:
- conditionReportContent = rentUmbrellaByUserRequest.conditionReport, + conditionReportContent = rentUmbrellaByUserRequest.conditionReport?.takeIf { it.isNotBlank() },Confirm UmbrellaRentedEvent.conditionReportContent is nullable (String?) if you apply this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/kotlin/upbrella/be/rent/dto/response/ConditionReportResponse.kt(1 hunks)src/main/kotlin/upbrella/be/rent/entity/ConditionReport.kt(1 hunks)src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt(1 hunks)src/main/kotlin/upbrella/be/rent/event/UmbrellaRentedEvent.kt(1 hunks)src/main/kotlin/upbrella/be/rent/repository/CustomConditionReportRepository.kt(1 hunks)src/main/kotlin/upbrella/be/rent/service/ConditionReportService.kt(1 hunks)src/main/kotlin/upbrella/be/rent/service/RentService.kt(3 hunks)src/test/kotlin/upbrella/be/config/event/NoOpPublisher.kt(1 hunks)src/test/kotlin/upbrella/be/rent/service/ConditionReportServiceTest.kt(4 hunks)src/test/kotlin/upbrella/be/rent/service/RentServiceTest.kt(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/kotlin/upbrella/be/rent/event/UmbrellaRentedEvent.kt
- src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
src/test/kotlin/upbrella/be/rent/service/RentServiceTest.kt (2)
151-153: LGTM: Test updated to use historyId with ConditionReportConstructing ConditionReport with
historyId = history.id!!aligns with the new entity design and keeps tests coherent.
16-19: Event decoupling in tests looks goodThe added event utilities and NoOp publisher wiring are appropriate to isolate unit tests from the event bus.
Also applies to: 23-23, 49-49
src/main/kotlin/upbrella/be/rent/repository/CustomConditionReportRepository.kt (1)
14-28: Fix unconstrained join and clarify DTO ID mapping
- The current
.join(history)has no join condition, which leads to an unbounded Cartesian product. SinceConditionReportno longer has a direct JPA association toHistory, you must either:
- Use a cross-join with an explicit
WHEREon the FK:.from(conditionReport, history).fetch()
.where(history.id.eq(conditionReport.historyId)).fetch()- Or—if your JPA provider supports it—use `JOIN … ON`: ```diff .from(conditionReport)
.join(history)
.join(history).on(history.id.eq(conditionReport.historyId)) .fetch()
- Confirm which “id” your API should expose in
ConditionReportResponse:
- If it must remain the history ID (legacy behavior), change:
Projections.constructor( ConditionReportResponse::class.java,conditionReport.id,
conditionReport.historyId, history.umbrella.uuid, conditionReport.content, conditionReport.etc- If it should be the **report** ID, keep `conditionReport.id` but rename the DTO field from `id` to `reportId` (or similar) to avoid confusion.src/test/kotlin/upbrella/be/config/event/NoOpPublisher.kt (2)
10-13: Simple, effective test double — LGTMUsing a singleton no-op ApplicationEventPublisher for tests is clean and prevents side effects.
10-13: Override of publishEvent(ApplicationEvent) is valid in Spring Boot 2.7.13
The project is on Spring Boot 2.7.13 (Spring Framework 5.3.x), which still declares both
publishEvent(Object)andpublishEvent(ApplicationEvent). The existing override will compile—no change needed.Likely an incorrect or invalid review comment.
src/test/kotlin/upbrella/be/rent/service/ConditionReportServiceTest.kt (2)
21-21: Switching to CustomConditionReportRepository aligns with service refactorImporting and mocking CustomConditionReportRepository is correct for DTO-based reads.
33-35: Inject mock for CustomConditionReportRepository — LGTMThe service can now be driven via the custom repo stub during tests.
| @OneToOne | ||
| @JoinColumn(name = "history_id") | ||
| val history: History, | ||
| val historyId: Long, |
There was a problem hiding this comment.
Explicitly map historyId to the DB column to avoid naming strategy surprises
Previously the FK column was history_id via @JoinColumn. Now that it's a scalar, relying on implicit naming may break if the physical naming strategy changes. Map it explicitly.
Apply this diff:
class ConditionReport(
- val historyId: Long,
+ @Column(name = "history_id", nullable = false)
+ val historyId: Long,
val content: String? = null,
val etc: String? = null,
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
val id: Long? = null
)📝 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.
| val historyId: Long, | |
| class ConditionReport( | |
| @Column(name = "history_id", nullable = false) | |
| val historyId: Long, | |
| val content: String? = null, | |
| val etc: String? = null, | |
| @Id | |
| @GeneratedValue(strategy = GenerationType.IDENTITY) | |
| val id: Long? = null | |
| ) |
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/rent/entity/ConditionReport.kt around line 7, the
scalar property historyId is not explicitly mapped to the DB column and can
break if the physical naming strategy changes; add an explicit JPA column
mapping by annotating the property with @Column(name = "history_id") (and import
javax.persistence.Column if not present) so the field is reliably persisted to
the expected history_id column.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/main/kotlin/upbrella/be/rent/entity/ImprovementReport.kt (1)
7-12: Switching from association to FK id requires query changes and explicit column mappingYou removed the association to History and now store only historyId. That’s fine, but:
- Queries must join via ON with history.id = improvementReport.historyId (see repository comment).
- Add explicit @column to lock the column name and nullability, avoiding surprises from naming strategies and enforcing integrity.
Apply this minimal diff:
@Entity class ImprovementReport( - val historyId: Long, + @Column(name = "history_id", nullable = false) + val historyId: Long, val content: String? = null, val etc: String? = null, @Id @GeneratedValue(strategy = GenerationType.IDENTITY) val id: Long? = null )src/main/kotlin/upbrella/be/rent/service/RentService.kt (1)
91-106: Missing event publication for umbrella return.The return flow doesn't emit an
UmbrellaReturnedEvent, which breaks the event-driven architecture pattern established for rentals. The event handler expects this event but it's never raised.Add event publication after saving the history:
returnedUmbrella.returnUmbrella(returnStore) rentRepository.save(history) + + // Raise return event + Events.raise( + UmbrellaReturnedEvent( + rentUserId = userToReturn.id!!, + rentStoreName = history.rentStoreMeta.name, + rentedAt = history.rentedAt, + returnStoreName = returnStore.name, + returnedAt = history.returnedAt!!, + historyId = history.id!!, + improvementReportContent = request.improvementReport, + returnedUmbrellaId = returnedUmbrella.id!! + ) + )Also import the event class:
+import upbrella.be.rent.event.UmbrellaReturnedEvent
♻️ Duplicate comments (4)
src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt (1)
24-51: Use@TransactionalEventListenerwithAFTER_COMMITphase for Slack notifications.The current
@EventListenerruns synchronously within the transaction. If Slack fails, it will roll back the rental. External notifications should occur after the transaction commits successfully.Apply this refactor to separate persistence from notifications:
+import org.springframework.transaction.event.TransactionalEventListener +import org.springframework.transaction.event.TransactionPhase - @EventListener(UmbrellaRentedEvent::class) - fun handleUmbrellaRentedEvent(event: UmbrellaRentedEvent) { + // Persist condition reports within the transaction + @TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT) + fun persistConditionReport(event: UmbrellaRentedEvent) { + event.conditionReportContent + ?.takeIf { it.isNotBlank() } + ?.let { content -> + val conditionReport = ConditionReport( + historyId = event.historyId, + content = content + ) + conditionReportService.saveConditionReport(conditionReport) + } + } + + // Send notifications after successful commit + @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) + fun notifyRentAfterCommit(event: UmbrellaRentedEvent) { slackAlarmService.notifyRent( NotifyRentInput( userId = event.userId, userName = event.userName, rentStoreName = event.rentStoreName ) ) event.conditionReportContent ?.takeIf { it.isNotBlank() } ?.let { content -> - ConditionReport( - historyId = event.historyId, - content = content - ).also { conditionReport -> - conditionReportService.saveConditionReport(conditionReport) - slackAlarmService.notifyConditionReport( - NotifyConditionReportInput( - umbrellaId = event.umbrellaId, - rentStoreName = event.rentStoreName, - content = content - ) + slackAlarmService.notifyConditionReport( + NotifyConditionReportInput( + umbrellaId = event.umbrellaId, + rentStoreName = event.rentStoreName, + content = content ) - } + ) } }src/main/kotlin/upbrella/be/slack/SlackAlarmService.kt (1)
65-71: Add error handling for Slack webhook failures.HTTP calls to external services can fail and shouldn't crash the application. Wrap the call in a try-catch block and log failures.
+import org.slf4j.LoggerFactory + +@Service +class SlackAlarmService( + private val slackBotConfig: SlackBotConfig, + private val restTemplate: RestTemplate +) { + private val logger = LoggerFactory.getLogger(SlackAlarmService::class.java) + private fun send(message: String) { val request = mutableMapOf<String, Any>( "text" to message ) val entity = HttpEntity(request) - restTemplate.exchange(slackBotConfig.webHookUrl, POST, entity, String::class.java) + try { + restTemplate.exchange(slackBotConfig.webHookUrl, POST, entity, String::class.java) + } catch (ex: Exception) { + // Log the error but don't propagate - Slack outage shouldn't break business logic + logger.warn("Failed to send Slack notification: ${ex.message}", ex) + } }src/main/kotlin/upbrella/be/rent/service/RentService.kt (2)
78-87: Event publishing within transaction needs careful consideration.The
Events.raise()call happens inside the transaction. Since the event handlers are not using@TransactionalEventListener, they execute synchronously within the same transaction. Any failure in the event handlers will roll back the rental.Verify the event infrastructure configuration:
#!/bin/bash # Check if Events uses ApplicationEventPublisher and how it's configured ast-grep --pattern 'object Events { $$$ fun raise($_) { $$$ } $$$ }' # Check for any TransactionSynchronizationManager usage rg "TransactionSynchronizationManager|afterCommit|TransactionPhase" --glob "**/*.kt"If Events publishes synchronously, ensure all event handlers use
@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)for external calls.
70-70: Add concurrency control for umbrella rental.The
umbrella.rentUmbrella()method modifies state without proper locking. Concurrent rentals could result in race conditions where multiple users rent the same umbrella.Consider one of these approaches:
Pessimistic locking - Fetch the umbrella with a write lock:
// In UmbrellaRepository @Lock(LockModeType.PESSIMISTIC_WRITE) fun findByIdForUpdate(id: Long): Optional<Umbrella>Optimistic locking - Add version control to the Umbrella entity:
// In Umbrella entity @Version var version: Long? = nullThen handle
OptimisticLockingFailureExceptionwith retry logic.
🧹 Nitpick comments (7)
src/main/kotlin/upbrella/be/rent/dto/response/ImprovementReportResponse.kt (2)
3-8: Avoid default placeholder values in DTO constructor to surface mapping issues earlyUsing defaults like 0 or empty strings can mask projection/mapping issues and leak invalid data to clients. Prefer non-defaulted constructor params and let QueryDSL projection populate all fields.
Apply this diff:
data class ImprovementReportResponse( - val id: Long = 0, - val umbrellaUuid: Long = 0, - val content: String? = "", - val etc: String? = "" + val id: Long, + val umbrellaUuid: Long, + val content: String?, + val etc: String? )
5-5: Naming consistency: umbrellaUuid vs umbrellaId elsewhereThis DTO exposes umbrellaUuid, while Slack inputs use umbrellaId. If you intend to expose the user-facing UUID consistently across APIs and notifications, consider aligning naming to avoid confusion.
src/main/kotlin/upbrella/be/rent/entity/ImprovementReport.kt (1)
5-13: Optional: Keep a read-only association for type-safe joins and future fetchesIf you frequently need History (or nested fields like umbrella.uuid), add a read-only association. This keeps the FK field while enabling association-based queries without accidental writes.
Apply in addition to the previous diff:
import javax.persistence.* +import upbrella.be.rent.entity.History @Entity class ImprovementReport( @Column(name = "history_id", nullable = false) val historyId: Long, val content: String? = null, val etc: String? = null, + @ManyToOne(fetch = FetchType.LAZY) + @JoinColumn(name = "history_id", insertable = false, updatable = false) + val history: History? = null, @Id @GeneratedValue(strategy = GenerationType.IDENTITY) val id: Long? = null )Note: This preserves the FK column as the source of truth but allows association-based read queries.
src/main/kotlin/upbrella/be/rent/repository/CustomImprovementReportRepository.kt (1)
16-24: Constructor projection is order-sensitive; consider field-based projection for resilienceThe current
Projections.constructorrelies on parameter order matching the data class constructor. To reduce brittleness when DTOs evolve, preferProjections.fieldswith aliases matching property names.Example:
- Projections.constructor( - ImprovementReportResponse::class.java, - improvementReport.id, - history.umbrella.uuid, - improvementReport.content, - improvementReport.etc - ) + Projections.fields( + ImprovementReportResponse::class.java, + improvementReport.id.`as`("id"), + history.umbrella.uuid.`as`("umbrellaUuid"), + improvementReport.content.`as`("content"), + improvementReport.etc.`as`("etc"), + )src/main/kotlin/upbrella/be/slack/dto/service/input/SlackAlarmInput.kt (2)
15-22: Prefer time types over String for timestampsPassing
rentedAt/returnedAtas String pushes formatting concerns to call sites and risks inconsistent or locale-dependent formats. AcceptInstant/ZonedDateTime/LocalDateTimehere and format in SlackAlarmService.data class NotifyReturnInput( val userId: Long, val rentStoreName: String, - val rentedAt: String, + val rentedAt: java.time.LocalDateTime, val returnStoreName: String, - val returnedAt: String?, + val returnedAt: java.time.LocalDateTime?, val unrefundedCount: Long, )
9-13: Naming inconsistency with umbrella identifierThese inputs use
umbrellaId, while other DTOs exposeumbrellaUuid. If Slack messages intend to show the UUID (commonly the user-facing number), rename toumbrellaUuidto avoid ambiguity and inadvertent mixing of DB PK vs UUID.src/test/kotlin/upbrella/be/slack/service/SlackAlarmServiceTest.kt (1)
138-141: Strengthen assertions to verify payload contentCurrently you only assert the title text. Also assert that key fields (store names, userId, unrefundedCount) are present to catch regressions in message templating.
For example:
assertTrue( - requestEntityCaptor.value.body.toString().contains("우산 반납 알림") + requestEntityCaptor.value.body.toString().contains("우산 반납 알림") ) +assertTrue(requestEntityCaptor.value.body.toString().contains(foundStoreMeta.name)) +assertTrue(requestEntityCaptor.value.body.toString().contains(userToRent.id!!.toString())) +assertTrue(requestEntityCaptor.value.body.toString().contains("1"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/kotlin/upbrella/be/rent/dto/response/ImprovementReportResponse.kt(1 hunks)src/main/kotlin/upbrella/be/rent/entity/ImprovementReport.kt(1 hunks)src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt(1 hunks)src/main/kotlin/upbrella/be/rent/event/UmbrellaReturnedEvent.kt(1 hunks)src/main/kotlin/upbrella/be/rent/repository/CustomImprovementReportRepository.kt(1 hunks)src/main/kotlin/upbrella/be/rent/service/ImprovementReportService.kt(1 hunks)src/main/kotlin/upbrella/be/rent/service/RentService.kt(3 hunks)src/main/kotlin/upbrella/be/slack/SlackAlarmService.kt(1 hunks)src/main/kotlin/upbrella/be/slack/dto/service/input/SlackAlarmInput.kt(1 hunks)src/test/kotlin/upbrella/be/slack/service/SlackAlarmServiceTest.kt(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
src/main/kotlin/upbrella/be/slack/dto/service/input/SlackAlarmInput.kt (1)
3-7: PII awareness: userName in SlackConfirm that including
userNamein Slack is compliant with your PII policy. If needed, prefer a non-PII alias (e.g., nickname) or hash, or ensure Slack channels are appropriately restricted.src/main/kotlin/upbrella/be/rent/service/ImprovementReportService.kt (2)
10-13: LGTM! Clean repository separation following SOLID principles.The introduction of
CustomImprovementReportRepositoryproperly separates complex query logic from basic CRUD operations, adhering to the Single Responsibility Principle.
15-16: Empty‐list cases are already handled
ThefindAll()method andImprovementReportPageResponse.of(...)both work with a non‐nullableList<…>, and Spring Data will return an empty list (nevernull) when no records are found. JSON serialization will produce an empty array forimprovementReports, so no additional null or empty‐list checks are required.src/main/kotlin/upbrella/be/rent/event/UmbrellaReturnedEvent.kt (1)
6-15: LGTM! Well-structured event payload.The event class properly extends the base
Eventclass and contains all necessary fields for umbrella return notifications. The use of nullableimprovementReportContentis appropriate for optional feedback.src/main/kotlin/upbrella/be/slack/SlackAlarmService.kt (1)
8-11: LGTM! Clean separation using DTOs.The refactoring to use input DTOs instead of domain entities properly decouples the Slack service from the domain model, following the Dependency Inversion Principle.
| @EventListener(UmbrellaReturnedEvent::class) | ||
| fun handleUmbrellaReturnedEvent(event: UmbrellaReturnedEvent) { | ||
|
|
||
| val unrefundedRentCount = rentService.countUnrefundedRent() | ||
| slackAlarmService.notifyReturn( | ||
| NotifyReturnInput( | ||
| userId = event.rentUserId, | ||
| rentStoreName = event.rentStoreName, | ||
| rentedAt = event.rentedAt.toString(), | ||
| returnStoreName = event.returnStoreName, | ||
| returnedAt = event.returnedAt.toString(), | ||
| unrefundedCount = unrefundedRentCount | ||
| ) | ||
| ) | ||
|
|
||
| event.improvementReportContent?.takeIf { it.isNotBlank() } | ||
| ?.let { content -> | ||
| ImprovementReport( | ||
| historyId = event.historyId, | ||
| content = content | ||
| ).also { improvementReport -> | ||
| improvementReportService.save(improvementReport) | ||
| slackAlarmService.notifyImprovementReport( | ||
| NotifyImprovementReportInput( | ||
| umbrellaId = event.returnedUmbrellaId, | ||
| rentStoreName = event.rentStoreName, | ||
| rentedAt = event.rentedAt.toString(), | ||
| returnStoreName = event.returnStoreName, | ||
| returnedAt = event.returnedAt.toString(), | ||
| content = content | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Apply the same transaction phase separation for return events.
Similar to the rental event handler, separate persistence from notifications to ensure Slack failures don't impact the return transaction.
- @EventListener(UmbrellaReturnedEvent::class)
- fun handleUmbrellaReturnedEvent(event: UmbrellaReturnedEvent) {
+ // Persist improvement reports within the transaction
+ @TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT)
+ fun persistImprovementReport(event: UmbrellaReturnedEvent) {
+ event.improvementReportContent?.takeIf { it.isNotBlank() }
+ ?.let { content ->
+ val improvementReport = ImprovementReport(
+ historyId = event.historyId,
+ content = content
+ )
+ improvementReportService.save(improvementReport)
+ }
+ }
+
+ // Send notifications after successful commit
+ @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)
+ fun notifyReturnAfterCommit(event: UmbrellaReturnedEvent) {
val unrefundedRentCount = rentService.countUnrefundedRent()
slackAlarmService.notifyReturn(
NotifyReturnInput(
userId = event.rentUserId,
rentStoreName = event.rentStoreName,
rentedAt = event.rentedAt.toString(),
returnStoreName = event.returnStoreName,
returnedAt = event.returnedAt.toString(),
unrefundedCount = unrefundedRentCount
)
)
event.improvementReportContent?.takeIf { it.isNotBlank() }
?.let { content ->
- ImprovementReport(
- historyId = event.historyId,
- content = content
- ).also { improvementReport ->
- improvementReportService.save(improvementReport)
- slackAlarmService.notifyImprovementReport(
- NotifyImprovementReportInput(
- umbrellaId = event.returnedUmbrellaId,
- rentStoreName = event.rentStoreName,
- rentedAt = event.rentedAt.toString(),
- returnStoreName = event.returnStoreName,
- returnedAt = event.returnedAt.toString(),
- content = content
- )
+ slackAlarmService.notifyImprovementReport(
+ NotifyImprovementReportInput(
+ umbrellaId = event.returnedUmbrellaId,
+ rentStoreName = event.rentStoreName,
+ rentedAt = event.rentedAt.toString(),
+ returnStoreName = event.returnStoreName,
+ returnedAt = event.returnedAt.toString(),
+ content = content
)
- }
+ )
}
}🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt around lines
53–87, the handler currently mixes persistence (saving improvement reports /
counting unrefunded rents) with Slack notification calls so Slack failures can
break the return transaction; separate persistence from notifications by
ensuring all DB actions run inside the transaction and notifications run only
after commit: either (A) split the logic into two methods — keep the DB work
(countUnrefundedRent, improvementReportService.save) in the transactional
listener, and move slackAlarmService.notifyReturn / notifyImprovementReport into
a separate listener annotated with @TransactionalEventListener(phase =
AFTER_COMMIT), or (B) persist the improvement report and compute unrefunded
count first, then trigger notifications asynchronously after the transaction
completes; implement one of these approaches so Slack calls cannot roll back or
fail the DB transaction.
| .from(improvementReport) | ||
| .join(history) | ||
| .fetchJoin() | ||
| .fetch() |
There was a problem hiding this comment.
Bug: Cartesian join and invalid fetchJoin due to missing association
.join(history).fetchJoin() without an ON clause produces a Cartesian product, because ImprovementReport no longer has an association to History. Also, fetchJoin is only valid for association path joins. This will return incorrect/duplicated rows and may throw at runtime depending on the JPA provider.
Fix the join by joining on the FK and remove fetchJoin:
.from(improvementReport)
- .join(history)
- .fetchJoin()
+ .join(history)
+ .on(history.id.eq(improvementReport.historyId))
.fetch()If some ImprovementReports could reference missing History rows (ideally they should not), use leftJoin instead of join.
📝 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.
| .from(improvementReport) | |
| .join(history) | |
| .fetchJoin() | |
| .fetch() | |
| // … | |
| .from(improvementReport) | |
| .join(history) | |
| .on(history.id.eq(improvementReport.historyId)) | |
| .fetch() |
🤖 Prompt for AI Agents
In
src/main/kotlin/upbrella/be/rent/repository/CustomImprovementReportRepository.kt
around lines 25 to 28, the code uses .join(history).fetchJoin() which creates a
Cartesian product and is invalid because ImprovementReport no longer has an
association to History; change the join to an explicit join-on using the FK
(e.g. join(history).on(improvementReport.historyId.eq(history.id))) and remove
fetchJoin() since fetchJoin only applies to path associations; if some
ImprovementReport rows may lack a matching History use leftJoin(...).on(...)
instead of join(...).
| slackAlarmService.notifyReturn( | ||
| NotifyReturnInput( | ||
| userId = userToRent.id!!, | ||
| rentStoreName = foundStoreMeta.name, | ||
| rentedAt = history.rentedAt.toString(), | ||
| returnStoreName = foundStoreMeta.name, | ||
| returnedAt = history.returnedAt.toString(), | ||
| unrefundedCount = 1L | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add coverage for null returnedAt and timestamp formatting
The input allows returnedAt to be null; add a test ensuring the Slack payload renders this case gracefully. Also, prefer formatting timestamps with a fixed formatter to avoid environment-dependent toString().
Example test addition (pseudo-diff):
@Test
@DisplayName("우산을 반납하면 Slack 봇으로 잔여 환급 개수와 함께 알림이 전송된다.")
fun notifyReturn() {
...
}
+
+@Test
+@DisplayName("반납 시간이 없을 때도 Slack 알림이 정상 전송된다.")
+fun notifyReturn_withoutReturnedAt() {
+ given(slackBotConfig.webHookUrl).willReturn("https://hooks.slack.com/services")
+ given(restTemplate.exchange(anyString(), any(HttpMethod::class.java), any(), any(Class::class.java))).willReturn(null)
+
+ slackAlarmService.notifyReturn(
+ NotifyReturnInput(
+ userId = userToRent.id!!,
+ rentStoreName = foundStoreMeta.name,
+ rentedAt = history.rentedAt.toString(),
+ returnStoreName = foundStoreMeta.name,
+ returnedAt = null,
+ unrefundedCount = 1L
+ )
+ )
+
+ then(restTemplate).should(times(1))
+ .exchange(anyString(), any(HttpMethod::class.java), any(), any(Class::class.java))
+}📝 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.
| slackAlarmService.notifyReturn( | |
| NotifyReturnInput( | |
| userId = userToRent.id!!, | |
| rentStoreName = foundStoreMeta.name, | |
| rentedAt = history.rentedAt.toString(), | |
| returnStoreName = foundStoreMeta.name, | |
| returnedAt = history.returnedAt.toString(), | |
| unrefundedCount = 1L | |
| ) | |
| ) | |
| @Test | |
| @DisplayName("우산을 반납하면 Slack 봇으로 잔여 환급 개수와 함께 알림이 전송된다.") | |
| fun notifyReturn() { | |
| slackAlarmService.notifyReturn( | |
| NotifyReturnInput( | |
| userId = userToRent.id!!, | |
| rentStoreName = foundStoreMeta.name, | |
| rentedAt = history.rentedAt.toString(), | |
| returnStoreName = foundStoreMeta.name, | |
| returnedAt = history.returnedAt.toString(), | |
| unrefundedCount = 1L | |
| ) | |
| ) | |
| } | |
| @Test | |
| @DisplayName("반납 시간이 없을 때도 Slack 알림이 정상 전송된다.") | |
| fun notifyReturn_withoutReturnedAt() { | |
| given(slackBotConfig.webHookUrl).willReturn("https://hooks.slack.com/services") | |
| given(restTemplate.exchange(anyString(), any(HttpMethod::class.java), any(), any(Class::class.java))) | |
| .willReturn(null) | |
| slackAlarmService.notifyReturn( | |
| NotifyReturnInput( | |
| userId = userToRent.id!!, | |
| rentStoreName = foundStoreMeta.name, | |
| rentedAt = history.rentedAt.toString(), | |
| returnStoreName = foundStoreMeta.name, | |
| returnedAt = null, | |
| unrefundedCount = 1L | |
| ) | |
| ) | |
| then(restTemplate).should(times(1)) | |
| .exchange(anyString(), any(HttpMethod::class.java), any(), any(Class::class.java)) | |
| } |
🤖 Prompt for AI Agents
In src/test/kotlin/upbrella/be/slack/service/SlackAlarmServiceTest.kt around
lines 112 to 121, the test calls NotifyReturnInput using
history.rentedAt.toString() and history.returnedAt.toString(), but returnedAt
can be null and using toString() makes the output environment-dependent; update
the test to (1) add a new case where returnedAt is null and assert the produced
Slack payload renders that field gracefully (e.g., a placeholder or omitted
value), and (2) replace uses of toString() with a fixed formatter (e.g.,
DateTimeFormatter.ISO_LOCAL_DATE_TIME or the project’s canonical formatter) when
building expected and actual payload strings so timestamps are deterministic
across environments. Ensure assertions check the exact formatted timestamp and
the null-handling behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/kotlin/upbrella/be/rent/service/ConditionReportServiceTest.kt (1)
117-130: Consider asserting the delegated repository call.This test now validates the wrapped response only. Since the refactor is specifically moving
findAll()ontoCustomConditionReportRepository, adding an interaction assertion would make that handoff explicit and catch delegation regressions sooner.Suggested assertion
assertAll( { assertThat(allConditionReports) .usingRecursiveComparison() .isEqualTo(conditionReportsResponse) }, { assertThat(allConditionReports.conditionReports.size).isEqualTo(1) } ) + then(customConditionReportRepository).should(times(1)).findAllConditionReport()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/upbrella/be/rent/service/ConditionReportServiceTest.kt` around lines 117 - 130, Add an interaction assertion that the service delegates to the custom repository: after calling conditionReportService.findAll() in ConditionReportServiceTest, verify that the CustomConditionReportRepository (or the mocked repository backing conditionReportService) had its findAll() method invoked exactly once (e.g., using Mockito/Kotlin verify/verifyExactly). This ensures the test asserts the delegation to CustomConditionReportRepository.findAll() in addition to the existing response assertions.src/test/kotlin/upbrella/be/rent/service/RentServiceTest.kt (1)
17-17: Remove unused import.The
@Autowiredimport is not used in this test class, which relies on Mockito's@Mock/@InjectMocksannotations for dependency injection.🧹 Proposed fix
import org.mockito.kotlin.mock -import org.springframework.beans.factory.annotation.Autowired import org.springframework.context.ApplicationEventPublisher🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/upbrella/be/rent/service/RentServiceTest.kt` at line 17, Remove the unused Spring import in the RentServiceTest class: delete the import line for org.springframework.beans.factory.annotation.Autowired because the test uses Mockito annotations (`@Mock` / `@InjectMocks`) for DI; ensure only necessary imports remain in RentServiceTest.kt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/upbrella/be/rent/entity/ImprovementReport.kt`:
- Around line 6-8: The change replaced the History association with a raw Long
(property historyId on class ImprovementReport) which removes the
one-to-one/foreign-key semantics; restore those constraints by either
reintroducing the association (use the original History relation on
ImprovementReport) or, if you must keep the flattened Long, add a database
migration and model-level metadata to enforce history_id as a FOREIGN KEY
referencing history(id) and UNIQUE to preserve one-to-one semantics (update the
ImprovementReport mapping/annotations and create a migration that adds UNIQUE
and FOREIGN KEY constraints on improvement_report.history_id).
In `@src/main/kotlin/upbrella/be/rent/service/ImprovementReportService.kt`:
- Around line 15-16: The read path now calls
CustomImprovementReportRepository.findAllImprovementReport() but its Querydsl
join still uses history to match via umbrella.uuid; update the repository query
(the method findAllImprovementReport using queryFactory and
Projections.constructor for ImprovementReportResponse) to join history by
history.id.eq(improvementReport.historyId) — i.e., replace the existing
join(...) with .join(history).on(history.id.eq(improvementReport.historyId)) so
the projection can select history.umbrella.uuid and other fields correctly when
ImprovementReport only stores historyId.
---
Nitpick comments:
In `@src/test/kotlin/upbrella/be/rent/service/ConditionReportServiceTest.kt`:
- Around line 117-130: Add an interaction assertion that the service delegates
to the custom repository: after calling conditionReportService.findAll() in
ConditionReportServiceTest, verify that the CustomConditionReportRepository (or
the mocked repository backing conditionReportService) had its findAll() method
invoked exactly once (e.g., using Mockito/Kotlin verify/verifyExactly). This
ensures the test asserts the delegation to
CustomConditionReportRepository.findAll() in addition to the existing response
assertions.
In `@src/test/kotlin/upbrella/be/rent/service/RentServiceTest.kt`:
- Line 17: Remove the unused Spring import in the RentServiceTest class: delete
the import line for org.springframework.beans.factory.annotation.Autowired
because the test uses Mockito annotations (`@Mock` / `@InjectMocks`) for DI; ensure
only necessary imports remain in RentServiceTest.kt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c609d8b-e5c7-4796-97f9-d680326123dc
📒 Files selected for processing (22)
src/main/kotlin/upbrella/be/config/event/EventConfiguration.ktsrc/main/kotlin/upbrella/be/rent/dto/response/ConditionReportResponse.ktsrc/main/kotlin/upbrella/be/rent/dto/response/ImprovementReportResponse.ktsrc/main/kotlin/upbrella/be/rent/entity/ConditionReport.ktsrc/main/kotlin/upbrella/be/rent/entity/ImprovementReport.ktsrc/main/kotlin/upbrella/be/rent/event/RentEventHandler.ktsrc/main/kotlin/upbrella/be/rent/event/UmbrellaRentedEvent.ktsrc/main/kotlin/upbrella/be/rent/event/UmbrellaReturnedEvent.ktsrc/main/kotlin/upbrella/be/rent/repository/CustomConditionReportRepository.ktsrc/main/kotlin/upbrella/be/rent/repository/CustomImprovementReportRepository.ktsrc/main/kotlin/upbrella/be/rent/service/ConditionReportService.ktsrc/main/kotlin/upbrella/be/rent/service/ImprovementReportService.ktsrc/main/kotlin/upbrella/be/rent/service/RentService.ktsrc/main/kotlin/upbrella/be/slack/SlackAlarmService.ktsrc/main/kotlin/upbrella/be/slack/dto/service/input/SlackAlarmInput.ktsrc/main/kotlin/upbrella/be/umbrella/entity/Umbrella.ktsrc/main/kotlin/upbrella/be/util/event/Event.ktsrc/main/kotlin/upbrella/be/util/event/Events.ktsrc/test/kotlin/upbrella/be/config/event/NoOpPublisher.ktsrc/test/kotlin/upbrella/be/rent/service/ConditionReportServiceTest.ktsrc/test/kotlin/upbrella/be/rent/service/RentServiceTest.ktsrc/test/kotlin/upbrella/be/slack/service/SlackAlarmServiceTest.kt
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/kotlin/upbrella/be/rent/repository/CustomConditionReportRepository.kt
- src/main/kotlin/upbrella/be/rent/repository/CustomImprovementReportRepository.kt
- src/main/kotlin/upbrella/be/rent/entity/ConditionReport.kt
- src/main/kotlin/upbrella/be/config/event/EventConfiguration.kt
- src/main/kotlin/upbrella/be/rent/event/RentEventHandler.kt
- src/main/kotlin/upbrella/be/rent/event/UmbrellaRentedEvent.kt
- src/main/kotlin/upbrella/be/rent/dto/response/ConditionReportResponse.kt
- src/main/kotlin/upbrella/be/rent/service/ConditionReportService.kt
- src/main/kotlin/upbrella/be/umbrella/entity/Umbrella.kt
| class ImprovementReport( | ||
| @OneToOne | ||
| @JoinColumn(name = "history_id") | ||
| val history: History, | ||
| val historyId: Long, | ||
| val content: String? = null, |
There was a problem hiding this comment.
Preserve history_id integrity when flattening the relation.
Replacing the History association with a bare Long also drops the one-to-one/fk semantics from the entity model. Unless improvement_report.history_id still has an explicit UNIQUE and FOREIGN KEY constraint, duplicate or orphaned reports become possible, especially if this event flow is retried.
Possible follow-up
-@Entity
+@Entity
+@Table(
+ uniqueConstraints = [UniqueConstraint(name = "uk_improvement_report_history_id", columnNames = ["history_id"])]
+)
class ImprovementReport(
- val historyId: Long,
+ `@Column`(name = "history_id", nullable = false)
+ val historyId: Long,Add a migration as well so history_id remains constrained to history(id).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/upbrella/be/rent/entity/ImprovementReport.kt` around lines 6
- 8, The change replaced the History association with a raw Long (property
historyId on class ImprovementReport) which removes the one-to-one/foreign-key
semantics; restore those constraints by either reintroducing the association
(use the original History relation on ImprovementReport) or, if you must keep
the flattened Long, add a database migration and model-level metadata to enforce
history_id as a FOREIGN KEY referencing history(id) and UNIQUE to preserve
one-to-one semantics (update the ImprovementReport mapping/annotations and
create a migration that adds UNIQUE and FOREIGN KEY constraints on
improvement_report.history_id).
| fun findAll(): ImprovementReportPageResponse = | ||
| ImprovementReportPageResponse.of(findAllImprovementReport()) | ||
| ImprovementReportPageResponse.of(customImprovementReportRepository.findAllImprovementReport()) |
There was a problem hiding this comment.
findAll() now depends on a stale Querydsl join.
Line 16 switches the read path to CustomImprovementReportRepository.findAllImprovementReport(), but the provided repository query still reads history.umbrella.uuid via join(history) even though ImprovementReport now only has historyId. This will break the improvement-report list flow once exercised. Update that query to join history.id to improvementReport.historyId before routing reads through it.
Expected repository shape
return queryFactory
.select(
Projections.constructor(
ImprovementReportResponse::class.java,
improvementReport.id,
history.umbrella.uuid,
improvementReport.content,
improvementReport.etc
)
)
.from(improvementReport)
.join(history).on(history.id.eq(improvementReport.historyId))
.fetch()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/upbrella/be/rent/service/ImprovementReportService.kt` around
lines 15 - 16, The read path now calls
CustomImprovementReportRepository.findAllImprovementReport() but its Querydsl
join still uses history to match via umbrella.uuid; update the repository query
(the method findAllImprovementReport using queryFactory and
Projections.constructor for ImprovementReportResponse) to join history by
history.id.eq(improvementReport.historyId) — i.e., replace the existing
join(...) with .join(history).on(history.id.eq(improvementReport.historyId)) so
the projection can select history.umbrella.uuid and other fields correctly when
ImprovementReport only stores historyId.
2 participants
🟢 구현내용
🧩 고민과 해결과정
Summary by CodeRabbit
New Features
Refactor
Tests