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

Replace both org.json and gson with jackson-databind #3206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sazzad16
Copy link
Collaborator

No description provided.

@sazzad16 sazzad16 added wait for more reviews discussion needed breakingchange Pull request that has breaking changes. Must include the breaking behavior in release notes. labels Nov 16, 2022
@sazzad16 sazzad16 added this to the 4.4.0 milestone Nov 17, 2022
@zeekling
Copy link
Contributor

Is it the same as the pr below:
#3205

@sazzad16
Copy link
Collaborator Author

@zeekling This PR is an extension of #3205 which replaces only org.json:json. This also replaces gson.

@sazzad16
Copy link
Collaborator Author

@chayim @dengliming @yangbodong22011

Do you have like or dislike for this or #3205 ?

Either of these is a breaking change (affects only RedisJSON commands) but we are considering for Jedis 4.4.0.

@yangbodong22011
Copy link
Collaborator

@chayim @dengliming @yangbodong22011

Do you have like or dislike for this or #3205 ?

Either of these is a breaking change (affects only RedisJSON commands) but we are considering for Jedis 4.4.0.

I read the related issue, is there an option for us to wait for the dust to settle on the license changes in this comment before making a decision?

Another question is why we rely on both org.json and google.gson now, can only relying on google.gson not meet our needs?

@sazzad16
Copy link
Collaborator Author

Another question is why we rely on both org.json and google.gson now, can only relying on google.gson not meet our needs?

Gson was being used in JRedisJSON. That's where we got it from.

But Gson (at least with default settings) was not so friendly with RedisJSON v2's replies while it is easy for org.json:

if (str.charAt(0) == '{') {
try {
return new JSONObject(str);
} catch (Exception ex) { }
} else if (str.charAt(0) == '[') {
try {
return new JSONArray(str);
} catch (Exception ex) { }
}

return new JSONArray(STRING.build(data));

Due to this and almost no development in Gson, org.json was chosen for RedisJSON v2.

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2022

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (13321d2) 66.93% compared to head (16ab2be) 66.94%.
Report is 231 commits behind head on master.

Files Patch % Lines
.../main/java/redis/clients/jedis/BuilderFactory.java 60.00% 4 Missing ⚠️
.../main/java/redis/clients/jedis/CommandObjects.java 88.57% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3206      +/-   ##
============================================
+ Coverage     66.93%   66.94%   +0.01%     
- Complexity     4609     4610       +1     
============================================
  Files           252      252              
  Lines         14881    14892      +11     
  Branches        923      919       -4     
============================================
+ Hits           9960     9970      +10     
- Misses         4523     4525       +2     
+ Partials        398      397       -1     

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

@sazzad16 sazzad16 removed this from the 4.4.0 milestone Nov 23, 2022
@sazzad16 sazzad16 added dependencies Pull request that updates a dependency and removed discussion needed labels Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakingchange Pull request that has breaking changes. Must include the breaking behavior in release notes. dependencies Pull request that updates a dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants