Skip to content
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

Patch jackson-core with locally modified class #92984

Merged
merged 7 commits into from
Jan 18, 2023

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Jan 17, 2023

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

@pgomulka pgomulka changed the title draft Patch jackson-core with locally modified class Jan 17, 2023
@pgomulka pgomulka self-assigned this Jan 17, 2023
@pgomulka pgomulka added >non-issue :Core/Infra/Core Core issues without another label v8.6.1 labels Jan 17, 2023
@pgomulka pgomulka marked this pull request as ready for review January 17, 2023 14:41
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 17, 2023
@pgomulka
Copy link
Contributor Author

we should upgrade this branch after jackson is updated to 2.14.1 #92990

Copy link
Contributor

@ChrisHegarty ChrisHegarty left a 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 {
Copy link
Member

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.

Copy link
Contributor Author

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'
}

Copy link
Contributor

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'
  }
}

Copy link
Member

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.

@pgomulka pgomulka added auto-backport Automatically create backport pull requests when merged and removed auto-backport-and-merge labels Jan 18, 2023
@pgomulka pgomulka merged commit 441e77c into elastic:main Jan 18, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jan 18, 2023
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
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.6

Copy link
Contributor

@breskeby breskeby left a 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 {
Copy link
Contributor

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'
  }
}

pgomulka added a commit that referenced this pull request Jan 30, 2023
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
@DaveCTurner
Copy link
Contributor

@pgomulka could this be documented as a known issue in the affected versions' release notes?

@pgomulka
Copy link
Contributor Author

@DaveCTurner good idea. will follow up with docs PRs

pgomulka added a commit that referenced this pull request Feb 22, 2023
pgomulka added a commit that referenced this pull request Feb 22, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 23, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 23, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 23, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 23, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 23, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 23, 2023
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 23, 2023
elasticsearchmachine pushed a commit that referenced this pull request Feb 23, 2023
elasticsearchmachine pushed a commit that referenced this pull request Feb 23, 2023
pgomulka added a commit that referenced this pull request Feb 23, 2023
zuiyu-main pushed a commit to zuiyu-main/elasticsearch that referenced this pull request Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.6.1 v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source includes triggers MapperParsingException/IllegalStateException if field is in an array
6 participants