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 3 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 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,15 +11,31 @@ 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 {
override fun queryUserByUserId(userId: UUID): User? {
return userClient.getUserInfoByUserId(userId)?.let {
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

queryUserByUserId 메서드의 null 처리 미흡

queryUserByUserId 메서드의 반환 타입이 User에서 User?로 변경됨에 따라, 일부 호출부에서 null 체크가 제대로 이루어지지 않았습니다. 다음 위치에서 null 처리를 추가해주세요:

  • application-domain/src/test/kotlin/hs/kr/equus/application/domain/application/service/CheckTelServiceTest.kt
  • application-domain/src/main/kotlin/hs/kr/equus/application/domain/application/usecase/GetApplicationUseCase.kt
  • application-domain/src/main/kotlin/hs/kr/equus/application/domain/application/usecase/CreateApplicationUseCase.kt
  • application-domain/src/main/kotlin/hs/kr/equus/application/domain/application/service/CheckTelService.kt
🔗 Analysis chain

queryUserByUserId 메서드의 반환 타입 변경에 따른 영향 확인 필요

queryUserByUserId 메서드의 반환 타입이 User에서 User?로 변경되었습니다. 이로 인해 이 메서드를 호출하는 모든 부분에서 null 처리 로직이 필요합니다. 기존 코드에서 이 변경으로 인해 발생할 수 있는 문제를 방지하기 위해 호출부에서 null 체크가 제대로 이루어졌는지 확인해주세요.

다음 스크립트를 실행하여 해당 메서드의 호출부를 찾아서 null 처리가 되어 있는지 확인할 수 있습니다:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: queryUserByUserId 메서드 호출부에서 null 처리가 되어 있는지 확인합니다.

rg --type kotlin -A 5 'queryUserByUserId\('

Length of output: 5771

User(
id = id,
phoneNumber = phoneNumber,
name = name,
isParent = isParent,
id = it.id,
phoneNumber = it.phoneNumber,
name = it.name,
isParent = it.isParent,
)
}
}

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으로 문자열을 가져오는 것이 맞지만, UserCacherole 필드 타입과 일치하는지 확인이 필요합니다. 타입 불일치로 인한 문제를 방지하기 위해 필드의 타입을 확인하고 적절하게 매핑해주세요.

만약 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으로 매핑하고 있습니다. UserCacherole 필드 타입이 문자열인지 확인해주세요. 만약 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
)

이렇게 하면 phoneNumbername이 비어있지 않도록 보장하고, 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
): 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 = "",
name = "",
isParent = false,
role = UserRole.USER
)
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

사용자 미조회 시 기본값 반환에 대한 위험성

캐시에서 사용자를 찾지 못한 경우 기본값을 가진 UserInfoElement를 반환하고 있습니다. 이는 실제 사용자 정보가 없는데도 불구하고 유효한 정보를 반환하는 것으로 오해되어 이후 로직에서 문제를 일으킬 수 있습니다. 사용자를 찾지 못한 경우 null을 반환하거나 적절한 예외를 발생시키는 것이 좋습니다.

다음과 같이 코드를 수정하는 것을 제안합니다:

return user?.let {
    UserInfoElement(
        id = it.id,
        isParent = it.isParent,
        phoneNumber = it.phoneNumber,
        name = it.name,
        role = UserRole.valueOf(it.role)
    )
- } ?: UserInfoElement(
-     id = userId,
-     phoneNumber = "",
-     name = "",
-     isParent = false,
-     role = UserRole.USER
- )
+ } ?: 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.

Suggested change
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)
)
} ?: UserInfoElement(
id = userId,
phoneNumber = "",
name = "",
isParent = false,
role = UserRole.USER
)
}
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)
)
} ?: null
}

}