From d8a9aea45801223fbaa9cb9387b88e4045c12293 Mon Sep 17 00:00:00 2001 From: David Nault Date: Tue, 21 Jan 2020 13:29:34 -0800 Subject: [PATCH] JCBC-1574 JsonObject/Array.put(String,Object) fails on Map or List Backported from SDK 3 commit fb2d3845d53ce202b91ad1f886fb7c701ea1ff4a Motivation ---------- There already exist specialized methods for putting Map and List values, but they are not invoked if the exact type of the value is not known at compile time. Support passing a Map or List to JsonObject/Array.put(String,Object). This is important since these methods are used by Spring Data Couchbase. Modifications ------------- Add a JsonValue.coerce(Object) method that converts the given object to a type that can be put in a JsonArray or JsonObject. Use this method everywhere a value of unknown type is added. For consistency, convert any ClassCastExceptions to InvalidArgumentExceptions. Change-Id: Iea39648a94743c9a05020ed7e5a78ee37ae2b589 Reviewed-on: http://review.couchbase.org/120924 Tested-by: David Nault Reviewed-by: Michael Nitschinger --- .../client/java/document/json/JsonArray.java | 56 +------------- .../client/java/document/json/JsonObject.java | 72 ++++------------- .../client/java/document/json/JsonValue.java | 25 +++++- .../java/document/json/JsonArrayTest.java | 77 +++++++++---------- .../java/document/json/JsonObjectTest.java | 65 ++++++++-------- 5 files changed, 108 insertions(+), 187 deletions(-) diff --git a/src/main/java/com/couchbase/client/java/document/json/JsonArray.java b/src/main/java/com/couchbase/client/java/document/json/JsonArray.java index c621ac76..90d80cf1 100644 --- a/src/main/java/com/couchbase/client/java/document/json/JsonArray.java +++ b/src/main/java/com/couchbase/client/java/document/json/JsonArray.java @@ -24,7 +24,6 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; -import java.util.ListIterator; import java.util.Map; /** @@ -36,8 +35,6 @@ * The {@link JsonArray} is backed by a {@link List} and is intended to work similar to it API wise, but to only * allow to store such objects which can be represented by JSON. * - * @author Michael Nitschinger - * @author Simon Baslé * @since 2.0 */ public class JsonArray extends JsonValue implements Iterable, Serializable { @@ -91,11 +88,7 @@ public static JsonArray create() { public static JsonArray from(Object... items) { JsonArray array = new JsonArray(items.length); for (Object item : items) { - if (checkType(item)) { - array.add(item); - } else { - throw new IllegalArgumentException("Unsupported type for JsonArray: " + item.getClass()); - } + array.add(coerce(item)); } return array; } @@ -120,47 +113,11 @@ public static JsonArray from(Object... items) { public static JsonArray from(List items) { if (items == null) { throw new NullPointerException("Null list unsupported"); - } else if (items.isEmpty()) { - return JsonArray.empty(); } JsonArray array = new JsonArray(items.size()); - ListIterator iter = items.listIterator(); - while (iter.hasNext()) { - int i = iter.nextIndex(); - Object item = iter.next(); - - if (item == JsonValue.NULL) { - item = null; - } - - if (item instanceof Map) { - try { - JsonObject sub = JsonObject.from((Map) item); - array.add(sub); - } catch (ClassCastException e) { - throw e; - } catch (Exception e) { - ClassCastException c = new ClassCastException("Couldn't convert sub-Map at " + i + " to JsonObject"); - c.initCause(e); - throw c; - } - } else if (item instanceof List) { - try { - JsonArray sub = JsonArray.from((List) item); - array.add(sub); - } catch (Exception e) { - //no risk of a direct ClassCastException here - ClassCastException c = new ClassCastException( - "Couldn't convert sub-List at " + i + " to JsonArray"); - c.initCause(e); - throw c; - } - } else if (checkType(item)) { - array.add(item); - } else { - throw new IllegalArgumentException("Unsupported type for JsonArray: " + item.getClass()); - } + for (Object item : items) { + array.add(coerce(item)); } return array; } @@ -206,13 +163,8 @@ public Object get(int index) { public JsonArray add(Object value) { if (value == this) { throw new IllegalArgumentException("Cannot add self"); - } else if (value == JsonValue.NULL) { - addNull(); - } else if (checkType(value)) { - content.add(value); - } else { - throw new IllegalArgumentException("Unsupported type for JsonArray: " + value.getClass()); } + content.add(coerce(value)); return this; } diff --git a/src/main/java/com/couchbase/client/java/document/json/JsonObject.java b/src/main/java/com/couchbase/client/java/document/json/JsonObject.java index d8fa8d0f..2890db39 100644 --- a/src/main/java/com/couchbase/client/java/document/json/JsonObject.java +++ b/src/main/java/com/couchbase/client/java/document/json/JsonObject.java @@ -33,6 +33,8 @@ import java.util.Map; import java.util.Set; +import static java.util.Objects.requireNonNull; + /** * Represents a JSON object that can be stored and loaded from Couchbase Server. * @@ -42,8 +44,6 @@ * The {@link JsonObject} is backed by a {@link Map} and is intended to work similar to it API wise, but to only * allow to store such objects which can be represented by JSON. * - * @author Michael Nitschinger - * @author Simon Baslé * @since 2.0 */ public class JsonObject extends JsonValue implements Serializable { @@ -127,50 +127,19 @@ public static JsonObject create() { * @throws ClassCastException if map contains a sub-Map or sub-List not supported (see above) */ public static JsonObject from(Map mapData) { - if (mapData == null) { - throw new NullPointerException("Null input Map unsupported"); - } else if (mapData.isEmpty()) { - return JsonObject.empty(); - } + requireNonNull(mapData, "Null input Map unsupported"); JsonObject result = new JsonObject(mapData.size()); - for (Map.Entry entry : mapData.entrySet()) { - String key = entry.getKey(); - Object value = entry.getValue(); - - if (value == JsonValue.NULL) { - value = null; - } - - if (key == null) { - throw new NullPointerException("The key is not allowed to be null"); - } else if (value instanceof Map) { - try { - JsonObject sub = JsonObject.from((Map) value); - result.put(key, sub); - } catch (ClassCastException e) { - throw e; - } catch (Exception e) { - ClassCastException c = new ClassCastException("Couldn't convert sub-Map " + key + " to JsonObject"); - c.initCause(e); - throw c; - } - } else if (value instanceof List) { - try { - JsonArray sub = JsonArray.from((List) value); - result.put(key, sub); - } catch (Exception e) { - //no risk of a direct ClassCastException here - ClassCastException c = new ClassCastException("Couldn't convert sub-List " + key + " to JsonArray"); - c.initCause(e); - throw c; - } - } else if (!checkType(value)) { - throw new IllegalArgumentException("Unsupported type for JsonObject: " + value.getClass()); - } else { - result.put(key, value); + try { + for (Map.Entry entry : mapData.entrySet()) { + String key = requireNonNull(entry.getKey(), "The key is not allowed to be null"); + Object value = entry.getValue(); + result.put(key, coerce(value)); } + } catch (ClassCastException e) { + throw new IllegalArgumentException("Map key must be String", e); } + return result; } @@ -203,13 +172,8 @@ public static JsonObject fromJson(String s) { public JsonObject put(final String name, final Object value) { if (this == value) { throw new IllegalArgumentException("Cannot put self"); - } else if (value == JsonValue.NULL) { - putNull(name); - } else if (checkType(value)) { - content.put(name, value); - } else { - throw new IllegalArgumentException("Unsupported type for JsonObject: " + value.getClass()); } + content.put(name, coerce(value)); return this; } @@ -230,15 +194,7 @@ public JsonObject put(final String name, final Object value) { */ public JsonObject putAndEncrypt(final String name, final Object value, String providerName) { addValueEncryptionInfo(name, providerName, true); - if (this == value) { - throw new IllegalArgumentException("Cannot put self"); - } else if (value == JsonValue.NULL) { - putNull(name); - } else if (checkType(value)) { - content.put(name, value); - } else { - throw new IllegalArgumentException("Unsupported type for JsonObject: " + value.getClass()); - } + put(name, value); return this; } @@ -1243,4 +1199,4 @@ public boolean equals(Object o) { public int hashCode() { return content.hashCode(); } -} \ No newline at end of file +} diff --git a/src/main/java/com/couchbase/client/java/document/json/JsonValue.java b/src/main/java/com/couchbase/client/java/document/json/JsonValue.java index bdc40b37..63925e66 100644 --- a/src/main/java/com/couchbase/client/java/document/json/JsonValue.java +++ b/src/main/java/com/couchbase/client/java/document/json/JsonValue.java @@ -17,11 +17,12 @@ import java.math.BigDecimal; import java.math.BigInteger; +import java.util.List; +import java.util.Map; /** * Represents a JSON value (either a {@link JsonObject} or a {@link JsonArray}. * - * @author Michael Nitschinger * @since 2.0 */ public abstract class JsonValue { @@ -29,7 +30,7 @@ public abstract class JsonValue { /** * Represents a Json "null". */ - public static JsonNull NULL = JsonNull.INSTANCE; + public static final JsonNull NULL = JsonNull.INSTANCE; /** * Static factory method to create an empty {@link JsonObject}. @@ -68,4 +69,24 @@ public static boolean checkType(Object item) { || item instanceof JsonArray; } + /** + * Returns the given value converted to a type that passes the {@link #checkType} test. + * @throws IllegalArgumentException if conversion is not possible. + */ + @SuppressWarnings("unchecked") + static Object coerce(Object value) { + if (checkType(value)) { + return value; + } + if (value instanceof Map) { + return JsonObject.from((Map) value); + } + if (value instanceof List) { + return JsonArray.from((List) value); + } + if (value instanceof JsonNull) { + return null; + } + throw new IllegalArgumentException("Unsupported type for JSON value: " + value.getClass()); + } } diff --git a/src/test/java/com/couchbase/client/java/document/json/JsonArrayTest.java b/src/test/java/com/couchbase/client/java/document/json/JsonArrayTest.java index 3486ea9f..d13e5643 100644 --- a/src/test/java/com/couchbase/client/java/document/json/JsonArrayTest.java +++ b/src/test/java/com/couchbase/client/java/document/json/JsonArrayTest.java @@ -28,6 +28,8 @@ import java.util.List; import java.util.Map; +import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; @@ -36,13 +38,6 @@ import static org.junit.Assert.fail; import static org.junit.Assume.assumeFalse; -/** - * Verifies the functionality provided by a {@link JsonArray}. - * - * @author Michael Nitschinger - * @author Simon Baslé - * @since 2.0 - */ public class JsonArrayTest { @Test @@ -139,7 +134,7 @@ public void shouldDetectIncorrectItemInList() { @Test public void shouldRecursiveParseList() { - List subList = Collections.singletonList("test"); + List subList = singletonList("test"); List source = new ArrayList(2); source.add("item1"); source.add(subList); @@ -154,7 +149,7 @@ public void shouldRecursiveParseList() { @Test public void shouldRecursiveParseMap() { - Map subMap = Collections.singletonMap("test", 2.5d); + Map subMap = singletonMap("test", 2.5d); List source = new ArrayList(2); source.add("item1"); source.add(subMap); @@ -168,48 +163,36 @@ public void shouldRecursiveParseMap() { } @Test - public void shouldClassCastOnBadSubMap() { - Map badMap1 = Collections.singletonMap(1, "test"); - Map badMap2 = Collections.singletonMap("key1", (Object) new CloneNotSupportedException()); + public void shouldThrowOnBadSubMap() { + Map badMap1 = singletonMap(1, "test"); + Map badMap2 = singletonMap("key1", (Object) new CloneNotSupportedException()); - List source = Collections.singletonList(badMap1); + List source = singletonList(badMap1); try { JsonArray.from(source); - fail("ClassCastException expected"); - } catch (ClassCastException e) { - if (e.getCause() != null) { - fail("No cause expected for sub map that are not Map"); - } - } catch (Exception e) { - fail("ClassCastException expected, not " + e); + fail("IllegalArgumentException expected"); + } catch (IllegalArgumentException e) { + // expected } - source = Collections.singletonList(badMap2); + source = singletonList(badMap2); try { JsonArray.from(source); - fail("ClassCastException expected"); - } catch (ClassCastException e) { - if (!(e.getCause() instanceof IllegalArgumentException)) { - fail("ClassCastException with an IllegalArgumentException cause expected"); - } - } catch (Exception e) { - fail("ClassCastException expected"); + fail("IllegalArgumentException expected"); + } catch (IllegalArgumentException e) { + // expected } } @Test - public void shouldClassCastWithCauseOnBadSubList() { - List badSubList = Collections.singletonList(new CloneNotSupportedException()); - List source = Collections.singletonList(badSubList); + public void shouldThrowOnBadSubList() { + List badSubList = singletonList(new CloneNotSupportedException()); + List source = singletonList(badSubList); try { JsonArray.from(source); - fail("ClassCastException expected"); - } catch (ClassCastException e) { - if (!(e.getCause() instanceof IllegalArgumentException)) { - fail("ClassCastException with an IllegalArgumentException cause expected"); - } - } catch (Exception e) { - fail("ClassCastException expected"); + fail("IllegalArgumentException expected"); + } catch (IllegalArgumentException e) { + // expected } } @@ -219,9 +202,9 @@ public void shouldTreatJsonValueNullConstantAsNull() { JsonArray arr = JsonArray.create(); arr.add(JsonValue.NULL); arr.add(JsonObject.from( - Collections.singletonMap("subNull", JsonValue.NULL))); + singletonMap("subNull", JsonValue.NULL))); arr.add(JsonArray.from( - Collections.singletonList(JsonValue.NULL))); + singletonList(JsonValue.NULL))); assertEquals(3, arr.size()); assertNull(arr.get(0)); @@ -399,4 +382,18 @@ public void shouldSupportBigDecimalConverted() throws Exception { assertEquals(1234.567890123457, decoded.getDouble(0), 0); assertTrue(decoded.getNumber(0) instanceof Double); } + + @Test + public void canPutWhenTypeIsUnknown() { + Object map = singletonMap("one", 1); + Object list = singletonList("red"); + JsonArray json = JsonArray.create() + .add(map) + .add(list); + + assertEquals(JsonArray.from(map, list), json); + assertEquals(JsonObject.from(singletonMap("one", 1)), json.getObject(0)); + assertEquals(JsonArray.from(singletonList("red")), json.getArray(1)); + } + } diff --git a/src/test/java/com/couchbase/client/java/document/json/JsonObjectTest.java b/src/test/java/com/couchbase/client/java/document/json/JsonObjectTest.java index 5f3f5389..d315686a 100644 --- a/src/test/java/com/couchbase/client/java/document/json/JsonObjectTest.java +++ b/src/test/java/com/couchbase/client/java/document/json/JsonObjectTest.java @@ -27,6 +27,8 @@ import java.util.List; import java.util.Map; +import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; @@ -35,13 +37,6 @@ import static org.junit.Assert.fail; import static org.junit.Assume.assumeFalse; -/** - * Verifies the functionality provided by a {@link JsonObject}. - * - * @author Michael Nitschinger - * @author Simon Baslé - * @since 2.0 - */ public class JsonObjectTest { @Test @@ -257,50 +252,38 @@ public void shouldRecursivelyParseLists() { } @Test - public void shouldClassCastOnBadSubMap() { - Map badMap1 = Collections.singletonMap(1, "test"); - Map badMap2 = Collections.singletonMap("key1", (Object) new CloneNotSupportedException()); + public void shouldThrowOnBadSubMap() { + Map badMap1 = singletonMap(1, "test"); + Map badMap2 = singletonMap("key1", (Object) new CloneNotSupportedException()); Map sourceMap = new HashMap(); sourceMap.put("subMap", badMap1); try { JsonObject.from(sourceMap); - fail("ClassCastException expected"); - } catch (ClassCastException e) { - if (e.getCause() != null) { - fail("No cause expected for sub map that are not Map"); - } - } catch (Exception e) { - fail("ClassCastException expected, not " + e); + fail("IllegalArgumentException expected"); + } catch (IllegalArgumentException e) { + // expected } sourceMap.clear(); sourceMap.put("subMap", badMap2); try { JsonObject.from(sourceMap); - fail("ClassCastException expected"); - } catch (ClassCastException e) { - if (!(e.getCause() instanceof IllegalArgumentException)) { - fail("ClassCastException with an IllegalArgumentException cause expected"); - } - } catch (Exception e) { - fail("ClassCastException expected"); + fail("IllegalArgumentException expected"); + } catch (IllegalArgumentException e) { + // expected } } @Test - public void shouldClassCastWithCauseOnBadSubList() { + public void shouldThrowOnBadSubList() { List badSubList = Collections.singletonList(new CloneNotSupportedException()); - Map source = Collections.singletonMap("test", badSubList); + Map source = singletonMap("test", badSubList); try { JsonObject.from(source); - fail("ClassCastException expected"); - } catch (ClassCastException e) { - if (!(e.getCause() instanceof IllegalArgumentException)) { - fail("ClassCastException with an IllegalArgumentException cause expected"); - } - } catch (Exception e) { - fail("ClassCastException expected"); + fail("IllegalArgumentException expected"); + } catch (IllegalArgumentException e) { + // expected } } @@ -309,7 +292,7 @@ public void shouldTreatJsonValueNullConstantAsNull() { JsonObject obj = JsonObject.create(); obj.put("directNull", JsonValue.NULL); obj.put("subMapWithNull", JsonObject.from( - Collections.singletonMap("subNull", JsonValue.NULL))); + singletonMap("subNull", JsonValue.NULL))); obj.put("subArrayWithNull", JsonArray.from( Collections.singletonList(JsonValue.NULL))); @@ -485,4 +468,16 @@ public void shouldSupportBigDecimalConverted() throws Exception { assertTrue(decoded.getNumber("value") instanceof Double); } -} \ No newline at end of file + @Test + public void canPutWhenTypeIsUnknown() { + Object map = singletonMap("one", 1); + Object list = singletonList("red"); + JsonObject json = JsonObject.create() + .put("map", map) + .put("list", list); + + assertEquals(JsonObject.from(singletonMap("one", 1)), json.getObject("map")); + assertEquals(JsonArray.from(singletonList("red")), json.getArray("list")); + } + +}