From f37c2d67c57f1217b0c42b29d636da0b2e750bfa Mon Sep 17 00:00:00 2001 From: "John J. Aylward" Date: Mon, 27 Jul 2020 11:39:55 -0400 Subject: [PATCH 1/6] Update for JSONArray.putAll methods * Adds a copy constructor for JSONArray * Updates the JSONArray.addAll(Object) method to be more lenient * Adds support for JSONArray.putAll of generic Iterables * Adds support for JSONArray.putAll of another JSONArray --- src/main/java/org/json/JSONArray.java | 85 ++++++++++++++++++- .../java/org/json/junit/JSONArrayTest.java | 71 +++++++++++++++- .../java/org/json/junit/JSONObjectTest.java | 7 ++ 3 files changed, 158 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/json/JSONArray.java b/src/main/java/org/json/JSONArray.java index 3082974a4..547bf9c0a 100644 --- a/src/main/java/org/json/JSONArray.java +++ b/src/main/java/org/json/JSONArray.java @@ -177,6 +177,35 @@ public JSONArray(Collection collection) { } } + /** + * Construct a JSONArray from an Iterable. This is a shallow copy. + * + * @param iter + * A Iterable collection. + */ + public JSONArray(Iterable iter) { + this(); + if (iter == null) { + return; + } + this.addAll(iter); + } + + /** + * Construct a JSONArray from another JSONArray. This is a shallow copy. + * + * @param collection + * A Collection. + */ + public JSONArray(JSONArray array) { + if (array == null) { + this.myArrayList = new ArrayList(); + } else { + this.myArrayList = new ArrayList(array.length()); + this.addAll(array.myArrayList); + } + } + /** * Construct a JSONArray from an array. * @@ -191,6 +220,10 @@ public JSONArray(Collection collection) { */ public JSONArray(Object array) throws JSONException { this(); + if (!array.getClass().isArray()) { + throw new JSONException( + "JSONArray initial value should be a string or collection or array."); + } this.addAll(array); } @@ -210,6 +243,11 @@ public JSONArray(int initialCapacity) throws JSONException { this.myArrayList = new ArrayList(initialCapacity); } + @Override + protected Object clone() { + return new JSONArray(this.myArrayList); + } + @Override public Iterator iterator() { return this.myArrayList.iterator(); @@ -1165,7 +1203,7 @@ public JSONArray put(int index, Object value) throws JSONException { } /** - * Put or replace a collection's elements in the JSONArray. + * Put a collection's elements in to the JSONArray. * * @param collection * A Collection. @@ -1175,9 +1213,33 @@ public JSONArray putAll(Collection collection) { this.addAll(collection); return this; } + + /** + * Put an Iterable's elements in to the JSONArray. + * + * @param iter + * A Collection. + * @return this. + */ + public JSONArray putAll(Iterable iter) { + this.addAll(iter); + return this; + } /** - * Put or replace an array's elements in the JSONArray. + * Put a JSONArray's elements in to the JSONArray. + * + * @param array + * A JSONArray. + * @return this. + */ + public JSONArray putAll(JSONArray array) { + this.addAll(array.myArrayList); + return this; + } + + /** + * Put an array's elements in to the JSONArray. * * @param array * Array. If the parameter passed is null, or not an array, an @@ -1520,7 +1582,6 @@ public boolean isEmpty() { return this.myArrayList.isEmpty(); } - /** * Add a collection's elements to the JSONArray. * @@ -1534,6 +1595,18 @@ private void addAll(Collection collection) { } } + /** + * Add an Iterable's elements to the JSONArray. + * + * @param iter + * An Iterable. + */ + private void addAll(Iterable iter) { + for (Object o: iter){ + this.myArrayList.add(JSONObject.wrap(o)); + } + } + /** * Add an array's elements to the JSONArray. * @@ -1553,6 +1626,12 @@ private void addAll(Object array) throws JSONException { for (int i = 0; i < length; i += 1) { this.put(JSONObject.wrap(Array.get(array, i))); } + } else if (array instanceof JSONArray) { + this.addAll(((JSONArray)array).myArrayList); + } else if (array instanceof Collection) { + this.addAll((Collection)array); + } else if (array instanceof Iterable) { + this.addAll((Iterable)array); } else { throw new JSONException( "JSONArray initial value should be a string or collection or array."); diff --git a/src/test/java/org/json/junit/JSONArrayTest.java b/src/test/java/org/json/junit/JSONArrayTest.java index 4a322c135..7673157ed 100644 --- a/src/test/java/org/json/junit/JSONArrayTest.java +++ b/src/test/java/org/json/junit/JSONArrayTest.java @@ -979,9 +979,9 @@ public void write() throws IOException { JSONArray jsonArray = new JSONArray(str); String expectedStr = str; StringWriter stringWriter = new StringWriter(); - jsonArray.write(stringWriter); - String actualStr = stringWriter.toString(); try { + jsonArray.write(stringWriter); + String actualStr = stringWriter.toString(); JSONArray finalArray = new JSONArray(actualStr); Util.compareActualVsExpectedJsonArrays(jsonArray, finalArray); assertTrue("write() expected " + expectedStr + @@ -1187,4 +1187,71 @@ public void testJSONArrayInt() { e.getMessage()); } } + + /** + * Verifies that the object constructor can properly handle any supported collection object. + */ + @Test + @SuppressWarnings({ "unchecked", "boxing" }) + public void testObjectConstructor() { + // should copy the array + Object o = new Object[] {2, "test2", true}; + JSONArray a = new JSONArray(o); + assertNotNull("Should not error", a); + assertEquals("length", 3, a.length()); + + // should NOT copy the collection + // this is required for backwards compatibility + o = new ArrayList(); + ((Collection)o).add(1); + ((Collection)o).add("test"); + ((Collection)o).add(false); + try { + a = new JSONArray(o); + assertNull("Should error", a); + } catch (JSONException ex) { + } + + // should NOT copy the JSONArray + // this is required for backwards compatibility + o = a; + try { + a = new JSONArray(o); + assertNull("Should error", a); + } catch (JSONException ex) { + } + } + + /** + * Verifies that the JSONArray constructor properly copies the original. + */ + @Test + public void testJSONArrayConstructor() { + // should copy the array + JSONArray a1 = new JSONArray("[2, \"test2\", true]"); + JSONArray a2 = new JSONArray(a1); + assertNotNull("Should not error", a2); + assertEquals("length", a1.length(), a2.length()); + + for(int i = 0; i < a1.length(); i++) { + assertEquals("index " + i + " are equal", a1.get(i), a2.get(i)); + } + } + + /** + * Verifies that the object constructor can properly handle any supported collection object. + */ + @Test + public void testJSONArrayPutAll() { + // should copy the array + JSONArray a1 = new JSONArray("[2, \"test2\", true]"); + JSONArray a2 = new JSONArray(); + a2.putAll(a1); + assertNotNull("Should not error", a2); + assertEquals("length", a1.length(), a2.length()); + + for(int i = 0; i < a1.length(); i++) { + assertEquals("index " + i + " are equal", a1.get(i), a2.get(i)); + } + } } diff --git a/src/test/java/org/json/junit/JSONObjectTest.java b/src/test/java/org/json/junit/JSONObjectTest.java index 2b4321261..51407f05e 100644 --- a/src/test/java/org/json/junit/JSONObjectTest.java +++ b/src/test/java/org/json/junit/JSONObjectTest.java @@ -3201,4 +3201,11 @@ public void testPutNullObject() { fail("Expected an exception"); } + @Test + public void testIssue548ObjectWithEmptyJsonArray() { + JSONObject jsonObject = new JSONObject("{\"empty_json_array\": []}"); + assertTrue("missing expected key 'empty_json_array'", jsonObject.has("empty_json_array")); + assertNotNull("'empty_json_array' should be an array", jsonObject.getJSONArray("empty_json_array")); + assertEquals("'empty_json_array' should have a length of 0", 0, jsonObject.getJSONArray("empty_json_array").length()); + } } From 3c9573cc3d3182ac5210c8337ac67cbca60a99c0 Mon Sep 17 00:00:00 2001 From: "John J. Aylward" Date: Mon, 27 Jul 2020 11:54:20 -0400 Subject: [PATCH 2/6] update some javadoc --- src/main/java/org/json/JSONArray.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/json/JSONArray.java b/src/main/java/org/json/JSONArray.java index 547bf9c0a..684fb3780 100644 --- a/src/main/java/org/json/JSONArray.java +++ b/src/main/java/org/json/JSONArray.java @@ -1242,12 +1242,12 @@ public JSONArray putAll(JSONArray array) { * Put an array's elements in to the JSONArray. * * @param array - * Array. If the parameter passed is null, or not an array, an + * Array. If the parameter passed is null, or not an array or Iterable, an * exception will be thrown. * @return this. * * @throws JSONException - * If not an array or if an array value is non-finite number. + * If not an array, JSONArray, Iterable or if an value is non-finite number. * @throws NullPointerException * Thrown if the array parameter is null. */ From c175a9eb622929c6a83b5bd3a8d54283ae5e5a32 Mon Sep 17 00:00:00 2001 From: "John J. Aylward" Date: Wed, 29 Jul 2020 15:30:02 -0400 Subject: [PATCH 3/6] remove clone --- src/main/java/org/json/JSONArray.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/main/java/org/json/JSONArray.java b/src/main/java/org/json/JSONArray.java index 684fb3780..7cba25592 100644 --- a/src/main/java/org/json/JSONArray.java +++ b/src/main/java/org/json/JSONArray.java @@ -243,11 +243,6 @@ public JSONArray(int initialCapacity) throws JSONException { this.myArrayList = new ArrayList(initialCapacity); } - @Override - protected Object clone() { - return new JSONArray(this.myArrayList); - } - @Override public Iterator iterator() { return this.myArrayList.iterator(); From 5d828d2c0b462dc22b9414a5d27067b4bd40aa24 Mon Sep 17 00:00:00 2001 From: "John J. Aylward" Date: Wed, 29 Jul 2020 16:13:54 -0400 Subject: [PATCH 4/6] update comments and increase speed of merging JSONArray objects --- src/main/java/org/json/JSONArray.java | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/json/JSONArray.java b/src/main/java/org/json/JSONArray.java index 7cba25592..f034499db 100644 --- a/src/main/java/org/json/JSONArray.java +++ b/src/main/java/org/json/JSONArray.java @@ -194,15 +194,16 @@ public JSONArray(Iterable iter) { /** * Construct a JSONArray from another JSONArray. This is a shallow copy. * - * @param collection - * A Collection. + * @param array + * A array. */ public JSONArray(JSONArray array) { if (array == null) { this.myArrayList = new ArrayList(); } else { - this.myArrayList = new ArrayList(array.length()); - this.addAll(array.myArrayList); + // shallow copy directly the internal array lists as any wrapping + // should have been done already in the original JSONArray + this.myArrayList = new ArrayList(array.myArrayList); } } @@ -1213,7 +1214,7 @@ public JSONArray putAll(Collection collection) { * Put an Iterable's elements in to the JSONArray. * * @param iter - * A Collection. + * An Iterable. * @return this. */ public JSONArray putAll(Iterable iter) { @@ -1229,7 +1230,9 @@ public JSONArray putAll(Iterable iter) { * @return this. */ public JSONArray putAll(JSONArray array) { - this.addAll(array.myArrayList); + // directly copy the elements from the source array to this one + // as all wrapping should have been done already in the source. + this.myArrayList.addAll(array.myArrayList); return this; } @@ -1606,8 +1609,9 @@ private void addAll(Iterable iter) { * Add an array's elements to the JSONArray. * * @param array - * Array. If the parameter passed is null, or not an array, an - * exception will be thrown. + * Array. If the parameter passed is null, or not an array, + * JSONArray, Collection, or Iterable, an exception will be + * thrown. * * @throws JSONException * If not an array or if an array value is non-finite number. @@ -1622,7 +1626,10 @@ private void addAll(Object array) throws JSONException { this.put(JSONObject.wrap(Array.get(array, i))); } } else if (array instanceof JSONArray) { - this.addAll(((JSONArray)array).myArrayList); + // use the built in array list `addAll` as all object + // wrapping should have been completed in the original + // JSONArray + this.myArrayList.addAll(((JSONArray)array).myArrayList); } else if (array instanceof Collection) { this.addAll((Collection)array); } else if (array instanceof Iterable) { From f35194bc1de99846ba1f86ebf0f55867fe838527 Mon Sep 17 00:00:00 2001 From: "John J. Aylward" Date: Thu, 30 Jul 2020 09:51:24 -0400 Subject: [PATCH 5/6] Updates the `addAll` methods to have optional wrapping. When called from the constructor, the individual items in the collection/array are wrapped as done originally before the `putAll` methods were added. However this commit changes how `putAll` works. The items are no longer wrapped in order to keep consistency with the other `put` methods. However this can lead to inconsistencies with expectations. For example code like this will create a mixed JSONArray, some items wrapped, others not: ```java SomeBean[] myArr = new SomeBean[]{ new SomeBean(1), new SomeBean(2) }; JSONArray jArr = new JSONArray(myArr); // these will be wrapped // these will not be wrapped jArr.putAll(new SomeBean[]{ new SomeBean(3), new SomeBean(4) }); ``` For consistency, it would be recommended that the above code is changed to look like 1 of 2 ways. Option 1: ```Java SomeBean[] myArr = new SomeBean[]{ new SomeBean(1), new SomeBean(2) }; JSONArray jArr = new JSONArray(); jArr.putAll(myArr); // will not be wrapped // these will not be wrapped jArr.putAll(new SomeBean[]{ new SomeBean(3), new SomeBean(4) }); // our jArr is now consistent. ``` Option 2: ```Java SomeBean[] myArr = new SomeBean[]{ new SomeBean(1), new SomeBean(2) }; JSONArray jArr = new JSONArray(myArr); // these will be wrapped // these will be wrapped jArr.putAll(new JSONArray(new SomeBean[]{ new SomeBean(3), new SomeBean(4) })); // our jArr is now consistent. ``` --- src/main/java/org/json/JSONArray.java | 64 +++++++++++++++++++-------- 1 file changed, 46 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/json/JSONArray.java b/src/main/java/org/json/JSONArray.java index f034499db..f11b328d8 100644 --- a/src/main/java/org/json/JSONArray.java +++ b/src/main/java/org/json/JSONArray.java @@ -173,7 +173,7 @@ public JSONArray(Collection collection) { this.myArrayList = new ArrayList(); } else { this.myArrayList = new ArrayList(collection.size()); - this.addAll(collection); + this.addAll(collection, true); } } @@ -188,7 +188,7 @@ public JSONArray(Iterable iter) { if (iter == null) { return; } - this.addAll(iter); + this.addAll(iter, true); } /** @@ -225,7 +225,7 @@ public JSONArray(Object array) throws JSONException { throw new JSONException( "JSONArray initial value should be a string or collection or array."); } - this.addAll(array); + this.addAll(array, true); } /** @@ -1206,7 +1206,7 @@ public JSONArray put(int index, Object value) throws JSONException { * @return this. */ public JSONArray putAll(Collection collection) { - this.addAll(collection); + this.addAll(collection, false); return this; } @@ -1218,7 +1218,7 @@ public JSONArray putAll(Collection collection) { * @return this. */ public JSONArray putAll(Iterable iter) { - this.addAll(iter); + this.addAll(iter, false); return this; } @@ -1250,7 +1250,7 @@ public JSONArray putAll(JSONArray array) { * Thrown if the array parameter is null. */ public JSONArray putAll(Object array) throws JSONException { - this.addAll(array); + this.addAll(array, false); return this; } @@ -1585,11 +1585,21 @@ public boolean isEmpty() { * * @param collection * A Collection. + * @param wrap + * {@code true} to call {@link JSONObject#wrap(Object)} for each item, + * {@code false} to add the items directly + * */ - private void addAll(Collection collection) { + private void addAll(Collection collection, boolean wrap) { this.myArrayList.ensureCapacity(this.myArrayList.size() + collection.size()); - for (Object o: collection){ - this.myArrayList.add(JSONObject.wrap(o)); + if (wrap) { + for (Object o: collection){ + this.put(JSONObject.wrap(o)); + } + } else { + for (Object o: collection){ + this.put(o); + } } } @@ -1598,10 +1608,19 @@ private void addAll(Collection collection) { * * @param iter * An Iterable. - */ - private void addAll(Iterable iter) { - for (Object o: iter){ - this.myArrayList.add(JSONObject.wrap(o)); + * @param wrap + * {@code true} to call {@link JSONObject#wrap(Object)} for each item, + * {@code false} to add the items directly + */ + private void addAll(Iterable iter, boolean wrap) { + if (wrap) { + for (Object o: iter){ + this.put(JSONObject.wrap(o)); + } + } else { + for (Object o: iter){ + this.put(o); + } } } @@ -1612,18 +1631,27 @@ private void addAll(Iterable iter) { * Array. If the parameter passed is null, or not an array, * JSONArray, Collection, or Iterable, an exception will be * thrown. + * @param wrap + * {@code true} to call {@link JSONObject#wrap(Object)} for each item, + * {@code false} to add the items directly * * @throws JSONException * If not an array or if an array value is non-finite number. * @throws NullPointerException * Thrown if the array parameter is null. */ - private void addAll(Object array) throws JSONException { + private void addAll(Object array, boolean wrap) throws JSONException { if (array.getClass().isArray()) { int length = Array.getLength(array); this.myArrayList.ensureCapacity(this.myArrayList.size() + length); - for (int i = 0; i < length; i += 1) { - this.put(JSONObject.wrap(Array.get(array, i))); + if (wrap) { + for (int i = 0; i < length; i += 1) { + this.put(JSONObject.wrap(Array.get(array, i))); + } + } else { + for (int i = 0; i < length; i += 1) { + this.put(Array.get(array, i)); + } } } else if (array instanceof JSONArray) { // use the built in array list `addAll` as all object @@ -1631,9 +1659,9 @@ private void addAll(Object array) throws JSONException { // JSONArray this.myArrayList.addAll(((JSONArray)array).myArrayList); } else if (array instanceof Collection) { - this.addAll((Collection)array); + this.addAll((Collection)array, wrap); } else if (array instanceof Iterable) { - this.addAll((Iterable)array); + this.addAll((Iterable)array, wrap); } else { throw new JSONException( "JSONArray initial value should be a string or collection or array."); From d30ecad7f88159188a423de16f8959962de5dfc8 Mon Sep 17 00:00:00 2001 From: "John J. Aylward" Date: Thu, 30 Jul 2020 10:13:01 -0400 Subject: [PATCH 6/6] Update README for best practices when using `putAll` on JSONArray --- README.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/README.md b/README.md index 237df9f1a..6f9a4e56d 100644 --- a/README.md +++ b/README.md @@ -118,6 +118,45 @@ Some notable exceptions that the JSON Parser in this library accepts are: * Unescaped literals like "tab" in string values `{ "key": "value with an unescaped tab" }` * Numbers out of range for `Double` or `Long` are parsed as strings +Recent pull requests added a new method `putAll` on the JSONArray. The `putAll` method +works similarly as other `put` mehtods in that it does not call `JSONObject.wrap` for items +added. This can lead to inconsistent object representation in JSONArray structures. + +For example, code like this will create a mixed JSONArray, some items wrapped, others +not: + +```java +SomeBean[] myArr = new SomeBean[]{ new SomeBean(1), new SomeBean(2) }; +// these will be wrapped +JSONArray jArr = new JSONArray(myArr); +// these will not be wrapped +jArr.putAll(new SomeBean[]{ new SomeBean(3), new SomeBean(4) }); +``` + +For structure consistency, it would be recommended that the above code is changed +to look like 1 of 2 ways. + +Option 1: +```Java +SomeBean[] myArr = new SomeBean[]{ new SomeBean(1), new SomeBean(2) }; +JSONArray jArr = new JSONArray(); +// these will not be wrapped +jArr.putAll(myArr); +// these will not be wrapped +jArr.putAll(new SomeBean[]{ new SomeBean(3), new SomeBean(4) }); +// our jArr is now consistent. +``` + +Option 2: +```Java +SomeBean[] myArr = new SomeBean[]{ new SomeBean(1), new SomeBean(2) }; +// these will be wrapped +JSONArray jArr = new JSONArray(myArr); +// these will be wrapped +jArr.putAll(new JSONArray(new SomeBean[]{ new SomeBean(3), new SomeBean(4) })); +// our jArr is now consistent. +``` + **Unit Test Conventions** Test filenames should consist of the name of the module being tested, with the suffix "Test".