Upgrade hibernate

[asolntsev]

See issue #1113

This pull request is not finally ready yet. I am sharing it just for your early reviews (if anybody is interested). I will continue working on it.

[tazmaniax]

hi @asolntsev,

Does the patch file (https://github.com/playframework/play1/blob/master/framework/patches/hibernate-4.2.19-patch-play.patch.patch) still need to be applied to the new version of Hibernate?

cheers

[asolntsev]

@tazmaniax Thank you for the hint! Actually I don't know an answer.
At least, everything works in my current project without this patch.

Can you help me: do you know why this hack was needed?

[asolntsev]

@tazmaniax I tried this patch, and it helped.
Thank you a lot! You really helped me with this annoying hibernate issue.

P.S. There is still a question in the air: why this hack was needed? Why does Play framework use such hidden unobvious hacks?

[flybyray]

@asolntsev The hibernate patch is part of the code base for a long time. Play! 1 started with hibernate from Version 3. There was a time when the hibernate3xxx.jar has been distrubuted as patched wihtout the patch source (that was really hidden).
Please read the jpa.textile @ explicit-save (linked the initial one from c931cbd on 1 Sep 2009)

Here just the essential hint of what was changed for Play!:

So, that exactly what we’ve changed in play. All the perstistent objects extending the JPASupport/JPAModel, will not be automactically saved without an explicit call to the save() method. So you can effectively rewrite the previous code as

Maybe it is a good time to enhence the JPA documentation to make an association to that patch (that is really missing for someone who wants to upgrade hibernate version).

[asolntsev]

@flybyray Thank you for clarification. I like the idea of maintaining object state manually and calling save explicitly. I just don't like that this implementation detail was so much hidden. It made me waste lot of time. :(

And I wonder: is it real that nobody but Play needs to disable dirty check in Hibernate? Probably there is a more common solution for this problem?

P.S. I created a simpler solution: I renamed hibernate-core-x.y.z.FINAL.jar to hibernate-core-x.y.z.PATCHED.jar. I think it show very well that the jar has been modified.

[asolntsev]

@xael-fry @tazmaniax @flybyray Yihhaaaa!!! This day came!
The build got green. The upgrade to hibernate5 is ready now.

Can you review the changes and merge the pull request?

[flybyray]

From my point of view looking the github pull request diff it is ok:
diff: https://patch-diff.githubusercontent.com/raw/playframework/play1/pull/1114.diff
compare: master...codeborne:upgrade-hibernate

But I have the feeling that something is wrong with github pull request comparison.

Github shows me 38 Files changed. But if i check the pull request locally there are more changes, which are missing or filtered by github pull request api.
How I checked that? I did checkout/fetch the a pull request locally as refs/heads/pull1114. For demonstration i added the codeborne repository too.

$ git config --get-regexp remote.*.url
remote.origin.url git@github.com:playframework/play1.git
remote.codeborne.url git@github.com:codeborne/play.git
$ git for-each-ref --sort=-committerdate refs/heads/
58c870efea9532b0e84edbb7828e578d374c999b commit	refs/heads/master
b128c17f0d34ef15dc3d94acbd83a0cdfef5feb6 commit	refs/heads/upgrade-hibernate
b128c17f0d34ef15dc3d94acbd83a0cdfef5feb6 commit	refs/heads/pull1114
$ git status
Auf Branch master
Ihr Branch ist auf dem selben Stand wie 'origin/master'.
$ git diff refs/heads/pull1114   --name-only | wc -l
110

The pull request has 110 files changed. Some file changes are less important, maybe github filtered them out. But those changes, changed/removed javadoc, reorganized import statements ...

Maybe it would be better to have a single commit pull request. Maybe github has some problems with multiple commits including hugh binaries. The patch is really big because of binaries https://github.com/playframework/play1/pull/1114.patch

I was somehow surprised that the hibernate patch does not need any modification at all for the upgrade from 4 to 5. Maybe I should check that in detail? Did you run the hibernate tests itself (yes some will be failing in local environment, round about 2 or 4 tests failed for hibernate 4)?

--- UPDATE ---
Sorry now I get the github logic. It uses always the same revision of the first commit (on 5. Mar, rev 02f7bac812ee1d3b14e6ba5f2d6883dc46d56863 ) an applies additional commits to that point. If i checkout that revision i get the same/similar result:

$ git diff refs/heads/pull1114   --name-only | wc -l
39

[asolntsev]

@flybyray I did git rebase meanwhile, squashing several commits into one. Now the PR has 5 commits (instead of former 15-20). Probably it also created a confusion.

[flybyray]

@asolntsev no is all ok, but maybe fix the line endings in samples-and-tests/just-test-cases/test/SimpleJPATest.java too with your pull request (windows line endings)

[asolntsev]

@flybyray done (removed file endings)

[xael-fry]

I have a lot of warning when running in test mode.

We need to investigate.
Don't forget to put issue number in commit message

11:58:08,287 INFO  ~ Connected to jdbc:h2:mem:play;MODE=PostgreSQL;LOCK_MODE=0;MV_STORE=FALSE;MVCC=FALSE; for default
11:58:08,352 DEBUG ~ Logging Provider: org.jboss.logging.Log4jLoggerProvider
11:58:10,059 WARN  ~ GenerationTarget encountered exception accepting command : Error executing DDL via JDBC Statement
org.hibernate.tool.schema.spi.CommandAcceptanceException: Error executing DDL via JDBC Statement
	at org.hibernate.tool.schema.internal.exec.GenerationTargetToDatabase.accept(GenerationTargetToDatabase.java:67)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.applySqlString(SchemaDropperImpl.java:375)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.applySqlStrings(SchemaDropperImpl.java:359)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.applyConstraintDropping(SchemaDropperImpl.java:331)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.dropFromMetadata(SchemaDropperImpl.java:230)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.performDrop(SchemaDropperImpl.java:154)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.doDrop(SchemaDropperImpl.java:126)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.doDrop(SchemaDropperImpl.java:112)
	at org.hibernate.tool.schema.spi.SchemaManagementToolCoordinator.performDatabaseAction(SchemaManagementToolCoordinator.java:144)
	at org.hibernate.tool.schema.spi.SchemaManagementToolCoordinator.process(SchemaManagementToolCoordinator.java:72)
	at org.hibernate.internal.SessionFactoryImpl.<init>(SessionFactoryImpl.java:309)
	at org.hibernate.boot.internal.SessionFactoryBuilderImpl.build(SessionFactoryBuilderImpl.java:452)
	at org.hibernate.jpa.boot.internal.EntityManagerFactoryBuilderImpl.build(EntityManagerFactoryBuilderImpl.java:889)
	at play.db.jpa.JPAPlugin.newEntityManagerFactory(JPAPlugin.java:183)
	at play.db.jpa.JPAPlugin.onApplicationStart(JPAPlugin.java:130)
	at play.plugins.PluginCollection.onApplicationStart(PluginCollection.java:616)
	at play.Play.start(Play.java:541)
	at play.Play.detectChanges(Play.java:659)
	at play.Invoker$Invocation.init(Invoker.java:209)
	at play.server.PlayHandler$NettyInvocation.init(PlayHandler.java:264)
	at play.Invoker$Invocation.run(Invoker.java:295)
	at play.server.PlayHandler$NettyInvocation.run(PlayHandler.java:303)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
Caused by: org.h2.jdbc.JdbcSQLException: Table "API_CONSTRAINTS" not found; SQL statement:
alter table api_constraints drop constraint FKlxvisbtqofqx7kil2jrrqukvd [42102-193]
	at org.h2.message.DbException.getJdbcSQLException(DbException.java:345)
	at org.h2.message.DbException.get(DbException.java:179)
	at org.h2.message.DbException.get(DbException.java:155)
	at org.h2.command.Parser.tableIfTableExists(Parser.java:5673)
	at org.h2.command.Parser.commandIfTableExists(Parser.java:5686)
	at org.h2.command.Parser.parseAlterTable(Parser.java:5511)
	at org.h2.command.Parser.parseAlter(Parser.java:4844)
	at org.h2.command.Parser.parsePrepared(Parser.java:344)
	at org.h2.command.Parser.parse(Parser.java:317)
	at org.h2.command.Parser.parse(Parser.java:289)
	at org.h2.command.Parser.prepareCommand(Parser.java:254)
	at org.h2.engine.Session.prepareLocal(Session.java:561)
	at org.h2.engine.Session.prepareCommand(Session.java:502)
	at org.h2.jdbc.JdbcConnection.prepareCommand(JdbcConnection.java:1203)
	at org.h2.jdbc.JdbcStatement.executeInternal(JdbcStatement.java:170)
	at org.h2.jdbc.JdbcStatement.execute(JdbcStatement.java:158)
	at com.mchange.v2.c3p0.impl.NewProxyStatement.execute(NewProxyStatement.java:75)
	at org.hibernate.tool.schema.internal.exec.GenerationTargetToDatabase.accept(GenerationTargetToDatabase.java:54)

[asolntsev]

@xael-fry I cannot reproduce your problem with warnings. Which application can I use to reproduce it? Which application contains Table "API_CONSTRAINTS"?

[xael-fry]

@asolntsev It is in my project it is just a simple, I will try to extract it for testing

[xael-fry]

@asolntsev
I change my mind, as the problem (warning logs) I had, doesn't seems to have impact on running behavior, I suggest we merged it, and prepare a 1.5.0 RC1, to have some return from users.

[asolntsev]

@xael-fry Thank you!
Merged. Executed all tests on my working project - everything is green. Ready for RC.

[flybyray]

the patch "hibernate-5.2.10-patch-play.patch" is not applyable to the hibernate-orm 5.2.10, at least the Interceptor.java needs changes
Maybe look at hibernate/hibernate-orm@master...flybyray:hibernate_5_2_10_patch_play to see what should be changed

[flybyray]

The play models are collected into PersistenceUnitInfoImpl.managedClassNames.
but those will never get read by hibernate for now @see
https://github.com/hibernate/hibernate-orm/blob/b38a9f40ac2bc8ccf16b05af09cb96988ea93fb2/hibernate-core/src/main/java/org/hibernate/jpa/boot/internal/EntityManagerFactoryBuilderImpl.java#L715-L715

as i can see you should put them for now into configuration with key org.hibernate.jpa.AvailableSettings#LOADED_CLASSES