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

Generated Kotlin PropertyAccessor uses wrong copy method [DATACMNS-1391] #1827

Closed
spring-projects-issues opened this issue Sep 12, 2018 · 13 comments
Assignees
Labels
in: mapping type: bug

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Marian Jureczko opened DATACMNS-1391 and commented

Given (in kotlin):

@Document(collection = "catalogue.CatalogueTags")
data class CatalogueTags internal constructor(
        @Id val id: ObjectId = ObjectId.get(),
        @Version val version: Int? = null,
        val shop: Shop,
...
       val activeInCatalogue: DateTimeRange = DateTimeRange()
) : CatalogueData<CatalogueTags>() {
    override fun copy(newActiveInCatalogue: DateTimeRange) = copy(activeInCatalogue = newActiveInCatalogue)
}

@Repository
interface CatalogueTagsRepository : RxJava2CrudRepository<CatalogueTags, ObjectId>  

When:

class MyTest : CatalogueBaseComponentTest() {

    @Autowired
    lateinit var repo: CatalogueTagsRepository

    @Test
    fun save() {
        val some = some<CatalogueTags>()
        val saved = repo.save(some).blockingGet()
    }
} 

Then:

java.lang.ClassCastException: java.lang.Integer cannot be cast to com.ocado.gembus.hub.commons.vos.DateTimeRange

at com.ocado.gembus.hub.catalogue.test.CatalogueTags_Accessor_jcjqjz.setProperty(Unknown Source)
at org.springframework.data.mapping.model.ConvertingPropertyAccessor.setProperty(ConvertingPropertyAccessor.java:61)
at org.springframework.data.mongodb.core.EntityOperations$AdaptibleMappedEntity.incrementVersion(EntityOperations.java:650)
at org.springframework.data.mongodb.core.ReactiveMongoTemplate.lambda$doSaveVersioned$33(ReactiveMongoTemplate.java:1407)
at org.springframework.data.mongodb.core.ReactiveMongoTemplate.lambda$createMono$8(ReactiveMongoTemplate.java:599)
at reactor.core.publisher.FluxFlatMap.trySubscribeScalarMap(FluxFlatMap.java:141)
at reactor.core.publisher.MonoFlatMap.subscribe(MonoFlatMap.java:53)
at reactor.core.publisher.MonoOnErrorResume.subscribe(MonoOnErrorResume.java:44)
at reactor.core.publisher.Mono.subscribe(Mono.java:3080)
at io.reactivex.internal.operators.flowable.FlowableFromPublisher.subscribeActual(FlowableFromPublisher.java:29)
at io.reactivex.Flowable.subscribe(Flowable.java:12995)
at io.reactivex.Flowable.subscribe(Flowable.java:12941)
at io.reactivex.internal.operators.observable.ObservableFromPublisher.subscribeActual(ObservableFromPublisher.java:31)
at io.reactivex.Observable.subscribe(Observable.java:10903)
at io.reactivex.internal.operators.observable.ObservableSingleMaybe.subscribeActual(ObservableSingleMaybe.java:30)
at io.reactivex.Maybe.subscribe(Maybe.java:3730)
at io.reactivex.internal.operators.maybe.MaybeToSingle.subscribeActual(MaybeToSingle.java:46)
at io.reactivex.Single.subscribe(Single.java:2700)
at io.reactivex.Single.blockingGet(Single.java:2153)
at com.ocado.gembus.hub.catalogue.test.MyTest.save(CatalogueTags.kt:54)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)

Affects: 2.1 RC2 (Lovelace)

Attachments:

Referenced from: pull request #312

1 votes, 4 watchers

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Oliver Drotbohm commented

Looks like we need to be more defensive about the copy(…) method we select and make sure it actually takes all parameters we need to hand it over, right?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Marian Jureczko commented

I am not sure what exactly was going on there. When decompiling CatalogueTags_Accessor_jcjqjz.setProperty(), there was nothing wrong, i.e. the correct copy method was called. But, with a black box approach, when there is an additional copy method with different arguments (DateTimeRange in my case), the additional one is called in place of the data class default one (accepting all class fields) when trying to set version of a new entity to 0. At least that is the only explanation I could come up with to justify the unsuccessful Int to DateTimeRange casting

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Mark Paluch commented

The culprit here is an optimization that we made for single-property classes (value types). We attempt to find a public copy method that takes a single argument and returns the entity type so it does not require setup of defaulting masks.

We should guard the optimization with a check on the parameter type and make sure the entity consists of a single property to use the public copy optimization

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Mark Paluch commented

I moved this ticket to Spring Data Commons as the fix will happen here

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Mark Paluch commented

I pushed a change to a bugfix branch and we have a snapshot build available

It would be cool if you could give the snapshots a try (use version 2.1.0.DATACMNS-1391-SNAPSHOT) and see if this behaves as expected. 

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 13, 2018

Marcin Sokołowski commented

Another problem occurred, I've prepared some minimal working example here: https://github.com/sokolek/DATACMNS-1391

Method

org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory#findPublicCopyMethod:1466

sometimes picks copy method from the data class , and sometimes overloaded copy method, because type.getDeclaredMethods()) returns them in random order. Result of findPublicCopyMethod method is then indirectly assigned in ClassGeneratingPropertyAccessorFactory.visitKotlinCopy:1102 to KotlinDefaultCopyMethod defaultMethod, but unfortunately this could be overloaded version of copy, which is not copy$default.
!screenshot-dataclass.png|width=1525,height=1017!

In result following Exception is thrown:

 
java.lang.RuntimeException: java.lang.IllegalArgumentException: Cannot resolve parameter name id to a index in method copy!

	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory.createAccessorClass(ClassGeneratingPropertyAccessorFactory.java:206)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory.potentiallyCreateAndRegisterPersistentPropertyAccessorClass(ClassGeneratingPropertyAccessorFactory.java:190)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory.getPropertyAccessor(ClassGeneratingPropertyAccessorFactory.java:100)
	at org.springframework.data.mapping.model.BasicPersistentEntity.getPropertyAccessor(BasicPersistentEntity.java:455)
	at org.springframework.data.mapping.model.PersistentEntityIsNewStrategy.lambda$new$0(PersistentEntityIsNewStrategy.java:48)
	at org.springframework.data.mapping.model.PersistentEntityIsNewStrategy.isNew(PersistentEntityIsNewStrategy.java:95)
	at org.springframework.data.mapping.model.BasicPersistentEntity.isNew(BasicPersistentEntity.java:483)
	at org.springframework.data.repository.core.support.PersistentEntityInformation.isNew(PersistentEntityInformation.java:44)
	at org.springframework.data.mongodb.repository.support.SimpleMongoRepository.save(SimpleMongoRepository.java:80)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.springframework.data.repository.core.support.RepositoryComposition$RepositoryFragments.invoke(RepositoryComposition.java:359)
	at org.springframework.data.repository.core.support.RepositoryComposition.invoke(RepositoryComposition.java:200)
	at org.springframework.data.repository.core.support.RepositoryFactorySupport$ImplementationMethodExecutionInterceptor.invoke(RepositoryFactorySupport.java:644)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185)
	at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.doInvoke(RepositoryFactorySupport.java:608)
	at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.lambda$invoke$3(RepositoryFactorySupport.java:595)
	at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.invoke(RepositoryFactorySupport.java:595)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185)
	at org.springframework.data.projection.DefaultMethodInvokingMethodInterceptor.invoke(DefaultMethodInvokingMethodInterceptor.java:59)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185)
	at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:92)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185)
	at org.springframework.data.repository.core.support.SurroundingTransactionDetectorMethodInterceptor.invoke(SurroundingTransactionDetectorMethodInterceptor.java:61)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185)
	at org.springframework.data.repository.core.support.MethodInvocationValidator.invoke(MethodInvocationValidator.java:99)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185)
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:212)
	at com.sun.proxy.$Proxy94.save(Unknown Source)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:338)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:197)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163)
	at org.springframework.dao.support.PersistenceExceptionTranslationInterceptor.invoke(PersistenceExceptionTranslationInterceptor.java:139)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185)
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:212)
	at com.sun.proxy.$Proxy94.save(Unknown Source)
	at com.mytest.MyTest.send new dtc product(MyTest.kt:39)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.springframework.test.context.junit4.statements.RunBeforeTestExecutionCallbacks.evaluate(RunBeforeTestExecutionCallbacks.java:73)
	at org.springframework.test.context.junit4.statements.RunAfterTestExecutionCallbacks.evaluate(RunAfterTestExecutionCallbacks.java:83)
	at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:75)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:86)
	at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:84)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:251)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:97)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61)
	at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:190)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Caused by: java.lang.IllegalArgumentException: Cannot resolve parameter name id to a index in method copy!
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$KotlinCopyByProperty.findIndex(ClassGeneratingPropertyAccessorFactory.java:1562)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$KotlinCopyByProperty.<init>(ClassGeneratingPropertyAccessorFactory.java:1549)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$KotlinDefaultCopyMethod.forProperty(ClassGeneratingPropertyAccessorFactory.java:1531)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$PropertyAccessorClassGenerator.visitKotlinCopy(ClassGeneratingPropertyAccessorFactory.java:1103)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$PropertyAccessorClassGenerator.visitSetProperty0(ClassGeneratingPropertyAccessorFactory.java:1014)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$PropertyAccessorClassGenerator.visitSetPropertySwitch(ClassGeneratingPropertyAccessorFactory.java:986)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$PropertyAccessorClassGenerator.visitSetProperty(ClassGeneratingPropertyAccessorFactory.java:936)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$PropertyAccessorClassGenerator.generateBytecode(ClassGeneratingPropertyAccessorFactory.java:350)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory$PropertyAccessorClassGenerator.generateCustomAccessorClass(ClassGeneratingPropertyAccessorFactory.java:326)
	at org.springframework.data.mapping.model.ClassGeneratingPropertyAccessorFactory.createAccessorClass(ClassGeneratingPropertyAccessorFactory.java:204)
	... 73 more

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 13, 2018

Mark Paluch commented

Thanks a lot. I'll have a look

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 14, 2018

Mark Paluch commented

I pushed another change to fix the issue. Care to give it a spin? (2.1.0.DATACMNS-1391-SNAPSHOT resolving to 2.1.0.DATACMNS-1391-20180914.084050-5)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 14, 2018

Marcin Sokołowski commented

Thanks for quick response. It works for the current setup, but it won't if the name of the arguments in overloaded copy method will be the same as the actual property names. See current master on https://github.com/sokolek/DATACMNS-1391/

The problem is caused by the logic in filtering at org.springframework.data.mapping.model.KotlinCopyMethod#findPublicCopyMethod:149-176. I think that the method should not rely how the parameters are named, but rather on their order, types and the total number of parameters. I guess that the intention there is just to find copy method generated by Kotlin (not the static copy$default one, and not the any overloaded copy from upper class), is my understanding correct?

Let me ask just one more question, why not just always use static copy$default method? I think you don't want to rely on the implementation details of copy methods by kotlin (as fas as I know, copy$default isn't documented anywhere), is that true?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 14, 2018

Mark Paluch commented

We have different approaches and cases where we need to distinguish between the public and the synthetic methods. Using reflection, we're using Kotlin's reflection API to invoke methods and to let Kotlin reflection apply defaulting. To do so, we need to lookup a KCallable. This is only possible by using the public generated copy method. Invoking ReflectJvmMapping.getKotlinFunction(…) with the copy$default method isn't able to resolve to a KCallable but returns null. Kotlin reflection is only able to resolve regular methods to KCallable and cannot use the synthetic ones.

So we need to find the generated copy method. There's next to no documentation on how to achieve that so we're still investigating. It looks as if the copy method has the same signature as the primary constructor and we should add another validation level.

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 17, 2018

Mark Paluch commented

We aligned the copy method discovery to use the primary constructor signature. A new build is available

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Marcin Sokołowski commented

Now it works like a charm

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Sep 18, 2018

Mark Paluch commented

Thanks a lot for verifying the patch. Creating a pull request so we can include the change in our upcoming release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: mapping type: bug
Projects
None yet
Development

No branches or pull requests

2 participants