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

Consider vavr collections to be collection like #2511

Closed
wants to merge 1 commit into from

Conversation

nexx512
Copy link

@nexx512 nexx512 commented Dec 11, 2021

Apparently vavr collections are currently not considered collection like.
To treat them as collection correclty for example in MappingMongoConverter from spring-data-mongodb they should also be considered collection like.

@odrotbohm
Copy link
Member

Assuming we accepted the contribution, TypeDiscoverer would consider Vavr collection types collections now. That'd cause downstream domain object converter implementations like MappingMongoConverter to run into the code path trying to assemble a collection of values read from the database for those properties now considered Collection. This however wouldn't work without actually plugging a conversion from a Java-native collection to a Vavr one. Is that something you currently do through further customizations right now?

@odrotbohm odrotbohm added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 13, 2021
@odrotbohm odrotbohm self-assigned this Dec 13, 2021
@nexx512
Copy link
Author

nexx512 commented Dec 13, 2021

Since MappingMongoConverter doesn't support vavr objects natively right now I had to write a custom converter implementing the Converter<java.util.list<?>,io.vavr.collection.List<?>> interface.

But then I ran into the problem that my converter is called with a java util List with unconverted Bson elements in the List. So I needed a pretty dirty hack to make it work. After inspecting the MappingMongoConverter code I found that it should work if

  1. MappingMongoConverter recognizes the vavr list as collectionLike to actually execute the routine to map all elements and pass the mapped elements to the custom converter.
  2. MappingMongoConverter doesn't execute the custom converter before mapping the elements, which it does right now. I also issued a Pull request in the spring-data-mongo Repo.

Since you now need a custom converter anyways for mapping vavr lists and the custom converter is executed before anything else, nothing would change for MappingMongoConverter. But once the PR for MappingMongoConverter is accepted (hopefully) then I can use a custom converter without dirty hack to convert from java lists to vavr lists.

However, I don't know, how this change would affect other libraries. But I would guess that since "coellctionLike" is not the same as "Collection" one can make no assumption of the underlying type when asking for isCollectionLike. So code that relies on isCollectionLike would need a fallback if it is not a Collection or extending Collection.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 13, 2021
@odrotbohm
Copy link
Member

That's very useful feedback, thanks! We were thinking in a similar direction. There already is a converter able to map between the native Java types and Vavr collections, but it's currently only used to adapt results returned from the internal execution of query methods that declare Vavr types. I.e. for those, the actual execution produces e.g. a Java List but is then eventually mapped onto a Vavr io.vavr.collection.List to satisfy a declared Seq.

I guess I'll see if registering that as converter in CustomConversions already does the trick for the object mapping so that we might be able to get this working without having to tweak the store modules individually.

odrotbohm added a commit that referenced this pull request Feb 14, 2022
…rsions.

Moved Vavr collection converters into a type in the utility package. Register the converters via CustomConversions.registerConvertersIn(…) to make sure that all the Spring Data object mapping converters automatically benefit from a ConversionService that is capable of translating between Java-native collections and Vavr ones.

Issue #2511.
odrotbohm added a commit that referenced this pull request Feb 14, 2022
…rsions.

Moved Vavr collection converters into a type in the utility package. Register the converters via CustomConversions.registerConvertersIn(…) to make sure that all the Spring Data object mapping converters automatically benefit from a ConversionService that is capable of translating between Java-native collections and Vavr ones.

Issue #2511.
@odrotbohm
Copy link
Member

The suggested changes have been polished and merged, just as the ones in #2517. That said, we still haven't made up our mind on how to adapt the actual object mapping converters in the store implementations. I.e. domain objects will currently not map properly as the code that sees typeInformation.isCollectionLike() will currently assume they see a Java-native array or collection currently. There are a couple of different approaches we could take here, and given the imminent next releases coming up on Friday we decided not rush things here. However, it's likely that we're gonna get there for the next one. Thanks for your patience.

@nexx512
Copy link
Author

nexx512 commented Mar 7, 2022

I got the chance to test it today with 2.7.0-M3 and it is working now. I can even remove some of my custom reading converters. Thank you for your effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants