Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Expand Up @@ -32,4 +32,6 @@ interface PoseRepositoryPort {
fun countPoses(headCount: HeadCount, excludeIds: List<Long>): Long

fun findPoseByOffset(offset: Long, headCount: HeadCount, excludeIds: List<Long>): Pose?

fun incrementViewCount(poseId: Long)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.neki.pose.application.port

interface PoseViewCachePort {

fun addViewer(poseId: Long, userId: Long): Boolean
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,30 @@ package com.neki.pose.application.usecase
import com.neki.common.annotation.UseCase
import com.neki.common.api.dto.ResultCode
import com.neki.common.exception.BusinessException
import com.neki.common.transaction.TransactionRunner
import com.neki.pose.application.command.GetPoseCommand
import com.neki.pose.application.contract.MediaStorageInfo
import com.neki.pose.application.port.MediaClientPort
import com.neki.pose.application.port.PoseRepositoryPort
import com.neki.pose.application.port.PoseViewCachePort
import com.neki.pose.application.result.GetPoseResult

@UseCase
class GetPoseUseCase(private val poseRepository: PoseRepositoryPort, private val mediaClient: MediaClientPort) {
class GetPoseUseCase(
private val poseRepository: PoseRepositoryPort,
private val mediaClient: MediaClientPort,
private val poseViewCache: PoseViewCachePort,
private val transactionRunner: TransactionRunner,
) {

fun execute(command: GetPoseCommand): GetPoseResult {
val (pose, isScraped) = poseRepository.getOwnedPoseWithScrap(command.userId, command.poseId)
?: throw BusinessException(ResultCode.NOT_FOUND)

if (poseViewCache.addViewer(command.poseId, command.userId)) {
transactionRunner.run { poseRepository.incrementViewCount(command.poseId) }

Choose a reason for hiding this comment

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

medium

The transactionRunner.run block is used here to increment the view count. While this ensures the database update is transactional, it's important to consider if incrementViewCount itself needs to be part of a larger business transaction or if it can be an independent operation. Given that addViewer is called outside this transaction, there's a potential for a view to be recorded in Redis but not in the database if the transaction fails later, or vice-versa if addViewer fails after the transaction commits. However, the current implementation's 'fail-open' for Redis suggests a preference for eventual consistency in view counts, so this might be acceptable. If strict consistency is required, the addViewer call should also be part of the transaction or handled with compensating actions.

}

val mediaInfo: MediaStorageInfo = mediaClient.getMediaStorageInfo(pose.mediaId)

return GetPoseResult(
Expand Down
3 changes: 3 additions & 0 deletions src/main/kotlin/com/neki/pose/domain/entity/Pose.kt
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,7 @@ class Pose(

@Column(name = "memo", nullable = true)
var memo: String? = null,

@Column(name = "view_count", nullable = false)
var viewCount: Long = 0,
) : BaseTimeEntity()
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.neki.pose.infra.cache.fake

import com.neki.pose.application.port.PoseViewCachePort
import org.springframework.context.annotation.Profile
import org.springframework.stereotype.Component
import java.util.concurrent.ConcurrentHashMap

@Component
@Profile("test")
class FakePoseViewCacheAdapter : PoseViewCachePort {

private val viewers = ConcurrentHashMap<Long, MutableSet<Long>>()

override fun addViewer(poseId: Long, userId: Long): Boolean {
val userSet = viewers.computeIfAbsent(poseId) { ConcurrentHashMap.newKeySet() }
return userSet.add(userId)
}

fun clearAll() {
viewers.clear()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.neki.pose.infra.cache.redis

internal object PoseViewRedisCacheKey {
private const val VIEW_PREFIX = "pose:view:"

fun viewKey(poseId: Long): String = "$VIEW_PREFIX$poseId"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package com.neki.pose.infra.cache.redis

import com.neki.pose.application.port.PoseViewCachePort
import org.slf4j.LoggerFactory
import org.springframework.context.annotation.Primary
import org.springframework.context.annotation.Profile
import org.springframework.data.redis.core.RedisTemplate
import org.springframework.stereotype.Component
import java.time.Instant
import java.time.LocalDate
import java.time.ZoneId

@Component
@Primary
@Profile("!test")
class RedisPoseViewCacheAdapter(private val redisTemplate: RedisTemplate<String, Any>) : PoseViewCachePort {

private val log = LoggerFactory.getLogger(javaClass)

override fun addViewer(poseId: Long, userId: Long): Boolean {
val key = PoseViewRedisCacheKey.viewKey(poseId)
return try {
val added = redisTemplate.opsForSet().add(key, userId) ?: 0
if (added > 0) {
redisTemplate.expireAt(key, getMidnight())
true
} else {
false
}
} catch (e: Exception) {
log.warn("[PoseViewCache] Failed to check viewer for pose: $poseId, user: $userId", e)
true // fail-open: Redis 장애 시 신규 조회로 처리
Comment on lines +31 to +32

Choose a reason for hiding this comment

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

medium

The fail-open strategy (true // fail-open: Redis 장애 시 신규 조회로 처리) means that if Redis is down or an exception occurs, the view count will still be incremented in the database. This prioritizes view count incrementation over preventing duplicate views during Redis outages. This is a valid design choice, but it's important to acknowledge the trade-off: in case of Redis failure, a single user might increment the view count multiple times within 24 hours. Ensure this behavior is acceptable for the business requirements.

}
Comment on lines 21 to 33

Choose a reason for hiding this comment

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

security-medium medium

The current implementation of addViewer uses a single Redis Set per pose (pose:view:{poseId}) to track unique viewers. The TTL of this set is reset to 24 hours every time a new viewer is added. This introduces two significant issues:

  1. Logic Flaw: The requirement is to allow a user to increment the view count once per day. However, because the TTL is reset for the entire set whenever any new user views the pose, an existing user's ID will remain in the set as long as the pose remains popular. This prevents the user from ever incrementing the view count again during that period, even if more than 24 hours have passed since their last view.
  2. Resource Exhaustion (DoS): For popular poses, the set of user IDs can grow indefinitely as long as the TTL keeps being reset, leading to significant memory consumption in Redis and potential Denial of Service.
    Remediation: Change the Redis key structure to be per-user and per-pose, for example: pose:view:{poseId}:{userId}. Set a fixed 24-hour TTL on this key when it is first created, and do not reset it.
        val key = "${PoseViewRedisCacheKey.viewKey(poseId)}:$userId"
        return try {
            redisTemplate.opsForValue().setIfAbsent(key, true, VIEW_TTL) ?: false
        } catch (e: Exception) {
            log.warn("[PoseViewCache] Failed to check viewer for pose: $poseId, user: $userId", e)
            true // fail-open: Redis 장애 시 신규 조회로 처리
        }

}

private fun getMidnight(): Instant {
val tomorrow = LocalDate.now(KOREA_ZONE).plusDays(1)
return tomorrow.atStartOfDay(KOREA_ZONE).toInstant()
}

companion object {
private val KOREA_ZONE = ZoneId.of("Asia/Seoul")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,8 @@ class PoseRepositoryAdapter(

override fun findPoseByOffset(offset: Long, headCount: HeadCount, excludeIds: List<Long>): Pose? =
queryRepository.findPoseByOffset(offset, headCount, excludeIds)

override fun incrementViewCount(poseId: Long) {
queryRepository.incrementViewCount(poseId)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,10 @@ class PosesQueryRepository(private val queryFactory: JPAQueryFactory) {
.offset(offset)
.limit(1)
.fetchOne()

fun incrementViewCount(poseId: Long): Long = queryFactory
.update(pose)
.set(pose.viewCount, pose.viewCount.add(1))
.where(pose.id.eq(poseId))
.execute()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE TB_POSE ADD COLUMN view_count BIGINT NOT NULL DEFAULT 0;
48 changes: 48 additions & 0 deletions src/test/kotlin/com/neki/e2e/pose/GetPoseE2ETest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.neki.media.domain.entity.MediaStatus
import com.neki.pose.domain.HeadCount
import com.neki.user.domain.entity.User
import io.restassured.RestAssured
import org.assertj.core.api.Assertions.assertThat
import org.hamcrest.Matchers.equalTo
import org.hamcrest.Matchers.notNullValue
import org.hamcrest.Matchers.startsWith
Expand Down Expand Up @@ -90,6 +91,53 @@ class GetPoseE2ETest : PoseE2ETestBase() {
.body("data.scrap", equalTo(true))
}

@Test
@DisplayName("포즈 상세 조회 성공 - 조회수 증가")
fun givenPoseExists_whenGetPose_thenViewCountIncreases() {
// given
val media = createMedia(ownerId = testUser.id!!, status = MediaStatus.UPLOADED)
val pose = createPose(userId = testUser.id!!, mediaId = media.id!!, headCount = HeadCount.ONE)

// when
RestAssured.given()
.header("Authorization", "Bearer $accessToken")
.`when`()
.get("/api/poses/${pose.id}")
.then()
.statusCode(HttpStatus.OK.value())

// then
val updated = poseRepository.findById(pose.id!!).get()
assertThat(updated.viewCount).isEqualTo(1)
}

@Test
@DisplayName("포즈 상세 조회 성공 - 동일 유저 중복 조회 시 조회수 미증가")
fun givenSameUser_whenGetPoseTwice_thenViewCountIncreasesOnce() {
// given
val media = createMedia(ownerId = testUser.id!!, status = MediaStatus.UPLOADED)
val pose = createPose(userId = testUser.id!!, mediaId = media.id!!, headCount = HeadCount.ONE)

// when: 같은 유저가 두 번 조회
RestAssured.given()
.header("Authorization", "Bearer $accessToken")
.`when`()
.get("/api/poses/${pose.id}")
.then()
.statusCode(HttpStatus.OK.value())

RestAssured.given()
.header("Authorization", "Bearer $accessToken")
.`when`()
.get("/api/poses/${pose.id}")
.then()
.statusCode(HttpStatus.OK.value())

// then: 조회수는 1만 증가
val updated = poseRepository.findById(pose.id!!).get()
assertThat(updated.viewCount).isEqualTo(1)
}

@Test
@DisplayName("포즈 상세 조회 성공 - 다양한 headCount")
fun givenPoseWithFiveOrMore_whenGetPose_thenReturnsPoseDetail() {
Expand Down
5 changes: 5 additions & 0 deletions src/test/kotlin/com/neki/e2e/pose/PoseE2ETestBase.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import com.neki.media.infra.persist.jpa.JpaMediaRepository
import com.neki.pose.domain.HeadCount
import com.neki.pose.domain.entity.Pose
import com.neki.pose.domain.entity.ScrapPose
import com.neki.pose.infra.cache.fake.FakePoseViewCacheAdapter
import com.neki.pose.infra.persist.jpa.JpaPoseRepository
import com.neki.pose.infra.persist.jpa.JpaScrapPoseRepository
import org.junit.jupiter.api.AfterEach
Expand All @@ -30,11 +31,15 @@ abstract class PoseE2ETestBase : E2ETestBase() {
@Autowired
protected lateinit var mediaRepository: JpaMediaRepository

@Autowired
protected lateinit var fakePoseViewCache: FakePoseViewCacheAdapter

@AfterEach
override fun tearDown() {
scrapPoseRepository.deleteAllInBatch()
poseRepository.deleteAllInBatch()
mediaRepository.deleteAllInBatch()
fakePoseViewCache.clearAll()
super.tearDown()
}

Expand Down