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

Add support for java getter #518

Merged
merged 8 commits into from
Nov 10, 2023

Conversation

jbl428
Copy link
Contributor

@jbl428 jbl428 commented Nov 8, 2023

Motivation

This pull request has not been completed yet.
It was created to receive a review to ensure that the work done so far has been implemented correctly.

Modifications

  • Change property type of JpqlEntityProperty and JpqlPathProperty.
  • Add introspector for KFunction1

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

  • User can use KFunction1 to get Path.

Closes

Comment on lines 19 to 20
private val classTableLookupCache: MutableMap<KClass<*>, JpqlEntityDescription> = ConcurrentHashMap()
private val propertyTableLookupCache: MutableMap<KCallable<*>, JpqlPropertyDescription> = ConcurrentHashMap()
Copy link
Member

Choose a reason for hiding this comment

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

The table prefix is a misnomer on my part - I named it table because I was planning to have native queries at that time, but now that it's JPQL-only, I think it's okay to name it entityLookupCache and propertyLookupCache.

Comment on lines 28 to 41
override fun introspect(property: KCallable<*>): JpqlPropertyDescription? {
return when (property) {
is KProperty1<*, *> -> JpqlProperty(property.name)
is KFunction1<*, *> -> JpqlProperty(resolvePropertyName(property))
else -> null
}
}

private fun resolvePropertyName(getter: KFunction1<*, *>): String =
if (getter.name.startsWith("is")) {
getter.name
} else {
getter.name.removePrefix("get").replaceFirstChar { it.lowercase() }
}
Copy link
Member

@shouwn shouwn Nov 9, 2023

Choose a reason for hiding this comment

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

I suppose the Property Introspector can be separated from Javax and Jakarta.

JpqlIntrospector is designed to delegate to another Intropsector if it cannot introspect itself. Therefore, I think we can reduce the duplication by creating a separate class that only introspects properties and adding it to the Context together.

class JavaxJpqlEntityIntrospector : JpqlIntrospector {
    override fun introspect(type: KClass<*>): JpqlEntityDescription? {
        // ...
    }

    override fun introspect(property: KCallable<*>): JpqlPropertyDescription? {
        return null
    }
}

class JakartaJpqlEntityIntrospector : JpqlIntrospector {
    override fun introspect(type: KClass<*>): JpqlEntityDescription? {
        // ...
    }

    override fun introspect(property: KCallable<*>): JpqlPropertyDescription? {
        return null
    }
}

class KotlinStyleJpqlPropertyIntrospector : JpqlIntrospector {
    override fun introspect(type: KClass<*>): JpqlEntityDescription? {
        return null
    }

    override fun introspect(property: KCallable<*>): JpqlPropertyDescription? {
        return when (property) {
            is KProperty1<*, *> -> JpqlProperty(property.name)
            is KFunction1<*, *> -> JpqlProperty(resolvePropertyName(property))
            else -> null
        }
    }
    
    // ...
}

private class DefaultModule : JpqlRenderModule {
    override fun setupModule(context: JpqlRenderModule.SetupContext) {
        // ...

        context.appendIntrospector(KotlinStyleJpqlPropertyIntrospector())

        // ...
    }
}

I think also you could create two abstract classes, JpqlEntityIntrospector and JpqlPropertyIntrospector, that implement JpqlIntropsector.

@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (d428e71) 92.14% compared to head (9de2e4a) 92.11%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #518      +/-   ##
===========================================
- Coverage    92.14%   92.11%   -0.04%     
===========================================
  Files          266      269       +3     
  Lines         2916     2942      +26     
  Branches       171      177       +6     
===========================================
+ Hits          2687     2710      +23     
- Misses         180      181       +1     
- Partials        49       51       +2     
Files Coverage Δ
...sl/querymodel/jpql/path/impl/JpqlEntityProperty.kt 100.00% <100.00%> (ø)
...jdsl/querymodel/jpql/path/impl/JpqlPathProperty.kt 100.00% <100.00%> (ø)
...necorp/kotlinjdsl/render/jpql/JpqlRenderContext.kt 89.92% <100.00%> (+0.79%) ⬆️
...nder/jpql/introspector/CombinedJpqlIntrospector.kt 100.00% <100.00%> (ø)
...render/jpql/introspector/JpqlEntityIntrospector.kt 100.00% <100.00%> (ø)
.../jpql/introspector/impl/JakartaJpqlIntrospector.kt 100.00% <100.00%> (ø)
...er/jpql/introspector/impl/JavaxJpqlIntrospector.kt 100.00% <100.00%> (ø)
...ql/serializer/impl/JpqlEntityPropertySerializer.kt 85.71% <100.00%> (+5.71%) ⬆️
...jpql/serializer/impl/JpqlPathPropertySerializer.kt 87.50% <100.00%> (+4.16%) ⬆️
...nder/jpql/introspector/JpqlPropertyIntrospector.kt 50.00% <50.00%> (ø)
... and 2 more

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

@shouwn
Copy link
Member

shouwn commented Nov 10, 2023

Thank you for your contributions! I will be releasing 3.1.0 on November the 24th, two weeks from now. Since it's a minor release, I'm hoping to include some of the string functions that I couldn't support, so I'll work on them and include them in the minor release.

@shouwn shouwn merged commit e15d1c7 into line:develop Nov 10, 2023
3 checks passed
@jbl428
Copy link
Contributor Author

jbl428 commented Nov 11, 2023

@shouwn

Ahh, this PR has already been merged. :)
I thought the path method that takes a Kfunction1 hasn't been registered in JpqlDsl yet, so it's not complete.

I'm wonder if the intention was to guide users to use the Paths factory class directly when trying to use KFunction.

@shouwn
Copy link
Member

shouwn commented Nov 11, 2023

@jbl428

Aha, I thought you wanted me to implement JpqlDsl, so I tried to implement it. I'd appreciate it if you implemented JpqlDsl as well.

If you make it a new PR, I'll review it. Have you thought about making a doc as well?

@jbl428
Copy link
Contributor Author

jbl428 commented Nov 11, 2023

I would like to make docs too :)

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.

3 participants