mapstruct / mapstruct-spring-extensions

Helpful additions to MapStruct when using the Spring Framework.
Apache License 2.0
147 stars 32 forks source link

Provide a ConversionService bean if missing #87

Closed joosehav closed 1 year ago

joosehav commented 1 year ago

Problem: Setting up a valid Spring context is quite difficult in tests. Most often the ConversionService provided via WebMvcAutoConfiguration is being used. When it is not present, for example when testing service & persistence layer interaction, can get quite ugly to get all plumbing done. Something we've had to do:

@SpringBootTest({WebMvcAutoConfiguration.class, Test.Config.class})
class Test {

    @Configuration
    @ComponentScan(basePackages = {"com.example.mappers", "com.example.conversionserviceadapterpackage", "com.example.etc"})
    static class Config {
    }

Without this, instantiating Spring context fails in ConversionServiceAdapter due to missing ConversionService bean. Duplicating this to several test cases gets old quite quick. It is hard to make it portable/re-usable due to how different test context configuration options work: for example, setup with @DataJpaTest looks quite different.

Suggestion: If detecting that ConversionService bean is missing AND ConversionServiceAdapter base package gets component scanned, should provide a fallback bean with available Converters (at least the ones detected as MapStruct Mappers).

Can be opt-in via a property in SpringMapperConfig or a separate annotation to be placed on test classes.

OR Provide an example reference documentation on how to conveniently set up a valid Spring context for the ConversionServiceAdapter when Spring web/reactive-web is not present.

Chessray commented 1 year ago

We're aware of the difficulties in manually configuring the ConversionService. The examples give an idea of how to do this "by hand". I agree that some form of fallback autoconfiguration would be convenient, especially for testing purposes in Spring Boot projects. However, I can't really see anything beyond what you've already provided in your example.

I'm happy to take a look at what WebMvcAutoConfiguration does. Maybe we can replicate the ConversionService creation and addition of the Converters. But I suspect the user will always have to at least create the ApplicationContext themselves (via @ComponentScan and/or manual @Bean configuration).

joosehav commented 1 year ago

I played around with my tests to get a easily reusable context configuration for tests that need ConversionServiceAdapter, but do not have Spring Web autoconfigured. How about something like this:

Annotation to be placed on test classes:

@ComponentScan
@ImportAutoConfiguration(classes = MapStructAutoConfiguration.class)
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
public @interface MapStructTest {

    @AliasFor(annotation = ComponentScan.class, attribute = "basePackages")
    String[] basePackages();

}

AutoConfiguration:

@AutoConfiguration
public class MapStructAutoConfiguration {

    @Bean
    @ConditionalOnMissingBean(ConversionService.class)
    ConversionService fallbackConversionService(List<Converter<?,?>> converters) {
        var conversionService = new DefaultConversionService();
        converters.forEach(conversionService::addConverter);

        return conversionService;
    }
}

Then test can look like:

@DataJpaTest
//this also works: @SpringBootTest(classes = MyTestConfig.class)
@MapStructTest(basePackages = "com.example.mappers")
class Test {
    @Autowired
    private ConversionServiceAdapter adapter;
}

This disadvantage of this would be that one needs to provide packages(s) of Mappers and ConversionServiceAdapter to the annotation (in my example they reside in the same package). Any ideas on how to solve it? Could annotation processing help here by generating a class with the required @ComponentScan packages and including that to the configuration?

Didn't spend too much time on the example, possibly some rough edges/useless annotations somewhere, but at least I have now a setup that can be easily reused between tests.

EDIT: Could also be worthwhile to investigate the option of enabling this autoconfiguration without explicitly adding the annotation.

EDIT2: Can use regular @Import and @Configuration variants instead of @ImportAutoConfiguration & @AutoConfiguration

EDIT3: Come to think of it, having to define the packages for the annotation might not be such a bad thing. If we add @Inherited to @MapStructTest, users can opt to create their own meta-annotations that define the packages for their project. Also, instead of @MapStructTest, @AutoConfigureMapStruct could be more fitting to follow Spring conventions

Chessray commented 1 year ago

This looks nice. We can consider a separate module for testing purposes and provide this kind of thing. I'd be even more on board if we could indeed avoid using things like @AutoConfiguration which are specific to Spring Boot and thus even make this available to teams that do "only" Spring core projects.

joosehav commented 1 year ago

I think it can work with plain Spring. At least it didn't need the @AutoConfiguration. I can make a PR next week for a separate module. Or do you prefer to take over yourself?

Chessray commented 1 year ago

I've already started a branch. 😃 Could you do me a favor and play around with it in order to see whether this is what you're looking for? I tried doing it all with Spring core mechanisms for now which means e.g. there's no @ConditionalOnMissingBean either.

joosehav commented 1 year ago

There's a problem with this when @SpringMapperConfig#conversionServiceBeanName is used. I've had to use it since Spring Data REST and WebMvcAutoConfiguration both provide a ConversionService bean. With yours (and mine) solution, the ConversionService provided by the new configuration is not accepted by the ConversionServiceAdapter due to mismatch in qualifiers. Not sure how it can be solved. I had to re-use the same qualifier in the new Configuration class.

Possibly some changes needed in how ConversionServiceAdapter selects the ConversionService if multiple are present.

Perhaps instead of:

public ConversionServiceAdapter(
    @Qualifier("valueFromConversionServiceBeanName") @Lazy final ConversionService conversionService) {
  this.conversionService = conversionService;
}

You do:

public ConversionServiceAdapter(@Lazy Map<String, ConversionService> conversionServices) {
  this.conversionService = // select bean from Map
}

Where selection logic could be something like:

  1. If bean qualifier by conversionServiceBeanName is found, use it
  2. If any bean other than the basicConversionService is found, use it
  3. Use basicConversionService
Chessray commented 1 year ago

conversionServiceBeanName has been giving some headaches since the start. I'm curious whether we could also use this opportunity to rectify that. I'll play around with generating a version of the annotation based on the property. This could then even be used beyond testing.