where 메소드 리팩토링

[jaeykweon]

안녕하세요. jdsl에 흥미를 느껴 사이드 프로젝트에 적용해보고 있습니다. where 절 관련해서 제언 사항이 있어 이슈와 pr을 올려보고자 합니다.

제가 하려고 하는 것은 무한 스크롤인데요. 일반적인 페이징이 아닌, no offset을 위해 입력받은 id를 기준으로 일정 개수만큼 쿼링하고자합니다. (참고 하고 있는 내용: https://jojoldu.tistory.com/528)

하지만 첫번째 페이지는 특성 상 id를 알기 어려운 것으로 알고 있습니다. 따라서 아래와 같은 코드를 작성하고 싶은데, null을 허용하지 않아 불가능합니다.

[그림 1 : 목표 코드]

우선 아래 두 가지 해결 방법을 찾았습니다. 하지만 코드가 (생각보다) 복잡해졌다고 생각합니다.

[그림 2: 해결 방법 1 - let 구문 안에 where 절 삽입] [그림 3 : 해결 방법 2 - PredicateSpec.empty 사용]

lastTermId가 null일 경우, 그림 2와 그림 3의 코드 모두 where절을 만들지 않고 아래와 같은 쿼리를 만들었습니다

select
        termentity0_.id as col_0_0_ 
    from
        tb_term termentity0_ 
    order by
        termentity0_.id desc limit ?

빈 and()을 활용하는 방법도 있으나, 이 경우 where 1=1 이 포함된 쿼리가 생성되고, 이 쿼리는 매우 좋지 않은 것으로 알고 있습니다.

라이브러리를 다음과 같이 변경하면 좀 더 편하고 간결하게 사용할 수 있지 않을까 싶습니다 [그림 4 : where interface 리팩토링] [그림 5 : QueryDslImpl 리팩토링] 또한 그림4와 그림 5의 코드를 통해 and()구문을 사용하지 않아도 되니, 좀 더 간결한 코드를 작성할 수 있지 않을까 합니다.

주니어 개발자라 아직 모르는 게 많아 틀릴 수 있으니 너그러이 봐주시면 감사하겠습니다.

[jaeykweon]

PR : https://github.com/line/kotlin-jdsl/pull/75

[shouwn]

-- korean @jaeykweon 님 안녕하세요. 먼저 제안 주셔서 감사합니다.

말씀해주신 것처럼 where 구문에서 null을 허용할 경우 사용자 입장에서 더 쉽게 사용할 수 있고, 읽기에도 어려움이 없는 것으로 보입니다. 이 부분은 반영하면 좋을 것 같아요.

상세하게 이미지를 남겨주셔서 더 확인하기 쉬웠습니다. 감사합니다!

하지만 where 구문에 PerdicateSpec을 여러개 받는 형태는 맞지 않는 것으로 보입니다. 왜냐하면 여러개의 Predicate 스펙을 and로 묶을지, or로 묶을지 메소드 시그니처에서 들어나지 않기 때문입니다.

만약 whereAnd(vararg predicates: PrediateSpec?) 과 같은 형태라면 and 절로 묶는 것이 메소드명으로 표현이 되기 때문에 괜찮지만 현재의 where는 그렇지 않기 떄문에 어렵다고 생각이 듭니다.

이번 기회에 whereAnd 메소드를 추가해주셔도 좋다고 생각 됩니다.

말씀해주신 내용은 사용자 입장에서 충분히 반영하면 좋을 내용으로 생각되어 PR에서 리뷰를 진행하도록 하겠습니다. 이렇게 제안 주셔서 감사합니다 ❤️

-- english Hello, jaeykweon. Thank you for your suggestion.

As you said, if JDSL allow null in the where syntax, it will be easier for users to use where clause, and it doesn't seem difficult to read for other people. I think it would be good to apply this content.

It was easier to check because you left a detailed image. Thank you!

However, it does not seem to fit the form of receiving multiple PredicateSpec in the where syntax. This is because the method signature does not indicate whether to combine multiple PredicateSpec with and or or.

If it's the same form as 'whereAnd(vararg predcates: PredicatesSpec?)', it's okay because it's expressed in a method name.

I think you can take this opportunity to add the whereAnd method.

It would be good to fully apply what you said from the user's point of view, so I will proceed with the review in the PR. Thank you for your suggestion ❤️

[shouwn]

cc. @pickmoment @huisam @cj848

[jaeykweon]

@shouwn 친절한 답변 감사합니다 말씀해준 사항대로 코드를 짜서 refactor: changed input argument from vararg to single predicate at w… feature: add whereAnd and whereOr method 커밋에 적용하였습니다.

[그림1 where interface] [그림2 where interface 구현]

whereAnd와 whereOr 의 구현이 조금 복잡하게 되어있는데, 위에서 말씀드린 1=1 쿼리가 나온 것 때문에 이렇게 해보았습니다

아래와 같이 조건절에 null이 들어가 있는 경우 1=1 쿼리를 만들어냅니다.

[그림3 : where 조건절에 null만 있는 경우]

[그림4 : where 조건절에 null만 있는 경우의 sql]

그림 3과 4는 조금 극단적이지만 매우 치명적인 sql문이 될 수 있는 것 같습니다.

예상하기로는 or나 and 입력 인자에 null이 있는 경우나 리스트가 비어있는 경우 PredicateSpec.empty 객체를 만들어 내고, 이것이 1=1 쿼리를 만드는 것이 아닌가 싶습니다.

그래서 whereAnd나 whereOr 에서는 null인 항목이 있거나, 리스트가 비어있으면 동작하지 않도록 해놓았는데 이것이 올바르게 짜여진 것인지 리뷰 부탁드리겠습니다.

감사합니다.

[jaeykweon]

우선 아래와 같이 null과 PredicateSpec.empty 모두 1=1 sql이 생성된 것을 확인하였습니다

해당 문제를 해결하고자 아래와 같이 변경해 보았는데, 맞는 코드인지 잘 모르겠습니다,, 테스트 코드를 통과하기는 했습니다

[shouwn]

-- korean @jaeykweon 안녕하세요.

이전부터 궁금했지만 where 1 = 1이 왜 나쁘다고 판단하시는지 잘 모르겠습니다. where 1 = 1은 mybatis 및 JPA 내부에서도 사용되고 있는 유용한 쿼리 생성 기법입니다.

만약 성능 문제로 생각되신다면 where 1 = 1은 각 DB 쿼리 최적화를 통해 제거 되며 성능에 영향이 나오지 않습니다.

만약 코드 작성의 실수를 우려하신다면 where 1 = 1은 코드 작성의 실수를 막아줄 수 없습니다. 아마도 링크1 링크2 이러한 이유 때문에 우려하신다고 생각이 드는데요. 여기서 나온 예제에서 where 1 = 1을 제거한 뒤에 로직을 확인해 보았을 때 where 1 = 1이 없어질 뿐이지 버그는 그대로 존재합니다. 즉 where 1 = 1이 문제가 아니라 코드 작성자가 null에 대한 핸들링 로직을 추가하지 않았기 때문에 발생된 버그입니다. 이는 테스트 코드 및 코드 리뷰를 통해서 풀어야지 where 1 = 1을 제거 해서 푼다는 것은 맞지 않다고 생각됩니다. 제거해도 동일한 문제가 발생하기 때문입니다.

만들어주신 코드의 예를 들어본다면 where 1 = 1이 사라진다고 해서 빈 predicates가 들어왔을 때를 문제가 없다고 판단할 수 있을까요?

그렇기 때문에 where 1 = 1을 사용하는 것에 문제가 없다고 판단됩니다.

-- english I've been wondering since before, but I don't know why you think 'where 1 = 1' is bad. 'where 1 = 1' is a useful query generation technique also used within mybatis and JPA.

If you think of it as a performance issue, 'where 1 = 1' is removed through each DB query optimization and there is no performance impact.

If you are concerned about a mistake in code writing, 'where 1 = 1' cannot prevent a mistake in code writing. Maybe link1 link2 I think this is why you are concerned. In this example, when you remove 'where 1 = 1', the bug still exists; it is not because 'where 1 = 1' is the problem, but because the code writer did not add the handling logic for the null. I don't think it's right to solve this problem by removing 'where 1 = 1' through test code and code review. This is because the same problem occurs when you uninstalling

If you take an example of the code you created, can you judge that there is no problem when empty predictions come in just because 'where 1 = 1' disappears?

Therefore, it is judged that there is no problem using 'where 1 = 1'.

[jaeykweon]

이번에도 자세한 답변 감사드립니다!

말씀하신대로 개발자의 문제지 코드의 문제가 아닌 것 같습니다. 너무 제 실력 기준에서 생길 수 있는 오류를 고려하였습니다.

혹시나 해서 말씀드리자면, 제가 생각했던 경우는 아래와 같습니다

[그림 8 : or 조건절에 null 조건 핸들링 한 코드] [그림 9: 그림 8을 통해 만들어진 sql]

그림 8처럼 where 내부에 or로만 감쌌을 경우, 1=1 구문이 만들어지고, 치명적일지도 모르는 sql문이 실행 될 것이라 생각하였습니다.

하지만 말씀주신대로 이런 경우는 테스트 코드나 코드 리팩토링을 통해 해결하면 될 것으로 생각합니다.

그리고 pr 쪽에 리뷰 해주신 테스트 코드 사항들을 반영하여 따로 커밋하도록 하겠습니다. 아직 테스트 코드에 익숙치 않아 조금 시간이 걸리겠지만, 리뷰 해주신 게 헛되지 않도록 해보겠습니다. 감사합니다!

[jaeykweon]

@shouwn 혹시 추가된 메소드들에 대한 내용들도 docs에 추가하여 올려야 할까요?

[shouwn]

혹시 추가된 메소드들에 대한 내용들도 docs에 추가하여 올려야 할까요?

PR 리뷰에 코멘트로 작성해주셨다고 생각했는데 아니었군요. PR 리뷰에서 huisam 님이 docs에 대해서 리뷰해주셨기 때문에 이 내용은 확인 되었다고 넘어가겠습니다.

[shouwn]

-- korean 위 내용은 #75 을 통해 머지가 되었고 8월 22일에 배포 진행하도록 하겠습니다. 배포 이후 이슈는 Close 하겠습니다.

-- english The above contents were merged through the #75 and we will proceed with the deploy on August 22nd. I will close the issue after deploy.

[shouwn]

cc. @cj848 @pickmoment @huisam

[shouwn]

@jaeykweon 2.0.5.RELEASE 배포 완료하였습니다. Maven Repository 검색은 안 되지만 디펜던시 추가로 확인해보실 수 있습니다.

[shouwn]

-- korean 변경사항이 머지 완료 되어 이슈를 종료합니다.

-- english The changes are merged, so close this issue.