Use jsonB instead of jackson #586

owaiskazi19 · 2024-03-16T01:21:17Z

Description

Based on the discussion on #566. Tried to use jsonB from jakarta but it's failing with the below upon running the test

 ./gradlew ':test' --tests "org.opensearch.flowframework.util.ParseUtilsTests.testParseArbitraryStringToObjectMapToString" -Dtests.seed=D5F664F8F9319CA -Dtests.security.manager=false -Dtests.locale=el-GR -Dtests.timezone=America/Argentina/Ushuaia -Druntime.java=17

 2> jakarta.json.JsonException: Provider org.glassfish.json.JsonProviderImpl not found
        at __randomizedtesting.SeedInfo.seed([D5F664F8F9319CA:2C7491821F916CBA]:0)
        at app//jakarta.json.spi.JsonProvider.provider(JsonProvider.java:75)
        at [email protected]/java.util.Optional.orElseGet(Optional.java:364)
        at app//org.eclipse.yasson.internal.JsonBinding.<init>(JsonBinding.java:49)
        at app//org.eclipse.yasson.internal.JsonBindingBuilder.build(JsonBindingBuilder.java:61)
        at app//jakarta.json.bind.JsonbBuilder.create(JsonbBuilder.java:86)
        at app//org.opensearch.flowframework.util.ParseUtils.parseArbitraryStringToObjectMapToString(ParseUtils.java:385)
        at app//org.opensearch.flowframework.util.ParseUtilsTests.testParseArbitraryStringToObjectMapToString(ParseUtilsTests.java:87)

        Caused by:
        java.lang.ClassNotFoundException: org.glassfish.json.JsonProviderImpl
            at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
            at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
            at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
            at java.base/java.lang.Class.forName0(Native Method)
            at java.base/java.lang.Class.forName(Class.java:375)
            at jakarta.json.spi.JsonProvider.provider(JsonProvider.java:72)

After including the glassfish dependency

implementation 'org.glassfish:javax.json:1.1.4'

it failed with jar hell!!

org.opensearch.flowframework.util.ParseUtilsTests > classMethod FAILED
    java.lang.RuntimeException: found jar hell in test classpath
        at org.opensearch.bootstrap.BootstrapForTesting.<clinit>(BootstrapForTesting.java:120)
        at org.opensearch.test.OpenSearchTestCase.<clinit>(OpenSearchTestCase.java:271)
        at java.base/java.lang.Class.forName0(Native Method)
        at java.base/java.lang.Class.forName(Class.java:467)
        at com.carrotsearch.randomizedtesting.RandomizedRunner$2.run(RandomizedRunner.java:623)

        Caused by:
        java.lang.IllegalStateException: jar hell!
        class: jakarta.json.EmptyArray
        jar1: /Users/kazabdu/.gradle/caches/modules-2/files-2.1/jakarta.json/jakarta.json-api/2.0.1/f118a476a74a435e89bb8e36578b65a2be6c191e/jakarta.json-api-2.0.1.jar
        jar2: /Users/kazabdu/.gradle/caches/modules-2/files-2.1/org.glassfish/jakarta.json/2.0.0/4c4a7c5cdcc038c2da2901f35fd8b27c27ffea20/jakarta.json-2.0.0.jar
            at org.opensearch.bootstrap.JarHell.checkClass(JarHell.java:316)
            at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:215)
            at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:102)
            at org.opensearch.bootstrap.BootstrapForTesting.<clinit>(BootstrapForTesting.java:118)
            ... 4 more


Suite: Test class org.opensearch.flowframework.util.ParseUtilsTests
  2> java.lang.RuntimeException: found jar hell in test classpath
        at org.opensearch.bootstrap.BootstrapForTesting.<clinit>(BootstrapForTesting.java:120)
        at org.opensearch.test.OpenSearchTestCase.<clinit>(OpenSearchTestCase.java:271)
        at java.base/java.lang.Class.forName0(Native Method)
        at java.base/java.lang.Class.forName(Class.java:467)
        at com.carrotsearch.randomizedtesting.RandomizedRunner$2.run(RandomizedRunner.java:623)

        Caused by:
        java.lang.IllegalStateException: jar hell!
        class: jakarta.json.EmptyArray
        jar1: /Users/kazabdu/.gradle/caches/modules-2/files-2.1/jakarta.json/jakarta.json-api/2.0.1/f118a476a74a435e89bb8e36578b65a2be6c191e/jakarta.json-api-2.0.1.jar
        jar2: /Users/kazabdu/.gradle/caches/modules-2/files-2.1/org.glassfish/jakarta.json/2.0.0/4c4a7c5cdcc038c2da2901f35fd8b27c27ffea20/jakarta.json-2.0.0.jar
            at org.opensearch.bootstrap.JarHell.checkClass(JarHell.java:316)
            at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:215)
            at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:102)
            at org.opensearch.bootstrap.BootstrapForTesting.<clinit>(BootstrapForTesting.java:118)
            ... 4 more

Looking for some idea here.

Issues Resolved

Part of #566

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Owais Kazi <[email protected]>

codecov · 2024-04-03T05:33:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.33%. Comparing base (33ea800) to head (2a2daab).

Additional details and impacted files

@@            Coverage Diff            @@
##               main     #586   +/-   ##
=========================================
  Coverage     72.33%   72.33%           
  Complexity      680      680           
=========================================
  Files            82       82           
  Lines          3528     3528           
  Branches        284      284           
=========================================
  Hits           2552     2552           
  Misses          850      850           
  Partials        126      126

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbwiddis · 2024-04-03T07:33:02Z

implementation 'org.glassfish:javax.json:1.1.4'

s/javax/jakarta/g

Signed-off-by: Daniel Widdis <[email protected]>

amitgalitz · 2024-04-03T17:49:10Z

Just wondering from a learning perspective why is jsonB better than jackson?

dbwiddis · 2024-04-03T17:51:40Z

Just wondering from a learning perspective why is jsonB better than jackson?

https://github.com/search?q=repo%3Aopensearch-project%2FOpenSearch+jackson+cve&type=issues

amitgalitz · 2024-04-03T18:13:08Z

Just wondering from a learning perspective why is jsonB better than jackson?

https://github.com/search?q=repo%3Aopensearch-project%2FOpenSearch+jackson+cve&type=issues

i didn't see how many more it had then JsonB :) I assumed any dependency that core is also using we should be okay with but yeah even if core is the one that upgrades, less upstream CVE's from core means less issues cut

dbwiddis · 2024-04-03T18:16:17Z

i didn't see how many more it had then JsonB :

Yeah. I've maintained an open source project since 2015 and it's an annual event to get Jackson CVEs. It's pretty awesome for whole-java-object transformations but if all you need is simple json parsing, it's overkill. The Java client uses JsonB and it used to be part of Java EE, so it just seems like a better choice for simple parsing.

owaiskazi19 · 2024-04-03T18:33:48Z

Just wondering from a learning perspective why is jsonB better than jackson?

Tbh here. Jackson supports much more advanced features like polymorphic collection deserialization than jsonB but the question is are we facing any complication because of that? No.

Also, as @dbwiddis pointed out the amount of CVEs we have targeted for Jackson is much more than JsonB.

Now looking at the performance metrics. Jackson is better than JsonB for serialization and deserialization.

I will leave the choice for you all to decide between CVEs and performance.

dbwiddis · 2024-04-03T18:54:56Z

I will leave the choice for you all to decide between CVEs and performance.

It's not just CVEs. Jackson uses reflection to do its magic and there have been concerns there as well: opensearch-project/OpenSearch#5504

We don't need phenomenal cosmic powers to do simple parsing that doesn't need them. :)

dbwiddis · 2024-04-03T21:37:33Z

I'm not too concerned about performance here, this is not a hot path thing, just one-time parsing of user-entered JSON values. Is the difference more than a single digit number of milliseconds?

owaiskazi19 · 2024-04-04T00:25:46Z

I'm not too concerned about performance here, this is not a hot path thing, just one-time parsing of user-entered JSON values. Is the difference more than a single digit number of milliseconds?

I found https://github.com/fabienrenaud/java-json-benchmark?tab=readme-ov-file#users-model for the performance of serialization and deserialization of various libraries. I am fine merging this in since we are using jsonB anyway for basic parsing.

dbwiddis · 2024-04-04T00:31:01Z

I found https://github.com/fabienrenaud/java-json-benchmark?tab=readme-ov-file#users-model for the performance of serialization and deserialization of various libraries.

We should use fastjson! 😉

But seriously, looks like our implementation (yasson) is about twice as slow as jackson; 4 microseconds vs. 2 microseconds on average. I can live with that.

opensearch-trigger-bot · 2024-04-04T00:32:09Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/flow-framework/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/flow-framework/backport-2.x
# Create a new branch
git switch --create backport/backport-586-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cbf675a12ec869af69fc4405cfc6c9631c425680
# Push it to GitHub
git push --set-upstream origin backport/backport-586-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/flow-framework/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-586-to-2.x.

* Use jsonB instead of jackson Signed-off-by: Owais Kazi <[email protected]> * Resolve Jar Hell with newest versions and jakarta dependencies Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Owais Kazi <[email protected]> Signed-off-by: Daniel Widdis <[email protected]> Co-authored-by: Daniel Widdis <[email protected]>

* Use jsonB instead of jackson (#586) * Use jsonB instead of jackson Signed-off-by: Owais Kazi <[email protected]> * Resolve Jar Hell with newest versions and jakarta dependencies Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Owais Kazi <[email protected]> Signed-off-by: Daniel Widdis <[email protected]> Co-authored-by: Daniel Widdis <[email protected]> * Removed 3.x dependency Signed-off-by: owaiskazi19 <[email protected]> --------- Signed-off-by: Owais Kazi <[email protected]> Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: owaiskazi19 <[email protected]> Co-authored-by: Daniel Widdis <[email protected]>

github-actions bot added the backport 2.x backport PRs to 2.x branch label Mar 16, 2024

Use jsonB instead of jackson

f48cd92

Signed-off-by: Owais Kazi <[email protected]>

dbwiddis force-pushed the use-jsonB branch from 55d8ca1 to f6a6501 Compare April 3, 2024 05:29

Resolve Jar Hell with newest versions and jakarta dependencies

2a2daab

Signed-off-by: Daniel Widdis <[email protected]>

dbwiddis force-pushed the use-jsonB branch from f6a6501 to 2a2daab Compare April 3, 2024 17:19

owaiskazi19 marked this pull request as ready for review April 3, 2024 17:28

owaiskazi19 requested review from dbwiddis, joshpalis, ohltyler, amitgalitz and jackiehanyang as code owners April 3, 2024 17:28

owaiskazi19 changed the title ~~[Draft] Use jsonB instead of jackson~~ Use jsonB instead of jackson Apr 3, 2024

owaiskazi19 added the skip-changelog label Apr 3, 2024

dbwiddis approved these changes Apr 3, 2024

View reviewed changes

joshpalis approved these changes Apr 3, 2024

View reviewed changes

dbwiddis merged commit cbf675a into opensearch-project:main Apr 4, 2024
33 of 41 checks passed

owaiskazi19 mentioned this pull request Apr 4, 2024

[Backport-2.x] Use jsonB instead of jackson (#586) #645

Merged

dbwiddis mentioned this pull request Apr 5, 2024

[INVESTIGATE] Find Potential Alternatives to Jackson #566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use jsonB instead of jackson #586

Use jsonB instead of jackson #586

owaiskazi19 commented Mar 16, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

dbwiddis commented Apr 3, 2024

amitgalitz commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

amitgalitz commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

owaiskazi19 commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

owaiskazi19 commented Apr 4, 2024 •

edited

Loading

dbwiddis commented Apr 4, 2024

opensearch-trigger-bot bot commented Apr 4, 2024

Use jsonB instead of jackson #586

Use jsonB instead of jackson #586

Conversation

owaiskazi19 commented Mar 16, 2024 • edited Loading

Description

Issues Resolved

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

dbwiddis commented Apr 3, 2024

amitgalitz commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

amitgalitz commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

owaiskazi19 commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

dbwiddis commented Apr 3, 2024

owaiskazi19 commented Apr 4, 2024 • edited Loading

dbwiddis commented Apr 4, 2024

opensearch-trigger-bot bot commented Apr 4, 2024

owaiskazi19 commented Mar 16, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

owaiskazi19 commented Apr 4, 2024 •

edited

Loading