Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disallow JsonObject Entry.setValue(null) #2167

Merged
merged 2 commits into from Aug 18, 2022
Merged

Disallow JsonObject Entry.setValue(null) #2167

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3df6bc7
Disallow JsonObject Entry.setValue(null)
Marcono1234 Aug 3, 2022
53ca5ba
Adjust comments in JsonObjectTest
Marcono1234 Aug 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions gson/src/main/java/com/google/gson/JsonObject.java
View file
Open in desktop
Expand Up @@ -17,7 +17,6 @@
package com.google.gson;

import com.google.gson.internal.LinkedTreeMap;

import java.util.Map;
import java.util.Set;

Expand All @@ -30,7 +29,7 @@
* @author Joel Leitch
*/
public final class JsonObject extends JsonElement {
private final LinkedTreeMap<String, JsonElement> members = new LinkedTreeMap<>();
private final LinkedTreeMap<String, JsonElement> members = new LinkedTreeMap<>(false);

/**
* Creates a deep copy of this element and all its children
Expand Down
42 changes: 33 additions & 9 deletions gson/src/main/java/com/google/gson/internal/LinkedTreeMap.java
View file
Open in desktop
Expand Up @@ -46,21 +46,33 @@ public final class LinkedTreeMap<K, V> extends AbstractMap<K, V> implements Seri
}
};

Comparator<? super K> comparator;
private final Comparator<? super K> comparator;
private final boolean allowNullValues;
Node<K, V> root;
int size = 0;
int modCount = 0;

// Used to preserve iteration order
final Node<K, V> header = new Node<>();
final Node<K, V> header;

/**
* Create a natural order, empty tree map whose keys must be mutually
* comparable and non-null.
* comparable and non-null, and whose values can be {@code null}.
*/
@SuppressWarnings("unchecked") // unsafe! this assumes K is comparable
public LinkedTreeMap() {
this((Comparator<? super K>) NATURAL_ORDER);
this((Comparator<? super K>) NATURAL_ORDER, true);
}

/**
* Create a natural order, empty tree map whose keys must be mutually
* comparable and non-null.
*
* @param allowNullValues whether {@code null} is allowed as entry value
*/
@SuppressWarnings("unchecked") // unsafe! this assumes K is comparable
public LinkedTreeMap(boolean allowNullValues) {
this((Comparator<? super K>) NATURAL_ORDER, allowNullValues);
}

/**
Expand All @@ -69,12 +81,15 @@ public LinkedTreeMap() {
*
* @param comparator the comparator to order elements with, or {@code null} to
* use the natural ordering.
* @param allowNullValues whether {@code null} is allowed as entry value
*/
@SuppressWarnings({ "unchecked", "rawtypes" }) // unsafe! if comparator is null, this assumes K is comparable
public LinkedTreeMap(Comparator<? super K> comparator) {
public LinkedTreeMap(Comparator<? super K> comparator, boolean allowNullValues) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an incompatible change, for client code that might be calling new LinkedTreeMap<>(comparator) directly. Of course it's an .internal. class, so code shouldn't be doing that. I find that Google's codebase has a few places where people are constructing LinkedTreeMap, but none where they are calling the (Comparator) constructor. So I think we could actually just delete that constructor and hardcode the use of natural ordering. That would allow considerable simplification of the find method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like other projects are using LinkedTreeMap as well, though they don't seem to use that constructor either. Should I add it back nonetheless?

So I think we could actually just delete that constructor and hardcode the use of natural ordering. That would allow considerable simplification of the find method.

Sounds in general reasonable to me, but I am not sure whether that should be part of this pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, we can do that as a follow-on.

this.comparator = comparator != null
? comparator
: (Comparator) NATURAL_ORDER;
this.allowNullValues = allowNullValues;
this.header = new Node<>(allowNullValues);
}

@Override public int size() {
Expand All @@ -94,6 +109,9 @@ public LinkedTreeMap(Comparator<? super K> comparator) {
if (key == null) {
throw new NullPointerException("key == null");
}
if (value == null && !allowNullValues) {
throw new NullPointerException("value == null");
}
Node<K, V> created = find(key, true);
V result = created.value;
created.value = value;
Expand Down Expand Up @@ -166,10 +184,10 @@ Node<K, V> find(K key, boolean create) {
if (comparator == NATURAL_ORDER && !(key instanceof Comparable)) {
throw new ClassCastException(key.getClass().getName() + " is not Comparable");
}
created = new Node<>(nearest, key, header, header.prev);
created = new Node<>(allowNullValues, nearest, key, header, header.prev);
root = created;
} else {
created = new Node<>(nearest, key, header, header.prev);
created = new Node<>(allowNullValues, nearest, key, header, header.prev);
if (comparison < 0) { // nearest.key is higher
nearest.left = created;
} else { // comparison > 0, nearest.key is lower
Expand Down Expand Up @@ -446,19 +464,22 @@ static final class Node<K, V> implements Entry<K, V> {
Node<K, V> next;
Node<K, V> prev;
final K key;
final boolean allowNullValue;
V value;
int height;

/** Create the header entry */
Node() {
Node(boolean allowNullValue) {
key = null;
this.allowNullValue = allowNullValue;
next = prev = this;
}

/** Create a regular entry */
Node(Node<K, V> parent, K key, Node<K, V> next, Node<K, V> prev) {
Node(boolean allowNullValue, Node<K, V> parent, K key, Node<K, V> next, Node<K, V> prev) {
this.parent = parent;
this.key = key;
this.allowNullValue = allowNullValue;
this.height = 1;
this.next = next;
this.prev = prev;
Expand All @@ -475,6 +496,9 @@ static final class Node<K, V> implements Entry<K, V> {
}

@Override public V setValue(V value) {
if (value == null && !allowNullValue) {
throw new NullPointerException("value == null");
}
V oldValue = this.value;
this.value = value;
return oldValue;
Expand Down
99 changes: 98 additions & 1 deletion gson/src/test/java/com/google/gson/JsonObjectTest.java
View file
Open in desktop
Expand Up @@ -17,7 +17,16 @@
package com.google.gson;

import com.google.gson.common.MoreAsserts;

import java.util.AbstractMap.SimpleEntry;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Deque;
import java.util.Iterator;
import java.util.List;
import java.util.Map.Entry;
import java.util.Set;
import junit.framework.TestCase;

/**
Expand Down Expand Up @@ -192,6 +201,7 @@ public void testDeepCopy() {
*/
public void testKeySet() {
JsonObject a = new JsonObject();
assertEquals(0, a.keySet().size());

a.add("foo", new JsonArray());
a.add("bar", new JsonObject());
Expand All @@ -200,5 +210,92 @@ public void testKeySet() {
assertEquals(2, a.keySet().size());
assertTrue(a.keySet().contains("foo"));
assertTrue(a.keySet().contains("bar"));

a.addProperty("1", true);
a.addProperty("2", false);

// Insertion order should be preserved by keySet()
Deque<String> expectedKeys = new ArrayDeque<>(Arrays.asList("foo", "bar", "1", "2"));
assertEquals(new ArrayList<>(expectedKeys), new ArrayList<>(a.keySet()));
Iterator<String> iterator = a.keySet().iterator();

// Remove keys one by one
for (int i = a.size(); i >= 1; i--) {
assertTrue(iterator.hasNext());
assertEquals(expectedKeys.getFirst(), iterator.next());
iterator.remove();
expectedKeys.removeFirst();

assertEquals(i - 1, a.size());
// Note: Must wrap in ArrayList because Deque implementations do not implement `equals`
Marcono1234 marked this conversation as resolved.
Show resolved Hide resolved
assertEquals(new ArrayList<>(expectedKeys), new ArrayList<>(a.keySet()));
}
}

public void testEntrySet() {
JsonObject o = new JsonObject();
assertEquals(0, o.entrySet().size());

o.addProperty("b", true);
Set<?> expectedEntries = Collections.singleton(new SimpleEntry<>("b", new JsonPrimitive(true)));
assertEquals(expectedEntries, o.entrySet());
assertEquals(1, o.entrySet().size());

o.addProperty("a", false);
// Insertion order should be preserved by entrySet()
List<?> expectedEntriesList = Arrays.asList(
new SimpleEntry<>("b", new JsonPrimitive(true)),
new SimpleEntry<>("a", new JsonPrimitive(false))
);
assertEquals(expectedEntriesList, new ArrayList<>(o.entrySet()));

Iterator<Entry<String, JsonElement>> iterator = o.entrySet().iterator();
// Test behavior of Entry.setValue
for (int i = 0; i < o.size(); i++) {
Entry<String, JsonElement> entry = iterator.next();
entry.setValue(new JsonPrimitive(i));

assertEquals(new JsonPrimitive(i), entry.getValue());
}

expectedEntriesList = Arrays.asList(
new SimpleEntry<>("b", new JsonPrimitive(0)),
new SimpleEntry<>("a", new JsonPrimitive(1))
);
assertEquals(expectedEntriesList, new ArrayList<>(o.entrySet()));

Entry<String, JsonElement> entry = o.entrySet().iterator().next();
try {
// null value is not permitted, only JsonNull is supported
entry.setValue(null);
fail();
} catch (NullPointerException e) {
assertEquals("value == null", e.getMessage());
}
assertNotNull(entry.getValue());

o.addProperty("key1", 1);
o.addProperty("key2", 2);

Deque<?> expectedEntriesQueue = new ArrayDeque<>(Arrays.asList(
new SimpleEntry<>("b", new JsonPrimitive(0)),
new SimpleEntry<>("a", new JsonPrimitive(1)),
new SimpleEntry<>("key1", new JsonPrimitive(1)),
new SimpleEntry<>("key2", new JsonPrimitive(2))
));
assertEquals(new ArrayList<>(expectedEntriesQueue), new ArrayList<>(o.entrySet()));
iterator = o.entrySet().iterator();

// Remove entries one by one
for (int i = o.size(); i >= 1; i--) {
assertTrue(iterator.hasNext());
assertEquals(expectedEntriesQueue.getFirst(), iterator.next());
iterator.remove();
expectedEntriesQueue.removeFirst();

assertEquals(i - 1, o.size());
// Note: Must wrap in ArrayList because Deque implementations do not implement `equals`
assertEquals(new ArrayList<>(expectedEntriesQueue), new ArrayList<>(o.entrySet()));
}
}
}
59 changes: 56 additions & 3 deletions gson/src/test/java/com/google/gson/internal/LinkedTreeMapTest.java
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@

package com.google.gson.internal;

import com.google.gson.common.MoreAsserts;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand All @@ -26,12 +27,10 @@
import java.util.Collections;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Random;

import junit.framework.TestCase;

import com.google.gson.common.MoreAsserts;

public final class LinkedTreeMapTest extends TestCase {

public void testIterationOrder() {
Expand Down Expand Up @@ -73,6 +72,59 @@ public void testPutNonComparableKeyFails() {
} catch (ClassCastException expected) {}
}

public void testPutNullValue() {
LinkedTreeMap<String, String> map = new LinkedTreeMap<>();
map.put("a", null);
assertEquals(1, map.size());
assertTrue(map.containsKey("a"));
assertTrue(map.containsValue(null));
assertNull(map.get("a"));
}

public void testPutNullValue_Forbidden() {
LinkedTreeMap<String, String> map = new LinkedTreeMap<>(false);
try {
map.put("a", null);
fail();
} catch (NullPointerException e) {
assertEquals("value == null", e.getMessage());
}
assertEquals(0, map.size());
assertFalse(map.containsKey("a"));
assertFalse(map.containsValue(null));
}

public void testEntrySetValueNull() {
LinkedTreeMap<String, String> map = new LinkedTreeMap<>();
map.put("a", "1");
assertEquals("1", map.get("a"));
Entry<String, String> entry = map.entrySet().iterator().next();
assertEquals("a", entry.getKey());
assertEquals("1", entry.getValue());
entry.setValue(null);
assertNull(entry.getValue());

assertTrue(map.containsKey("a"));
assertTrue(map.containsValue(null));
assertNull(map.get("a"));
}


public void testEntrySetValueNull_Forbidden() {
LinkedTreeMap<String, String> map = new LinkedTreeMap<>(false);
map.put("a", "1");
Entry<String, String> entry = map.entrySet().iterator().next();
try {
entry.setValue(null);
fail();
} catch (NullPointerException e) {
assertEquals("value == null", e.getMessage());
}
assertEquals("1", entry.getValue());
assertEquals("1", map.get("a"));
assertFalse(map.containsValue(null));
}

public void testContainsNonComparableKeyReturnsFalse() {
LinkedTreeMap<String, String> map = new LinkedTreeMap<>();
map.put("a", "android");
Expand All @@ -81,6 +133,7 @@ public void testContainsNonComparableKeyReturnsFalse() {

public void testContainsNullKeyIsAlwaysFalse() {
LinkedTreeMap<String, String> map = new LinkedTreeMap<>();
assertFalse(map.containsKey(null));
map.put("a", "android");
assertFalse(map.containsKey(null));
}
Expand Down