Replace the fastjson of the ArcticMetaStore class in ams-server with jackson #1392
Conversation
github-actions
bot
added
module:ams-server
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #1392 +/- ##
============================================
- Coverage 44.01% 43.82% -0.19%
+ Complexity 541 539 -2
============================================
Files 46 46
Lines 4276 4276
Branches 502 502
============================================
- Hits 1882 1874 -8
- Misses 2192 2206 +14
+ Partials 202 196 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| public static Boolean getBoolean(JsonNode jsonNode, String fieldName){ | ||
| if(jsonNode == null){ | ||
| throw new NullPointerException(); |
There was a problem hiding this comment.
You can use Preconditions.checkNotNull
| if(jsonNode == null){ | ||
| throw new NullPointerException(); | ||
| } | ||
| if(jsonNode.get(fieldName) == null){ |
There was a problem hiding this comment.
We should create a local variable like this: JsonNode jsonNode = jsonNode.get(fieldName), because the jsonNode.get(fieldName) method is being called twice.
| for(JsonNode jsonNode : catalogs){ | ||
| System.out.println(jsonNode.get("name").asText()); | ||
| } | ||
| System.out.println(systemConfig.has("arctic.ams.expire.thread.pool-size")); |
There was a problem hiding this comment.
We cannot simply output at this point, we need to assert the correctness of the value. You can refer to other unit tests for how to do this.
|
|
||
| @Test | ||
| public void jacksonTest(){ | ||
| String configPath = System.getProperty("user.dir") + "/src/test/resources/test.yaml"; |
There was a problem hiding this comment.
You can find this file from classpath
|
@329724814 Thank you for your outstanding work！ Since the PR mainly involves changes to the AMS module, I suggest waiting for #1372 to be resolved before merging this PR, otherwise, there may be many conflicts. |
|
Cloud you please to handle the merge conflict with master branch? @329724814 |
Why are the changes needed?
There are many high-risk vulnerabilities in the previous version of fastjosn,We need to replace fastjson with jackson
Brief change log
Replace the fastjson of the ArcticMetaStore class in ams-server with jackson,
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation