Use LinkedHashMap as the default Map.#3057
Conversation
The JDK and Android have had DoS protection at least as good as LinkedTreeMap for decades now.
Marcono1234
left a comment
Marcono1234
left a comment
There was a problem hiding this comment.
Thanks a lot for addressing this!
Maybe some of the tests need some further adjustments to avoid confusion, see my review comments. What do you think?
| // For all Map types with non-String key, should use JDK LinkedHashMap by default | ||
| // This is also done to avoid ClassCastException later, because Gson's LinkedTreeMap requires | ||
| // that keys are Comparable |
There was a problem hiding this comment.
Maybe this comment is overly specific or a bit misleading now, given that LinkedHashMap is always used?
| @@ -158,15 +158,12 @@ | |||
|
|
|||
| @Test | |||
| public void testStringMapCreation() { | |||
There was a problem hiding this comment.
Maybe rename this test method as well to make it more generic?
| public void testStringMapCreation() { | |
| public void testMapCreation() { |
Previously the test was specific to how Map<String, ...> was handled, but now with all the comments removed the test method name might be a bit misleading.
| @Test | ||
| public void testMapStringSupertypeKeyDeserialization() { | ||
| // Should only use Gson's LinkedTreeMap for String as key, but not for supertypes (e.g. Object) | ||
| Type typeOfMap = new TypeToken<Map<Object, Integer>>() {}.getType(); | ||
| Map<?, ?> map = gson.fromJson("{\"a\":1}", typeOfMap); | ||
|
|
||
| assertWithMessage("Map<Object, ...> should not use Gson Map implementation") | ||
| .that(map) | ||
| .isNotInstanceOf(LinkedTreeMap.class); | ||
|
|
||
| Map<?, ?> expectedMap = Collections.singletonMap("a", 1); | ||
| assertThat(map).isEqualTo(expectedMap); | ||
| } | ||
|
|
||
| @Test | ||
| public void testMapNonStringKeyDeserialization() { | ||
| Type typeOfMap = new TypeToken<Map<Integer, Integer>>() {}.getType(); | ||
| Map<?, ?> map = gson.fromJson("{\"1\":1}", typeOfMap); | ||
|
|
||
| assertWithMessage("Map<Integer, ...> should not use Gson Map implementation") | ||
| .that(map) | ||
| .isNotInstanceOf(LinkedTreeMap.class); | ||
|
|
||
| Map<?, ?> expectedMap = Collections.singletonMap(1, 1); | ||
| assertThat(map).isEqualTo(expectedMap); | ||
| } |
There was a problem hiding this comment.
Might be good to remove or adjust these test methods (testMapStringSupertypeKeyDeserialization and testMapNonStringKeyDeserialization) as well, since they might be a bit misleading now?
2 participants
The JDK and Android have had DoS protection at least as good as LinkedTreeMap for decades now.
Fixes #3037. Fixes #2737. Fixes #1247.