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

Handling collections and map types before running an element converter #3912

Open
wants to merge 5 commits into
base: 3.4.x
Choose a base branch
from

Conversation

nexx512
Copy link

@nexx512 nexx512 commented Dec 10, 2021

When the custom conversions are checked before collection and map converters, it is not possible to use custom converters for lists and maps.
If a custom converter for a list or a map is defined, it is executed here and not from within the collectionConverter where it should be executed.
So I think collections and maps should be handled beforehand.
Maybe lines 2064-2066 might not be needed ad all as this case is covered with line 2081.

When the custom conversions are checked before collection and map converters, it is not possible to use custom converters for lists and maps.
If a custom converter for a list or a map is defined, it is executed here and not from within the `collectionConverter` where it should be executed.
So I think collections and maps should be handled beforehand.
Maybe lines 2064-2066 might not be needed ad all as this case is covered with line 2081
@pivotal-cla
Copy link

@nexx512 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 10, 2021
@pivotal-cla
Copy link

@nexx512 Thank you for signing the Contributor License Agreement!

… the elements of a list or map have been converted to be able to use `Converter<List<?>, List<?>>` and `Converter<Map<?, ?>, Map<?, ?>>` Converters.
@nexx512
Copy link
Author

nexx512 commented Dec 17, 2021

I extendend this functionality to cover lists and maps of custom types, even allowing for collection converters whose target is not a collection. Being not so restrictive leaved more options to the user to implement custom converters.

It should now be possible to map vavr lists and maps with a simple converter like

@ReadingConverter
public ListReadingConverter implements Converter<List<ListType>, io.vavr.collection.List<ListType>> {
        @Override
        public io.vavr.collection.List<ListType> convert(List<ListType> source) {
            return io.vavr.collection.List.ofAll(source);
        }
}

However for maps to work correctly, the change from spring-projects/spring-data-commons#2517 is required.

I also removed some mapping tests that tested a mapping with a class extending java.util.Map. The extending class however didn't expose a map like interface. So i would say that this case should be covered by a separate class containing a map instead of extending one. This pretty much looked like a good example where aggregation should be preferred over inheritance.

Would be great if someone can have a look at it and discuss it if there are concerns.

@odrotbohm
Copy link
Member

Relates to spring-projects/spring-data-commons#2511.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants