allow null input and no need to use and() at WhereDsl #75

jaeykweon · 2022-07-31T16:01:16Z

Motivation:

Modifications:

allow nullable predicates in where()
support multi 'and conditions' with whereAnd(), whereOr()
add test cases of changes
add examples of changes

Commit Convention Rule

Please commit your modification based by conventional commit
This commit convention is referred from angular convention

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)

Result:

we can

use nullable predicates in where()
use whereAnd(), whereOr() for multi 'and conditions'
test the changes
refer examples of changes

Closes #. (If this resolves the issue.)

Describe the consequences that a user will face after this PR is merged.

CLAassistant · 2022-07-31T16:01:20Z

All committers have signed the CLA.

core/src/main/kotlin/com/linecorp/kotlinjdsl/querydsl/QueryDslImpl.kt

…here

shouwn · 2022-08-01T23:26:51Z

core/src/test/kotlin/com/linecorp/kotlinjdsl/querydsl/where/QueryDslImplWhereTest.kt

@@ -67,8 +67,7 @@ internal class QueryDslImplWhereTest : WithKotlinJdslAssertions {
        val actual = QueryDslImpl(Data1::class.java).apply {
            select(distinct = true, Data1::class.java)
            from(entity(Data1::class))
-            where(predicateSpec1)
-            where(predicateSpec2)
+            where(predicateSpec1,predicateSpec2)


-- korean
where 메소드는 predicates를 받을 수 없기 때문에 잘못된 테스트로 보입니다.

-- english
The test appears to be an invalid test because where method cannot receive predicates as parameter.

-- korean
where 메소드에 대한 검증은 총 6개로 이루어져야 할 것으로 보입니다.
where, where - null, whereAnd - vararg, whereAnd - list, whereOr - vararg, whereOr - list

-- english
It seems that there should be a total of six tests for the where method.
where, where - null, whereAnd - vararg, whereAnd - list, whereOr - vararg, whereOr - list

Code formatting seems to be necessary as well.

where(predicateSpec1, predicateSpec2)

@shouwn 말씀하신 테스트 케이스들 추가하였습니다.
feature: add whereAnd and whereOr method

@cj848 refactor: rollback wheres() testCase에 해당 내용 적용하였습니다. 현재는 다른 메소드로 변경되었습니다.

jaeykweon · 2022-08-04T14:38:59Z

core/src/test/kotlin/com/linecorp/kotlinjdsl/querydsl/where/QueryDslImplWhereTest.kt

    @Test
-    fun nullInFirstWheres() {
+    fun allTypeWheres() {
        // given
-        val predicateSpec1: PredicateSpec? = null
+        val nullPredicateSpec: PredicateSpec? = null
+        val predicateSpec1: PredicateSpec = mockk()
        val predicateSpec2: PredicateSpec = mockk()
-        val predicateSpec3: PredicateSpec = mockk()

        // when
        val actual = QueryDslImpl(Data1::class.java).apply {
            select(distinct = true, Data1::class.java)
            from(entity(Data1::class))
-            where(predicateSpec1,predicateSpec2,predicateSpec3)
+            where(nullPredicateSpec)
+            where(predicateSpec1)
+            whereAnd(nullPredicateSpec, predicateSpec1, predicateSpec2)
+            whereOr(nullPredicateSpec, predicateSpec1, predicateSpec2)
        }

        // then
        val criteriaQuerySpec = actual.createCriteriaQuerySpec()

        assertThat(criteriaQuerySpec.where).isEqualTo(
-            WhereClause(AndSpec(listOf(predicateSpec2, predicateSpec3)))
+            WhereClause(
+                AndSpec(
+                    listOf(
+                        predicateSpec1,
+                        AndSpec(listOf(predicateSpec1, predicateSpec2)),
+                        OrSpec(listOf(predicateSpec1, predicateSpec2))
+                    )
+                )
+            )
        )

        val subquerySpec = actual.createSubquerySpec()

        assertThat(subquerySpec.where).isEqualTo(
-            WhereClause(AndSpec(listOf(predicateSpec2, predicateSpec3)))
+            WhereClause(
+                AndSpec(
+                    listOf(
+                        predicateSpec1,
+                        AndSpec(listOf(predicateSpec1, predicateSpec2)),
+                        OrSpec(listOf(predicateSpec1, predicateSpec2))
+                    )
+                )
+            )
        )
    }


각 메소드 별 단위 테스트는 있는데, 여러 조건들이 다 붙은 경우의 케이스도 검증이 필요하다고 생각하여 넣어보았습니다.

jaeykweon · 2022-08-04T14:42:03Z

query/src/main/kotlin/com/linecorp/kotlinjdsl/querydsl/where/WhereDsl.kt

+    fun whereAnd(vararg predicates: PredicateSpec?) = whereAnd(predicates.toList())
+    fun whereAnd(predicates: List<PredicateSpec?>)
+
+    fun whereOr(vararg predicates: PredicateSpec?) = whereOr(predicates.toList())
+    fun whereOr(predicates: List<PredicateSpec?>)


@shouwn whereAnd에 vararg를 인자로 넣어도, 내부에서 리스트로 변환되어 리스트를 인자로 넣은 경우와 같은데, 테스트 케이스를 따로 가져가야 할 이유가 있을까요? 내부에선 같더라도 어쨌든 다른 인터페이스이기때문에 그런 걸까요??

-- korean
@jaeykweon 넵 말씀해주신 다른 인터페이스이기 때문입니다.

내부 구현이 동일하다는 것은 개발자만이 알고 있는 사실입니다. 개인적으로는 테스트는 내부 구현을 알고 작성한다기 보다 외부의 시점에서 작성하는 것이 맞다고 생각하기 때문에 vararg와 list 테스트를 별도로 가져가도록 말씀드렸습니다.

-- english
Yes, it's because different interface.

Only developers know that internal implementations are same. Personally, The vararg and list test should be separated because I think it is right to write the test from an external perspective rather than knowing the internal implementation.

jaeykweon · 2022-08-04T16:02:44Z

@shouwn reactive쪽도 영향이 있어서 추가하였습니다

shouwn · 2022-08-05T08:48:31Z

cc. @huisam @cj848 @pickmoment

jaeykweon · 2022-08-05T17:00:41Z

@shouwn commit lint 오류난 부분은 수정하여 push -f 하면 될까요??

core/src/main/kotlin/com/linecorp/kotlinjdsl/querydsl/QueryDslImpl.kt

reactive-core/src/main/kotlin/com/linecorp/kotlinjdsl/querydsl/ReactiveQueryDslImpl.kt

cj848 · 2022-08-06T06:07:04Z

@shouwn commit lint 오류난 부분은 수정하여 push -f 하면 될까요??

please check lint and force push too.

-- korean
lint 확인해주시고 force push 해주세요.

jaeykweon · 2022-08-06T06:36:14Z

please check lint and force push too.

-- korean lint 확인해주시고 force push 해주세요.

@cj848 변경하였습니다!

cj848 · 2022-08-06T06:40:12Z

It would be nice if the readme.md explanation could also be added.

-- korean
readme.md 설명도 추가도 되면 좋겠습니다.

jaeykweon · 2022-08-06T08:28:06Z

It would be nice if the readme.md explanation could also be added.

-- korean readme.md 설명도 추가도 되면 좋겠습니다.

@cj848 readMe 설명 추가하였습니다.
docs: add docs for nullable where condition and multi where conditions 에서 확인하실 수 있습니다.
어느 부분에 추가해야 할 지 모르겠어서 임의대로 작성하였는데, 맞는 지 확인 부탁 드리겠습니다. 감사합니다.

cj848 · 2022-08-06T08:54:24Z

CLA 사인 부탁드립니다.

please sign a CLA

jaeykweon · 2022-08-06T09:37:05Z

CLA 사인 부탁드립니다.

please sign a CLA

@cj848 서명하였습니다!

huisam · 2022-08-07T12:34:31Z

README.md

+#### with nullable condition
+


-- korean
안녕하세요. :)
해당 documentation 을 조금 더 자세하게 작성해주시면 좋겠습니다. 왜냐하면 where PredicateSpec 에 대하여 null 을 허용되는 스펙으로 되었기 때문인데요.

where 의 PredicateSpec 을 nullable 로 바꾸게 되면 말씀주신 것처럼 nullable 한 검색조건에 대하여
코드를 조금 더 가독성 있게 작성할 수 있다는 장점을 얻었지만,
반대로 얻었던 단점은 where 조건에 대하여 개발자가 empty spec이 들어갈 수 있다라는 컴파일 타임에 인지할 수 있는 상황이 사라졌다고 생각합니다.

이는 충분한 테스트와 코드 리뷰를 통해서 발견할 수도 있지만, 라이브러리를 제공하는 사람의 입장에서도 어느정도 주의점으로 명시할 정도라고 생각되기도 하는데요.
설명 과 주의점 에 대하여 작성해주셔도 괜찮을 것 같은 의견인데, 어떠신가요?

-- english
Hello. :)
I would appreciate it if you could write the documentation in more detail. Because null is allowed spec for where Predict Spec.

If you change the predicate spec to nullable, you can write the code more legibly for null search conditions, as you mentioned
Conversely, I think that the situation that developers can recognize about where conditions can be filled with empty spec has disappeared.

This can be found through sufficient testing and code review, but I think it's a caution point for the library provider.
I think it would be okay if you write down 'Explanation' and 'Caution', what do you think?

@huisam 좋은 리뷰 감사합니다!

with nullable condition

null in where condition converts to EmptyPredicateSpec object.

note that EmptyPredicateSpec turns into 1=1 sql

val books: List<Book> = queryFactory.listQuery { select(entity(Book::class)) from(entity(Book::class)) where( name?.run { column(Book::author).equal(this) } ) }

이렇게 작성해보았는데 괜찮을까요?

-- korean
네 저는 충분하다고 생각합니다.

-- english
Yes, I think that's enough.

@huisam
docs: add descriptions for where method
말씀하신 null condition에 대한 설명을 적은 커밋입니다.
또한 whereAnd와 whereOr에 대한 설명도 필요하다고 생각해서 적어보았는데 괜찮은지 피드백 부탁드리겠습니다 감사합니다!

jaeykweon added 3 commits July 31, 2022 22:47

feat: allow null input at WhereDsl

e3e8331

feat: support predicate list at where

c4b66e2

refactor: add up two where into one (using vararg)

161de44

jaeykweon requested review from pickmoment, huisam, cj848 and shouwn as code owners July 31, 2022 16:01

jaeykweon mentioned this pull request Jul 31, 2022

where 메소드 리팩토링 #74

Closed

shouwn reviewed Aug 1, 2022

View reviewed changes

core/src/main/kotlin/com/linecorp/kotlinjdsl/querydsl/QueryDslImpl.kt Outdated Show resolved Hide resolved

refactor: changed input argument from vararg to single predicate at w…

83c9ce9

…here

shouwn reviewed Aug 1, 2022

View reviewed changes

jaeykweon commented Aug 4, 2022

View reviewed changes

shouwn approved these changes Aug 5, 2022

View reviewed changes

cj848 reviewed Aug 5, 2022

View reviewed changes

core/src/main/kotlin/com/linecorp/kotlinjdsl/querydsl/QueryDslImpl.kt Outdated Show resolved Hide resolved

cj848 reviewed Aug 5, 2022

View reviewed changes

reactive-core/src/main/kotlin/com/linecorp/kotlinjdsl/querydsl/ReactiveQueryDslImpl.kt Outdated Show resolved Hide resolved

jaeykweon added 8 commits August 6, 2022 15:32

feat: add whereAnd and whereOr method

38e096a

refactor: rollback wheres() testCase

d7e996a

test: add testCase

9aececa

feat: support null at where (reactive)

ed4ca7d

feat: implemented whereAnd, whereOr method (reactive)

ec7a773

test: add testCase for where (reactive)

8b991e0

style: formatting

b1057bc

refactor: using 'run' instead of 'let'

7bdb0de

jaeykweon force-pushed the jay/allow-null-at-where branch from 2ade308 to 7bdb0de Compare August 6, 2022 06:34

docs: add docs for nullable where condition and multi where conditions

5f26b67

cj848 added enhancement New feature or request good first issue Good for newcomers labels Aug 6, 2022

cj848 requested review from cj848 and shouwn August 6, 2022 08:46

cj848 approved these changes Aug 6, 2022

View reviewed changes

huisam reviewed Aug 7, 2022

View reviewed changes

docs: add descriptions for where method

fbc3081

huisam approved these changes Aug 9, 2022

View reviewed changes

cj848 merged commit 01a2757 into line:main Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow null input and no need to use and() at WhereDsl #75

allow null input and no need to use and() at WhereDsl #75

jaeykweon commented Jul 31, 2022 •

edited

CLAassistant commented Jul 31, 2022 •

edited

shouwn Aug 1, 2022

shouwn Aug 1, 2022

cj848 Aug 2, 2022

jaeykweon Aug 4, 2022

jaeykweon Aug 4, 2022

jaeykweon Aug 4, 2022

jaeykweon Aug 4, 2022

shouwn Aug 5, 2022

jaeykweon commented Aug 4, 2022

shouwn commented Aug 5, 2022

jaeykweon commented Aug 5, 2022

cj848 commented Aug 6, 2022

jaeykweon commented Aug 6, 2022 •

edited

cj848 commented Aug 6, 2022

jaeykweon commented Aug 6, 2022

cj848 commented Aug 6, 2022

jaeykweon commented Aug 6, 2022

huisam Aug 7, 2022 •

edited

jaeykweon Aug 7, 2022

huisam Aug 8, 2022 •

edited

jaeykweon Aug 8, 2022

allow null input and no need to use and() at WhereDsl #75

allow null input and no need to use and() at WhereDsl #75

Conversation

jaeykweon commented Jul 31, 2022 • edited

Motivation:

Modifications:

Commit Convention Rule

Result:

Closes #. (If this resolves the issue.)

CLAassistant commented Jul 31, 2022 • edited

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

Choose a reason for hiding this comment

jaeykweon commented Aug 4, 2022

shouwn commented Aug 5, 2022

jaeykweon commented Aug 5, 2022

cj848 commented Aug 6, 2022

jaeykweon commented Aug 6, 2022 • edited

cj848 commented Aug 6, 2022

jaeykweon commented Aug 6, 2022

cj848 commented Aug 6, 2022

jaeykweon commented Aug 6, 2022

huisam Aug 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

with nullable condition

huisam Aug 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeykweon commented Jul 31, 2022 •

edited

CLAassistant commented Jul 31, 2022 •

edited

jaeykweon commented Aug 6, 2022 •

edited

huisam Aug 7, 2022 •

edited

huisam Aug 8, 2022 •

edited