Upgrade jackson and jsonrpc libs

[rmoreliovlabs]

Description

Upgrade jackson and jsonrpc libs.

Jackson ObjectMapper is handling null values in this new version and some tests were expecting NullPointerExceptions and JsonMappingExceptions. I just made those methods expecting Jackson Object Mapper exceptions to have a similar behavior to keep it working as previously expected.

Motivation and Context

We are using an old version of com.fasterxml.jackson.core:jackson-databind, currently set to 2.8.7, being updated to 2.8.11 on scope of Java upgrade effort. There is already a 2.13 version and 2.14 is coming soon.

The reason for not updating now is that we’ve detected some changes on method outputs. As an example, ObjectMapper.readTree is now returning null in cases when it was throwing an NullPointerException (tested on co.rsk.config.RemascConfigFactoryTest#createRemascConfigInvalidConfig). Even if code compiles and tests pass (after fixing the previously mentioned difference), we could be creating some bugs that would remain unnoticed until runtime, as we do a wide usage of this library.

It looks like the change was introduced on 2.10.0, as everything seemed to be working fine up to 2.9.10. According to Jackson release strategy, minor versions should not introduce any breaking change, so probably this was a bugfix or alike. However, we think this upgrade would need further and deeper review just in case.

Update

After investigating a bit, it looks like we might also have an incompatibility with com.github.briandilley.jsonrpc4j, that depends on Jackson. This library could only be updated to 1.5.3, as with latest 1.6 it was depending on Jackson's 2.10.2 and causing the same issue mentioned above even when keeping Jackson’s declared dependency at original 2.8.7

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• Tests for the changes have been added (for bug fixes / features)
• Requires Activation Code (Hard Fork)

• Other information:
  PowPeg PR: Update verification metadata in regards to lib upgrade in RSKj powpeg-node#112

fed:upgrade-jackson-and-json-rpc-libs

[illuque-iov]

Have you run the tests after the change?

This one, among other, is failing for me: co.rsk.config.RemascConfigFactoryTest#createRemascConfigInvalidConfig

It most probably will fail also in CI/CD when the Gradle verification issue is fixed.

[rmoreliovlabs]

Have you run the tests after the change?

This one, among other, is failing for me: co.rsk.config.RemascConfigFactoryTest#createRemascConfigInvalidConfig

It most probably will fail also in CI/CD when the Gradle verification issue is fixed.

The previous behavior of the library produced a nullpointer exception when you sent null to public <T> T treeToValue(TreeNode n, Class<T> valueType) in the n argument. New version's implementation checks for null and if n is null then returns null hence avoid NullPointerException to happen.

I tried to fix this issue by following the same behavior after treeToValue is executed.

[rmoreliovlabs]

pipeline:run

[illuque-iov]

As a general note, IMO the wrapper we discussed is not needed, we only call mapper.treeToValue from 3 places in the production code, I think we should deal with the new behavior from Jackson.

[illuque-iov]

IMO wrapping just to keep previous library behavior is not ideal, if Jackson changed it is probably for a reason. I would instead let each calling place decide what to do based on business rules. But no big deal, if you prefer this approach I'm ok.

But I still think we could now fix the "problem" with the NPE I've mentioned here.

[Vovchyk]

IMO wrapping just to keep previous library behavior is not ideal, if Jackson changed it is probably for a reason. I would instead let each calling place decide what to do based on business rules. But no big deal, if you prefer this approach I'm ok.

But I still think we could now fix the "problem" with the NPE I've mentioned here.

imho, I'd try to preserve compatibility with the previous versions of the api, unless there's another reason not to do it.

rewriting each place where null's may occur is error prone, so perhaps keeping this check for null in one place is simpler

[illuque-iov]

IMO wrapping just to keep previous library behavior is not ideal, if Jackson changed it is probably for a reason. I would instead let each calling place decide what to do based on business rules. But no big deal, if you prefer this approach I'm ok.
But I still think we could now fix the "problem" with the NPE I've mentioned here.

imho, I'd try to preserve compatibility with the previous versions of the api, unless there's another reason not to do it.

rewriting each place where null's may occur is error prone, so perhaps keeping this check for null in one place is simpler

Yeah, I generally agree, I explained myself wrong probably. I meant wrapping was not ideal in this scenario, where wrapped method is only being called in 3 places and where the check for null that we are doing is just throwing manually a NPE because the library was doing that before. For example, doing this is preventing us from properly handling the exception in these two places, that's why I thing it should me manually handled:

1. Not releasing content as mentioned here
2. Not returning anything to the caller as mentioned here

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

94.4% Coverage
0.0% Duplication

[Vovchyk]

pipeline:run

[rmoreliovlabs]

pipeline:run

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

94.4% Coverage
0.0% Duplication

[Vovchyk]

pipeline:run

[rmoreliovlabs]

pipeline:run

[rmoreliovlabs]

pipeline:run

[rmoreliovlabs]

pipeline:run

[Vovchyk]

pipeline:run

[rmoreliovlabs]

pipeline:run

[Vovchyk]

pipeline:run

[rmoreliovlabs]

pipeline:run

[rmoreliovlabs]

pipeline:run

[rmoreliovlabs]

pipeline:run