Code review action items - add comments and consistent error messages…

… for strict mode
stleary · Dec 21, 2024 · 2dcef89 · 2dcef89
1 parent d3c7eaf
commit 2dcef89
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 49 deletions.
diff --git a/src/main/java/org/json/JSONArray.java b/src/main/java/org/json/JSONArray.java
@@ -67,8 +67,10 @@ public class JSONArray implements Iterable<Object> {
      */
     private final ArrayList<Object> myArrayList;
 
+    // strict mode checks after constructor require access to this object
     private JSONTokener jsonTokener;
 
+    // strict mode checks after constructor require access to this object
     private JSONParserConfiguration jsonParserConfiguration;
 
     /**
@@ -138,8 +140,16 @@ public JSONArray(JSONTokener x, JSONParserConfiguration jsonParserConfiguration)
                         throw x.syntaxError("Expected a ',' or ']'");
                     }
                     if (nextChar == ']') {
+                        // trailing commas are not allowed in strict mode
                         if (jsonParserConfiguration.isStrictMode()) {
-                            throw x.syntaxError("Expected another array element");
+                            throw x.syntaxError("Strict mode error: Expected another array element");
+                        }
+                        return;
+                    }
+                    if (nextChar == ',') {
+                        // consecutive commas are not allowed in strict mode
+                        if (jsonParserConfiguration.isStrictMode()) {
+                            throw x.syntaxError("Strict mode error: Expected a valid array element");
                         }
                         return;
                     }
@@ -166,12 +176,10 @@ public JSONArray(JSONTokener x, JSONParserConfiguration jsonParserConfiguration)
      */
     public JSONArray(String source) throws JSONException {
         this(source, new JSONParserConfiguration());
-        if (this.jsonParserConfiguration.isStrictMode()) {
-                char c = jsonTokener.nextClean();
-                if (c != 0) {
-                    throw jsonTokener.syntaxError(String.format("invalid character '%s' found after end of array", c));
-
-            }
+        // Strict mode does not allow trailing chars
+        if (this.jsonParserConfiguration.isStrictMode() &&
+                this.jsonTokener.nextClean() != 0) {
+                    throw jsonTokener.syntaxError("Strict mode error: Unparsed characters found at end of input text");
         }
     }
 
@@ -188,12 +196,10 @@ public JSONArray(String source) throws JSONException {
      */
     public JSONArray(String source, JSONParserConfiguration jsonParserConfiguration) throws JSONException {
         this(new JSONTokener(source), jsonParserConfiguration);
-        if (this.jsonParserConfiguration.isStrictMode()) {
-            char c = jsonTokener.nextClean();
-            if (c != 0) {
-                throw jsonTokener.syntaxError(String.format("invalid character '%s' found after end of array", c));
-
-            }
+        // Strict mode does not allow trailing chars
+        if (this.jsonParserConfiguration.isStrictMode() &&
+                this.jsonTokener.nextClean() != 0) {
+                    throw jsonTokener.syntaxError("Strict mode error: Unparsed characters found at end of input text");
         }
     }
 

diff --git a/src/main/java/org/json/JSONObject.java b/src/main/java/org/json/JSONObject.java
@@ -152,8 +152,10 @@ public Class<? extends Map> getMapType() {
      */
     public static final Object NULL = new Null();
 
+    // strict mode checks after constructor require access to this object
     private JSONTokener jsonTokener;
 
+    // strict mode checks after constructor require access to this object
     private JSONParserConfiguration jsonParserConfiguration;
 
     /**
@@ -268,13 +270,15 @@ public JSONObject(JSONTokener x, JSONParserConfiguration jsonParserConfiguration
 
             switch (x.nextClean()) {
             case ';':
+                // In strict mode semicolon is not a valid separator
                 if (jsonParserConfiguration.isStrictMode()) {
-                    throw x.syntaxError("Invalid character ';' found in object in strict mode");
+                    throw x.syntaxError("Strict mode error: Invalid character ';' found");
                 }
             case ',':
                 if (x.nextClean() == '}') {
+                    // trailing commas are not allowed in strict mode
                     if (jsonParserConfiguration.isStrictMode()) {
-                        throw x.syntaxError("Expected another object element");
+                        throw x.syntaxError("Strict mode error: Expected another object element");
                     }
                     return;
                 }
@@ -452,9 +456,10 @@ public JSONObject(Object object, String ... names) {
      */
     public JSONObject(String source) throws JSONException {
         this(source, new JSONParserConfiguration());
+        // Strict mode does not allow trailing chars
         if (this.jsonParserConfiguration.isStrictMode() &&
                 this.jsonTokener.nextClean() != 0) {
-            throw new JSONException("Unparsed characters found at end of input text");
+            throw new JSONException("Strict mode error: Unparsed characters found at end of input text");
         }
     }
 
@@ -474,12 +479,10 @@ public JSONObject(String source) throws JSONException {
      */
     public JSONObject(String source, JSONParserConfiguration jsonParserConfiguration) throws JSONException {
         this(new JSONTokener(source), jsonParserConfiguration);
-        if (this.jsonParserConfiguration.isStrictMode()) {
-            char c = jsonTokener.nextClean();
-            if (c != 0) {
-                throw jsonTokener.syntaxError(String.format("invalid character '%s' found after end of array", c));
-
-            }
+        // Strict mode does not allow trailing chars
+        if (this.jsonParserConfiguration.isStrictMode() &&
+                this.jsonTokener.nextClean() != 0) {
+            throw new JSONException("Strict mode error: Unparsed characters found at end of input text");
         }
     }
 

diff --git a/src/main/java/org/json/JSONParserConfiguration.java b/src/main/java/org/json/JSONParserConfiguration.java
@@ -64,6 +64,18 @@ public JSONParserConfiguration withOverwriteDuplicateKey(final boolean overwrite
         return clone;
     }
 
+    /**
+     * Sets the strict mode configuration for the JSON parser with default true value
+     * <p>
+     * When strict mode is enabled, the parser will throw a JSONException if it encounters an invalid character
+     * immediately following the final ']' character in the input. This is useful for ensuring strict adherence to the
+     * JSON syntax, as any characters after the final closing bracket of a JSON array are considered invalid.
+     * @return a new JSONParserConfiguration instance with the updated strict mode setting
+     */
+    public JSONParserConfiguration withStrictMode() {
+        return withStrictMode(true);
+    }
+
     /**
      * Sets the strict mode configuration for the JSON parser.
      * <p>
@@ -92,13 +104,7 @@ public boolean isOverwriteDuplicateKey() {
     }
 
     /**
-     * Retrieves the current strict mode setting of the JSON parser.
-     * <p>
-     * Strict mode, when enabled, instructs the parser to throw a JSONException if it encounters an invalid character
-     * immediately following the final ']' character in the input. This ensures strict adherence to the JSON syntax, as
-     * any characters after the final closing bracket of a JSON array are considered invalid.
-     *
-     * @return the current strict mode setting. True if strict mode is enabled, false otherwise.
+     * @return the current strict mode setting.
      */
     public boolean isStrictMode() {
         return this.strictMode;

diff --git a/src/main/java/org/json/JSONTokener.java b/src/main/java/org/json/JSONTokener.java
@@ -32,6 +32,7 @@ public class JSONTokener {
     /** the number of characters read in the previous line. */
     private long characterPreviousLine;
 
+    // access to this object is required for strict mode checking
     private JSONParserConfiguration jsonParserConfiguration;
 
     /**
@@ -443,10 +444,11 @@ public Object nextValue() throws JSONException {
     Object nextSimpleValue(char c) {
         String string;
 
+        // Strict mode only allows strings with explicit double quotes
         if (jsonParserConfiguration != null &&
                 jsonParserConfiguration.isStrictMode() &&
                 c == '\'') {
-            throw this.syntaxError("Single quote wrap not allowed in strict mode");
+            throw this.syntaxError("Strict mode error: Single quoted strings are not allowed");
         }
         switch (c) {
         case '"':
@@ -477,10 +479,11 @@ Object nextSimpleValue(char c) {
             throw this.syntaxError("Missing value");
         }
         Object obj = JSONObject.stringToValue(string);
+        // Strict mode only allows strings with explicit double quotes
         if (jsonParserConfiguration != null &&
                 jsonParserConfiguration.isStrictMode() &&
                 obj instanceof String) {
-            throw this.syntaxError(String.format("Value '%s' is not surrounded by quotes", obj));
+            throw this.syntaxError(String.format("Strict mode error: Value '%s' is not surrounded by quotes", obj));
         }
         return obj;
     }

diff --git a/src/test/java/org/json/junit/JSONArrayTest.java b/src/test/java/org/json/junit/JSONArrayTest.java
@@ -481,7 +481,7 @@ public void unquotedText() {
             System.out.println("Skipping JSONArrayTest unquotedText() when strictMode default is true");
         } else {
             String str = "[value1, something!, (parens), [email protected], 23, 23+45]";
-            JSONArray jsonArray = new JSONArray(str);
+                JSONArray jsonArray = new JSONArray(str);
             List<Object> expected = Arrays.asList("value1", "something!", "(parens)", "[email protected]", 23, "23+45");
             assertEquals(expected, jsonArray.toList());
         }

diff --git a/src/test/java/org/json/junit/JSONObjectTest.java b/src/test/java/org/json/junit/JSONObjectTest.java
@@ -83,7 +83,7 @@ public void tearDown() {
         Singleton.getInstance().setSomeInt(0);
         Singleton.getInstance().setSomeString(null);
     }
-    
+
     /**
      * Tests that the similar method is working as expected.
      */

diff --git a/src/test/java/org/json/junit/JSONParserConfigurationTest.java b/src/test/java/org/json/junit/JSONParserConfigurationTest.java
@@ -184,7 +184,8 @@ public void givenInvalidStringArray_testStrictModeTrue_shouldThrowJsonException(
                 .withStrictMode(true);
         String testCase = "[badString]";
         JSONException je = assertThrows(JSONException.class, () -> new JSONArray(testCase, jsonParserConfiguration));
-        assertEquals("Value 'badString' is not surrounded by quotes at 10 [character 11 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Value 'badString' is not surrounded by quotes at 10 [character 11 line 1]",
+                je.getMessage());
     }
 
     @Test
@@ -193,7 +194,8 @@ public void givenInvalidStringObject_testStrictModeTrue_shouldThrowJsonException
                 .withStrictMode(true);
         String testCase = "{\"a0\":badString}";
         JSONException je = assertThrows(JSONException.class, () -> new JSONObject(testCase, jsonParserConfiguration));
-        assertEquals("Value 'badString' is not surrounded by quotes at 15 [character 16 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Value 'badString' is not surrounded by quotes at 15 [character 16 line 1]",
+                je.getMessage());
     }
 
     @Test
@@ -289,7 +291,8 @@ public void givenInvalidInputArray_testStrictModeTrue_shouldThrowInvalidCharacte
         String testCase = "[1,2];[3,4]";
         JSONException je = assertThrows("expected non-compliant array but got instead: " + testCase,
                 JSONException.class, () -> new JSONArray(testCase, jsonParserConfiguration));
-        assertEquals("invalid character ';' found after end of array at 6 [character 7 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Unparsed characters found at end of input text at 6 [character 7 line 1]",
+                je.getMessage());
     }
 
     @Test
@@ -299,7 +302,7 @@ public void givenInvalidInputObject_testStrictModeTrue_shouldThrowInvalidCharact
         String testCase = "{\"a0\":[1,2];\"a1\":[3,4]}";
         JSONException je = assertThrows("expected non-compliant array but got instead: " + testCase,
                 JSONException.class, () -> new JSONObject(testCase, jsonParserConfiguration));
-        assertEquals("Invalid character ';' found in object in strict mode at 12 [character 13 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Invalid character ';' found at 12 [character 13 line 1]", je.getMessage());
     }
 
     @Test
@@ -309,7 +312,8 @@ public void givenInvalidInputArrayWithNumericStrings_testStrictModeTrue_shouldTh
         String testCase = "[\"1\",\"2\"];[3,4]";
         JSONException je = assertThrows("expected non-compliant array but got instead: " + testCase,
                 JSONException.class, () -> new JSONArray(testCase, jsonParserConfiguration));
-        assertEquals("invalid character ';' found after end of array at 10 [character 11 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Unparsed characters found at end of input text at 10 [character 11 line 1]",
+                je.getMessage());
     }
 
     @Test
@@ -319,7 +323,7 @@ public void givenInvalidInputObjectWithNumericStrings_testStrictModeTrue_shouldT
         String testCase = "{\"a0\":[\"1\",\"2\"];\"a1\":[3,4]}";
         JSONException je = assertThrows("expected non-compliant array but got instead: " + testCase,
                 JSONException.class, () -> new JSONObject(testCase, jsonParserConfiguration));
-        assertEquals("Invalid character ';' found in object in strict mode at 16 [character 17 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Invalid character ';' found at 16 [character 17 line 1]", je.getMessage());
     }
 
     @Test
@@ -329,7 +333,8 @@ public void givenInvalidInputArray_testStrictModeTrue_shouldThrowValueNotSurroun
         String testCase = "[{\"test\": implied}]";
         JSONException je = assertThrows("expected non-compliant array but got instead: " + testCase,
                 JSONException.class, () -> new JSONArray(testCase, jsonParserConfiguration));
-        assertEquals("Value 'implied' is not surrounded by quotes at 17 [character 18 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Value 'implied' is not surrounded by quotes at 17 [character 18 line 1]",
+                je.getMessage());
     }
 
     @Test
@@ -339,7 +344,8 @@ public void givenInvalidInputObject_testStrictModeTrue_shouldThrowValueNotSurrou
         String testCase = "{\"a0\":{\"test\": implied}]}";
         JSONException je = assertThrows("expected non-compliant array but got instead: " + testCase,
                 JSONException.class, () -> new JSONObject(testCase, jsonParserConfiguration));
-        assertEquals("Value 'implied' is not surrounded by quotes at 22 [character 23 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Value 'implied' is not surrounded by quotes at 22 [character 23 line 1]",
+                je.getMessage());
     }
 
     @Test
@@ -381,13 +387,13 @@ public void givenNonCompliantQuotesArray_testStrictModeTrue_shouldThrowJsonExcep
                 "Expected a ',' or ']' at 10 [character 11 line 1]",
                 jeOne.getMessage());
         assertEquals(
-                "Single quote wrap not allowed in strict mode at 2 [character 3 line 1]",
+                "Strict mode error: Single quoted strings are not allowed at 2 [character 3 line 1]",
                 jeTwo.getMessage());
         assertEquals(
-                "Single quote wrap not allowed in strict mode at 2 [character 3 line 1]",
+                "Strict mode error: Single quoted strings are not allowed at 2 [character 3 line 1]",
                 jeThree.getMessage());
         assertEquals(
-                "Single quote wrap not allowed in strict mode at 3 [character 4 line 1]",
+                "Strict mode error: Single quoted strings are not allowed at 3 [character 4 line 1]",
                 jeFour.getMessage());
     }
 
@@ -414,13 +420,13 @@ public void givenNonCompliantQuotesObject_testStrictModeTrue_shouldThrowJsonExce
                 "Expected a ':' after a key at 10 [character 11 line 1]",
                 jeOne.getMessage());
         assertEquals(
-                "Single quote wrap not allowed in strict mode at 2 [character 3 line 1]",
+                "Strict mode error: Single quoted strings are not allowed at 2 [character 3 line 1]",
                 jeTwo.getMessage());
         assertEquals(
-                "Single quote wrap not allowed in strict mode at 6 [character 7 line 1]",
+                "Strict mode error: Single quoted strings are not allowed at 6 [character 7 line 1]",
                 jeThree.getMessage());
         assertEquals(
-                "Single quote wrap not allowed in strict mode at 2 [character 3 line 1]",
+                "Strict mode error: Single quoted strings are not allowed at 2 [character 3 line 1]",
                 jeFour.getMessage());
     }
 
@@ -467,7 +473,8 @@ public void givenInvalidInputArray_testStrictModeTrue_shouldThrowKeyNotSurrounde
         JSONException je = assertThrows("expected non-compliant array but got instead: " + testCase,
                 JSONException.class, () -> new JSONArray(testCase, jsonParserConfiguration));
 
-        assertEquals("Value 'test' is not surrounded by quotes at 6 [character 7 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Value 'test' is not surrounded by quotes at 6 [character 7 line 1]",
+                je.getMessage());
     }
 
     @Test
@@ -479,7 +486,8 @@ public void givenInvalidInputObject_testStrictModeTrue_shouldThrowKeyNotSurround
         JSONException je = assertThrows("expected non-compliant json but got instead: " + testCase,
                 JSONException.class, () -> new JSONObject(testCase, jsonParserConfiguration));
 
-        assertEquals("Value 'test' is not surrounded by quotes at 5 [character 6 line 1]", je.getMessage());
+        assertEquals("Strict mode error: Value 'test' is not surrounded by quotes at 5 [character 6 line 1]",
+                je.getMessage());
     }
 
     /**
@@ -492,6 +500,8 @@ private List<String> getNonCompliantJSONArrayList() {
         return Arrays.asList(
                 "[1],",
                 "[1,]",
+                "[,]",
+                "[,,]",
                 "[[1],\"sa\",[2]]a",
                 "[1],\"dsa\": \"test\"",
                 "[[a]]",
-Original file line number
+Diff line change
@@ Expand Up / @@ -83,7 +83,7 @@ public void tearDown() { @@
             Singleton.getInstance().setSomeInt(0);
             Singleton.getInstance().setSomeString(null);
         }
         /**
          * Tests that the similar method is working as expected.
          */
@@ Expand Down @@