Support for immutable collection types [DATACMNS-1381]

[spring-projects-issues]

John Butler opened DATACMNS-1381 and commented

Currently the MappingMongoConverter makes a static call to CollectionFactory.createCollection and CollectionFactory.createMap. And currently CollectionFactory does not support any of the Guava Immutable collection. Until Java comes up with a List interface that expresses that it is Immutable, I believe there is still good reason to declare class fields as ImmutableList instead of just List. For this reason it would be create to provide a mechanism to support these collections: ImmutableList, ImmutableSet, ImmutableMultiSet, ImmutableMap, ImmutableBiMap.

In order to do this the create method would need to take a set of values to populated in the created collection or a Builder interface would need to be returned.

Use Case: I have a domain object as below...

public class MyDomain{
  private List<String> listOfValues;
} 

I want the above to be thread-safe and so I design it to be immutable. When I create it I appropriate use ImmutableList. However, in the above there are two issues: 1) the getter returns List which gives the caller no indication that it is immutable 2) when marshalled via Spring Data it is created as a mutable List thereby breaking my thread-safety through immutability. Therefore the below would be better:

public class MyDomain{
 private ImmutableList<String> listOfValues;
} 

 

---

Issue Links:

• SPR-16797 Allow creation of immutable collections through CollectionFactory

• DATACMNS-1322 Add support for immutable objects in PersistentPropertyAccessor

1 votes, 4 watchers

[spring-projects-issues]

John Butler commented

My initial thought on this would be two-fold. First a "new" CollectionFactory interface would be created with the two methods: createCollection, createMap. Second, these methods would return builder-style interfaces that would have a finalize / build method to allow the collection to be locked once populated. Another option would be to provide the set of created objects to the factory on creation (so the collection would be created after content creation instead of before)

[spring-projects-issues]

Mark Paluch commented

Thanks for reporting this issue. I think there's an underlying assumption that we should discuss first. Code changes are merely a consequence. 

What you're heading for is immutability in some sense. A fully immutable object would declare all fields final allowing construction using factory methods or a constructor.
 

public class MyDomain{
 private final String id;
 private final List<String> listOfValues;
} 

Right now, we're able to construct immutable objects through constructor creation without regard to immutability of nested objects. List s, Map s and objects that originate from application code are not required to be immutable. To ensure immutability (also in the context of Kotlin and Java 9 immutable collections) we could inspect persistent entities whether they are immutable from a structural perspective (e.g. no setters at all or immutable property that has no setter) and make the assumption that the property, if it's a collection or map-type should be immutable as well

[spring-projects-issues]

John Butler commented

Here is the problem with List:

public class MyDomain{ 
  private final String id; 
  private final List<String> listOfValues;
  public List<String> getListOfValues()
  { 
   return listOfValues; 
  }
}

// some other code
 myDomain.getListOfValues().add("blah");

Therefore, if the getter returns a List the list could be modified. Yes you could make the case that the getter could make a defensive copy but I have answers to that: 1) I use Lombok a lot and Lombok @Getter doesn't do that 2) returning an ImmutableList is more performant than making copies 3) most importantly, if you were to return a List there is no indication in the signature that the object should be immutable. 

This is why in my immutable domain objects I prefer using ImmutableList to trying to work with List and protecting around it

[spring-projects-issues]

John Butler commented

Hey everyone, I would really like to try to submit / contribute a branch for this. If that would work I would like to get some thoughts on a design that would be consistent with the design you work towards.. I think there are a few options:

First, for creating the Immutable collection there are two options:

1. Create a new interface that allows for finalizing the collection
2. Instead of creating the collection first and adding values into it, collect the values first and then pass the values to the collection factory. This would be my preferred method as it does not require a new interface and the factory method signature is simple to understand.

With either of these solutions, the method signature in CollectionFactory would need to change so I don't think that updating org.springframework.core.CollectionFactory is viable. In order to make things flexible for the future I think that the new MongoDBCollectionFactory should be an instance in MappingMongoConverter to allow it to be overridden. Couple options remain:

A. Leave the MongoDBCollectionFactory simply forward to Spring core's CollectionFactory and allow users to add the guava functionality.

B. Implement the Guava immutables in the MongoDBCollectionFactory but by having it be an overrideable instance in MappingMongoConverter allow users to add any additional functionality. This seems like the simplest solution that meets the requirements.

C. Allow for the registration of modules (like Jackson's Guava module) to the MongoDBCollectionFactory

My suggestion would be 2 with B

[spring-projects-issues]

Mark Paluch commented

Thanks a lot for your active contribution. I've edited your description to replace "guys" with "everyone". While it may seem a small thing, some people feel excluded by "guys" and we don't want them to.

Besides that, we've decided to move the general issue towards Spring Framework with SPR-16797 as immutable collections are a broader topic. However, we would not like to only get an improvement for MongoDB but also for other store modules.

You might want to contribute in the scope of SPR-16797 and elaborate with the Spring Framework maintainers what they prefer. Typically, we pick up what Spring Framework provides to us and reuse existing functionality

[spring-projects-issues]

John Butler commented

Per the comment in SPR-16797, "Maybe Spring Data MongoDB can introduce Vavr and Guava support in a custom fashion for the time being, just delegating to the core CollectionFactory for regular collection types?"

Can we discuss a way forward on this? I would like to propose option B I mentioned above. It seems a simple solution that allows for expansion

[spring-projects-issues]

Oliver Drotbohm commented

Upon closer inspection I don't think it's enough to only tweak the collection creation as we also have to actually make sure these types are considered collections when all the mapping metadata is build. Guava is not actually creating an issue here as ImmutableCollection actually is a Java Collection. For Vavr's immutable collections however it's not as easy as they only implement Iterable. I.e. that newly to create CollectionFactory would also have to expose methods to augment our collection and map detection to those types.

Well keep this on the radar for Lovelace without a dedicated promise that we'd actually get to it

[spring-projects-issues]

John Butler commented

My thought was the the new interface would take the set of elements to include in the collection. Although ImmutableCollection is a Collection it throws an exception on add(). Therefore it was true of both (and all immutables) that the set of elements to wrap must be provided to the factory method. So it would require a slight refactor to create a temp collection to hold the elements as they are parsed. Then once all are parsed, pass the set to the factory method.

This with the slight refactor to use a factory object instead of a static call would allow for users to extend the functionality to create custom collection types

[spring-projects-issues]

Oliver Drotbohm commented

As indicated, there's a lot more to this than the actual immutable collection creation (for which we basically just need to wrap the Java collection we create into the immutable type using one of the latter's constructors or factory methods) which makes the issue harder to tackle than anticipated. Imagine the following domain object:

class Order {
  private ImmutableList<LineItem> lineItems;
}

Before we even get to the place where a MongoDB document is mapped onto that object we need to build up metadata about it. In this particular case, we have to find out that lineItems logically is a collection and we have to inspect the LineItem class as well. This decision is currently solely based on the assignability to Collection which – as indicated above – would work for Guava as all its collection types extend Collection. For other libraries we integrate with, and which would be subject for similar support, like Vavr, that is not the case and we'd have to detect that Seq etc. have to be treated like collections as well. Also, there is code that accesses those properties and checks for the collection nature of a property to – if that's the case – casts the property value to Collection which would break in the Vavr case as well.

So again, creating the instances eventually is the most straight forward part of this. Keeping track of all downstream aspects to make sure this stuff really works is the hard part here 🙃

[spring-projects-issues]

John Butler commented

I noticed that this got bumped. Would you please consider adding support for Eclipse Collections as well when this is worked? I personally love that Eclipse Collections has immutable interfaces that do not extends the Java mutable List (etc) interfaces

[spring-projects-issues]

Oliver Drotbohm commented

Of course. Rest assured that once we have the necessary infrastructure in place we need to reliably detect custom collections, it'll be easy to add new collection libraries. Eclipse Collections is pretty widely used so that we're probably going to add support for it out of the box

[donraab]

Hi @odrotbohm, this question was asked a couple years ago in StackOverflow.

https://stackoverflow.com/questions/61215497/integration-eclipse-collections-with-spring-data-jpa

I did some searching today and found this issue. I linked to the issue in an answer to the question.

Have there been any further updates on this issue? Thanks!