Skip to content
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

๐Ÿ”€ :: (entry-238) application fullback method์ถ”๊ฐ€ #93

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package hs.kr.equus.application.domain.application.spi

import hs.kr.equus.application.domain.user.model.User
import hs.kr.equus.application.domain.user.model.UserCache
import java.util.UUID

interface ApplicationQueryUserPort {
fun queryUserByUserId(userId: UUID): User

fun queryUserByUserIdInCache(userId: UUID): UserCache?
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package hs.kr.equus.application.domain.user.model

import java.util.*

data class UserCache (
val id: UUID,
val phoneNumber: String,
val name: String,
val isParent: Boolean,
val receiptCode: Long?,
val role: String,
val ttl: Long
)
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package hs.kr.equus.application.domain.user.domain

import hs.kr.equus.application.domain.user.domain.repository.UserCacheRepository
import hs.kr.equus.application.domain.user.model.User
import hs.kr.equus.application.domain.user.model.UserCache
import hs.kr.equus.application.domain.user.spi.UserPort
import hs.kr.equus.application.global.feign.client.UserClient
import org.springframework.stereotype.Component
Expand All @@ -9,6 +11,7 @@ import java.util.UUID
@Component
class UserPersistenceAdapter(
private val userClient: UserClient,
private val userCacheRepository: UserCacheRepository
) : UserPort {
override fun queryUserByUserId(userId: UUID): User {
return userClient.getUserInfoByUserId(userId).run {
Expand All @@ -20,4 +23,19 @@ class UserPersistenceAdapter(
)
}
}

override fun queryUserByUserIdInCache(userId: UUID): UserCache? {
return userCacheRepository.findById(userId)
.map {
UserCache(
id = it.id,
phoneNumber = it.phoneNumber,
name = it.name,
isParent = it.isParent,
receiptCode = it.receiptCode,
role = it.role.name,
ttl = it.ttl
)
Comment on lines +30 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Potential issue

UserCache ๊ฐ์ฒด ์ƒ์„ฑ ์‹œ ํ•„๋“œ ๋งคํ•‘ ์ •ํ™•์„ฑ ๊ฒ€ํ† 

UserCache ๊ฐ์ฒด๋ฅผ ์ƒ์„ฑํ•  ๋•Œ role = it.role.name์œผ๋กœ ์„ค์ •ํ•˜๊ณ  ์žˆ์Šต๋‹ˆ๋‹ค. ์ด๋•Œ it.role์ด Enum์ด๊ฑฐ๋‚˜ ๊ฐ์ฒด๋ผ๋ฉด .name์œผ๋กœ ๋ฌธ์ž์—ด์„ ๊ฐ€์ ธ์˜ค๋Š” ๊ฒƒ์ด ๋งž์ง€๋งŒ, UserCache์˜ role ํ•„๋“œ ํƒ€์ž…๊ณผ ์ผ์น˜ํ•˜๋Š”์ง€ ํ™•์ธ์ด ํ•„์š”ํ•ฉ๋‹ˆ๋‹ค. ํƒ€์ž… ๋ถˆ์ผ์น˜๋กœ ์ธํ•œ ๋ฌธ์ œ๋ฅผ ๋ฐฉ์ง€ํ•˜๊ธฐ ์œ„ํ•ด ํ•„๋“œ์˜ ํƒ€์ž…์„ ํ™•์ธํ•˜๊ณ  ์ ์ ˆํ•˜๊ฒŒ ๋งคํ•‘ํ•ด์ฃผ์„ธ์š”.

๋งŒ์•ฝ role ํ•„๋“œ์˜ ํƒ€์ž…์ด Enum์ด๋ผ๋ฉด, it.role ๊ทธ๋Œ€๋กœ ํ• ๋‹นํ•˜๊ฑฐ๋‚˜ ํ•„์š”ํ•œ ํƒ€์ž…์œผ๋กœ ๋ณ€ํ™˜ํ•ด์•ผ ํ•ฉ๋‹ˆ๋‹ค. ์˜ˆ๋ฅผ ๋“ค์–ด:

- role = it.role.name,
+ role = it.role,

๋˜๋Š”

- role = it.role.name,
+ role = it.role.toString(),

Committable suggestion was skipped due to low confidence.

}.orElse(null)
}
Comment on lines +27 to +40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ› ๏ธ Refactor suggestion

์ƒˆ๋กœ์šด queryUserByUserIdInCache ๋ฉ”์„œ๋“œ์— ๋Œ€ํ•œ ํ”ผ๋“œ๋ฐฑ

  1. ์บ์‹œ ๊ธฐ๋Šฅ ๊ตฌํ˜„: ์ƒˆ๋กœ์šด ๋ฉ”์„œ๋“œ๊ฐ€ PR์˜ ๋ชฉ์ ์— ๋งž๊ฒŒ ์บ์‹œ ๊ธฐ๋Šฅ์„ ๊ตฌํ˜„ํ•œ ๊ฒƒ์œผ๋กœ ๋ณด์ž…๋‹ˆ๋‹ค. ๐Ÿ‘

  2. role ํ•„๋“œ ๋งคํ•‘: 36๋ฒˆ ์ค„์—์„œ role = it.role.name์œผ๋กœ ๋งคํ•‘ํ•˜๊ณ  ์žˆ์Šต๋‹ˆ๋‹ค. UserCache์˜ role ํ•„๋“œ ํƒ€์ž…์ด ๋ฌธ์ž์—ด์ธ์ง€ ํ™•์ธํ•ด์ฃผ์„ธ์š”. ๋งŒ์•ฝ Enum์ด๋ผ๋ฉด it.role์„ ์ง์ ‘ ํ• ๋‹นํ•˜๋Š” ๊ฒƒ์ด ๋” ์ ์ ˆํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค.

  3. ์บ์‹œ ๋ฏธ์Šค ์ฒ˜๋ฆฌ: ์บ์‹œ์—์„œ ์‚ฌ์šฉ์ž๋ฅผ ์ฐพ์ง€ ๋ชปํ–ˆ์„ ๋•Œ null์„ ๋ฐ˜ํ™˜ํ•˜๊ณ  ์žˆ์Šต๋‹ˆ๋‹ค. ์ด๋Š” ์˜๋„๋œ ๋™์ž‘์ธ์ง€ ํ™•์ธํ•ด์ฃผ์„ธ์š”. ์บ์‹œ ๋ฏธ์Šค ์‹œ ๋กœ๊น…์„ ์ถ”๊ฐ€ํ•˜๊ฑฐ๋‚˜, ํ•„์š”ํ•˜๋‹ค๋ฉด ์›๋ณธ ๋ฐ์ดํ„ฐ ์†Œ์Šค์—์„œ ์กฐํšŒํ•˜๋Š” ๋กœ์ง์„ ๊ณ ๋ คํ•ด๋ณผ ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค.

  4. ์˜ˆ์™ธ ์ฒ˜๋ฆฌ: ํ˜„์žฌ ์ฝ”๋“œ์—๋Š” ์˜ˆ์™ธ ์ฒ˜๋ฆฌ๊ฐ€ ์—†์Šต๋‹ˆ๋‹ค. ์บ์‹œ ์กฐํšŒ ์ค‘ ๋ฐœ์ƒํ•  ์ˆ˜ ์žˆ๋Š” ์˜ˆ์™ธ์— ๋Œ€ํ•œ ์ฒ˜๋ฆฌ๋ฅผ ์ถ”๊ฐ€ํ•˜๋Š” ๊ฒƒ์ด ์ข‹๊ฒ ์Šต๋‹ˆ๋‹ค.

์บ์‹œ ๋ฏธ์Šค ์‹œ ๋กœ๊น…์„ ์ถ”๊ฐ€ํ•˜๋Š” ์˜ˆ์‹œ:

override fun queryUserByUserIdInCache(userId: UUID): UserCache? {
    return userCacheRepository.findById(userId)
        .map {
            UserCache(
                // ... (ํ˜„์žฌ ๋งคํ•‘ ๋กœ์ง)
            )
        }.orElse(null)
        .also { if (it == null) logger.debug("Cache miss for user ID: $userId") }
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package hs.kr.equus.application.domain.user.domain.entity

import hs.kr.equus.application.global.security.jwt.UserRole
import org.springframework.data.annotation.Id
import org.springframework.data.redis.core.RedisHash
import org.springframework.data.redis.core.TimeToLive
import java.util.*

@RedisHash(value = "status_cache")
class UserCacheRedisEntity (
@Id
val id: UUID,
val phoneNumber: String,
val name: String,
val isParent: Boolean,
val receiptCode: Long?,
val role: UserRole,
@TimeToLive
val ttl: Long
)
Comment on lines +11 to +20
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ› ๏ธ Refactor suggestion

์ผ๋ถ€ ์†์„ฑ์— ๋Œ€ํ•œ ์œ ํšจ์„ฑ ๊ฒ€์‚ฌ ์–ด๋…ธํ…Œ์ด์…˜ ์ถ”๊ฐ€๋ฅผ ๊ณ ๋ คํ•ด๋ณด์„ธ์š”.

๋ช‡๋ช‡ ์†์„ฑ์— ๋Œ€ํ•ด ์œ ํšจ์„ฑ ๊ฒ€์‚ฌ ์–ด๋…ธํ…Œ์ด์…˜์„ ์ถ”๊ฐ€ํ•˜๋ฉด ๋ฐ์ดํ„ฐ์˜ ๋ฌด๊ฒฐ์„ฑ์„ ๋ณด์žฅํ•˜๋Š” ๋ฐ ๋„์›€์ด ๋  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค. ์˜ˆ๋ฅผ ๋“ค์–ด:

import javax.validation.constraints.NotBlank
import javax.validation.constraints.Pattern

class UserCacheRedisEntity(
    @Id
    val id: UUID,
    
    @NotBlank
    @Pattern(regexp = "^\\d{11}$", message = "์ „ํ™”๋ฒˆํ˜ธ๋Š” 11์ž๋ฆฌ ์ˆซ์ž์—ฌ์•ผ ํ•ฉ๋‹ˆ๋‹ค")
    val phoneNumber: String,
    
    @NotBlank
    val name: String,
    
    val isParent: Boolean,
    
    val receiptCode: Long?,
    
    val role: UserRole,
    
    @TimeToLive
    val ttl: Long
)

์ด๋ ‡๊ฒŒ ํ•˜๋ฉด phoneNumber์™€ name์ด ๋น„์–ด์žˆ์ง€ ์•Š๋„๋ก ๋ณด์žฅํ•˜๊ณ , phoneNumber๊ฐ€ ์˜ฌ๋ฐ”๋ฅธ ํ˜•์‹์ธ์ง€ ํ™•์ธํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package hs.kr.equus.application.domain.user.domain.repository

import hs.kr.equus.application.domain.user.domain.entity.UserCacheRedisEntity
import org.springframework.data.repository.CrudRepository
import java.util.UUID

interface UserCacheRepository : CrudRepository<UserCacheRedisEntity, UUID> {
}
Original file line number Diff line number Diff line change
@@ -1,15 +1,48 @@
package hs.kr.equus.application.global.feign.client

import hs.kr.equus.application.domain.status.spi.StatusPort
import hs.kr.equus.application.domain.user.model.UserCache
import hs.kr.equus.application.domain.user.spi.UserPort
import hs.kr.equus.application.global.feign.client.dto.response.StatusInfoElement
import hs.kr.equus.application.global.feign.client.dto.response.UserInfoElement
import hs.kr.equus.application.global.security.jwt.UserRole
import org.springframework.cloud.openfeign.FeignClient
import org.springframework.context.annotation.Lazy
import org.springframework.stereotype.Component
import org.springframework.web.bind.annotation.GetMapping
import org.springframework.web.bind.annotation.PathVariable
import java.util.UUID

@FeignClient(name = "UserClient", url = "\${url.user}")
@FeignClient(name = "UserClient", url = "\${url.user}", fallback = UserFallBack::class)
interface UserClient {
@GetMapping("/user/{userId}")
fun getUserInfoByUserId(
@PathVariable("userId") userId: UUID,
): UserInfoElement

}

@Component
class UserFallBack(
@Lazy private val userPort: UserPort
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ› ๏ธ Refactor suggestion

@lazy ์–ด๋…ธํ…Œ์ด์…˜ ์‚ฌ์šฉ์˜ ํ•„์š”์„ฑ ๊ฒ€ํ† 

UserPort๋ฅผ ์ฃผ์ž…ํ•  ๋•Œ @Lazy ์–ด๋…ธํ…Œ์ด์…˜์„ ์‚ฌ์šฉํ•˜๊ณ  ์žˆ์Šต๋‹ˆ๋‹ค. ์ˆœํ™˜ ์˜์กด์„ฑ ๋ฌธ์ œ๋ฅผ ํ•ด๊ฒฐํ•˜๊ธฐ ์œ„ํ•œ ๊ฒƒ์ด๋ผ๋ฉด ๊ดœ์ฐฎ์ง€๋งŒ, ๊ทธ๋ ‡์ง€ ์•Š๋‹ค๋ฉด ์ง€์—ฐ ์ดˆ๊ธฐํ™”๋กœ ์ธํ•ด ์˜ˆ๊ธฐ์น˜ ์•Š์€ ๋ฌธ์ œ๊ฐ€ ๋ฐœ์ƒํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค. @Lazy ์‚ฌ์šฉ์ด ํ•„์š”ํ•œ์ง€ ๊ฒ€ํ† ํ•ด ์ฃผ์„ธ์š”.

) : UserClient {

override fun getUserInfoByUserId(userId: UUID): UserInfoElement {
val user = userPort.queryUserByUserIdInCache(userId)
return user?.let {
UserInfoElement(
id = it.id,
isParent = it.isParent,
phoneNumber = it.phoneNumber,
name = it.name,
role = UserRole.valueOf(it.role)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Potential issue

UserRole ๋ณ€ํ™˜ ์‹œ ์˜ˆ์™ธ ์ฒ˜๋ฆฌ ํ•„์š”

UserRole.valueOf(it.role)์„ ์‚ฌ์šฉํ•˜์—ฌ ๋ฌธ์ž์—ด์„ UserRole enum์œผ๋กœ ๋ณ€ํ™˜ํ•˜๊ณ  ์žˆ์Šต๋‹ˆ๋‹ค. ๋งŒ์•ฝ it.role์— ์˜ˆ์ƒ์น˜ ๋ชปํ•œ ๊ฐ’์ด ๋“ค์–ด์˜ฌ ๊ฒฝ์šฐ IllegalArgumentException์ด ๋ฐœ์ƒํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค. ์•ˆ์ „ํ•œ ๋ณ€ํ™˜์„ ์œ„ํ•ด ์˜ˆ์™ธ ์ฒ˜๋ฆฌ๋ฅผ ์ถ”๊ฐ€ํ•˜๊ฑฐ๋‚˜ ๊ธฐ๋ณธ๊ฐ’์„ ์„ค์ •ํ•˜๋Š” ๊ฒƒ์ด ์ข‹์Šต๋‹ˆ๋‹ค.

๋‹ค์Œ๊ณผ ๊ฐ™์ด ์ˆ˜์ •ํ•˜์—ฌ ์˜ˆ์™ธ ๋ฐœ์ƒ์„ ๋ฐฉ์ง€ํ•  ์ˆ˜ ์žˆ์Šต๋‹ˆ๋‹ค:

- role = UserRole.valueOf(it.role)
+ role = UserRole.values().find { role -> role.name == it.role } ?: UserRole.USER
๐Ÿ“ 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.

Suggested change
role = UserRole.valueOf(it.role)
role = UserRole.values().find { role -> role.name == it.role } ?: UserRole.USER

)
} ?: UserInfoElement(
id = userId,
phoneNumber = "01000000000",
name = "ํ™๊ธธ๋™",
isParent = false,
role = UserRole.USER
)
}
}