-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Patch jackson-core with locally modified class #92984
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
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
we should upgrade this branch after jackson is updated to 2.14.1 #92990 |
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.
LGTM
mapping from: /jackson-.*/, to: 'jackson' | ||
} | ||
|
||
shadowJar { |
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 believe this needs to exclude the original FilterDelegateParser class file, otherwise there will be two copies, the original and the one added by this project.
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.
The resulting jar only have one FilteringParserDelegate (new).
I think it wouldn't work when excluding with this snippet (similar to the hadoop approach)
tasks.named('shadowJar').configure {
exclude 'com/fasterxml/jackson/core/filter/FilteringParserDelegate.class'
}
This results in a jar without FilteringParserDelegate
class at all. This is because in the docs https://imperceptiblethoughts.com/shadow/configuration/filtering/
it says:
When using exclude/include with a ShadowJar task, the resulting copy specs are applied to the final JAR contents. This means that, the configuration is applied to the individual files from both the project source set or any of the dependencies to be merged.
In hadoop api is removing the original inner classes and replacing the public class.
jar -tf hadoop-client-api-3.3.3.jar | grep ShutdownHookManager
org/apache/hadoop/util/ShutdownHookManager.class
org/apache/hadoop/util/ShutdownHookManager$HookEntry.class
org/apache/hadoop/util/ShutdownHookManager$1.class
org/apache/hadoop/util/ShutdownHookManager$2.class
The snippet has a $
in it
tasks.named('shadowJar').configure {
exclude 'org/apache/hadoop/util/ShutdownHookManager$*.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.
yeah lets filter out the jar version of FilteringParserDelegate
explicitly. Otherwise which version will be picked is a gradle implementation detail and could change anytime when gradle or the shadow jar plugin is updated.
We can differ between the jar version and our patched version by using the following api:
shadowJar {
exclude { element ->
element.file == null && element.path.endsWith("FilteringParserDelegate.class")
}
manifest {
attributes 'Multi-Release' : '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.
Ah, I remember now why we have that filter. It's specifically for the anonymous inner classes. So I think it's ok to just overwrite. However, please double check the jar manually to confirm there are not eg 2 entries in the zip manifest.
while jackson 2.14.2 with FasterXML/jackson-core#882 is still not released we want to patch the jackson-core used by x-content with the modified class that fixes the bug elastic#92480 closes elastic#92480
💚 Backport successful
|
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.
minor change please. otherwise lgtm
mapping from: /jackson-.*/, to: 'jackson' | ||
} | ||
|
||
shadowJar { |
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.
yeah lets filter out the jar version of FilteringParserDelegate
explicitly. Otherwise which version will be picked is a gradle implementation detail and could change anytime when gradle or the shadow jar plugin is updated.
We can differ between the jar version and our patched version by using the following api:
shadowJar {
exclude { element ->
element.file == null && element.path.endsWith("FilteringParserDelegate.class")
}
manifest {
attributes 'Multi-Release' : 'true'
}
}
before the jackson 2.14.2 elasticserach had to override the jackson locally to avoid a bug when filtering empty arrays. #92984 This commit reverts the local override and upgrades jackson to 2.14.2 which contain the fix to the bug
@pgomulka could this be documented as a known issue in the affected versions' release notes? |
@DaveCTurner good idea. will follow up with docs PRs |
…rsing bug (elastic#93915) relates elastic#92480 relates elastic#92984
…rsing bug (elastic#93915) relates elastic#92480 relates elastic#92984
…rsing bug (elastic#93915) relates elastic#92480 relates elastic#92984
while jackson 2.14.2 with FasterXML/jackson-core#882 is still not released
we want to patch the jackson-core used by x-content with the modified class that fixes the bug #92480
closes #92480