Migrate UserType implementations to hibernate 6
#40
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/test/java/org/openrewrite/hibernate/MigrateUserTypeTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/test/java/org/openrewrite/hibernate/MigrateUserTypeTest.java
Outdated
Show resolved
Hide resolved
| @Nullable | ||
| private static J.FieldAccess parameterizedType = null; |
There was a problem hiding this comment.
Mutable static fields in a recipe are not ideal; can we move this field into the visitor instead? As each call the getVisitor returns a new isolated instance, whereas the Recipe instance can be shared.
There was a problem hiding this comment.
| @Nullable | |
| private static J.FieldAccess parameterizedType = null; |
There was a problem hiding this comment.
Maybe we should check if we have any legitimate cases of this. If not, we can add a best practice recipe, maybe?
Co-authored-by: Tim te Beek <[email protected]>
Order methodmatchers alphabetically
src/test/java/org/openrewrite/hibernate/MigrateUserTypeTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/hibernate/MigrateUserTypeTest.java
Outdated
Show resolved
Hide resolved
| private static final MethodMatcher REPLACE = new MethodMatcher("* replace(java.lang.Object, java.lang.Object, java.lang.Object)"); | ||
| private static final MethodMatcher RESULT_SET_STRING_PARAM = new MethodMatcher("java.sql.ResultSet *(java.lang.String)"); | ||
| private static final MethodMatcher RETURNED_CLASS = new MethodMatcher("* returnedClass()"); | ||
| private static final MethodMatcher SQL_TYPES = new MethodMatcher("* sqlTypes()"); |
There was a problem hiding this comment.
Can we equally use org.hibernate.usertype.UserType here instead of *?
There was a problem hiding this comment.
The way I'm using them to match method declarations it seems you can only really use a wildcard pattern or the type of the enclosing J.ClassDeclaration
| return maybeAutoFormat(method, md, ctx); | ||
| } | ||
|
|
||
| private J.MethodDeclaration changeParameterTypes(J.MethodDeclaration md, List<String> paramNames) { |
There was a problem hiding this comment.
Matching on parameter names isn't safe here, as folks might have named their variables differently; I suggest you change this to use the parameter index instead, or another way of ensuring only the correct types are changed.
There was a problem hiding this comment.
Applied parameter index instead of name
Anything in particular you'd like reviewers to focus on?
Test coverage
Anyone you would like to review specifically?
@timtebeek
Any additional context
https://docs.jboss.org/hibernate/orm/5.6/javadocs/org/hibernate/usertype/UserType.html
https://docs.jboss.org/hibernate/orm/6.0/javadocs/org/hibernate/usertype/UserType.html
Checklist