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.

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

docs: add guide to using value classes #707

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

erie0210
Copy link
Contributor

@erie0210 erie0210 commented May 16, 2024

Please let me know which file I should add it to and I will move it.
I'm trying to write a Korean document and translate it into English.

Motivation

  • Add guide to how to use value class.

Modifications

Commit Convention Rule

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

  • Please describe the result after this PR is merged.

Closes

@cj848
Copy link
Collaborator

cj848 commented May 16, 2024

You need to remove the .DS_Store files.

@erie0210
Copy link
Contributor Author

Sure, why not including '.DS_Store' for .gitinore? @cj848

@erie0210 erie0210 force-pushed the docs/add-value-class-guide branch from 3ce09ca to 49aaaae Compare May 17, 2024 00:43
@shouwn
Copy link
Member

shouwn commented May 20, 2024

Sure, why not including '.DS_Store' for .gitinore?

I guess I forgot to put it in.

@shouwn
Copy link
Member

shouwn commented May 20, 2024

Please move the file to the FAQ folder.

Comment on lines 74 to 78
val property = value::class.memberProperties.first()
val propertyValue = property.getter.call(value)

writer.writeParam(propertyValue)
return
Copy link
Member

Choose a reason for hiding this comment

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

value class가 java로 컴파일될 때 boxing과 unboxing 메소드가 생성되어서 이걸 호출해야 한다고 생각했는데, property를 통해서 접근하면 되는 거군요.

image

그런데 혹시 조회의 경우는 Spring에서 boxing 해서 반환해주나요?

select(
  path(User::id)
)
/// ...

위 Projection의 결과로 UseId를 얻을 수 있나 궁금해서요.

만약 된다면 Spring에서 구현한 코드를 통해서 가능한 것 같은데, 그렇다면 value class 지원은 Spring 기반에서만 동작하기 때문에 문서 최상단에 Spring을 사용하실 경우와 같이 한정자가 들어가야 할 것 같아서요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

조회 시에 boxing 해서 반환하네요. 다만 boxing 하는 주체가 Spring인지 Hibernate인지는 추가적으로 확인해보겠습니다!

Copy link
Contributor Author

@erie0210 erie0210 May 26, 2024

Choose a reason for hiding this comment

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

java로 트랜스파일 한 결과에서는 long으로 변환되기에 jdsl의 findAll 호출 결과에는 long으로 담기는걸 확인했습니다.

data class RecipeDto(
   val id: RecipeId // value class
) 
public final class RecipeDto {
    private final long id;
    
    public final long getId_RwJKGeI/* $FF was: getId-RwJKGeI*/() {
      return this.id;
    }
}  

boxing이 되는 위치는 코틀린 코드에서 해당 필드를 참조하는 곳이고, Java로 트랜스파일 시 boxing하는 코드를 코틀린이 넣어주는거 같아요.

data class RecipeDto(
   val id: RecipeId // value class
) {
    fun temp() {
        println(id)
    }
}
public final void temp() {
  RecipeId var1 = RecipeId.box-impl(this.id);
  System.out.println(var1);
}

만약 value class를 nullable로 선언하면 long 타입 대신 RecipeId가 그대로 유지되고,
이 경우 Hibernate에서 에러가 발생합니다.

data class RecipeDto(
   val id: RecipeId? // value class
) 

다만 value class가 nullable인 경우는 value class 를 사용하는 의미가 없어지는 것 같아 해당 케이스는 고려 대상이 아닌 것 같습니다.

Copy link
Contributor Author

@erie0210 erie0210 May 26, 2024

Choose a reason for hiding this comment

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

더불어 답변 주신 내용에서 spring 이라고 말씀해주신 이유가 어떤 내용인지도 조금 더 설명해주실 수 있을까요?

만약 된다면 Spring에서 구현한 코드를 통해서 가능한 것 같은데, 그렇다면 value class 지원은 Spring 기반에서만 동작하기 때문에

Copy link
Member

Choose a reason for hiding this comment

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

@erie0210 안녕하세요! 상세하게 확인해주셔서 감사합니다!

먼저 Spring을 언급한 이유는 Spring JPA에서 value class를 지원하게 되면서 Kotlin JDSL도 Spring과 같이 지원이 가능할지 고민하게 되었기 때문입니다.
image


다음으로 nullable한 value class도 충분히 사용하는 의미가 있다고 생각합니다. value class는 기본 자료형에 추가적인 validation이나 메소드들을 추가하기 위한 것으로 사용할 수 있기 때문입니다. BookPrice라는 value class가 있고 Book에 supplyPrice라는 필드가 있는데, 이 supplyPrice 필드가 nullable일 수 있다고 생각해요.


개인적으로는 nullable한 value class 또한 지원되어야 완벽한 지원이라 생각이 들기 떄문에, DTO Projection의 경우는 value class를 완벽하게 지원하지 않는 것으로 확인된 것 같습니다.

가이드 문서에 DTO Projection의 경우는 완벽한 지원이 안 되기 때문에 직접 value class를 사용하는 것보다, DTO Projection에서는 기본 자료형을 사용하고 조회 후에 map 등으로 변환을 추천한다는 내용을 추가해주실 수 있으신가요?


마지막으로 가이드 문서를 너무 잘 작성해주셔서 여러가지 부탁드리게 되네요. 도움을 주셔서 너무 감사합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

말씀해주신 내용 반영했습니다.

map으로 변환하는 경우 class를 두 개 선언하는 불편함이 있을 것 같아
DTO class에서 value class로 변환하도록 예시 코드를 작성해봤는데 어떨까요? @shouwn

Copy link
Member

Choose a reason for hiding this comment

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

@erie0210 오 좋다고 생각합니다!

Copy link
Contributor Author

@erie0210 erie0210 left a comment

Choose a reason for hiding this comment

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

추가적으로 확인해야하는 내용을 제외하고 리뷰 주신 내용을 반영했습니다.

Comment on lines 74 to 78
val property = value::class.memberProperties.first()
val propertyValue = property.getter.call(value)

writer.writeParam(propertyValue)
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

조회 시에 boxing 해서 반환하네요. 다만 boxing 하는 주체가 Spring인지 Hibernate인지는 추가적으로 확인해보겠습니다!

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.01%. Comparing base (95304dd) to head (0eb4a45).
Report is 24 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #707   +/-   ##
========================================
  Coverage    92.01%   92.01%           
========================================
  Files          335      335           
  Lines         3419     3419           
  Branches       203      203           
========================================
  Hits          3146     3146           
  Misses         213      213           
  Partials        60       60           

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

@erie0210 erie0210 force-pushed the docs/add-value-class-guide branch from 2805b2e to 512a600 Compare May 22, 2024 02:40
@erie0210 erie0210 requested a review from shouwn May 26, 2024 07:31
먼저 다음과 같은 커스텀 Seriailzer를 생성합니다.

```kotlin
class ValueClassAwareJpqlValueSerializer(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@shown How about adding this code to the actual jdsl rather than the documentation? It seems inevitable to use reflection. From the perspective of using value class, there seems to be enough merit.

Copy link
Member

Choose a reason for hiding this comment

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

The reason for not providing value classes as built-in in the Kotlin JDSL is not only because it adds points to use reflection. It's because we don't know how value classes will change in the future. Please see the following comment for more details.

#559 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems ambiguous. I think it would be a good idea to consider it again in the future. Consider reapplying when uncertainty is removed!

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if that's the case, wouldn't it be a good idea to also include the reasons you mentioned above in the document above? Since there are still some ambiguous aspects to support, why not just leave a guide as a document?

Copy link
Member

Choose a reason for hiding this comment

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

Once the Valhalla project in Java is finished and Kotlin supports multiple values in a value class instead of just one, I'll consider supporting it.

And I think it's better not to document this background for now, because I think it would be better to interview users and see what questions they have after reading this guide, and then write it up in a document.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All right. I respect your opinion.

@erie0210 erie0210 force-pushed the docs/add-value-class-guide branch from 28c47b5 to 0eb4a45 Compare May 31, 2024 09:56
@cj848 cj848 merged commit 3bdf3ba into line:develop Jun 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants