Do not use ObjectMapper directly #21867
Conversation
It doesn't have the configuration that we need
|
|
||
| private static final int MAX_JSON_LENGTH_IN_ERROR_MESSAGE = 10_000; | ||
|
|
||
| // Note: JsonFactory is mutable, instances cannot be shared openly. | ||
| public static JsonFactory createJsonFactory() | ||
| { | ||
| return jsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build(); | ||
| return jsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build().setCodec(OBJECT_MAPPED_UNORDERED); |
There was a problem hiding this comment.
This didn't use OBJECT_MAPPED_UNORDERED before. Why this change?
| @@ -151,7 +151,7 @@ private static <T> T parseJson(JsonNode node, String jsonPointer, Class<T> javaT | |||
|
|
|||
| public static JsonFactory jsonFactory() | |||
| { | |||
| return jsonFactoryBuilder().build(); | |||
| return jsonFactoryBuilder().build().setCodec(OBJECT_MAPPER); | |||
There was a problem hiding this comment.
Does this break encapsulation of OBJECT_MAPPER?
There was a problem hiding this comment.
it indeed does:
((ObjectMapper) jsonFactory().getCodec()).configure(ORDER_MAP_ENTRIES_BY_KEYS, false)
| @@ -151,7 +151,7 @@ private static <T> T parseJson(JsonNode node, String jsonPointer, Class<T> javaT | |||
|
|
|||
| public static JsonFactory jsonFactory() | |||
| { | |||
| return jsonFactoryBuilder().build(); | |||
| return jsonFactoryBuilder().build().setCodec(OBJECT_MAPPER); | |||
There was a problem hiding this comment.
it indeed does:
((ObjectMapper) jsonFactory().getCodec()).configure(ORDER_MAP_ENTRIES_BY_KEYS, false)
|
@findepi Is first commit ok? I'll drop the second one. |
wendigo
force-pushed
the
serafin/use-object-mapper-provider
branch
from
428bf14 to
0ab046d
Compare
| @@ -56,7 +59,13 @@ public final class JsonInputFunctions | |||
| public static final String VARBINARY_UTF16_TO_JSON = "$varbinary_utf16_to_json"; | |||
| public static final String VARBINARY_UTF32_TO_JSON = "$varbinary_utf32_to_json"; | |||
|
|
|||
| private static final ObjectMapper MAPPER = new ObjectMapper(); | |||
There was a problem hiding this comment.
if new ObjectMapper() isn't supposed to be called, let's flag it out with eg modernizer
There was a problem hiding this comment.
It's called in a couple of places (i.e. tests) and in most of them it's harmless but in the engine code it should be banned because it operates on both user input and these instances live long.
There was a problem hiding this comment.
makes sense. so how do we prevent new not harmless direct uses of ObjectMapper?
|
|
||
| static { | ||
| // Changes factory. Necessary for JsonParser.readValueAsTree to work. | ||
| new ObjectMapper(JSON_FACTORY); |
There was a problem hiding this comment.
do we have to switch to json factory?
or would using new ObjectMapperProvider().get() (instead of new ObjectMapper()) be enough?
| static { | ||
| // Changes factory. Necessary for JsonParser.readValueAsTree to work. | ||
| new ObjectMapper(JSON_FACTORY); |
There was a problem hiding this comment.
the code here does not call readValueAsTree()
is the static block redundant?
|
in the commit message there is explanation:
i think a reader would naturally ask -- what was the configuration that we need that was missing. |
It doesn't have the configuration that we need
Follow-up to #21356