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

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

Open
eepstein opened this issue Aug 3, 2015 · 18 comments
Labels
type: enhancement A general enhancement

Comments

@eepstein
Copy link

eepstein commented Aug 3, 2015

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

snicoll commented Aug 4, 2015

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 😄 )

@eepstein
Copy link
Author

eepstein commented Aug 5, 2015

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

snicoll commented Aug 5, 2015

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.

@snicoll snicoll self-assigned this Aug 5, 2015
@eepstein
Copy link
Author

eepstein commented Aug 5, 2015

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

snicoll commented Aug 5, 2015

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

@eepstein
Copy link
Author

eepstein commented Aug 7, 2015

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

@snicoll
Copy link
Member

snicoll commented Aug 7, 2015

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

@snicoll
Copy link
Member

snicoll commented Aug 7, 2015

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.

@snicoll snicoll changed the title Spring Boot JPA with multiple entity managers ignores naming_strategy EntityManagerFactoryBuilder does not apply spring.jpa.hibernate customizations automatically Aug 7, 2015
@snicoll snicoll added the type: enhancement A general enhancement label Aug 7, 2015
snicoll added a commit to snicoll/spring-boot that referenced this issue Aug 7, 2015
While the EntityManagerFactoryBuilder holds the JpaProperties object, it
does not apply the Hibernate-specific customizations. Currently, the
auto-configuration pass an extra `properties` map that is created by
default based on these customizations.

Unfortunately, we cannot change that as the JTA auto-configuration is
relying on the content of the vendor properties.

To mitigate the problem, EntityManagerFactoryBuilder applies the
Hibernate customizations if no specific properties have been set.

Closes spring-projectsgh-3654
@eepstein
Copy link
Author

eepstein commented Aug 7, 2015

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:

  • I expected the Builder to have all the default settings baked in, and loaded from the properties. In short I expected it to be more like the DataSourceBuilder. It actually seems that the EntityManagerFactoryBuilder does have some things baked in and others not. This latter is what originally led me to open the issue.
  • I agree about the single JpaProperties. In my particular case it works as the values are shared, but in general something like what's in your sample is the way I'd choose to go.
  • I set the spring.jpa.properties.hibernate.ejb.naming_strategy and it had no effect unless applied manually to the builder. As mentioned this was the original reason for opening the issue. Also, although the property name on the hibernate side is hibernate.ejb.naming_strategy, the common way to set it via spring is just: spring.jpa.hibernate.naming_strategy (no "properties" and no "ejb" in the name). It (sort of) matters, since the common way to configure things when there is a single data source and single entity manager is with the shorter name ... and, again, would be nice if the same worked when there are multiple DSs & EMs
  • What I had before is working well now. In fact changed it to apply all the hibernate properties and changed also to use the regular Spring Boot classes for the primary data source and entity manager creation. Not ideal, but didn't feel like going the copy & paste route. May follow what you've done instead.

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

@snicoll
Copy link
Member

snicoll commented Aug 11, 2015

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

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 [email protected] 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
Copy link
Member

snicoll commented Aug 11, 2015

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 snicoll assigned wilkinsona and unassigned snicoll Aug 27, 2015
@snicoll
Copy link
Member

snicoll commented Aug 31, 2015

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

snicoll commented Sep 8, 2015

@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 snicoll@161c629

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

@ghostbuster91
Copy link

ghostbuster91 commented Jun 29, 2018

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

ghostbuster91 commented Jul 1, 2018

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()
    }

@snicoll snicoll removed their assignment Aug 20, 2018
@petenattress
Copy link

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

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants