-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: 우산 대여 코드 이벤트 분리 #502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package upbrella.be.config.event | ||
|
|
||
| import org.springframework.beans.factory.InitializingBean | ||
| import org.springframework.context.ApplicationContext | ||
| 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 | ||
| ) { | ||
|
|
||
| @Bean | ||
| fun eventsInitializer(): InitializingBean { | ||
| return InitializingBean { Events.setPublisher(applicationContext) } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,8 @@ | ||
| package upbrella.be.rent.dto.response | ||
|
|
||
| import upbrella.be.rent.entity.ConditionReport | ||
|
|
||
| data class ConditionReportResponse( | ||
| val id: Long, | ||
| val umbrellaUuid: Long, | ||
| val content: String?, | ||
| val etc: String? | ||
| ) { | ||
| companion object { | ||
| fun fromConditionReport(conditionReport: ConditionReport): ConditionReportResponse { | ||
| return ConditionReportResponse( | ||
| id = conditionReport.history.id!!, | ||
| umbrellaUuid = conditionReport.history.umbrella.uuid, | ||
| content = conditionReport.content, | ||
| etc = conditionReport.etc | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| package upbrella.be.rent.event | ||
|
|
||
| import org.springframework.context.event.EventListener | ||
| import org.springframework.stereotype.Service | ||
| import upbrella.be.rent.entity.ConditionReport | ||
| import upbrella.be.rent.service.ConditionReportService | ||
| import upbrella.be.slack.SlackAlarmService | ||
| import upbrella.be.slack.dto.service.input.NotifyConditionReportInput | ||
| import upbrella.be.slack.dto.service.input.NotifyRentInput | ||
|
|
||
| @Service | ||
| class RentEventHandler( | ||
| private val slackAlarmService: SlackAlarmService, | ||
| private val conditionReportService: ConditionReportService, | ||
| ) { | ||
|
|
||
| @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( | ||
| historyId = event.historyId, | ||
| content = content | ||
| ).also { conditionReport -> | ||
| conditionReportService.saveConditionReport(conditionReport) | ||
| slackAlarmService.notifyConditionReport( | ||
| NotifyConditionReportInput( | ||
| umbrellaId = event.umbrellaId, | ||
| rentStoreName = event.rentStoreName, | ||
| content = content | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package upbrella.be.rent.event | ||
|
|
||
| import upbrella.be.util.event.Event | ||
|
|
||
| class UmbrellaRentedEvent( | ||
| val userId: Long, | ||
| val userName: String, | ||
| val rentStoreName: String, | ||
| val conditionReportContent: String? = null, | ||
| val umbrellaId: Long, | ||
| val historyId: Long, | ||
| ) : Event() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package upbrella.be.rent.repository | ||
|
|
||
| import com.querydsl.core.types.Projections | ||
| import com.querydsl.jpa.impl.JPAQueryFactory | ||
| import org.springframework.stereotype.Repository | ||
| import upbrella.be.rent.dto.response.ConditionReportResponse | ||
| import upbrella.be.rent.entity.QConditionReport.conditionReport | ||
| import upbrella.be.rent.entity.QHistory.history | ||
|
|
||
| @Repository | ||
| class CustomConditionReportRepository( | ||
| private val queryFactory: JPAQueryFactory | ||
| ) { | ||
| fun findAllConditionReport(): List<ConditionReportResponse> { | ||
| return queryFactory | ||
| .select( | ||
| Projections.constructor( | ||
| ConditionReportResponse::class.java, | ||
| conditionReport.id, | ||
| history.umbrella.uuid, | ||
| conditionReport.content, | ||
| conditionReport.etc | ||
| ) | ||
| ) | ||
| .from(conditionReport) | ||
| .join(history) | ||
| .fetch() | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,16 +7,18 @@ import upbrella.be.rent.dto.request.HistoryFilterRequest | |
| import upbrella.be.rent.dto.request.RentUmbrellaByUserRequest | ||
| import upbrella.be.rent.dto.request.ReturnUmbrellaByUserRequest | ||
| import upbrella.be.rent.dto.response.* | ||
| import upbrella.be.rent.entity.ConditionReport | ||
| import upbrella.be.rent.entity.History | ||
| import upbrella.be.rent.entity.ImprovementReport | ||
| import upbrella.be.rent.exception.* | ||
| import upbrella.be.rent.event.UmbrellaRentedEvent | ||
| import upbrella.be.rent.exception.CannotBeRentedException | ||
| import upbrella.be.rent.exception.ExistingUmbrellaForRentException | ||
| import upbrella.be.rent.exception.NonExistingHistoryException | ||
| import upbrella.be.rent.exception.NonExistingUmbrellaForRentException | ||
| import upbrella.be.rent.repository.RentRepository | ||
| import upbrella.be.slack.SlackAlarmService | ||
| import upbrella.be.store.entity.StoreMeta | ||
| import upbrella.be.store.repository.StoreMetaReader | ||
| import upbrella.be.umbrella.entity.Umbrella | ||
| import upbrella.be.umbrella.exception.MissingUmbrellaException | ||
| import upbrella.be.umbrella.exception.NonExistingBorrowedHistoryException | ||
| import upbrella.be.umbrella.service.UmbrellaService | ||
| import upbrella.be.user.dto.response.AllHistoryResponse | ||
|
|
@@ -25,6 +27,7 @@ import upbrella.be.user.dto.response.SingleHistoryResponse | |
| import upbrella.be.user.entity.User | ||
| import upbrella.be.user.repository.UserReader | ||
| import upbrella.be.user.service.BlackListService | ||
| import upbrella.be.util.event.Events | ||
| import java.time.LocalDateTime | ||
| import java.time.temporal.ChronoUnit | ||
|
|
||
|
|
@@ -35,8 +38,6 @@ class RentService( | |
| private val slackAlarmService: SlackAlarmService, | ||
| private val improvementReportService: ImprovementReportService, | ||
| private val rentRepository: RentRepository, | ||
| private val conditionReportService: ConditionReportService, | ||
| private val lockerService: LockerService, | ||
| private val blackListService: BlackListService, | ||
| private val userReader: UserReader, | ||
| ) { | ||
|
|
@@ -66,30 +67,25 @@ class RentService( | |
| throw ExistingUmbrellaForRentException("[ERROR] 해당 유저가 대여 중인 우산이 있습니다.") | ||
| } | ||
| val umbrella = umbrellaService.findUmbrellaById(rentUmbrellaByUserRequest.umbrellaId) | ||
| 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) | ||
|
|
||
|
Comment on lines
-69
to
+71
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 검증 로직 umbrella 객체가 처리하도록 변경
Comment on lines
+70
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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:
🤖 Prompt for AI Agents |
||
| val rentalStore = storeMetaReader.findById(rentUmbrellaByUserRequest.storeId) | ||
| val history = rentRepository.save( | ||
| History.ofCreatedByNewRent(umbrella, userToRent, rentalStore) | ||
| ) | ||
| slackAlarmService.notifyRent(userToRent, history) | ||
|
|
||
| rentUmbrellaByUserRequest.conditionReport | ||
| ?.takeIf { it.isNotBlank() } | ||
| ?.let { content -> | ||
| ConditionReport(history = history, content = content).also { conditionReport -> | ||
| conditionReportService.saveConditionReport(conditionReport) | ||
| slackAlarmService.notifyConditionReport(conditionReport) | ||
| } | ||
| } | ||
| // umbrellaRentedEvent | ||
| Events.raise( | ||
| UmbrellaRentedEvent( | ||
| userId = userToRent.id, | ||
| userName = userToRent.name, | ||
| rentStoreName = rentalStore.name, | ||
| conditionReportContent = rentUmbrellaByUserRequest.conditionReport, | ||
| umbrellaId = umbrella.id!!, | ||
| historyId = history.id!!, | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| @Transactional | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,9 +5,10 @@ import org.springframework.http.HttpMethod.POST | |||||||||||||||||||||||||||||||||||
| import org.springframework.stereotype.Service | ||||||||||||||||||||||||||||||||||||
| import org.springframework.web.client.RestTemplate | ||||||||||||||||||||||||||||||||||||
| import upbrella.be.config.SlackBotConfig | ||||||||||||||||||||||||||||||||||||
| import upbrella.be.rent.entity.ConditionReport | ||||||||||||||||||||||||||||||||||||
| import upbrella.be.rent.entity.History | ||||||||||||||||||||||||||||||||||||
| import upbrella.be.rent.entity.ImprovementReport | ||||||||||||||||||||||||||||||||||||
| import upbrella.be.slack.dto.service.input.NotifyConditionReportInput | ||||||||||||||||||||||||||||||||||||
| import upbrella.be.slack.dto.service.input.NotifyRentInput | ||||||||||||||||||||||||||||||||||||
| import upbrella.be.user.entity.User | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| @Service | ||||||||||||||||||||||||||||||||||||
|
|
@@ -16,12 +17,12 @@ class SlackAlarmService( | |||||||||||||||||||||||||||||||||||
| private val restTemplate: RestTemplate | ||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| fun notifyRent(user: User, history: History) { | ||||||||||||||||||||||||||||||||||||
| fun notifyRent(input: NotifyRentInput) { | ||||||||||||||||||||||||||||||||||||
| val message = buildString { | ||||||||||||||||||||||||||||||||||||
| append("*우산 대여 알림: 입금을 확인해주세요.*\n\n") | ||||||||||||||||||||||||||||||||||||
| append("사용자 ID : ${user.id}\n") | ||||||||||||||||||||||||||||||||||||
| append("예금주 이름 : ${user.name}\n") | ||||||||||||||||||||||||||||||||||||
| append("대여 지점 이름 : ${history.rentStoreMeta.name}\n") | ||||||||||||||||||||||||||||||||||||
| append("사용자 ID : ${input.userId}\n") | ||||||||||||||||||||||||||||||||||||
| append("예금주 이름 : ${input.userName}\n") | ||||||||||||||||||||||||||||||||||||
| append("대여 지점 이름 : ${input.rentStoreName}\n") | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| send(message) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
@@ -39,20 +40,12 @@ class SlackAlarmService( | |||||||||||||||||||||||||||||||||||
| send(message) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| fun notifyConditionReport(conditionReport: ConditionReport) { | ||||||||||||||||||||||||||||||||||||
| fun notifyConditionReport(input: NotifyConditionReportInput) { | ||||||||||||||||||||||||||||||||||||
| val message = buildString { | ||||||||||||||||||||||||||||||||||||
| append("*우산 상태 신고 접수*\n\n") | ||||||||||||||||||||||||||||||||||||
| append("우산 ID : ${conditionReport.history.umbrella.id}\n") | ||||||||||||||||||||||||||||||||||||
| append("대여 지점 이름 : ${conditionReport.history.rentStoreMeta.name}\n") | ||||||||||||||||||||||||||||||||||||
| append("신고 내용 : ${conditionReport.content}\n") | ||||||||||||||||||||||||||||||||||||
| append("우산 ID : ${input.umbrellaId}\n") | ||||||||||||||||||||||||||||||||||||
| append("대여 지점 이름 : ${input.rentStoreName}\n") | ||||||||||||||||||||||||||||||||||||
| append("신고 내용 : ${input.content}\n") | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| send(message) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
@@ -69,4 +62,12 @@ class SlackAlarmService( | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| send(message) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+65
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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:
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package upbrella.be.slack.dto.service.input | ||
|
|
||
| data class NotifyRentInput( | ||
| val userId: Long, | ||
| val userName: String, | ||
| val rentStoreName: String, | ||
| ) | ||
|
|
||
| data class NotifyConditionReportInput( | ||
| val umbrellaId: Long, | ||
| val rentStoreName: String, | ||
| val content: String, | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| package upbrella.be.umbrella.entity | ||
|
|
||
| import upbrella.be.rent.exception.NotAvailableUmbrellaException | ||
| import upbrella.be.rent.exception.UmbrellaStoreMissMatchException | ||
| import upbrella.be.store.entity.StoreMeta | ||
| import upbrella.be.umbrella.dto.request.UmbrellaCreateRequest | ||
| import upbrella.be.umbrella.dto.request.UmbrellaModifyRequest | ||
| import upbrella.be.umbrella.exception.MissingUmbrellaException | ||
| import java.time.LocalDateTime | ||
| import javax.persistence.* | ||
|
|
||
|
|
@@ -49,7 +52,16 @@ class Umbrella( | |
| this.missed = request.missed | ||
| } | ||
|
|
||
| fun rentUmbrella() { | ||
| 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 | ||
|
Comment on lines
+55
to
65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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:
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package upbrella.be.util.event | ||
|
|
||
| abstract class Event( | ||
| val timestamp: Long = System.currentTimeMillis(), | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package upbrella.be.util.event | ||
|
|
||
| import org.springframework.context.ApplicationEventPublisher | ||
|
|
||
| object Events { | ||
| private var publisher: ApplicationEventPublisher? = null | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Harden the publisher and narrow the API surface
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 |
||
|
|
||
| fun setPublisher(publisher: ApplicationEventPublisher) { | ||
| this.publisher = publisher | ||
| } | ||
|
|
||
| fun raise(event: Any) { | ||
| publisher?.publishEvent(event) | ||
| ?: throw IllegalStateException("ApplicationEventPublisher가 설정되지 않았습니다.") | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly map historyId to the DB column to avoid naming strategy surprises
Previously the FK column was
history_idvia@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:
📝 Committable suggestion
🤖 Prompt for AI Agents