-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add transfers for using arguments #16
base: main
Are you sure you want to change the base?
Conversation
I'd prefer to keep Can we move this visitor into a either the existing |
a24ce0d
to
725e9d0
Compare
...java/org/openrewrite/java/jackson/codehaus/TransferUsingArgumentFromCodehausToFasterXML.java
Outdated
Show resolved
Hide resolved
...java/org/openrewrite/java/jackson/codehaus/TransferUsingArgumentFromCodehausToFasterXML.java
Outdated
Show resolved
Hide resolved
...java/org/openrewrite/java/jackson/codehaus/TransferUsingArgumentFromCodehausToFasterXML.java
Outdated
Show resolved
Hide resolved
...ain/java/org/openrewrite/java/jackson/codehaus/RemoveDoublyAnnotatedCodehausAnnotations.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Thanks for the work & rework here @nielsdebruin ; ended up removing the troublesome auto format; when in doubt that's typically easiest.
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.
Added a few more polishes, like also converting known serializer packages to fasterxml databind. Thanks for getting this most of the way there!
@JsonSerialize(include = NON_NULL, using = None.class) | ||
@com.fasterxml.jackson.databind.annotation.JsonSerialize(using = None.class) |
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.
What I found slightly odd about the "after" usage example here is that we're one-to-one moving the argument over, whereas the using
argument here, as well as the contentUsing
, keyUsing
and nullsUsing
sibling only accept a FasterXml JsonSerializer, whereas the moved argument is a Codehaus JsonSerialzer, That means after this recipe the code would not yet compile. Compare the before and after packages:
That means we likely need to expand the converted packages (ideally) and types (for exceptional cases) seen here:
rewrite-jackson/src/main/resources/META-INF/rewrite/codehaus-to-fasterxml.yml
Lines 32 to 62 in b5fab13
name: org.openrewrite.java.jackson.CodehausClassesToFasterXML | |
displayName: Migrate classes from Jackson Codehaus (legacy) to Jackson FasterXML | |
description: >- | |
In Jackson 2, the package and dependency coordinates moved from Codehaus to FasterXML. | |
recipeList: | |
- org.openrewrite.java.jackson.codehaus.JsonIncludeAnnotation | |
- org.openrewrite.java.ChangeType: | |
oldFullyQualifiedTypeName: org.codehaus.jackson.map.JsonSerializer | |
newFullyQualifiedTypeName: com.fasterxml.jackson.databind.JsonSerializer | |
- org.openrewrite.java.ChangeType: | |
oldFullyQualifiedTypeName: org.codehaus.jackson.map.annotate.JsonSerialize$Inclusion | |
newFullyQualifiedTypeName: com.fasterxml.jackson.annotation.JsonInclude$Include | |
- org.openrewrite.java.ChangeType: | |
oldFullyQualifiedTypeName: org.codehaus.jackson.map.annotate.JsonSerialize | |
newFullyQualifiedTypeName: com.fasterxml.jackson.databind.annotation.JsonSerialize | |
- org.openrewrite.java.ChangeType: | |
oldFullyQualifiedTypeName: org.codehaus.jackson.map.ObjectMapper | |
newFullyQualifiedTypeName: com.fasterxml.jackson.databind.ObjectMapper | |
- org.openrewrite.java.ChangeType: | |
oldFullyQualifiedTypeName: org.codehaus.jackson.map.SerializationConfig$Feature | |
newFullyQualifiedTypeName: com.fasterxml.jackson.databind.SerializationFeature | |
- org.openrewrite.java.ChangeType: | |
oldFullyQualifiedTypeName: org.codehaus.jackson.map.DeserializationConfig$Feature | |
newFullyQualifiedTypeName: com.fasterxml.jackson.databind.DeserializationFeature | |
- org.openrewrite.java.ChangePackage: | |
oldPackageName: org.codehaus.jackson.annotate | |
newPackageName: com.fasterxml.jackson.annotation | |
recursive: true |
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.
Pushed up in 736fce0.
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.
Before we merge; would you mind handling the other using arguments the same?
contentUsing, keyUsing and nullsUsing
On it |
// Map from codehaus -> fasterxml annotation | ||
Map<J.Annotation, J.Annotation> doubleAnnotated = new FindDoublyAnnotatedVisitor().reduce(tree, new HashMap<>()); | ||
|
||
transferArgument(doubleAnnotated, "using"); |
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.
Perhaps we could keep a list of field names that should be transferred and reduce this to a single statement?
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.
Done
What's changed?
The
RemoveDoublyAnnotatedCodehausAnnotations
recipe has been extended with the functionality to transfer the value of "using" in the codehaus annotations@JsonSerialize(using=XYZ)
to the faster XML annotation.The transfer only occurs if no value forusing
has been specified in the fasterxml annotation. An option has been added to enable or disable this behaviorAnything in particular you'd like reviewers to focus on?
Anyone you would like to review specifically?
@Laurens-W @timtebeek
Checklist