Remove abuse of new ObjectMapper.#8736
Conversation
Also some minor possible bugs detected by static code analyzers have been fixed
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
This is great! This makes our json config much easier to manage
Codecov Report
@@ Coverage Diff @@
## master #8736 +/- ##
=============================================
- Coverage 69.67% 28.95% -40.73%
+ Complexity 4612 45 -4567
=============================================
Files 1730 1718 -12
Lines 90312 89964 -348
Branches 13421 13386 -35
=============================================
- Hits 62928 26048 -36880
- Misses 23015 61494 +38479
+ Partials 4369 2422 -1947
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Thanks for cleaning them up! Let's try to keep one method per functionality in the JsonUtils
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
| public static final ObjectWriter DEFAULT_WRITER = DEFAULT_MAPPER.writer(); | ||
| public static final ObjectWriter DEFAULT_PRETTY_WRITER = DEFAULT_MAPPER.writerWithDefaultPrettyPrinter(); | ||
| private static final TypeReference<HashMap<String, Object>> GENERIC_JSON_TYPE = | ||
| public static final TypeReference<HashMap<String, Object>> GENERIC_JSON_TYPE = |
There was a problem hiding this comment.
Suggest not exposing this one. We may add a helper method stringToMap(String jsonString)
There was a problem hiding this comment.
Why not? I understand we should not expose the ObjectMapper because it may be mutated, but object is immutable and may be useful to have a single instance if someone has to do some advance usage of Jackson.
I mean, I would think that JsonUtils is used to avoid repetition and useless object creation, not to prevent devs to know about Jackson. The same we expose DEFAULT_WRITER, DEFAULT_PRETTY_WRITER and DEFAULT_READER, we can (and I would say should) expose GENERIC_JSON_TYPE
There was a problem hiding this comment.
We can definitely expose common type reference. My only concern about exposing this type is that we name the HashMap type as GENERIC_JSON which is a little bit confusing to me, but I guess it might be okay
There was a problem hiding this comment.
I'm renaming it to MAP_TYPE_REFERENCE, which should be more clear.
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
LGTM. Please fix the compilation error in the tests
| public static final ObjectWriter DEFAULT_WRITER = DEFAULT_MAPPER.writer(); | ||
| public static final ObjectWriter DEFAULT_PRETTY_WRITER = DEFAULT_MAPPER.writerWithDefaultPrettyPrinter(); | ||
| private static final TypeReference<HashMap<String, Object>> GENERIC_JSON_TYPE = | ||
| public static final TypeReference<HashMap<String, Object>> GENERIC_JSON_TYPE = |
There was a problem hiding this comment.
We can definitely expose common type reference. My only concern about exposing this type is that we name the HashMap type as GENERIC_JSON which is a little bit confusing to me, but I guess it might be okay
| return DEFAULT_READER.readTree(new ByteArrayInputStream(jsonBytes)); | ||
| } | ||
|
|
||
| public static Map<String, Object> jsonNodeToObject(JsonNode jsonNode) |
There was a problem hiding this comment.
Rename this to jsonNodeToMap, and remove the one using the MAPPER which has worse performance IIRC
There was a problem hiding this comment.
jsonNodeToMap already exists. Removing this method and changing its usages.
Ok, I've just understood what you indicated me to do
3 participants
Remove abuse of new ObjectMapper. Fix issue #79.
Also some minor possible bugs detected by static code analyzers have been fixed