Remove support for hibernate.properties (long deprecated)

[Sanne]

#Fixes #17537

[gsmet]

@Sanne could you have a look at George's comment?

[yrodiere]

I'll copy my comment from the issue:

but it appears from report #17515 that this was no longer working at all.

The question is how long has this not been working. At least on April 13th, it was still working, since there's a reproducer here that uses hibernate.properties as a workaround: #16463

After some investigation, my use of hibernate.properties in #16463 apparently didn't work. I don't know why I thought it did.

Anyway... It led me to investigate a bit more. I believe hibernate.properties does work in very few cases. Hibernate ORM discovers the hibernate.properties by itself, at runtime, in org.hibernate.cfg.Environment, and these properties are used in a few places.

In particular:

• In org.hibernate.dialect.MySQLDialect#MySQLDialect to determine the storage engine.
• In a few global settings that are deprecated, but still used:
  • org.hibernate.cfg.Environment#ENABLE_BINARY_STREAMS
    • Controls the value of org.hibernate.internal.FastSessionServices#useStreamForLobBinding
  • org.hibernate.cfg.Environment#ENABLE_REFLECTION_OPTIMIZER
    • Controls the use of reflection optimizers in org.hibernate.tuple.entity.PojoEntityTuplizer and org.hibernate.tuple.component.PojoComponentTuplizer.
  • org.hibernate.cfg.Environment#ENABLE_LEGACY_PROXY_CLASSNAMES
    • Controls the value of org.hibernate.bytecode.internal.bytebuddy.BasicProxyFactoryImpl#PROXY_NAMING_SUFFIX and org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper#PROXY_NAMING_SUFFIX
• In org.hibernate.cfg.Environment#BYTECODE_PROVIDER_INSTANCE, which is used:
  • When deserializing proxies: org.hibernate.proxy.pojo.bytebuddy.SerializableProxy#readResolve
  • In Ant/Maven/Gradle plugins: org.hibernate.tool.enhance.EnhancementTask#execute/org.hibernate.orm.tooling.maven.MavenEnhancePlugin#execute/org.hibernate.orm.tooling.gradle.EnhancementHelper#enhance
• Some other places that are not relevant to Quarkus, as far as I can see.

So I don't know how I should feel about this patch. Yes it removes dead code, but it doesn't completely remove support for hibernate.properties, which will still work... sometimes.

If we really want to close this topic for good, I'd suggest maybe adding some code to log a warning on startup if there is a hibernate.properties, and remove the file entirely? That way, we're sure there won't be any confusion as to whether it works or not.

[Sanne]

updated the commit

[Sanne]

hi @yrodiere , thanks for the analysis! And sorry I've only seen your comment after pushing the rebased version.

So I don't know how I should feel about this patch. Yes it removes dead code, but it doesn't completely remove support for hibernate.properties, which will still work... sometimes.

If we really want to close this topic for good, I'd suggest maybe adding some code to log a warning on startup if there is a hibernate.properties, and remove the file entirely? That way, we're sure there won't be any confusion as to whether it works or not.

Regarding having the warning, you mean like the warning we had in QuarkusEnvironment ? (the very few lines this PR is removing?)

Regarding "and remove the file entirely?" , WDYM - to remove the hibernate.properties file from the project? I don't think we should try to remove source files.

[yrodiere]

Regarding having the warning, you mean like the warning we had in QuarkusEnvironment ? (the very few lines this PR is removing?)

I mean that a user including a hibernate.properties in his project should get a warning during the build, because that hibernate.properties will be ignored and that's very much non-standard behavior. Principle of least surprise and whatnot.

Regarding "and remove the file entirely?" , WDYM - to remove the hibernate.properties file from the project? I don't think we should try to remove source files.

I meant not adding it to the JAR. I agree we shouldn't alter source files.

[Sanne]

I meant not adding it to the JAR. I agree we shouldn't alter source files.

I don't think we have the ability to override what the build tool will do?

Regarding the warning, yes good idea. So you mean similar to the warning we had until now, but processed by the deployment component, right?

[gastaldi]

LGTM

[yrodiere]

I meant not adding it to the JAR. I agree we shouldn't alter source files.

I don't think we have the ability to override what the build tool will do?

Too bad. I thought, since we can add class files, why not remove resource files... But ok. The warning should address the problem for the vast majority of users.

Regarding the warning, yes good idea. So you mean similar to the warning we had until now, but processed by the deployment component, right?

Yes.

[Sanne]

@yrodiere adapted and rebased. I went a step further and decided to throw an exception rather than logging a warning - I know it's rather aggressive, but I prefer motivating feedback on this with more urgency than what I'd get with a warning.

[yrodiere]

I went a step further and decided to throw an exception rather than logging a warning

I'm not sure that's wise, as there are some people out there who pull legacy JARs, which contain a persistence.xml. It's not that much of a stretch to assume that other people pull legacy JARs that contain a hibernate.properties, and they can't change that.

Anyway, +1 to merge as long as everyone is okay with breaking existing applications that contain a hibernate.properties. I think @gsmet might want to know about this before we merge.

[Sanne]

. It's not that much of a stretch to assume that other people pull legacy JARs that contain a hibernate.properties, and they can't change that.

Right, that's why I have the error message print the location.

Regarding this being problematic for legacy applications, I agree with your concern but still think we need to hear about those and you also pointed out yourself how problematic that can be - and Quarkus has always made it clear it's not meant for legacy.