-
Notifications
You must be signed in to change notification settings - Fork 29
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
Create rules for migration to Hibernate 5.6 #206
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jason Froehlich <[email protected]>
Signed-off-by: Jason Froehlich <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some minor requests
or: | ||
- builtin.filecontent: | ||
filePattern: (hibernate\.properties|persistence\.xml|cfg\.xml|application\.properties|application\.yaml) | ||
pattern: hibernate.bytecode.provider=javassist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pattern would only be valid for *.properties
files, right?
- builtin.filecontent: | ||
filePattern: (hibernate\.properties|persistence\.xml|cfg\.xml|application\.properties|application\.yaml) | ||
pattern: hibernate.bytecode.provider=javassist | ||
- as: hibernate_cfg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this as
field, since it's now being used anywhere. The same with the one below, as: persistence_files
.
- java.referenced: | ||
location: FIELD | ||
pattern: java.sql.Clob | ||
- as: config_files # Check for PostgreSQL 8.1 dialect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this as
field. This is basically used when the outcome of a subcondition is needed in the next subcondition, providing some sort of "chaining", but here is not necessary.
- as: config_files # Check for PostgreSQL 8.1 dialect | ||
builtin.filecontent: | ||
filePattern: .*\.(java|properties|xml) | ||
pattern: org.hibernate.dialect.PostgreSQL81Dialect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a subcondition looking for PostgreSQL dependency too, or would it be enough with this? Not sure if this property is always used. If we don't need the dep condition, then better.
message: | | ||
Using `@Lob` or `java.sql.Clob` with PostgreSQL 8.1 dialect might require DDL type changes for CLOBs. | ||
|
||
Consider reviewing DDL generation for CLOB columns and potential migration to 'oid' type if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add some information about this potential data loss, which looks quite worrying:
All PostgreSQL JDBC drivers unfortunately just store the oid it created for a java.sql.Clob into the text column. Although reading back the value with the CLOB API works, PostgreSQL has no knowledge of the reference to the LOB, because the oid is not known to PostgreSQL, leading to data loss when vacuumlo (the utility to clean up unused LOBs) runs. To avoid the data loss, it is required to use the oid type so that vacuumlo can see the reference.
Updating to 5.6.2 does not require any schema or application changes by default, but we highly recommend that you migrate existing text columns for LOBs to oid to prevent data loss due to the activity of vacuumlo.
These were the only 2 items found in the hibernate migration guide