Support Datetime function CURRENT_DATE & CURRENT_TIME #624

meengi07 · 2024-02-02T17:17:14Z

Motivation

Support an currentDate & currentTime function.

Modifications

Add jpql model and serializer for currentDate
Add jpql model and serializer for currentTime

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

The Date function currentDate is added.
The Date function currentTime is added.

Closes

CLAassistant · 2024-02-02T17:17:20Z

All committers have signed the CLA.

codecov-commenter · 2024-02-05T00:22:45Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (4219a32) 91.97% compared to head (a237af3) 66.40%.
Report is 89 commits behind head on develop.

Files	Patch %	Lines
...in/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt	50.00%	1 Missing and 1 partial ⚠️
.../jpql/serializer/impl/JpqlCurrentDateSerializer.kt	66.66%	1 Missing ⚠️
.../jpql/serializer/impl/JpqlCurrentTimeSerializer.kt	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           develop     #624       +/-   ##
============================================
- Coverage    91.97%   66.40%   -25.57%     
============================================
  Files          297      465      +168     
  Lines         3215     4748     +1533     
  Branches       195      277       +82     
============================================
+ Hits          2957     3153      +196     
- Misses         204     1538     +1334     
- Partials        54       57        +3

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

shouwn · 2024-02-05T01:05:40Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+    fun currentDate(): Expression<LocalDate> {
+        return Expressions.currentDate()
+    }


In the Criteria API, the return type of currentDate() is the java.sql.Date.

The Jakarta specification also specifies that the return type is the java.sql.Date. Therefore, please change the return type to the java.sql.Date.

For the same reason as above, I would like to request that the return type of currentTime() be changed to the java.sql.Time.

i got it, I'll change currentTime & currentDate

waahhh · 2024-02-05T05:46:44Z

dsl/jpql/src/test/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/expression/CurrentTimeDslTest.kt

+
+class CurrentTimeDslTest : WithAssertions {
+    @Test
+    fun `currentDate() with a property`() {


테스트 함수 이름이 테스트 대상(currentTime)과 일치하지 않습니다. 알맞게 변경해주시면 좋을것같습니다.

waahhh · 2024-02-05T05:47:25Z

.../kotlin/com/linecorp/kotlinjdsl/render/jpql/serializer/impl/JpqlCurrentDateSerializerTest.kt

+    private val expression = currentDate()
+
+    @Test
+    fun handledType() { // when


코드 스타일을 맞추기위해 // when 주석의 줄내림이 필요해 보입니다.
JpqlCurrentTimeSerializerTest.kt 코드에도 동일한 부분이 있으니 같이 수정해주시면 좋을것같습니다.

waahhh · 2024-02-05T06:04:14Z

...main/kotlin/com/linecorp/kotlinjdsl/render/jpql/serializer/impl/JpqlCurrentDateSerializer.kt

+        val delegate = context.getValue(JpqlRenderSerializer)
+
+        writer.write("CURRENT_DATE")
+
+        writer.writeParentheses {
+            delegate.serialize(part, writer, context)
+        }


serialize에 part: JpqlCurrent.CurrentDate을 전달하면,
JpqlCurrent.CurrentDate 타입에 대해 serialize 재귀 호출이 일어나서 StackOverflowError가 발생합니다.

JpqlCurrentDateSerializer의 경우 괄호와 추가적인 serialize 호출이 필요가 없어보이므로
writer.write("CURRENT_DATE") 한줄만 있어도 충분해 보입니다.

JpqlCurrentTimeSerializer.kt도 마찬가지이므로 같이 수정해주시면 좋을것같습니다.

각 SerializerTest도 수정된 내용에 맞게 다시 봐주시면 좋을것같습니다!

waahhh · 2024-02-05T06:09:01Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

@@ -486,6 +488,26 @@ open class Jpql : JpqlDsl {
        return Expressions.times(this.toExpression(), value.toExpression())
    }

+    /**
+     * Creates an expression that represents  the current date.


represents와 the 사이에 불필요한 whitespace가 하나 더 포함 되어있습니다.

currentTime()의 주석도 둥일하니 같이 수정해주시면 좋을것같습니다.

waahhh · 2024-02-05T06:10:22Z

dsl/jpql/src/test/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/expression/CurrentDateDslTest.kt

+    @Test
+    fun `currentDate() with a expression`() {
+        // when
+        val expression = queryPart {
+            currentDate()
+        }.toExpression()
+
+        val actual: Expression<LocalDate> = expression // for type check
+
+        // then
+        val expected = Expressions.currentDate()
+
+        assertThat(actual).isEqualTo(expected)
+    }


위 테스트와 중복된 내용이므로 제거해도 될것같아요.

waahhh · 2024-02-05T06:16:05Z

dsl/jpql/src/test/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/expression/CurrentDateDslTest.kt

+
+class CurrentDateDslTest : WithAssertions {
+    @Test
+    fun `currentDate() with a property`() {


특정 property나 expression을 가지고 테스트하는 코드가 아니므로
테스트 함수 이름이 fun currentDate() 정도로 표현되도 좋을것같습니다.

CurrentTimeDslTest 쪽도 비슷하니 같이 수정해주시면 좋을것같습니다.

shouwn · 2024-02-06T01:08:18Z

...l/src/main/kotlin/com/linecorp/kotlinjdsl/querymodel/jpql/expression/impl/JpqlCurrentDate.kt

+import com.linecorp.kotlinjdsl.querymodel.jpql.expression.Expression
+import java.sql.Date
+
+object JpqlCurrentDate : Expression<Date>


Please add the Internal annotation because this class belongs to an implementation.

@Internal object JpqlCurrentDate : Expression<Date>

shouwn · 2024-02-06T01:10:37Z

dsl/jpql/src/test/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/expression/CurrentDateDslTest.kt

+
+class CurrentDateDslTest : WithAssertions {
+    @Test
+    fun `currentDate()`() {


If the name of the test exposes only the name of the method being tested, it is not parenthesized. Please modify it like this:

@Test fun currentDate() { // ...test codes }

shouwn · 2024-02-06T01:14:05Z

.../kotlin/com/linecorp/kotlinjdsl/render/jpql/serializer/impl/JpqlCurrentDateSerializerTest.kt

+    @MockK
+    private lateinit var serializer: JpqlRenderSerializer
+
+    private val expression = currentDate()


It would be nice if you could use Expressions.currentDate() instead of static import.

shouwn · 2024-02-06T01:16:03Z

And could you sign our Contributor License Agreement?

shouwn · 2024-02-06T02:28:57Z

Some of the formatting is off, but I'll fix that! Thanks for your help. Finally, I'd like to ask you to sign the Contributor License Agreement.

…/currentDate&time

meengi07 requested review from pickmoment, huisam, cj848 and shouwn as code owners February 2, 2024 17:17

meengi07 changed the base branch from main to develop February 2, 2024 17:17

meengi07 changed the title ~~Feat/current date&time~~ Support Datetime function CURRENT_DATE & CURRENT_TIME Feb 2, 2024

meengi07 added 2 commits February 5, 2024 09:37

feat: implement current date & time function

c1219b4

docs: add currentDate, currentTime function description

c963cc7

meengi07 force-pushed the feat/currentDate&time branch from f436846 to c963cc7 Compare February 5, 2024 00:40

shouwn reviewed Feb 5, 2024

View reviewed changes

waahhh reviewed Feb 5, 2024

View reviewed changes

fix: JpqlCurrent date,time separation and code clean up

8e2574d

shouwn reviewed Feb 6, 2024

View reviewed changes

fix: add @internal, refactor code

a237af3

shouwn approved these changes Feb 6, 2024

View reviewed changes

meengi07 and others added 3 commits February 6, 2024 13:42

fix: jpqlCurrent date,time separation and code clean up

4ad8cab

fix: add @internal, refactor code

491a1a1

Merge remote-tracking branch 'origin/feat/currentDate&time' into feat…

0555264

…/currentDate&time

shouwn merged commit 6f48b77 into line:develop Feb 6, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Datetime function CURRENT_DATE & CURRENT_TIME #624

Support Datetime function CURRENT_DATE & CURRENT_TIME #624

meengi07 commented Feb 2, 2024

CLAassistant commented Feb 2, 2024 •

edited

Loading

codecov-commenter commented Feb 5, 2024 •

edited

Loading

shouwn Feb 5, 2024

shouwn Feb 5, 2024

meengi07 Feb 5, 2024

waahhh Feb 5, 2024

waahhh Feb 5, 2024

waahhh Feb 5, 2024

waahhh Feb 5, 2024

waahhh Feb 5, 2024

waahhh Feb 5, 2024

shouwn Feb 6, 2024

shouwn Feb 6, 2024

shouwn Feb 6, 2024

shouwn commented Feb 6, 2024

shouwn commented Feb 6, 2024

Support Datetime function CURRENT_DATE & CURRENT_TIME #624

Support Datetime function CURRENT_DATE & CURRENT_TIME #624

Conversation

meengi07 commented Feb 2, 2024

Motivation

Modifications

Commit Convention Rule

Result

Closes

CLAassistant commented Feb 2, 2024 • edited Loading

codecov-commenter commented Feb 5, 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

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 commented Feb 6, 2024

shouwn commented Feb 6, 2024

CLAassistant commented Feb 2, 2024 •

edited

Loading

codecov-commenter commented Feb 5, 2024 •

edited

Loading