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

Remove explicit Hibernate subclass literal from JpaProperties #1327

Closed
philwebb opened this issue Aug 1, 2014 · 9 comments

Comments

Projects
None yet
4 participants
@philwebb
Copy link
Member

commented Aug 1, 2014

It's not very nice to have JpaProperties know about Hibernate.

@philwebb philwebb added this to the 1.2.0 milestone Aug 1, 2014

@snicoll

This comment has been minimized.

Copy link
Member

commented Aug 1, 2014

👍

@dsyer

This comment has been minimized.

Copy link
Member

commented Aug 2, 2014

It's trivial actually, since the only dependency on Hibernate is through the namingStrategy whose value is a Class but it is only ever used as a String.

@dsyer dsyer changed the title Try to extract Hibernate subclass from JpaProperties Remove ecplicitt Hibernate subclass literal from JpaProperties Aug 15, 2014

@dsyer dsyer changed the title Remove ecplicitt Hibernate subclass literal from JpaProperties Remove explicit Hibernate subclass literal from JpaProperties Aug 15, 2014

@dsyer

This comment has been minimized.

Copy link
Member

commented Aug 15, 2014

It looks like this was already fixed in commit ac2ab39. Maybe we just need to remove the deprecated method at some point?

@dsyer dsyer closed this Aug 15, 2014

@dsyer dsyer modified the milestones: 1.2.0, 1.1.5 Aug 15, 2014

@philwebb

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2014

That was for #1268, this one is about extracting the Hibernate subclass.

@philwebb philwebb reopened this Aug 15, 2014

@dsyer

This comment has been minimized.

Copy link
Member

commented Aug 15, 2014

Why? It doesn't break anything.

@philwebb

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2014

It seems like a smell to have Hibernate specifics in a general JPA class. If we ever support EclipseLink that class is going to get even more messy.

@dsyer

This comment has been minimized.

Copy link
Member

commented Aug 15, 2014

I guess. But the same argument applies to Tomcat and Jetty and that doesn't stop us having Tomcat specific config in ServerProperties. The issue actually comes down to the fact that you can use @ConfigurationProperties to bind "spring.jpa.hibernate.*" unless the hibernate properties live in JpaProperties. If you can think of a good way round that it would be interesting (we already have a tiny hack in the groovy template config I think, to work around the same problem).

@philwebb

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2014

This is certainly low down the list of things to look at :)

Would a new class with @ConfigurationProperties("spring.jpa.hibernate") work or will that just cause problems because the .hibernate part will also get bound to JpaProperties?

@dsyer

This comment has been minimized.

Copy link
Member

commented Aug 15, 2014

The new class would work but the old one would break because you are trying to bind unknown properties to it. I didn't see an obvious nice way to deal with that.

@wilkinsona wilkinsona removed this from the 1.1.5 milestone Oct 14, 2015

@snicoll snicoll self-assigned this Aug 25, 2017

snicoll added a commit to snicoll/spring-boot that referenced this issue Aug 25, 2017

snicoll added a commit to snicoll/spring-boot that referenced this issue Aug 25, 2017

@snicoll snicoll added this to the 2.1.0 milestone Mar 9, 2018

@snicoll snicoll removed their assignment Mar 9, 2018

@philwebb philwebb modified the milestones: Icebox, Backlog May 4, 2018

@snicoll snicoll self-assigned this Jun 6, 2018

snicoll added a commit to snicoll/spring-boot that referenced this issue Jun 6, 2018

@snicoll snicoll modified the milestones: Backlog, 2.1.0.M1 Jun 6, 2018

@snicoll snicoll closed this in ab19db1 Jun 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.