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

bagger3025 · 2024-10-11T04:27:32Z

Motivation

Spring Data JPA를 Pageable과 사용하면서 에러로 동작하지 않는 상황을 발견하였는데, 해당 상황을 해결하는 풀리퀘를 하려고 합니다.

Modifications

Spring Data JPA 코드를 참고하였습니다.
Count Query를 생성하는 경우 파라미터 바인딩하는 로직을 수정하였습니다.
- RuntimeException을 만나는 경우 log를 찍는 함수를 만들어 그 함수로 파라미터 바인딩하도록 변경하였습니다.

Commit Convention Rule

Please commit your modification based by conventional commit:

Commit type	Description
feat	New Feature
fix	Fix bug
docs	Documentation only changed
ci	Change CI configuration
refactor	Not a bug fix or add feature, just refactoring code
test	Add Test case or fix wrong test case
style	Only change the code style(ex. white-space, formatting)
chore	It refers to minor tasks such as library version upgrade, typo correction, etc.

If you want to add some more commit type please describe it on the Pull Request

Result

파라미터 바인딩이 실패하더라도 Count Query가 문제 없이 실행되고, debug모드로 실행한 경우 로그를 출력합니다.

Closes

Count Query Fails with Spring Data JPA Pageable #780

CLAassistant · 2024-10-11T04:27:38Z

All committers have signed the CLA.

...pa/src/main/kotlin/com/linecorp/kotlinjdsl/support/spring/data/jpa/JpqlEntityManagerUtils.kt

.../main/kotlin/com/linecorp/kotlinjdsl/support/spring/data/jpa/javax/JpqlEntityManagerUtils.kt

bagger3025 · 2024-10-12T07:29:38Z

hibernate-reactive 패키지와 hibernate-reactive-javax 패키지에는 Query가 아닌 Mutiny.SelectionQuery<T> 등의 쿼리를 사용해서 getParameters와 같은 메서드가 존재하지 않습니다. Mutiny.SelectionQuery doc

Mutiny.SelectionQuery<T>뿐 아니라 Query 대신 사용되는 모든 타입이 getParameters와 같은 메서드를 지원하지 않았습니다. 따라서 원래 제안하였던 try-catch문을 사용해 구현하였고, log를 찍기 위해 build.gradle.kts 파일에 slf4j를 모두 포함해주었습니다.

codecov-commenter · 2024-10-12T10:00:11Z

Codecov Report

Attention: Patch coverage is 25.00000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 90.79%. Comparing base (e6a4209) to head (fe8ff03).

Files with missing lines	Patch %	Lines
...upport/eclipselink/javax/JpqlEntityManagerUtils.kt	25.00%	5 Missing and 1 partial ⚠️
...jdsl/support/eclipselink/JpqlEntityManagerUtils.kt	25.00%	5 Missing and 1 partial ⚠️
...pport/spring/batch/javax/JpqlEntityManagerUtils.kt	25.00%	5 Missing and 1 partial ⚠️
...dsl/support/spring/batch/JpqlEntityManagerUtils.kt	25.00%	5 Missing and 1 partial ⚠️
...rt/spring/data/jpa/javax/JpqlEntityManagerUtils.kt	25.00%	5 Missing and 1 partial ⚠️
.../support/spring/data/jpa/JpqlEntityManagerUtils.kt	25.00%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #781      +/-   ##
==========================================
- Coverage   91.73%   90.79%   -0.94%     
==========================================
  Files         337      337              
  Lines        3447     3489      +42     
  Branches      209      221      +12     
==========================================
+ Hits         3162     3168       +6     
- Misses        223      253      +30     
- Partials       62       68       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cj848 · 2024-10-12T21:46:24Z

.../kotlin/com/linecorp/kotlinjdsl/support/hibernate/reactive/JpqlStageStatelessSessionUtils.kt

        }
    }
 }
+
+private val log = LoggerFactory.getLogger(JpqlStageStatelessSessionUtils::class.java)


logger 가 클래스 안에서만 쓰이는걸로 보이는데 파일 스코프로 선언하신 이유가 있을까요?

render/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/render/jpql/JpqlRenderer.kt에서 logger를 선언하고 사용하는 부분을 참고해 같은 방식으로 하였습니다.

logger를 클래스 안으로 선언하는 것이 좋다고 생각하신다면 반영하겠습니다.

이유가 궁금했던 것인데요, 기존에 있던 사례라면 따라도 상관은 없을 것 같습니다 :)

cj848 · 2024-10-14T02:05:36Z

.../src/main/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/javax/JpqlEntityManagerUtils.kt

+                query.setParameter(name, value)
+            } else if (log.isDebugEnabled) {
+                log.debug(
+                    "No parameter named '{}' in query with named parameters [{}], parameter binding skipped",


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

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

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

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

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

.../src/main/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/javax/JpqlEntityManagerUtils.kt

shouwn

도움을 주셔서 정말 감사합니다!

shouwn · 2024-10-15T08:55:25Z

.../src/main/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/javax/JpqlEntityManagerUtils.kt

-            query.setParameter(name, value)
+            if (parameterNameSet.contains(name)) {
+                query.setParameter(name, value)
+            } else if (log.isDebugEnabled) {


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

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

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

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

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

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

cj848 · 2024-10-15T21:43:53Z

@shouwn 머지 진행하겠습니다. 릴리즈는 @shouwn 님이 해주시면 되겠습니다 👍

shouwn · 2024-10-18T00:02:59Z

@bagger3025 오래 기다리셨습니다. 오늘 배포진행하겠습니다.

bagger3025 requested review from pickmoment, huisam, cj848 and shouwn as code owners October 11, 2024 04:27

shouwn reviewed Oct 11, 2024

View reviewed changes

...pa/src/main/kotlin/com/linecorp/kotlinjdsl/support/spring/data/jpa/JpqlEntityManagerUtils.kt Outdated Show resolved Hide resolved

bagger3025 changed the base branch from develop to main October 11, 2024 07:37

bagger3025 changed the base branch from main to develop October 11, 2024 07:38

bagger3025 added 2 commits October 11, 2024 16:41

fix: silently ignores exception when parameter setting to count query

13f5e17

fix: use map instead of try-catch

fe2b4b4

bagger3025 force-pushed the bugfix-count-query branch from ecb7a8b to fe2b4b4 Compare October 11, 2024 07:42

bagger3025 changed the base branch from develop to main October 11, 2024 07:43

bagger3025 added 3 commits October 11, 2024 17:19

fix: test error fix and change map to set

e914ced

fix: revert divided methods into one method

20cf102

fix: debug logging when parameter binding is skipped

f9af12c

bagger3025 commented Oct 11, 2024

View reviewed changes

.../main/kotlin/com/linecorp/kotlinjdsl/support/spring/data/jpa/javax/JpqlEntityManagerUtils.kt Outdated Show resolved Hide resolved

fix: minor grammar fix

5be754e

shouwn reviewed Oct 11, 2024

View reviewed changes

.../main/kotlin/com/linecorp/kotlinjdsl/support/spring/data/jpa/javax/JpqlEntityManagerUtils.kt Show resolved Hide resolved

fix: apply exception ignore to all in support

78b44f6

cj848 reviewed Oct 12, 2024

View reviewed changes

cj848 reviewed Oct 14, 2024

View reviewed changes

refactor: logging with string template

2e58f1e

cj848 reviewed Oct 14, 2024

View reviewed changes

.../src/main/kotlin/com/linecorp/kotlinjdsl/support/eclipselink/javax/JpqlEntityManagerUtils.kt Outdated Show resolved Hide resolved

shouwn approved these changes Oct 15, 2024

View reviewed changes

refactor: debug logic into else statement

fe8ff03

cj848 approved these changes Oct 15, 2024

View reviewed changes

cj848 merged commit 7105c9a into line:main Oct 15, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bagger3025 commented Oct 11, 2024 •

edited

Loading

CLAassistant commented Oct 11, 2024 •

edited

Loading

bagger3025 commented Oct 12, 2024

codecov-commenter commented Oct 12, 2024 •

edited

Loading

cj848 Oct 12, 2024

bagger3025 Oct 13, 2024

cj848 Oct 13, 2024

cj848 Oct 14, 2024

bagger3025 Oct 14, 2024

cj848 Oct 14, 2024

bagger3025 Oct 14, 2024

shouwn left a comment

shouwn Oct 15, 2024

cj848 Oct 15, 2024

bagger3025 Oct 15, 2024 •

edited

Loading

cj848 commented Oct 15, 2024

shouwn commented Oct 18, 2024

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

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

Conversation

bagger3025 commented Oct 11, 2024 • edited Loading

Motivation

Modifications

Commit Convention Rule

Result

Closes

CLAassistant commented Oct 11, 2024 • edited Loading

bagger3025 commented Oct 12, 2024

codecov-commenter commented Oct 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shouwn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagger3025 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

cj848 commented Oct 15, 2024

shouwn commented Oct 18, 2024

bagger3025 commented Oct 11, 2024 •

edited

Loading

CLAassistant commented Oct 11, 2024 •

edited

Loading

codecov-commenter commented Oct 12, 2024 •

edited

Loading

bagger3025 Oct 15, 2024 •

edited

Loading