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 KotlinAndJavaCompositeArbitraryIntrospector for supporting instantiating kotlin class with java reference Type #948

Merged
merged 18 commits into from
Mar 18, 2024

Conversation

jinia91
Copy link
Contributor

@jinia91 jinia91 commented Mar 14, 2024

This is a pr regarding the issue at
#946

I have implemented the KotlinAndJavaCompositeArbitraryIntrospector to facilitate the smooth creation of objects for Kotlin classes that have Java classes as properties.

The default Kotlin plugin uses this ArbitraryIntrospector but has been modified to accept other Introspector as a plugin argument.

additionaly added a corresponding test case.

I personally feel that modifying the behavior of the default kotlin plugin is a bit excessive, so I hope the maintainer will make an appropriate judgment and edit.🙏
=> only imple KotlinAndJavaCompositeArbitraryIntrospector

add KotlinAndJavaCompositeArbitraryIntrospector benchmark

How Has This Been Tested?

check through test case

Is the Document updated?

add KotlinAndJavaCompositeArbitraryIntrospector in introspectors-for-kotlin

@jinia91 jinia91 marked this pull request as ready for review March 14, 2024 03:10
private val javaIntrospector: ArbitraryIntrospector,
) : ArbitraryIntrospector {
override fun introspect(context: ArbitraryGeneratorContext): ArbitraryIntrospectorResult {
val type = Types.getActualType(context.resolvedType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the extension method .actualType() would be better.

@seongahjo
Copy link
Contributor

seongahjo commented Mar 15, 2024

@jinia91
Hello, thank you for your contributing.
It is a known issue so PrimaryConstructorArbitraryIntrospector would implements Matcher in 1.1.x so that it generates only a Kotlin type instance.

Your approach seems to be a good intermediate step in 1.0.x, but I am worried about whether it would cause any performance problems.

Can you add a new benchmark for KotlinAndJavaCompositeArbitraryIntrospector using jmh?

Comment on lines 49 to 59
class KotlinPlugin(
private val kotlinIntrospector: ArbitraryIntrospector = PrimaryConstructorArbitraryIntrospector.INSTANCE,
private val javaIntrospector: ArbitraryIntrospector = BeanArbitraryIntrospector.INSTANCE,
) : Plugin {
override fun accept(optionsBuilder: FixtureMonkeyOptionsBuilder) {
optionsBuilder.objectIntrospector { PrimaryConstructorArbitraryIntrospector.INSTANCE }
optionsBuilder.objectIntrospector {
KotlinAndJavaCompositeArbitraryIntrospector(
kotlinIntrospector = kotlinIntrospector,
javaIntrospector = javaIntrospector
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that javaIntrospector is not concern of KotlinPlugin.
It may cause a breaking change in the near future.

It is better to use objectInrospector option to apply new KotlinAndJavaCompositeArbitraryIntrospector with desired javaIntrospector

Copy link
Contributor Author

@jinia91 jinia91 Mar 16, 2024

Choose a reason for hiding this comment

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

I also feel that it's excessive for the KotlinPlugin to be setting up the JavaIntrospector. However, it seems good for objects to be smoothly created with the KotlinPlugin alone(dont use other api), regardless of whether Java reference types are used internally.

If a design is planned for a future version (1.1.x) that allows for such behavior,

then in the current PR, I plan to exclude changes to the KotlinPlugin and only keep the implementation of KotlinAndJavaCompositeArbitraryIntrospector.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jinia91

then in the current PR, I plan to exclude changes to the KotlinPlugin and only keep the implementation of KotlinAndJavaCompositeArbitraryIntrospector.

👍 It is good idea.

Comment on lines 29 to 30
private val kotlinIntrospector: ArbitraryIntrospector,
private val javaIntrospector: ArbitraryIntrospector,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about kotlinArbitraryIntrospector and javaArbitraryIntrospector?

@jinia91 jinia91 requested a review from seongahjo March 16, 2024 10:12
@jinia91 jinia91 changed the title Enhance Kotlin Plugin for supporting instantiating kotlin class with java reference Type Add KotlinAndJavaCompositeArbitraryIntrospector for supporting instantiating kotlin class with java reference Type Mar 16, 2024
Comment on lines 41 to 43
import java.lang.reflect.Modifier
import org.apiguardian.api.API
import org.apiguardian.api.API.Status.MAINTAINED
import java.lang.reflect.Modifier
Copy link
Contributor

Choose a reason for hiding this comment

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

import ordering is not valid

Comment on lines 105 to 113
val sut: FixtureMonkey = FixtureMonkey.builder()
.plugin(KotlinPlugin())
.objectIntrospector(
KotlinAndJavaCompositeArbitraryIntrospector(
kotlinArbitraryIntrospector = PrimaryConstructorArbitraryIntrospector.INSTANCE,
javaArbitraryIntrospector = ConstructorPropertiesArbitraryIntrospector.INSTANCE
)
)
.build()
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is not aligned with given.

@@ -180,7 +180,7 @@ fun sampleOrder() {
* @[sangy515](https://github.com/sangy515)
* @[yunseok-jeong0](https://github.com/yunseok-jeong0)
* @[wicksome](https://github.com/wicksome)
* @[Wonjin Choi](https://github.com/jinia91)
* @[jinia91](https://github.com/jinia91)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it okay to change my nickname? 🥲

@@ -48,4 +49,16 @@ open class KotlinObjectGenerationBenchMark {

private fun generateKotlinOrderSheet(fixtureMonkey: FixtureMonkey): List<KotlinOrderSheet> =
List(COUNT) { fixtureMonkey.giveMeOne(KotlinOrderSheet::class.java) }

@Benchmark
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add benchmark for KotlinAndJavaCompositeArbitraryIntrospector

@jinia91
Copy link
Contributor Author

jinia91 commented Mar 18, 2024

Benchmark Mode Threads Samples Score Score Error (99.9%) Unit
com.navercorp.fixturemonkey.ManipulationBenchmark.fixed avgt 1 10 586.739041 3.623991 ms/op
com.navercorp.fixturemonkey.ManipulationBenchmark.thenApply avgt 1 10 1341.850457 8.021999 ms/op
com.navercorp.fixturemonkey.ObjectGenerationBenchmark.beanGenerateOrderSheetWithFixtureMonkey avgt 1 10 596.574669 6.019172 ms/op
com.navercorp.fixturemonkey.ObjectGenerationBenchmark.builderGenerateOrderSheetWithFixtureMonkey avgt 1 10 505.341249 4.760138 ms/op
com.navercorp.fixturemonkey.ObjectGenerationBenchmark.fieldReflectionGenerateOrderSheetWithFixtureMonkey avgt 1 10 594.315525 10.230895 ms/op
com.navercorp.fixturemonkey.ObjectGenerationBenchmark.jacksonGenerateOrderSheetWithFixtureMonkey avgt 1 10 666.664955 3.442750 ms/op
Benchmark Mode Threads Samples Score Score Error (99.9%) Unit
com.navercorp.fixturemonkey.kotlin.KotlinObjectGenerationBenchMark.beanGenerateJavaOrderSheetWithFixtureMonkey avgt 1 10 1015.830312 11.054345 ms/op
com.navercorp.fixturemonkey.kotlin.KotlinObjectGenerationBenchMark.beanGenerateKotlinJavaCompositeOrderSheetWithFixtureMonkey avgt 1 10 1009.098525 12.771965 ms/op
com.navercorp.fixturemonkey.kotlin.KotlinObjectGenerationBenchMark.beanGenerateKotlinOrderSheetWithFixtureMonkey avgt 1 10 1040.497933 21.421765 ms/op

@jinia91 jinia91 requested a review from seongahjo March 18, 2024 06:25
Copy link
Contributor

@seongahjo seongahjo left a comment

Choose a reason for hiding this comment

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

👍

@seongahjo seongahjo merged commit 37b1990 into naver:main Mar 18, 2024
8 checks passed
@seongahjo seongahjo added this to the 1.0.15 milestone Mar 18, 2024
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.

2 participants