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

Provide a ConversionService bean if missing #87

Closed
joosehav opened this issue Aug 11, 2023 · 7 comments · Fixed by #91
Closed

Provide a ConversionService bean if missing #87

joosehav opened this issue Aug 11, 2023 · 7 comments · Fixed by #91

Comments

@joosehav
Copy link

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
Copy link
Collaborator

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
Copy link
Author

joosehav commented Aug 11, 2023

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

Chessray commented Aug 12, 2023

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
Copy link
Author

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
Copy link
Collaborator

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.

@Chessray Chessray linked a pull request Aug 28, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants