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

Calling PersistentPropertyAccessor.setProperty(…) on Kotlin data class objects with multiple copy methods can lead to ArrayIndexOutOfBoundsException #2324

Closed
tano opened this issue Mar 10, 2021 · 3 comments
Labels
in: kotlin Kotlin support type: bug A general bug

Comments

@tano
Copy link

tano commented Mar 10, 2021

There is flaky problem which leads to ArrayIndexOutOfBoundsException when dealing with Kotlin data classes with copy method having default values as entities in ClassGeneratingPropertyAccessorFactory.PropertyAccessorClassGenerator.generateCustomAccessorClass.

Please check this demo which reproduces the issue (just try to run ProblemDemoTests for several times and you'll receive ArrayIndexOutOfBoundsException): https://github.com/tano/aioob-demo/blob/master/src/test/kotlin/org/springframework/data/mapping/model/ProblemDemoTests.kt

Removing attempts: Int = this.attempts default parameter solves the issues.

Version details:
Java: openjdk version "11.0.9.1" 2020-11-04 LTS
Kotlin: 1.4.30
spring-data-commons: 2.4.5

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 10, 2021
@mp911de mp911de added in: kotlin Kotlin support type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 12, 2021
@mp911de
Copy link
Member

mp911de commented Mar 12, 2021

The issue here is that the generated Kotlin class has two synthetic and two public copy methods:

The decompiled bytecode shows the following signatures:

// #1
public final copy(java.lang.String arg0, java.time.LocalDateTime arg1, java.lang.String arg2, int arg3)

// synthetic variant of #1
public static synthetic copy$default(org.springframework.data.mapping.model.SampleDataClass arg0, java.lang.String arg1, java.time.LocalDateTime arg2, java.lang.String arg3, int arg4, int arg5, java.lang.Object arg6) 


// #2
public final copy(java.lang.String arg0, java.lang.String arg1, java.lang.String arg2, int arg3, java.time.LocalDateTime arg4, java.time.LocalDateTime arg5, java.lang.String arg6)

// synthetic variant of #2
public static synthetic copy$default(org.springframework.data.mapping.model.SampleDataClass arg0, java.lang.String arg1, java.lang.String arg2, java.lang.String arg3, int arg4, java.time.LocalDateTime arg5, java.time.LocalDateTime arg6, java.lang.String arg7, int arg8, java.lang.Object arg9) { 

Our detection assumed only a single copy method expecting that there is only one method taking all arguments. As it seems, we need to refine our assumptions because there are multiple copy methods and looking up a copy method solely by entity type isn't sufficient, we need to take into account which property should be copied.

@mp911de mp911de changed the title ArrayIndexOutOfBoundsException for Kotlin classes with copy methods in generateCustomAccessorClass Kotlin data classes with multiple copy methods can lead to ArrayIndexOutOfBoundsException Mar 12, 2021
@mp911de mp911de changed the title Kotlin data classes with multiple copy methods can lead to ArrayIndexOutOfBoundsException Calling PersistentPropertyAccessor.setProperty(…) on Kotlin data class objects with multiple copy methods can lead to ArrayIndexOutOfBoundsException Mar 12, 2021
@mp911de
Copy link
Member

mp911de commented Mar 12, 2021

Going forward, the question arises, which copy method we should use. The question can be partially answered by presence/absence of a particular property. But what if a property is present in both copy methods as in the sample below.
Consider a call to provide a new status value. It think it would make sense to stick with the primary copy method.

data class SampleDataClass(
	val id: String?,
	val userId: String,
	val status: String,
	val attempts: Int,
	val createdAt: LocalDateTime,
	val updatedAt: LocalDateTime,
	val sessionId: String?
) {


	fun copy(
		status: String,
		updatedAt: LocalDateTime,
		sessionId: String,
		attempts: Int = this.attempts
	) = SampleDataClass(
		this.id,
		this.userId,
		status,
		attempts,
		this.createdAt,
		updatedAt,
		sessionId
	)

}

@mp911de mp911de added type: bug A general bug and removed type: enhancement A general enhancement labels Mar 12, 2021
@mp911de mp911de added this to the 2.4.6 (2020.0.6) milestone Mar 12, 2021
@mp911de
Copy link
Member

mp911de commented Mar 12, 2021

This change bears a certain risk of breaking existing application code due to the refined copy method lookup. Therefore, we're backporting it to 2.4.6 and master for now.

mp911de added a commit that referenced this issue Mar 12, 2021
We now resolve the copy method for Kotlin data classes that match the primary constructor. Previously, copy method resolution could find a secondary copy method as we didn't check for the primary constructor structure (parameter names and types).

Closes #2324.
mp911de added a commit that referenced this issue Mar 12, 2021
Reduce test method visibility.

See #2324.
mp911de added a commit that referenced this issue Mar 12, 2021
Reduce test method visibility.

See #2324.
mp911de added a commit that referenced this issue Mar 12, 2021
Use ResolvableType for type assignability check when resolving Type from a KType.

See #2324.
mp911de added a commit that referenced this issue Mar 12, 2021
Use ResolvableType for type assignability check when resolving Type from a KType.

See #2324.
mp911de added a commit that referenced this issue Mar 22, 2021
We now resolve only the raw class when checking if a primary constructor argument is assignable to method parameters of the synthetic copy method.
Previously we used ResolvableType's assignability check which considered generic type arguments. As the type resolution between the KType and copy method type is not symmetric, the check only succeeded in cases where both types could be resolved to the same type/assignable type. Using projections or Any caused asymmetric resolution and therefor the assignability check returned non-assignable.

Closes #2324.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: kotlin Kotlin support type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants