auto-configuration and config location conflict. about spring-boot-starter HOT 26 CLOSED

I prefer the repository package 😄
I think this solution will should provide since a next minor release (e.g. 1.1.0).

from spring-boot-starter.

Hi @jcbvm are you talking about this?

/cc @emacarron

from spring-boot-starter.

@eddumelendez I'm talking about the AutoConfiguredMapperScannerRegistrar class in the class MybatisAutoConfiguration. The method registerBeanDefinitions is automatically scanning the base package. Not sure if this is intended, it seems so, because the documentation of the MapperScannerRegistrarNotFoundConfiguration class says:

"If it is not used, however, then this will bring in a bean registrar and automatically register components based on the same component-scanning path as Spring Boot itself."

My question is why it is by default scanning the package if the mappers are hard coded in the config file.

from spring-boot-starter.

It is working as spring-boot itself, if you are working with jpa in spring-boot yo don't need to put @Repository in your repositories classes, spring-boot does for you. The same happen with mybatis-spring-boot. The difference is, this project provided support for xml and java config. And as you can see there are some combinations that doesn't work as expected. I do not know if we can cover each case but, at the beginning, we add xml support to help people in the transition.

We can solve this removing the xml support :)
I will take a look with some ideas in my mind but take into account that now xml and java config will be working together in the next version.

Thanks

from spring-boot-starter.

Thanks, for now I will move out any interface from the spring boot application package, so it will not get picked up as a mapper file.

from spring-boot-starter.

Hi,

@jcbvm If you add you mapper files using the mybatis-config.xml file, MyBatis will know about the mappers but spring don't. You need that your mappers are scanned so they can be injected afterwards. When using Spring we expect that mappers become spring beans and scanning is the heart of spring boot :)

I am not familiar with spring boot JPA. I will take a look to have a better opinion about this topic, but it indeed looks like a bug that all the interfaces in a project are registered as mappers.

You can bypass this by adding a @MapperScan annotation that lets you specify lots of filters.

But honestly I do not like to mix the usage of @MapperScan with spring boot. I would prefer to specify everyting in the application.properties. The AutoConfiguredMapperScannerRegistrar can take lots of parameters to make a finer search like for example the base package. This can be achieved easily by porting code from @MapperScan (MapperScannerRegistrar).

If you don't mind lets keep ths issue open because it is worth having a discussion about how this feature works.

from spring-boot-starter.

Hi again,

Does anybody know how can we read the MyBatisProperties bean in AutoConfiguredMapperScannerRegistrar???

from spring-boot-starter.

@emacarron I don't know ...
Can use the MapperScannerConfigurer instead of ?

from spring-boot-starter.

I don't think so. The registrar is the right bean to provide addional beans to a @configuration object. But looks like when it is executed the MyBatisProperties bean has not yet been created. I tried to get the object out of the beanFactory but it is not yet there :(

BTW I have just had a fast look at spring-boot-starter-data-jpa. Looks like by default it searches for classes that implements the interface Repository or are annotated with @RepositoryDefinition so seems that it won't get all the interfaces in the project but just the right ones.

See https://github.com/spring-projects/spring-data-commons/blob/master/src/main/java/org/springframework/data/repository/config/RepositoryComponentProvider.java#L71

super.addIncludeFilter(new InterfaceTypeFilter(Repository.class));
super.addIncludeFilter(new AnnotationTypeFilter(RepositoryDefinition.class, true, true));

Unfortunately MyBatis mappers are not marked (by design) so a good idea would be for example to search with a pattern like basepackage + "/**/mapper" so mappers are expected to be inside a package called whatever/mappers. The problem now is that this will break existing code. But I wount care too much about that given the project has just born.

from spring-boot-starter.

Have a look at this, what do you think?

emacarron@e4a2c4b

from spring-boot-starter.

Can get property value via Environment instead of type-safe properties using EnvironmentAware ?

For example:

public static class AutoConfiguredMapperScannerRegistrar implements BeanFactoryAware,
        ImportBeanDefinitionRegistrar, ResourceLoaderAware , EnvironmentAware {
    // ...
    private Environment environment;
    @Override
    public void setEnvironment(Environment environment) {
        this.environment = environment;
    }
    // ...
    @Override
    public void registerBeanDefinitions(AnnotationMetadata importingClassMetadata,
            BeanDefinitionRegistry registry) {

        ClassPathMapperScanner scanner = new ClassPathMapperScanner(registry);

        try {
            String[] mapperPackages = StringUtils.tokenizeToStringArray(environment.getProperty("mybatis.mapper-packages"),
                    ConfigurableApplicationContext.CONFIG_LOCATION_DELIMITERS);
            if(mapperPackages == null || mapperPackages.length == 0) {
                List<String> pkgs = AutoConfigurationPackages.get(this.beanFactory);
                for (String pkg : pkgs) {
                    log.debug("Found MyBatis auto-configuration package '" + pkg + "'");
                }
                if (this.resourceLoader != null) {
                    scanner.setResourceLoader(this.resourceLoader);
                }
                mapperPackages = pkgs.toArray(new String[pkgs.size()]);
            }
            scanner.registerFilters();
            scanner.doScan(mapperPackages);
        }
        catch (IllegalStateException ex) {
            log.debug("Could not determine auto-configuration "
                    + "package, automatic mapper scanning disabled.");
        }
    }
    // ...
}

What do you think ?

from spring-boot-starter.

Probably, above my idea is not good ... (sorry ...)

from spring-boot-starter.

Any idea is welcome @kazuki43zoo!! But I am afraid I already checked that and does not work.

Anyway, what about scanning only interfaces inside **/mapper packages. What do you think (@eddumelendez @jcbvm @kazuki43zoo) ?

from spring-boot-starter.

@emacarron , If possible i hope to support **/repository package. I use Mapper interface instead of Repository of DDD design(Spring's @Repository).
What do you think ?

from spring-boot-starter.

Sounds good. I will probably make sense to search in these package names by default:
**/mapper -> an opinionated mybatis tools should accept the "mybatis" main element
**/repository -> in case you prefer the spring jargon, though this may collide if you are also using JPA
**/persistence?? -> in case you copy & pasted from jpetstore-6? Should I better change the jpetstore-6?

Note that you yourself used the term "mapper" in the sample you make for gh-38:
https://github.com/kazuki43zoo/mybatis-spring-boot-gh-38

This is because the term mapper rocks!!! doesn't it?? ;)

Already added to the sample 0a2c27d

Note again that this will break existing code.

edit: I changed jpetstore-6 repo and the docs so the mappers package is now called "mapper"

from spring-boot-starter.

From my opinion, no package should be scanned by default. The base package should be defined in the config of spring boot. Which is in line with the legacy xml config property <mybatis:scan base-package="org.mybatis.spring.sample.mapper" />

from spring-boot-starter.

@jcbvm That makes sense when using raw spring but boot is supposed to use convention over configuration. In my opinion, an application should work by just adding a mapper and an autowired, with no further manual configuration.

Taking all interfaces as mappers will fulfill the objective but may cause too many side effects because you will have for sure interfaces that are not mappers.

OTOH I do agree in that we should be able to change the base package. Now that can only be done by adding a @MapperScan annotation to your SpringBoot main class. What is not so bad..

from spring-boot-starter.

@emacarron That's truth, the question is how many configuration is allowed for using convention over configuration :P.

But I agree that it should be able to overwrite the defaults or even disable the whole scanning.

from spring-boot-starter.

I would not expect that someone that uses spring boot would not want to use scanning, because that is in fact the main feature of boot! :)

Right now scanning can only be disabled by adding one dummy Mapper and a @MapperScan configured to find it. Otherwise, if no MapperFactoryBean is found in the context the auto-scan will happen.

from spring-boot-starter.

@emacarron Well for example if you have some complex queries and you want to build a Map with all kind of parameters for the query, you'll most likely want to create this Map in the Mapper implementation class in which you call the SqlSession yourself. In this case scanning is not necessary.

from spring-boot-starter.

Of course there are cases when you do not want your interface to be picked as a mapper. That is clear. But if you have a mapper (a mybatis interface with SQL annotations or an attached XML) then it should be always ok that it is autodiscovered.

The only situation I can imagine when auto-scanning does not work is when you have different datasources / sqlSessionFactories. That is a complex case. I would say it is good that simple cases work by default and complex cases require some deeper knowledge and finer tuning.

Anyway your comment reveals an interesting point. We should not scan in too many places so you can use other tecnologies without collision:

• If you want daos then place them in a "dao" or "persistence" package
• if you want to use JPA place objects in repository or repositories package.

So to scan into a repository package we should invert the filters that jpa is using. That will be more or less:

scanner.addExcludeFilter(new InterfaceTypeFilter(Repository.class));
scanner.addExcludeFilter(new AnnotationTypeFilter(RepositoryDefinition.class, true, true));

The problem is that Repository objects may not be in the classpath so we need to add some annoying code to check its existance and add the optional dependency to pom.xml.

from spring-boot-starter.

@kazuki43zoo see commit f361971 with the exclusions. Not sure if this is a good approach. I am thinking that maybe we should not scan into "repository" to avoid the colision.... but please let me know your thoughts.

from spring-boot-starter.

I am not agree hardcoding packages to exclude because I have seen projects with other conventions and maybe we are going to open the door to support to other package exclusions and manages this as a issues, which will be crazy.

By default, we can exclude .mappers package but can be replaced by a property in MybatisProperties to support the customization.

One extreme approach could be remove the mybatis.config but I have seen several people requesting to support this property in combination with the other ones.

from spring-boot-starter.

Hi @eddumelendez !

More than exclusions we should decide which packages to include. Or... what sounds much better: which conventions are we going to use, following a "convention over configuration" strategy.

What I expect from spring boot is to have an aplication up and running with zero boilerplate. For that to happen, we need conventions that can be overriden only for special cases.

Right now, the mybatis boot starter module takes all the interfaces it founds in a project and registers them as mappers. Ok, that is not good but it is not so bad. The point is that we should be able to identify if an interface is a mapper or not.

The problem is that mappers are not annotated (the @Mapper annotation does not exist). We did this because we would like to build applications with no mybatis imports at all. So they way you select your mappers when using classic configuraiton like MapperScannerConfigurer or @MapperScan is by:

• specifiying a base package
• specifiying a marker interface
• specifiying a marker annotation

It would be great to be able to set up this same configuraion in the boot's application.properties file but unfortunately I do no know how to do it!! (see former messages). And anyway we still have @MapperScan for that (that is what you used in the 1st versions before Josh Long's contribution)

So, my proposal is to use this convention: mappers are supposed to be interfaces held in a **/mapper package. For any other configuration, use the @MapperScan annotation that lets you configure everything.

What do you think?

from spring-boot-starter.

Given that the original request is to disable scanning when there is a configuration set in the application.properties I will close this issue and start a new one about the scanner.

The reply for @jcbvm is that using a config file registers beans inside MyBatis but are not known to Spring, so in order to be able to inject mappers you need the mappers to be scanned also. Config and scanning can be used together.

There can be scenarios when a user does not want scanning at all for any reason, but we should handle that as an exception and give a solution that may be awful given that this is an edge case.

Lets follow the autoscan topic in #46

from spring-boot-starter.

@jcbvm Jacob, can you please check issue #46 ?

from spring-boot-starter.