spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
73.59k stars 40.31k forks source link

EntityManagerFactoryBuilder does not apply spring.jpa.hibernate customizations automatically #3654

Open eepstein opened 8 years ago

eepstein commented 8 years ago

When using a single data source and entity manager in a Spring Boot JPA project, the hibernate settings are picked up from the application properties (e.g., the application.yml file) and applied as expected.

In the case of multiple data source and entity managers, the naming_strategy setting is not applied and thus we get the default naming strategy. A minor bug with a likely quick fix I imagine.

I've put together a sample project showing the issue:

https://github.com/eepstein/multids-demo

(which was forked from https://github.com/gratiartis/multids-demo)

The relevant lines are in the FooConfig and BarConfig classes. For example, from com.sctrcd.multidsdemo.integration.config.foo.FooConfig:

                    // This is to work-around a bug in Spring boot, which is not setting the naming strategy when there
                    // are multiple data sources and entity managers.
            .properties(Collections.singletonMap("hibernate.ejb.naming_strategy",
                    jpaProperties.getHibernate().getNamingStrategy()))

If you comment out that line you'll see an assertion in the test will fail, even though the naming strategy is being correctly set in the application-test.yml file in the test resources folder.

snicoll commented 8 years ago

Thanks for the sample!

Well you are creating the EntityManager yourself so there is no magic. Spring Boot can't intercept that and apply the naming strategy. We have a related issue to create a sample using two database (#3456) so we could use your sample as a base to show you what needs to be done

(and maybe fix a bug or two along the way :smile: )

eepstein commented 8 years ago

Curiously all the other attributes are set automagically via the builders. This is true for both the DataSource and the EntityManagerFactory. Plus, the value in question is available via the autowired JpaProperties (which is automatically created by the spring-boot packages). So, I think this is a bug (or missing feature if you prefer).

There is a need to slap an @Primary on one of the DataSource instances or else spring complains. But otherwise, and except for the naming_strategy, everything gets wired up from the config/properties file with zero intervention. I'm of the opinion that naming_strategy should be included in that default auto-wiring.

snicoll commented 8 years ago

Compare our LocalContainerEntityManagerFactoryBean and yours.

I haven't looked deeper than that but it's fairly different IMO. It's not a terminology problem (I don't have a problem calling something a bug when there is one). I just think your configuration is different. Maybe we can also fix that via a template method. I'll have a look.

eepstein commented 8 years ago

Hi Stéphane,

So, this becomes a usage question on my end. How do I configure things so that the HibernateJpaAutoConfiguration is configured and used in the BarConfig class ? In particular, what's the recommended way to get or setup the EntityManagerFactory so that it is configured with all the vendor-specific settings (Hibernate in this case)?

(I see how to do so for the primary data source. What's unclear is how to leverage that code for a secondary or tertiary data source. In my particular case I go a semi-custom route, but then I'm not using JTA, not using WebSphere, and not using an alternate vendor/provider. In short, would be nice if eithe some of the protected methods were public, or if the configuration methods took a datasource so that the subsequent layers could be configured using all the logic embedded in the existing Spring code.)

Thank you.

snicoll commented 8 years ago

Hi @eepstein, no worries! I am not sure yet. I'll have a look tomorrow morning. Cheers!

eepstein commented 8 years ago

Perhaps XML config is the way to go in this case?

snicoll commented 8 years ago

What leads you to such conclusion? I haven't looked at it yet (last minute M3 stuff kept me busy)

snicoll commented 8 years ago

Okay so I have a good overview of the problem now.

I have created a sample that exposes several steps. You can browse the initial version that is similar to what you end up doing.

Like I said, you are creating the entity manager yourself so any customization that is done in the auto-configuration is lost (since you are overriding it). One of these customizations is to auto-detect the hbm2ddl.auto mode (i.e. enable create if the database is embedded). The other customization is to set the naming strategy to a sensible value if none was defined. If you look at the Hibernate customizations of JpaProperties there are two fields really and these are the ones that are customized.

I am not sure I understand why we don't apply these in the builder since we have everything that we need. I'll probably change that.

You can workaround the issue by specifying the following in your configuration

spring.jpa.properties.hibernate.ejb.naming_strategy=FooBar

Now, what I find a bit disturbing about this setup is that the JPA settings are shared between the two persistence units. What if you want different settings? To demonstrate this, I have completely disabled the auto-configuration and created a two sets of JpaProperties (each on their own namespace)

Finally, that project also showcases how you can generate the relevant meta-data for those new keys so that you get content assistance in the IDE. I hope that helps.

TL;DR the builder should apply the customization internally, I see no reason why it doesn't do it but keep in mind that since you are overriding things, it is normal that you lose some of the auto-magic features.

eepstein commented 8 years ago

Hi Stéphane,

So some of that lines up with my own observations and conclusions and some is either new or didn't line up with my own explorations and I'm not sure what I should have done instead. So, in no particular order:

Should I look for an enhanced builder or some template method in a subsequent release?

snicoll commented 8 years ago

I set the spring.jpa.properties.hibernate.ejb.naming_strategy and it had no effect unless applied manually to the builder.

Something should be wrong in your project then since I got it working. That demonstrates ddl-auto but it works for the naming strategy as well via spring.jpa.properties.hibernate.ejb.naming_strategy=YourNamingStrategy

Should I look for an enhanced builder or some template method in a subsequent release?

We're still discussing my PR at this point.

eepstein commented 8 years ago

I'd tried

spring.jpa.hibernate.naming_strategy

That's the one that works with a single DS/EM. Didn't try the one with .properties. in the path/name.

Sent from my handheld, please excuse typos.

On Aug 11, 2015, at 7:03 AM, Stéphane Nicoll notifications@github.com wrote:

I set the spring.jpa.properties.hibernate.ejb.naming_strategy and it had no effect unless applied manually to the builder.

Something should be wrong in your project then since I got it working. That demonstrates ddl-auto but it works for the naming strategy as well via spring.jpa.properties.hibernate.ejb.naming_strategy=YourNamingStrategy

Should I look for an enhanced builder or some template method in a subsequent release?

We're still discussing my PR at this point.

— Reply to this email directly or view it on GitHub.

snicoll commented 8 years ago

The key is spring.jpa.properties.hibernate.ejb.naming_strategy That key of yours does not map to anything, no surprise it didn't work :)

snicoll commented 8 years ago

I think you misunderstood my point. Yes, the standard property does not work (that's why this issue exists). I am proposing a workaround for the time being.

You can workaround the issue by specifying the following in your configuration

spring.jpa.properties.hibernate.ejb.naming_strategy=FooBar

snicoll commented 8 years ago

@wilkinsona if we want to refactor the JPA auto-configuration to be independent of Hibernate somehow, maybe we should take into account the Hibernate 4/5 support as certains keys need to be handled differently according to the hibernate version.

See https://github.com/snicoll/spring-boot/commit/161c629e9fbc22ef967412e618d4f443504ac300

From what I can see, JpaProperties#Hibernate may be enough (using some ClassUtils hackery).

ghostbuster91 commented 6 years ago

@snicoll Although it was indeed possible for me to configure custom naming strategy for hibernate following your examples I cannot figure out how to provide a custom naming strategy which needs to be created by spring (because of some dependencies). Could you point me somehow the right direction?

Before going with multi datasource it was as simple as defining a bean:

    @Bean
    fun customPhysicalNamingStrategy(@Value("\${TARGET_TABLE}") targetTable: String) = object : SpringPhysicalNamingStrategy() {
        override fun toPhysicalTableName(name: Identifier, context: JdbcEnvironment): Identifier {
            return if (name.text == SynchronizationInfo::class.java.simpleName) {
                Identifier.toIdentifier(targetTable)
            } else {
                super.toPhysicalTableName(name, context)
            }
        }
    }

But it no longer works. Specifying just a property for hibernate obviously fails with cannot find default constructor exception. I tried attaching it to sessionFactory by sessionFactoryBuilder.setPhysicalNamingStrategy(myCustomStrategy) but even when my code was invoked it doesn't seem to have a desired effect. Probably this new session factory must be somehow attached to the custom transactionManager but I have no idea how.

ghostbuster91 commented 6 years ago

If anybody ever run into similar problem, here is how I solved it:

    @Primary
    @Bean(name = ["synchronization.entityManagerFactory"])
    fun entityManagerFactory(
            builder: EntityManagerFactoryBuilder,
            @Qualifier("synchronization.dataSource") dataSource: DataSource,
            jpaProperties: JpaProperties,
            @Qualifier("synchronization.namingStrategy") namingStrategy: SpringPhysicalNamingStrategy
    ): LocalContainerEntityManagerFactoryBean {
        return builder
                .dataSource(dataSource)
                .packages(SynchronizationDbConfig::class.java.`package`.name)
                .persistenceUnit(SynchronizationDbConfig::class.java.simpleName)
                .properties(jpaProperties.getHibernateProperties(HibernateSettings().physicalNamingStrategy(namingStrategy)))
                .build()
    }
petenattress commented 5 years ago

Hi - I appreciate this is an old issue which is probably quite niche. However, I've recently encountered it and it was difficult to debug because I wasn't aware that Spring was applying this naming strategy for me, so the mapping errors I was getting were quite confusing. Lesson learned on that count.

For my use case, it would be ideal if the EntityManagerFactoryBuilder came with the magic properties already set (including ddl-auto being enabled for an embedded database), so I could create my entity managers as closely as possible to how Spring does by default (this was my assumption about the builder's behaviour from the documentation).

I understand why this may not be possible or desirable, but in any case may I ask that you consider updating the relevant section of the documentation to make it clear that the properties must be explicitly passed in to the builder? This would have saved me a fair amount of time! Thank you.

wilkinsona commented 5 years ago

@petenattress Could you please open a new issue to capture your suggestions? We can then use it to decide if we should update the docs or make a change to how EntityManagerFactoryBuilder currently works.