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

MongoTemplate cannot read immutable Objects that compute properties in constructor [DATAMONGO-2066] #2932

Closed
spring-projects-issues opened this issue Aug 21, 2018 · 5 comments
Assignees
Labels
in: mapping Mapping and conversion infrastructure status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug

Comments

@spring-projects-issues
Copy link

spring-projects-issues commented Aug 21, 2018

Konrad Lötzsch opened DATAMONGO-2066 and commented

This was introduced in Lovelace RC1. If i have an immutable entity that computes a property in a constructor, I get an exception in the mapping layer when reading the entity from the database:

Exception in thread "main" java.lang.UnsupportedOperationException: No accessor to set property private final java.lang.Integer com.example.TestEntity.propB!Exception in thread "main" java.lang.UnsupportedOperationException: No accessor to set property private final java.lang.Integer com.example.TestEntity.propB! at com.example.TestEntity_Accessor_aat7hi.setProperty(Unknown Source) at org.springframework.data.mapping.model.ConvertingPropertyAccessor.setProperty(ConvertingPropertyAccessor.java:60) at org.springframework.data.mongodb.core.convert.MappingMongoConverter.readProperties(MappingMongoConverter.java:354) at org.springframework.data.mongodb.core.convert.MappingMongoConverter.read(MappingMongoConverter.java:284) at org.springframework.data.mongodb.core.convert.MappingMongoConverter.read(MappingMongoConverter.java:245) at org.springframework.data.mongodb.core.convert.MappingMongoConverter.read(MappingMongoConverter.java:194) at org.springframework.data.mongodb.core.convert.MappingMongoConverter.read(MappingMongoConverter.java:190) at org.springframework.data.mongodb.core.convert.MappingMongoConverter.read(MappingMongoConverter.java:78) at org.springframework.data.mongodb.core.MongoTemplate$ReadDocumentCallback.doWith(MongoTemplate.java:2991) at org.springframework.data.mongodb.core.MongoTemplate.executeFindMultiInternal(MongoTemplate.java:2633) at org.springframework.data.mongodb.core.MongoTemplate.doFind(MongoTemplate.java:2364) at org.springframework.data.mongodb.core.MongoTemplate.doFind(MongoTemplate.java:2347) at org.springframework.data.mongodb.core.MongoTemplate.find(MongoTemplate.java:818) at org.springframework.data.mongodb.repository.support.SimpleMongoRepository.findAll(SimpleMongoRepository.java:360) at org.springframework.data.mongodb.repository.support.SimpleMongoRepository.findAll(SimpleMongoRepository.java:194) at org.springframework.data.mongodb.repository.support.SimpleMongoRepository.findAll(SimpleMongoRepository.java:51) 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:632) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185) at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.doInvoke(RepositoryFactorySupport.java:596) at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.lambda$invoke$3(RepositoryFactorySupport.java:583) at org.springframework.data.repository.core.support.RepositoryFactorySupport$QueryExecutorMethodInterceptor.invoke(RepositoryFactorySupport.java:583) 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.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:212) at com.sun.proxy.$Proxy54.findAll(Unknown Source) at com.example.TestApplication.main(TestApplication.java:19)

My Entity class looks like:

@Document 
public class TestEntity {

  private final @Id Integer id; 
  private final Integer propA; 
  private final Integer propB; 

  public TestEntity(Integer id, Integer propA) { 
      this.id = id; 
      this.propB = propA + 5; 
      this.propA = propA; 
   }
}

I made the code as simple as possible. In practice, the computed field is used to generate values from nested objects to be able to have simpler queries. In Lovelace M3 the code still runs. An complete example that illustrates the problem can be found here: https://github.com/kloetzsch/DATAMONGO-2066


Affects: 2.1 RC2 (Lovelace)

Reference URL: https://github.com/kloetzsch/DATAMONGO-2066

Issue Links:

  • DATACMNS-1374 Document requirements and supported modes for mutable and immutable types

Referenced from: commits e52b5d4, 7bbe1cd, a54ab6a

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 21, 2018

Oliver Drotbohm commented

I guess the problem is stemming from the question why you design your class that way. Here's the issue: you declare a writable property propB that will result in a field propB written to the MongoDB document. Now when that document gets read into your TestEntity again, what is supposed to happen with the value contained in the document?

Can I assume you'd expect the field propB just not to be set to the value in the document, i.e. the document value sliently being ignored? I can see this being a valid use case, I just wonder if that might lead to hard to debug problems as you read a document end up with attributes with a value different from the original document's field.

With Lovelace M3 you'd see this phenomenon if you write an entity of this code to the database, the change the code to e.g. … + 6. When you now read that document back into an object you would've gotten then computed value overridden by our previous mapping implementation overriding the value of the final field – which one could argue is wrong. The effect you see in RC1 is basically just making obvious that you have a sort of problematic situation here.

You could still work around this by adding a (private) constructor (annotated with @PersistenceConstructor) taking propB as additional parameter and ignoring it.

I guess we and should rightfully skip populating immutable properties, I am just concerned about that causing unexpected entity states

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 21, 2018

Konrad Lötzsch commented

Thanks for your reply. 

Your hint with the private constructor annotated with @PersistenceConstructor works for us pretty well.

I can see your concern and maybe everyone should get such 'warning' from the framework. We do actually extract ids from nested objects in the constructor and ensure their uniqueness in the collection with a unique index. We cannot set the unique index in the nested field, because it isn't unique there.

Can be closed my from perspective

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 21, 2018

Oliver Drotbohm commented

Glad you got it working. I'll take the idea to exclude immutable properties from the population to the team for further discussion. If at all, I'd much rather ship this with a GA release than "fixing" this in a subsequent service release. I am assuming that – as we were pretty lenient with such "pseudo immutable" types before – there a re quite a few such designs out there in the world. And if we move to proper handling of immutability, I'd much rather properly handle such cases than having users work around glitches like these.

Just to make sure I understood you right: assuming you'd change the the code to add a different number, would you expect to see the calculated number in the entity returned from the read or the number that's present in the document?

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 22, 2018

Konrad Lötzsch commented

In the property is state to illustrate the example, it is really hard to decide. In our case the saved documents are groups of lists (results from algorithms) of items. The calculated field is the ids of the items with a unique key to ensure, that every item can only be on one group/document. If anything would change on that field (for example we extract another id, type of ids are changed), we would most probably migrate the database first before we deploy the new code (which is easy for us, because our team has no external users, we can always shut down the app after work hours). In our case, we always expect the values from the db to be in the object after mapping. In fact, till yesterday we assumed, that spring data mongodb uses reflection to set the properties directly (unlike for example jackson, that uses setters). We learned just yesterday, that constructors are used (and I will read the docu to find out in which cases). So we assumed that the objects are exaclty like in the document and are not affected by the constructor but rather are a copy of the state after the construction.

We introduced immutability on the object to get more readable code in cases that results are added or removed from groups or complicated cases, than groups must be merged or splitted

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 22, 2018

Oliver Drotbohm commented

We've discussed this in the team this morning and we decided to not proceed with my suggested optimization. We much rather let the exception appear and the user react by changing their class design to accommodate the tightened rules. The reason for that is that by silently ignoring the offending property, we'd create instances of those types that – if written back – might cause invalid data being written unnoticed.

In the end we think it's a good thing that you have to design your classes in a way that they can be materialized from persistent state but at the same time expose a nice design to application code. I'll resolve this as "Works as designed" for now, keep my draft implementation around in a feature branch and will make sure we leave a word on this scenario in the changes to be made for DATACMNS-1374

@spring-projects-issues spring-projects-issues added type: bug A general bug status: declined A suggestion or change that we don't feel we should currently apply in: mapping Mapping and conversion infrastructure labels Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: mapping Mapping and conversion infrastructure status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants