Merge pull request #552 from johnjaylward/JSONArrayCopyConstructor

Updates for JSONArray.putAll methods

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". 

src/main/java/org/json/JSONArray.java

@@ -173,7 +173,37 @@ public class JSONArray implements Iterable<Object> {
             this.myArrayList = new ArrayList<Object>();
         } else {
             this.myArrayList = new ArrayList<Object>(collection.size());
-            this.addAll(collection);
+            this.addAll(collection, true);
+        }
+    }
+
+    /**
+     * 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, true);
+    }
+
+    /**
+     * Construct a JSONArray from another JSONArray. This is a shallow copy.
+     *
+     * @param array
+     *            A array.
+     */
+    public JSONArray(JSONArray array) {
+        if (array == null) {
+            this.myArrayList = new ArrayList<Object>();
+        } else {
+            // shallow copy directly the internal array lists as any wrapping
+            // should have been done already in the original JSONArray
+            this.myArrayList = new ArrayList<Object>(array.myArrayList);
         }
     }
 
@@ -191,7 +221,11 @@ public class JSONArray implements Iterable<Object> {
      */
     public JSONArray(Object array) throws JSONException {
         this();
-        this.addAll(array);
+        if (!array.getClass().isArray()) {
+            throw new JSONException(
+                    "JSONArray initial value should be a string or collection or array.");
+        }
+        this.addAll(array, true);
     }
 
     /**
@@ -1165,32 +1199,58 @@ public class JSONArray implements Iterable<Object> {
     }
 
     /**
-     * Put or replace a collection's elements in the JSONArray.
+     * Put a collection's elements in to the JSONArray.
      *
      * @param collection
      *            A Collection.
      * @return this. 
      */
     public JSONArray putAll(Collection<?> collection) {
-        this.addAll(collection);
+        this.addAll(collection, false);
+        return this;
+    }
+    
+    /**
+     * Put an Iterable's elements in to the JSONArray.
+     *
+     * @param iter
+     *            An Iterable.
+     * @return this. 
+     */
+    public JSONArray putAll(Iterable<?> iter) {
+        this.addAll(iter, false);
         return this;
     }
 
     /**
-     * Put or replace an array's elements in the JSONArray.
+     * Put a JSONArray's elements in to the JSONArray.
      *
      * @param array
-     *            Array. If the parameter passed is null, or not an array, an
+     *            A JSONArray.
+     * @return this. 
+     */
+    public JSONArray putAll(JSONArray array) {
+        // 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;
+    }
+
+    /**
+     * Put an array's elements in to the JSONArray.
+     *
+     * @param array
+     *            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.
      */
     public JSONArray putAll(Object array) throws JSONException {
-        this.addAll(array);
+        this.addAll(array, false);
         return this;
     }
     
@@ -1520,39 +1580,88 @@ public class JSONArray implements Iterable<Object> {
         return this.myArrayList.isEmpty();
     }
 
-
     /**
      * Add a collection's elements to the JSONArray.
      *
      * @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){
+        if (wrap) {
-            this.myArrayList.add(JSONObject.wrap(o));
+            for (Object o: collection){
+                this.put(JSONObject.wrap(o));
+            }
+        } else {
+            for (Object o: collection){
+                this.put(o);
+            }
         }
     }
 
+    /**
+     * Add an Iterable's elements to the JSONArray.
+     *
+     * @param iter
+     *            An Iterable.
+     * @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);
+            }
+        }
+    }
+    
     /**
      * Add an array's elements 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,
-     *            exception will be thrown.
+     *            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) {
+            if (wrap) {
-                this.put(JSONObject.wrap(Array.get(array, i)));
+                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
+            // wrapping should have been completed in the original
+            // JSONArray
+            this.myArrayList.addAll(((JSONArray)array).myArrayList);
+        } else if (array instanceof Collection) {
+            this.addAll((Collection<?>)array, wrap);
+        } else if (array instanceof Iterable) {
+            this.addAll((Iterable<?>)array, wrap);
         } else {
             throw new JSONException(
                     "JSONArray initial value should be a string or collection or array.");

src/test/java/org/json/junit/JSONArrayTest.java

@@ -979,9 +979,9 @@ public class JSONArrayTest {
         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 class JSONArrayTest {
                     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<Object>();
+        ((Collection<Object>)o).add(1);
+        ((Collection<Object>)o).add("test");
+        ((Collection<Object>)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));
+        }
+    }
 }

src/test/java/org/json/junit/JSONObjectTest.java

@@ -3201,4 +3201,11 @@ public class JSONObjectTest {
         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());
+    }
 }