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

Switch to jackson library from org.json #272

Merged
merged 28 commits into from Oct 27, 2022

Conversation

tanmaya-panda1
Copy link
Collaborator

@tanmaya-panda1 tanmaya-panda1 commented Sep 8, 2022

Pull Request Description

org.json:json is used for json parsing and reading capabilities

But org.json:json uses a non-free JSON license, which has already been banned by

Apache Software Foundation.
Debian

Hence org.json dependency is enforced as banned dependency in apache-nifi base pom.xml.

Considering this org.json needs to be removed and replaced with com.fasterxml.jackson


Future Release Comment

[Add description of your change, to include in the next release]
[Delete any or all irrelevant sections, e.g. if your change does not warrant a release comment at all]

Breaking Changes:

  • Due to datatype changes, there can be an impact

Features:

  • None

Fixes:

  • None

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Unit Test Results

247 tests  +1   242 ✔️ +1   43s ⏱️ - 1m 28s
  20 suites ±0       5 💤 ±0 
  20 files   ±0       0 ±0 

Results for commit 598eaac. ± Comparison against base commit cd8ff13.

♻️ This comment has been updated with latest results.

@tanmaya-panda1 tanmaya-panda1 changed the title Feature/switch tojackson for json Switch to jackson library from org.json Sep 8, 2022
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #272 (598eaac) into master (cd8ff13) will decrease coverage by 0.37%.
The diff coverage is 43.55%.

@@             Coverage Diff              @@
##             master     #272      +/-   ##
============================================
- Coverage     54.48%   54.11%   -0.38%     
- Complexity      565      575      +10     
============================================
  Files            95       96       +1     
  Lines          3232     3284      +52     
  Branches        307      340      +33     
============================================
+ Hits           1761     1777      +16     
- Misses         1349     1368      +19     
- Partials        122      139      +17     
Impacted Files Coverage Δ
...crosoft/azure/kusto/data/KustoOperationResult.java 0.00% <0.00%> (ø)
...com/microsoft/azure/kusto/data/auth/CloudInfo.java 36.14% <0.00%> (-0.45%) ⬇️
.../data/exceptions/JsonPropertyMissingException.java 0.00% <0.00%> (ø)
.../kusto/data/exceptions/KustoServiceQueryError.java 25.00% <0.00%> (+25.00%) ⬆️
...ava/com/microsoft/azure/kusto/data/ClientImpl.java 21.73% <14.28%> (+1.13%) ⬆️
...osoft/azure/kusto/data/exceptions/OneApiError.java 57.14% <28.57%> (-23.81%) ⬇️
...rosoft/azure/kusto/ingest/IngestionProperties.java 62.63% <33.33%> (ø)
...icrosoft/azure/kusto/data/KustoResultSetTable.java 38.29% <55.93%> (+0.43%) ⬆️
.../azure/kusto/data/exceptions/DataWebException.java 53.33% <66.66%> (+3.33%) ⬆️
...soft/azure/kusto/data/ClientRequestProperties.java 74.79% <72.00%> (-0.41%) ⬇️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

data/pom.xml Show resolved Hide resolved
data/pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
quickstart/pom.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@ohadbitt ohadbitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a risky change - make sure its not breaking.
Also maybe we should also add a test for the nested exception (an exception parsed in the middle of the execution with EXCEPTIONS_PROPERTY_NAME)

…re-kusto-java into feature/switchTojacksonForJson
Copy link
Contributor

@AsafMah AsafMah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, just fix the one thing

@tanmaya-panda1
Copy link
Collaborator Author

@microsoft-github-policy-service agree company="Microsoft"

@tanmaya-panda1
Copy link
Collaborator Author

@microsoft-github-policy-service agree

@tanmaya-panda1
Copy link
Collaborator Author

@microsoft-github-policy-service agree company="Microsoft"

@tanmaya-panda1
Copy link
Collaborator Author

@microsoft-github-policy-service agree company="Microsoft"

@tanmaya-panda1
Copy link
Collaborator Author

tanmaya-panda1 commented Oct 26, 2022

@microsoft-github-policy-service agree

@tanmaya-panda1 tanmaya-panda1 merged commit c3bc043 into master Oct 27, 2022
AsafMah pushed a commit that referenced this pull request Dec 7, 2022
* switch to jackson for json

* added formatter

* fixed failing test cases

* fixed testcases

* ran formatter

* added jackson-core to pom

* fixed json string

* fixed string conversion

* bumped release version

* bumped up release version

* addressed Ram's review comments

* formatter changes

* refactored object mapper

* addressed Ram's review comments

* formatter changes

* changes done as per Ohad's comments

* added exception testcase for ResultSetTest

* added bigdecimal for precision in decimal values

* added comment for bigdecimal deserilaization

* made options and parameters mandatory in tojson

* fixed test case

* accept cla

* added formatter

* commit for cla

Co-authored-by: AsafMah <asafmahlev@microsoft.com>

(cherry picked from commit c3bc043)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants