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

Add findOne function in spring boot jpa supporter #531

Closed
wants to merge 2 commits into from

Conversation

kihwankim
Copy link
Contributor

@kihwankim kihwankim commented Nov 17, 2023

Motivation

  • [Eng] motivated spring data jpa fun findByUsername(username: String): User? function
  • [Kor] spring data jpa의 fun findByUsername(username: String): User? 함수로 부터 염감을 받았습니다

Modifications

  • [Eng] when I use unique key and if I use findAll then I need to check in (one/null/more than two rows) application/adapter layer or using extension function in repository layer, I thinks it's not simple so I create new function

  • [Kor] unique key 를 사용할 때 그리고 findAll를 사용할 경우에는 1개/null/2개이상 검증을 application/adpater 레이어에서 확인하던지 아니면 repository layer의 확장함수를 만들어야 합니다, 간단한 방식이 더 조아 하기 때문에 새로운 기능을 개발 하였습니다

Result

  • findOne 함수 처리 완료

Closes

@shouwn
Copy link
Member

shouwn commented Nov 18, 2023

In the case of findOne and findFirst, we removed them from the 3.0.0 Snapshot version because they were confusing from a user perspective.

The findOne you wrote is written simply, but if you match the interface provided by JpqRepository, it should be written as the following code:

    override fun <T : Any, DSL : JpqlDsl> findOne(
        dsl: JpqlDsl.Constructor<DSL>,
        init: DSL.() -> JpqlQueryable<SelectQuery<T>>,
    ): T? {
        val query: SelectQuery<T> = jpql(dsl, init)
        val jpa = JpqlEntityManagerUtils.createQuery(entityManager, query, renderContext)

        return try {
            jpa.setMaxResults(2).singleResult
        } catch (e: NonUniqueResultException) {
            @Suppress("NULLABILITY_MISMATCH_BASED_ON_JAVA_ANNOTATIONS")
            throw IncorrectResultSizeDataAccessException(e.message, 1, e)
        } catch (e: NoResultException) {
            null
        }
    }

The important thing to note about this code is that it contains setMaxResults(). Since Kotlin JDSL supports JPQL, it is possible to call findOne with a query that includes a fetch join.

In this case, the fetch join and max result will cause a memory join to occur and can lead to a fatal error.

Users should be aware of this, like Pageable, but unfortunately, since Kotlin JDSL is not a popular library, it doesn't have many communication points, so it's hard to communicate this risk.

We decided not to provide methods like findOne or findFirst because even if we wrote it in the comments, users who didn't read the comments might use them incorrectly and leave Kotlin JDSL if they get a bug.

We apologize for not being able to merge the PR you wrote to help you with this.


findOne 및 findFirst의 경우는 3.0.0 Snapshot 버전에서 제공했다가 사용자 관점에서 헷갈리는 요소가 많아 제외하게 되었습니다.

작성해주신 findOne은 심플하게 작성되어 있지만 JpqRepository이 제공하는 인터페이스와 동일하게 맞추면 아래와 같은 코드로 작성 되어야 합니다.

    override fun <T : Any, DSL : JpqlDsl> findOne(
        dsl: JpqlDsl.Constructor<DSL>,
        init: DSL.() -> JpqlQueryable<SelectQuery<T>>,
    ): T? {
        val query: SelectQuery<T> = jpql(dsl, init)
        val jpa = JpqlEntityManagerUtils.createQuery(entityManager, query, renderContext)

        return try {
            jpa.setMaxResults(2).singleResult
        } catch (e: NonUniqueResultException) {
            @Suppress("NULLABILITY_MISMATCH_BASED_ON_JAVA_ANNOTATIONS")
            throw IncorrectResultSizeDataAccessException(e.message, 1, e)
        } catch (e: NoResultException) {
            null
        }
    }

이 코드의 주의점은 setMaxResults()가 들어간다는 점입니다. Kotlin JDSL은 JPQL을 지원하기 때문에 fetch join이 포함된 쿼리로 findOne을 호출할 수 있습니다.

이 경우 fetch join과 max result로 인해 메모리 조인이 발생하게되고 장애로 이어질 수 있습니다.

사용자는 Pageable과 같이 이점을 유의해서 사용해야 하지만 아쉽게도 Kotlin JDSL은 유명한 라이브러리가 아니기 때문에 사용자 커뮤니케이션 층이 많지 않아 이 위험성을 알리기 어렵습니다.

주석에 작성해놓는다고 해도 주석을 읽지 않은 사용자가 잘못 사용해서 장애가 나면 Kotlin JDSL을 떠날 가능성이 있어 findOne 혹은 findFirst 같은 메소드를 제공하지 않게 결정하였습니다.

이렇게 도움을 주시기 위해 PR을 작성해주셨는데, 머지하지 못 하게 되어 정말 죄송합니다.

@kihwankim
Copy link
Contributor Author

oh thank you I close this pr
감사합니다 pr close 하겠습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants