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

Kotlin data class support for FlatFileItemReaderBuilder #4568

Open
0x1306e6d opened this issue Mar 27, 2024 · 2 comments
Open

Kotlin data class support for FlatFileItemReaderBuilder #4568

0x1306e6d opened this issue Mar 27, 2024 · 2 comments
Labels
status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter type: feature

Comments

@0x1306e6d
Copy link

Please do a quick search on Github issues first, the feature you are about to request might have already been requested.

Expected Behavior

Make FlatFileItemReaderBuilder detect whether the target type is Kotlin data class and sets proper FieldSetMapper which is not BeanWrapperFieldSetMapper.

RecordFieldSetMapper works with a Kotlin data class but we might be better to introduce a new dedicated FieldSetMapper.

Current Behavior

The FlatFileItemReaderBuilder only detects whether the target type is record or not. So it sets the BeanWrapperFieldSetMapper which instantiates the target type by the default constructor (no-args) and causes NotWritablePropertyException:

if (this.targetType != null && this.targetType.isRecord()) {
RecordFieldSetMapper<T> mapper = new RecordFieldSetMapper<>(this.targetType);
lineMapper.setFieldSetMapper(mapper);
}
else {

Context

As a workaround, we might make the data class fields nullable and mutable like:

data class Player(var lastName: String? = null)

but as you know, it doesn't leverage the Kotlin's features.

@fmbenhassine
Copy link
Contributor

Thank you for the suggestion.

Make FlatFileItemReaderBuilder detect whether the target type is Kotlin data class

Is there a reliable way to detect that? My concern with this request is that it would introduce code that is specific to Kotlin data classes (and possibly other JVM languages in the future if we get similar requests). As you mentioned, RecordFieldSetMapper works with a Kotlin data class so Kotlin users can just set that mapper or a custom one directly. Wdyt?

@fmbenhassine fmbenhassine added status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter and removed status: waiting-for-triage Issues that we did not analyse yet labels May 7, 2024
@0x1306e6d
Copy link
Author

0x1306e6d commented May 8, 2024

As you mentioned, RecordFieldSetMapper works with a Kotlin data class so Kotlin users can just set that mapper or a custom one directly.

You makes sense.

But when both targetType and fieldSetMapper are specified in the FlatFileItemReaderBuilder, the builder doesn't use the fieldSetMapper and do create a new one by the targetType. It might be better to warn when the fieldSetMapper is suppressed by the targetType.Supporting data class when the targetType is specified is just another improvement for Kotlin users.

Is there a reliable way to detect that? My concern with this request is that it would introduce code that is specific to Kotlin data classes (and possibly other JVM languages in the future if we get similar requests).

My first idea was that using KotlinDetector and JvmClassMappingKt as Spring Framework does:

https://github.com/spring-projects/spring-framework/blob/168276aaab14456cfcc1b0bd440a26e0682f8f90/spring-core/src/main/java/org/springframework/aot/hint/BindingReflectionHintsRegistrar.java#L120-L121

https://github.com/spring-projects/spring-framework/blob/168276aaab14456cfcc1b0bd440a26e0682f8f90/spring-core/src/main/java/org/springframework/aot/hint/BindingReflectionHintsRegistrar.java#L223-L224

Optional Kotlin dependency would not affect users who don't use Kotlin.

Or we'd be better to introduce a FieldSetMapperFactory that creates a new FieldSetMapper by the given targetType so that other JVM languages can easily integrate with. e.g.:

if (this.targetType != null || StringUtils.hasText(this.prototypeBeanName)) {
  // this.fieldSetMapperFactories contains BeanWrapperFieldSetMapper and RecordFieldSetMapper by default
  for (FieldSetMapperFactory factory : this.fieldSetMapperFactories) {
    if (factory.canMap(this.targetType) {
      lineMapper.setFieldSetMapper(factory.create(this.targetType);
      break;
    }
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter type: feature
Projects
None yet
Development

No branches or pull requests

2 participants