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.

Sign up for GitHub

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

fix: silently ignores exception when parameter setting to count query #781

Merged
merged 9 commits into from
Oct 15, 2024
+512 −20
Merged

fix: silently ignores exception when parameter setting to count query #781

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
13f5e17
fix: silently ignores exception when parameter setting to count query
bagger3025 Oct 11, 2024
fe2b4b4
fix: use map instead of try-catch
bagger3025 Oct 11, 2024
e914ced
fix: test error fix and change map to set
bagger3025 Oct 11, 2024
20cf102
fix: revert divided methods into one method
bagger3025 Oct 11, 2024
f9af12c
fix: debug logging when parameter binding is skipped
bagger3025 Oct 11, 2024
5be754e
fix: minor grammar fix
bagger3025 Oct 11, 2024
78b44f6
fix: apply exception ignore to all in support
bagger3025 Oct 12, 2024
2e58f1e
refactor: logging with string template
bagger3025 Oct 14, 2024
fe8ff03
refactor: debug logic into else statement
bagger3025 Oct 15, 2024
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
1 change: 1 addition & 0 deletions support/eclipselink-javax/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
dependencies {
compileOnly(libs.eclipselink2)
compileOnly(libs.javax.persistence.api)
compileOnly(libs.slf4j)
compileOnly(projects.jpqlDsl)
compileOnly(projects.jpqlQueryModel)
compileOnly(projects.jpqlRender)
Expand Down
15 changes: 14 additions & 1 deletion ...c/main/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/javax/JpqlEntityManagerUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.linecorp.kotlinjdsl.support.eclipselink.javax

import com.linecorp.kotlinjdsl.querymodel.jpql.JpqlQuery
import com.linecorp.kotlinjdsl.render.RenderContext
import org.slf4j.LoggerFactory
import javax.persistence.EntityManager
import javax.persistence.Query
import javax.persistence.TypedQuery
Expand Down Expand Up @@ -74,8 +75,20 @@ internal object JpqlEntityManagerUtils {
}

private fun setParams(query: Query, params: Map<String, Any?>) {
val parameterNameSet = query.parameters.map { it.name }.toHashSet()

params.forEach { (name, value) ->
query.setParameter(name, value)
if (parameterNameSet.contains(name)) {
query.setParameter(name, value)
} else if (log.isDebugEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

이건 개인적인 취향이기 때문에 반영해주셔도 되고 안 해주셔도 됩니다.

개인적으로는 if else의 레벨을 맞추는 것을 선호합니다.
parameterNameSet에 name이 포함되어 있는지 여부와 log가 debug 레벨인지 여부는 같은 레벨의 비지니스로 보기 어렵다고 생각합니다.

그래서 개인적으로는 else 블록 안에서 if를 추가하는 것을 선호합니다.
그러면 코드를 해석할 때, 파라미터 이름이 없으면 다른 작업을 하게 되는데 그 중에서 로그 레벨이 debug면 로그를 출력한다와 같이 읽을 수 있기 때문입니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이거는 저도 조금 이상해 보이긴 했습니다 저는 수정하는게 좋을것 같습니다

Copy link
Contributor Author

@bagger3025 bagger3025 Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

else 안에 if 들어가는 코드여서 바깥으로 뺐는데, 말씀해주신 대로 코드를 바꾸는 게 맞는 것 같습니다.

수정해서 커밋하겠습니다!

log.debug(
"No parameter named '{}' in query with named parameters [{}], parameter binding skipped",
Copy link
Collaborator

Choose a reason for hiding this comment

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

{} 로 로그를 파라메터 파싱하는것보다는 $name 과 같이 처리하는게 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

성능상 문제 있을까 하여 따로 파라미터로 전달한 것이었는데, log.isDebugEnabled로 출력할 수 있는지 확인하고 있고 가독성 측면에서도 좋을 것 같아 로그 출력 방식을 string template으로 바꿨습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

{} 로 가져가는게 오히려 성능에 더 문제가 있을 수 있습니다 (일단 스트링 파싱을 해야하고 replace 후 재생성을 해야하니 문자열 연결보다 더 성능이 좋을 수는 없을것 같습니다 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{} 로 가져가는게 오히려 성능에 더 문제가 있을 수 있습니다 (일단 스트링 파싱을 해야하고 replace 후 재생성을 해야하니 문자열 연결보다 더 성능이 좋을 수는 없을것 같습니다 :)

그렇겠네요. 하나 배웠습니다!

name,
parameterNameSet.joinToString(),
)
}
}
}
}

private val log = LoggerFactory.getLogger(JpqlEntityManagerUtils::class.java)
33 changes: 33 additions & 0 deletions ...st/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/javax/JpqlEntityManagerUtilsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.extension.ExtendWith
import javax.persistence.EntityManager
import javax.persistence.Parameter
import javax.persistence.TypedQuery

@ExtendWith(MockKExtension::class)
Expand All @@ -35,6 +36,12 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
@MockK
private lateinit var stringTypedQuery1: TypedQuery<String>

@MockK
private lateinit var stringTypedQueryParam1: Parameter<String>

@MockK
private lateinit var stringTypedQueryParam2: Parameter<String>

private val renderedQuery1 = "query"
private val renderedParam1 = "queryParam1" to "queryParamValue1"
private val renderedParam2 = "queryParam2" to "queryParamValue2"
Expand All @@ -51,6 +58,8 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

excludeRecords { JpqlRendererHolder.get() }
excludeRecords { stringTypedQuery1.equals(any()) }
excludeRecords { stringTypedQueryParam1.hashCode() }
excludeRecords { stringTypedQueryParam2.hashCode() }
}

@Test
Expand All @@ -60,7 +69,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>(), any<Class<String>>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils.createQuery(entityManager, query1, String::class, context)
Expand All @@ -71,6 +83,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, context)
entityManager.createQuery(rendered1.query, String::class.java)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand All @@ -83,7 +98,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>(), any<Class<String>>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils
Expand All @@ -95,6 +113,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, mapOf(queryParam1, queryParam2), context)
entityManager.createQuery(rendered1.query, String::class.java)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand All @@ -107,7 +128,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils.createQuery(entityManager, query1, context)
Expand All @@ -118,6 +142,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, context)
entityManager.createQuery(rendered1.query)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand All @@ -130,7 +157,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils
Expand All @@ -142,6 +172,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, mapOf(queryParam1, queryParam2), context)
entityManager.createQuery(rendered1.query)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand Down
1 change: 1 addition & 0 deletions support/eclipselink/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
dependencies {
compileOnly(libs.eclipselink3)
compileOnly(libs.jakarta.persistence.api)
compileOnly(libs.slf4j)
compileOnly(projects.jpqlDsl)
compileOnly(projects.jpqlQueryModel)
compileOnly(projects.jpqlRender)
Expand Down
15 changes: 14 additions & 1 deletion ...ink/src/main/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/JpqlEntityManagerUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.linecorp.kotlinjdsl.render.RenderContext
import jakarta.persistence.EntityManager
import jakarta.persistence.Query
import jakarta.persistence.TypedQuery
import org.slf4j.LoggerFactory
import kotlin.reflect.KClass

internal object JpqlEntityManagerUtils {
Expand Down Expand Up @@ -74,8 +75,20 @@ internal object JpqlEntityManagerUtils {
}

private fun setParams(query: Query, params: Map<String, Any?>) {
val parameterNameSet = query.parameters.map { it.name }.toHashSet()

params.forEach { (name, value) ->
query.setParameter(name, value)
if (parameterNameSet.contains(name)) {
query.setParameter(name, value)
} else if (log.isDebugEnabled) {
log.debug(
"No parameter named '{}' in query with named parameters [{}], parameter binding skipped",
name,
parameterNameSet.joinToString(),
)
}
}
}
}

private val log = LoggerFactory.getLogger(JpqlEntityManagerUtils::class.java)
33 changes: 33 additions & 0 deletions ...src/test/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/JpqlEntityManagerUtilsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import io.mockk.junit5.MockKExtension
import io.mockk.mockkObject
import io.mockk.verifySequence
import jakarta.persistence.EntityManager
import jakarta.persistence.Parameter
import jakarta.persistence.TypedQuery
import org.assertj.core.api.WithAssertions
import org.junit.jupiter.api.BeforeEach
Expand All @@ -35,6 +36,12 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
@MockK
private lateinit var stringTypedQuery1: TypedQuery<String>

@MockK
private lateinit var stringTypedQueryParam1: Parameter<String>

@MockK
private lateinit var stringTypedQueryParam2: Parameter<String>

private val renderedQuery1 = "query"
private val renderedParam1 = "queryParam1" to "queryParamValue1"
private val renderedParam2 = "queryParam2" to "queryParamValue2"
Expand All @@ -51,6 +58,8 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

excludeRecords { JpqlRendererHolder.get() }
excludeRecords { stringTypedQuery1.equals(any()) }
excludeRecords { stringTypedQueryParam1.hashCode() }
excludeRecords { stringTypedQueryParam2.hashCode() }
}

@Test
Expand All @@ -60,7 +69,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>(), any<Class<String>>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils.createQuery(entityManager, query1, String::class, context)
Expand All @@ -71,6 +83,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, context)
entityManager.createQuery(rendered1.query, String::class.java)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand All @@ -83,7 +98,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>(), any<Class<String>>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils
Expand All @@ -95,6 +113,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, mapOf(queryParam1, queryParam2), context)
entityManager.createQuery(rendered1.query, String::class.java)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand All @@ -107,7 +128,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils.createQuery(entityManager, query1, context)
Expand All @@ -118,6 +142,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, context)
entityManager.createQuery(rendered1.query)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand All @@ -130,7 +157,10 @@ class JpqlEntityManagerUtilsTest : WithAssertions {

every { renderer.render(any(), any(), any()) } returns rendered1
every { entityManager.createQuery(any<String>()) } returns stringTypedQuery1
every { stringTypedQuery1.parameters } returns setOf(stringTypedQueryParam1, stringTypedQueryParam2)
every { stringTypedQuery1.setParameter(any<String>(), any()) } returns stringTypedQuery1
every { stringTypedQueryParam1.name } returns renderedParam1.first
every { stringTypedQueryParam2.name } returns renderedParam2.first

// when
val actual = JpqlEntityManagerUtils
Expand All @@ -142,6 +172,9 @@ class JpqlEntityManagerUtilsTest : WithAssertions {
verifySequence {
renderer.render(query1, mapOf(queryParam1, queryParam2), context)
entityManager.createQuery(rendered1.query)
stringTypedQuery1.parameters
stringTypedQueryParam1.name
stringTypedQueryParam2.name
stringTypedQuery1.setParameter(renderedParam1.first, renderedParam1.second)
stringTypedQuery1.setParameter(renderedParam2.first, renderedParam2.second)
}
Expand Down
1 change: 1 addition & 0 deletions support/hibernate-javax/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ dependencies {
@Suppress("VulnerableLibrariesLocal", "RedundantSuppression")
compileOnly(libs.hibernate5.core)
compileOnly(libs.javax.persistence.api)
compileOnly(libs.slf4j)
compileOnly(projects.jpqlDsl)
compileOnly(projects.jpqlQueryModel)
compileOnly(projects.jpqlRender)
Expand Down
15 changes: 14 additions & 1 deletion ...javax/src/main/kotlin/com/linecorp/kotlinjdsl/support/hibernate/JpqlEntityManagerUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package com.linecorp.kotlinjdsl.support.hibernate

import com.linecorp.kotlinjdsl.querymodel.jpql.JpqlQuery
import com.linecorp.kotlinjdsl.render.RenderContext
import org.slf4j.LoggerFactory
import javax.persistence.EntityManager
import javax.persistence.Query
import javax.persistence.TypedQuery
Expand Down Expand Up @@ -76,8 +77,20 @@ internal object JpqlEntityManagerUtils {
}

private fun setParams(query: Query, params: Map<String, Any?>) {
val parameterNameSet = query.parameters.map { it.name }.toHashSet()

params.forEach { (name, value) ->
query.setParameter(name, value)
if (parameterNameSet.contains(name)) {
query.setParameter(name, value)
} else if (log.isDebugEnabled) {
log.debug(
"No parameter named '{}' in query with named parameters [{}], parameter binding skipped",
name,
parameterNameSet.joinToString(),
)
}
}
}
}

private val log = LoggerFactory.getLogger(JpqlEntityManagerUtils::class.java)
Loading
Loading