상호 회의 6팀 -> 7팀

[JunBye]

우선 리뷰 드리기에 앞서서 프론트엔드 서비스와 함께 백엔드 로직과 코드를 살펴보았는데,
실제 당근마켓처럼 굉장히 디테일하게 잘 만드신게 보였습니다.

채팅 구현

가장 기능적으로 놀란 부분이었습니다

채팅을 백엔드에서 구현하신게 정말 대단하신것 같습니다.

웹 소켓 사용하신 것 같은데 실제 기말 총회에서 작동하는 모습이 임팩트가 굉장히 클 것 같습니다 !

다양한 기능 구현과 실서비스 사용이 가능하다는 점

당근마켓에 나와있는 디테일한 기능들 ( 매너온도부터 우리 동네 게시글, 채팅)

그리고 추가기능으로 하신 경매 시스템까지 서비스의 완성도도 상당히 높고, 실제 배포환경에서도 제가 직접 게시글을 올려봤는데

바로 이미지 업로드도 잘되고 댓글이나 하트도 변경되는 것에서 완성도 있는 코드를 작성해주신 것 같습니다 !

이하는 저희 팀원들과 이야기하면서 "이 부분은 수정을 하면 더 코드가 깔끔해지겠다 " 싶은 부분들을 작성해두었습니다!

이미지 관련 피드백

---

Image 코드 모듈화 필요성

현재 Article, feed, Auction 의 Serivce단에 이미지 Url을 발급하고 갱신하는 코드가 중복되어 있습니다. 중복된 부분들을 ImageService 내부에 구현하여 모듈화한다면, 전체적으로 코드가 깔끔해질 것 같습니다. 추가로, 각 Service 단에서 혼선을 줄일 수 있을 것 같습니다.

val imagePutPresingedUrls: MutableList<String> = mutableListOf()
        if (request.imageCount > 0) {
            for (number in 1..request.imageCount) {
                val imageUrlEntity: ImageUrlEntity = imageService.postImageUrl("article", articleEntity.id!!, number)

                val imagePutPresignedUrl: String = imageService.generatePutPresignedUrl(imageUrlEntity.s3)
                imagePutPresingedUrls += imagePutPresignedUrl
								
								// 다운로드용 이미지 URL 생성후 imageUrlEntity.presigned 저장
                imageService.generateGetPresignedUrl(imageUrlEntity)
                articleEntity.imageUrls += imageUrlEntity
            }
            articleEntity.updatedAt = Instant.now()
        }
        articleRepository.save(articleEntity)
				
				// DTO 생성시 ImagePresignedUrl변수에 이미지 GET Url에 저장
				val article = Article.fromEntity(articleEntity)
				
				// 다시 업로드용 URL로 갱신
        article.imagePresignedUrl = imagePutPresingedUrls

        return article

또한, 이미지 URL에 대한 변수명을 ImageGetPresignedUrl, ImagePutPresignedUrl과 같이 분리하는 것이 좋아 보입니다.

위 코드블럭에서 imageUrlEntity의 presigned에는 이미지를 조회하는 목적의 Presigned Url이 저장되고, response용 article DTO 생성 시에도 조회 목적의 presigned URL이 저장됩니다 (fromEntity)

하지만, 마지막에 aritcle DTO의 imagePresignedUrl을 이미지 업로드 목적(Put) Presigned URL로 갱신하는데,

url 변수에 어떤 URL(Put/Get)이 저장되는지 구분이 어려워, 코드 리뷰 과정에서 혼동이 많았습니다.

개선책

1. 변수명 통일 (article에서는 imagePresignedUrl, imageUrlEntity에서 presigned …) 혹은
2. 변수명 분리 (imagePutPresignedUrl, imageGetPresignedUrl)과 같이 수정하는 것이 좋아 보입니다.

---

S3 image presigned GET URL 관련

현재 코드에서는 article, feed, auction에 대한 GET 요청이 들어올 때 이미지 URL을 presigned 형식으로 리턴하고 있습니다 (접근에 보안이 적용된 Url).

그러나 당근 마켓이라는 서비스의 특성 상, 특별히 접근 제한을 걸어야 하는 경우가 아니라면 static URL (CloudFront Url)을 사용하는 것이 더 효율적일 것이라 생각됩니다.

개선책 제안

S3 bucket의 보안 설정을 수정하여 presigned URL 없이 key만으로도 접근이 가능하도록 하고, presigned URL 대신 static URL을 사용하면 좋을 것 같습니다.

---

ImageUrlEntity의 presigned 필드 관련

현재 ImageUrlEntity를 생성할 때,

feed/article/auction에 대해 GET요청을 하는 시점에서는 ImageUrlEntity 에 Presigned URL이 들어있다고 가정된 상태입니다.

업로드는 업로드 URL만 만들어주면 되는데, 다운로드 URL도 있어야 만들어주는

추후 서비스 확장성, 더 많은 사용자 환경에 놓였을때 오류가 생길 여지가 있을 것 같아 조심스럽게 말씀드려 봅니다.

로직의 완성도를 높이기 위해서는 아래와 같은 문제점을 고려해보신다면 더 좋은 코드가 되지 않을까 싶습니다 !

각 imageUrlEntity의 presigned 필드가 null이 아닌지 확인하는 로직이 없고, expired 확인 로직만 존재하기 때문에 POST 요청 처리 시에 GET presigned URL을 생성하지 않으면 null 주소가 리턴됩니다.

@Transactional
fun getFeed(
    feedId: Long,
    id: String,
): Feed {
    val feedEntity = feedRepository.findByIdOrNull(feedId) ?: throw FeedNotFoundException()
    feedRepository.incrementViewCount(feedId)

    refreshPresignedUrlIfExpired(listOf(feedEntity))

    val feed = Feed.fromEntity(feedEntity)
    feed.isLiked = feedLikesRepository.existsByUserIdAndFeedId(id, feed.id)

    feed.commentList.forEach { comment ->
        comment.isLiked = commentService.userLikesComment(id, comment.id)
    }

    return feed
}

@Transactional
fun refreshPresignedUrlIfExpired(feeds: List<FeedEntity>) {
    feeds.forEach { feed ->
        if (feed.imageUrls.isNotEmpty() && ChronoUnit.MINUTES.between(feed.updatedAt, Instant.now()) >= 10) {
            for (number in 1..feed.imageUrls.size) {
                imageService.generateGetPresignedUrl(feed.imageUrls[number - 1])
            }
            feed.updatedAt = Instant.now()
            feedRepository.save(feed)
        }
    }
}

그렇기에 POST 요청 핸들러에서도 반드시 GET presigned url을 생성해야 합니다.

// editFeed()
val imagePutPresignedUrls: MutableList<String> = mutableListOf()
if (request.imageCount > 0) {
    for (number in 1..request.imageCount) {
        val imageUrlEntity: ImageUrlEntity = imageService.postImageUrl("feed", feedEntity.id!!, number)

        val imagePutPresignedUrl: String = imageService.generatePutPresignedUrl(imageUrlEntity.s3)
        imagePutPresignedUrls += imagePutPresignedUrl

        imageService.generateGetPresignedUrl(imageUrlEntity)
        feedEntity.imageUrls += imageUrlEntity
    }
    feedEntity.updatedAt = Instant.now()
}

POST 요청 핸들러를 구현하는 사람은 GET 요청 핸들러가 정확히 어떻게 구현되어 있는지 모르기 때문에, 이런 구조는 대형 프로젝트에서는 버그에 취약할 수 있다 생각됩니다..!

개선책 제안

POST handler에서 GET presigned url 생성 로직을 없애고, GET handler에 presigned 필드 존재 여부를 확인하는 로직이 추가되면 좋을 것 같습니다. 또, refreshPresignedUrlIfExpired 의 nested Transactional 문제도 수정하면 좋을 듯합니다.

---

Refresh Presigned URL If Expired

image url을 refresh하는 함수에서 updateAt field로 url의 expire 여부를 check 중인데, 일반적으로 updatedAt은 entity 정보(글의 제목, 제품 가격 등)의 최종 수정 시간을 기록하는 용도로 사용된다고 생각합니다.

이미지 url refresh는 entity 정보가 수정된다고 보기엔 어려운 것 같아, PresignedURL 의 유효 기간 체크를 위해서는 다른 이름의 field를 활용하는 편이 좋을 것 같습니다.

그리고 entity의 정보가 변경될 때 마다, 자동으로 updatedAt을 갱신하는 annoation (Prepersist 등)이 존재하는데, 해당 부분을 활용해보는 것도 좋을 것 같습니다.

기타 피드백

---

Like, Unlike에서 mappedBy 필드 수정 불필요

@Transactional
    fun likeFeed(
        feedId: Long,
        id: String,
    ) {
        val feedEntity = feedRepository.findByIdOrNull(feedId) ?: throw FeedNotFoundException()
        val userEntity = userService.getUserEntityById(id)
        if (feedEntity.feedLikes.any { it.user.id == userEntity.id }) {
            return
        }
        val feedLikesEntity =
            feedLikesRepository.save(
                FeedLikesEntity(feed = feedEntity, user = userEntity, createdAt = Instant.now(), updatedAt = Instant.now()),
            )
        feedEntity.feedLikes += feedLikesEntity
    }

    @Transactional
    fun unlikeFeed(
        feedId: Long,
        id: String,
    ) {
        val feedEntity = feedRepository.findByIdOrNull(feedId) ?: throw FeedNotFoundException()
        val toBeRemoved: FeedLikesEntity = feedLikesRepository.findByUserIdAndFeedId(id, feedId) ?: return
        feedEntity.feedLikes.remove(toBeRemoved)
        feedLikesRepository.delete(toBeRemoved)
    }

FeedLikesEntity와 FeedEntity와의 관계를 살펴보면 N :1 관계로 연관관계의 주인이 FeedLikeEntity로 설정되어 있습니다.

이때, 자동으로 FeedLikeEntity 생성만 하더라도, 자동으로 FeedEntity와의 매핑관계는 형성되므로

feedEntity.feedLikes += feedLikesEntity

feedEntity.feedLikes.remove(toBeRemoved) 의 필요성에 의문이 제기될 수 있습니다.

이미 save를 하거나, feedLikeEntity 자체만을 삭제하면 알아서 DB 테이블에서 연관관계가 끊기기 때문입니다.

개선책

feedEntity에서 추가적으로 다른 서비스 로직이나 컨트롤러에서 feedLikes를 직접적으로 호출해서 사용하는 경우가 있다면 해당 로직을 유지해도 되나, (feedLikes를 호출하는 일이 생긴다면 DB 쿼리문을 다시 짜서 불러와야하므로 인메모리로 저장해두면 쿼리를 날릴 필요가 없기에 DB Workload가 줄어듭니다)

별도로 feedLikes를 사용하는 서비스로직이 없다면

• feedLikes 필드 삭제
• feedEntity.feedLikes += feedLikesEntity 삭제
• feedEntity.feedLikes.remove(toBeRemoved) 삭제

---

로그인 WhiteLabel Page

프론트와 관련된 이슈일수도 있는데,

배포 환경에서 카카오 로그인, 구글 로그인 했을 때 로그인 하자마자 White Label Page가 뜨고있습니다.

로그인 관련한 이슈 화이팅입니다.. !!

---

이미지가 처음엔 보였다가 안보임

처음에 배포사이트 접속시 이미지가 잘 보였다가

1시간 정도 뒤에 와서 창닫고 다시 로그인하니 이미지가 안뜨고 있습니다.

f12 > 네트워크 체크하였을떄 화면은 다음과 같습니다.

---

getUserEntityById 함수

현재 userId를 통해, userEntity를 가져오는 함수가 userService에 getUserEntityById로 구현이 되어 있는 것을 확인했습니다.

userService가 아닌 userRepository를 직접 불러와서, JPARepository의 기본 기능인 findByIdOrNull 함수를 사용하는 것도 좋은 방법일 것 같습니다.

---

코드리뷰하면서 저희도 놓치고 있던 부분들은 다시금 상기할 수 있는 좋은 기회가 되었습니다.

그리고 항상 갱커피 가실때마다 다같이 모이셔서 열심히 하시는 모습이 정말 인상깊었습니다.. !!

지금까지 정말 수고 많으셨고 기말 총회에서도 좋은 발표 기대하겠습니다!