Streams support #768

stleary · 2023-09-04T16:48:55Z

What problem does this code solve?
JSONObject and JSONArray should support streams for functional programming.
There are multiple ways this could be added. If you know of a better way, please post your comments to this PR.

This change requires Java 8. Going forward, Java 6 and Java 7 library builds are no longer supported. The last release for Java 6/7 is 20230618. Users who still need Java 6/7 support should open an issue in this repo.

Risks
Low

Changes to the API?
New API methods were added

Will this require a new release?
Yes

Should the documentation be updated?
No

Does it break the unit tests?
No. New unit tests were added

Was any code refactored in this commit?

gradlew permissions were updated
ParserConfiguration warnings were addressed

Review status
code review action items pending

…ethod to each class

This reverts commit 4cea301.

…radlew permissions

johnjaylward · 2023-09-05T16:43:53Z

I'm trying to add review notes here, but it seems that GitHub is having issues.

I think for this implementation we should also consider #673 and #674.

johnjaylward

Overall this looks like a good start, However I'd personally like to see a few more test cases around getting data out and null handling in both the JSONObject and JSONArray streams.

Looking at #673 and #674, I think we should look towards always returning valid JSON value types from the streams. It is something we missed out on with the iterators and prevents people from having a consistent interface.

We should also be sure that the streams themselves (specifically the ones provided as part of the API, not the follow-on calls as part of the tests) do not throw exceptions by using opt operations wherever possible.

johnjaylward · 2023-09-05T16:46:12Z

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

+     * @return Stream of array elements
+     */
+    public Stream<Object> stream() {
+        return IntStream.range(0, length()).mapToObj(this::get);


I think this should not be this::get and instead should be this::opt. Stream should not throw exceptions, which get will do if a value is null.

We should also be sure to include at least one test case that has null in the array.

I think this should not be this::get and instead should be this::opt. Stream should not throw exceptions, which get will do if a value is null.

We should also be sure to include at least one test case that has null in the array.

Seems to be working as is, but not sure why. Test cases are updated, let me know if this code or the tests should still be changed.

I stepped through the code and that Parser is converting null to JSONObject.NULL, which is expected.

Let's add these 2 additional tests, which are very similar, but add different ways of composing the data instead of just using the parser. This second test shows the issue I was worried about:

@Test public void testStream_ComposeFromArray() { @SuppressWarnings("boxing") Object[] expectedList = new Object[] { 1, 2, 3, true, false, null, "abc", "def", new JSONObject("{ \"a\": \"b\" }"), new JSONArray()}; JSONArray jsonArray = new JSONArray(expectedList); List<Object> actualList = jsonArray.stream() .collect(Collectors.toList()); System.out.println(actualList); assertEquals(actualList.size(), expectedList.length); for (int i = 0; i < actualList.size(); ++i) { Object actual = actualList.get(i); Object expected = expectedList[i]; if (actual instanceof JSONObject) { assertTrue("Json Object test",((JSONObject) actual).similar(expected)); } else if (actual instanceof JSONArray) { assertTrue("Json Array test", ((JSONArray) actual).similar(expected)); } else if (JSONObject.NULL.equals(expected)) { assertTrue("Null test", JSONObject.NULL.equals(actual)); } else { assertEquals("value test: "+expected, expected, actual); } } }

@Test public void testStream_ComposeFromCode() { JSONArray jsonArray = new JSONArray(); jsonArray.put(1).put(2).put(3) .put(true).put(false) .put((Object)null) .put("abc").put("def") .put(new JSONObject("{ \"a\": \"b\" }")) .put(new JSONArray()) ; @SuppressWarnings("boxing") Object[] expectedList = new Object[] { 1, 2, 3, true, false, null, "abc", "def", new JSONObject("{ \"a\": \"b\" }"), new JSONArray()}; List<Object> actualList = jsonArray.stream() .collect(Collectors.toList()); System.out.println(actualList); assertEquals(actualList.size(), expectedList.length); for (int i = 0; i < actualList.size(); ++i) { Object actual = actualList.get(i); Object expected = expectedList[i]; if (actual instanceof JSONObject) { assertTrue("Json Object test",((JSONObject) actual).similar(expected)); } else if (actual instanceof JSONArray) { assertTrue("Json Array test", ((JSONArray) actual).similar(expected)); } else if (JSONObject.NULL.equals(expected)) { assertTrue("Null test", JSONObject.NULL.equals(actual)); } else { assertEquals("value test: "+expected, expected, actual); } } }

Added tests.
Replaced this::get with this::opt

johnjaylward · 2023-09-05T16:52:06Z

src/main/java/org/json/JSONObject.java

@@ -81,7 +74,13 @@
 * @author JSON.org
 * @version 2016-08-15
 */
-public class JSONObject {
+public class JSONObject implements Iterable<String> {


I'm not sure this is the best Iterable<> that could have been chosen.

Would it make more sense to implement this closer to the Map<>.entrySet()? Something like: public class JSONObject implements Iterable<Map.Entry<String, Object>>?

Or maybe even better would be to keep it very similar to Map<> and not have it directly implement Iterable<> at all. Instead offer similar functions that the Map interface does:

keySet

entrySet

values

Each of those methods would return proper Iterable implementations.

Implementation now streams entrySet() without Iterable<>.

johnjaylward · 2023-09-05T16:57:15Z

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

+        JSONArray jsonArray = new JSONArray(data);
+
+        // Get employees from the engineering dept
+        List<String> actualList = StreamSupport.stream(jsonArray.spliterator(), false)


This test doesn't seem to be testing the new Stream method. It appears to only be testing Iteratable<>.spliterator() method.

This is a valid test, but I don't think this is doing the coverage that you wanted for the new method.

Replaced with new tests

… ParserConfiguration

stleary · 2023-09-16T17:36:09Z

I'm trying to add review notes here, but it seems that GitHub is having issues.

I think for this implementation we should also consider #673 and #674.

Can this be done in a subsequent commit? The other issues seem out of scope for this initial implementation.

johnjaylward · 2023-09-19T19:13:49Z

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

+    public void testStream_1_3_2() {
+        JSONArray jsonArray = null;
+
+        assertThrows(NullPointerException.class, () -> {


I don't think this test is adding anything here. It seems obvious that an NPE would be thrown in this case.

johnjaylward · 2023-09-19T19:14:04Z

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

+    public void test_2_3_2() {
+        JSONObject jsonObject = null;
+
+        assertThrows(NullPointerException.class, () -> {


I don't think this test is adding anything here. It seems obvious that an NPE would be thrown in this case.

johnjaylward · 2023-09-19T19:47:02Z

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

+     * @return Stream of array elements
+     */
+    public Stream<Object> stream() {
+        return IntStream.range(0, length()).mapToObj(this::get);


I stepped through the code and that Parser is converting null to JSONObject.NULL, which is expected.

Let's add these 2 additional tests, which are very similar, but add different ways of composing the data instead of just using the parser. This second test shows the issue I was worried about:

@Test public void testStream_ComposeFromArray() { @SuppressWarnings("boxing") Object[] expectedList = new Object[] { 1, 2, 3, true, false, null, "abc", "def", new JSONObject("{ \"a\": \"b\" }"), new JSONArray()}; JSONArray jsonArray = new JSONArray(expectedList); List<Object> actualList = jsonArray.stream() .collect(Collectors.toList()); System.out.println(actualList); assertEquals(actualList.size(), expectedList.length); for (int i = 0; i < actualList.size(); ++i) { Object actual = actualList.get(i); Object expected = expectedList[i]; if (actual instanceof JSONObject) { assertTrue("Json Object test",((JSONObject) actual).similar(expected)); } else if (actual instanceof JSONArray) { assertTrue("Json Array test", ((JSONArray) actual).similar(expected)); } else if (JSONObject.NULL.equals(expected)) { assertTrue("Null test", JSONObject.NULL.equals(actual)); } else { assertEquals("value test: "+expected, expected, actual); } } }

@Test public void testStream_ComposeFromCode() { JSONArray jsonArray = new JSONArray(); jsonArray.put(1).put(2).put(3) .put(true).put(false) .put((Object)null) .put("abc").put("def") .put(new JSONObject("{ \"a\": \"b\" }")) .put(new JSONArray()) ; @SuppressWarnings("boxing") Object[] expectedList = new Object[] { 1, 2, 3, true, false, null, "abc", "def", new JSONObject("{ \"a\": \"b\" }"), new JSONArray()}; List<Object> actualList = jsonArray.stream() .collect(Collectors.toList()); System.out.println(actualList); assertEquals(actualList.size(), expectedList.length); for (int i = 0; i < actualList.size(); ++i) { Object actual = actualList.get(i); Object expected = expectedList[i]; if (actual instanceof JSONObject) { assertTrue("Json Object test",((JSONObject) actual).similar(expected)); } else if (actual instanceof JSONArray) { assertTrue("Json Array test", ((JSONArray) actual).similar(expected)); } else if (JSONObject.NULL.equals(expected)) { assertTrue("Null test", JSONObject.NULL.equals(actual)); } else { assertEquals("value test: "+expected, expected, actual); } } }

stleary · 2023-09-20T00:46:54Z

Added the new unit tests

stleary · 2023-10-05T12:15:03Z

Closing due to lack of activity.

Sean Leary added 6 commits September 4, 2023 09:05

streams - JSONObject and JSONArray implement iterable, add stream() m…

252bfbb

…ethod to each class

streams - remove implements from JSONObject and JSONArray

4cea301

Revert "streams - remove implements from JSONObject and JSONArray"

c619419

This reverts commit 4cea301.

streams - implement meaningful iterator() for JSONObject. Also, fix g…

e4a3337

…radlew permissions

streams - comments and test

960b374

streams - removed unneeded overrides

870b5f2

stleary mentioned this pull request Sep 4, 2023

(JSONArray).toList() breaks format of entries #737

Open

johnjaylward suggested changes Sep 5, 2023

View reviewed changes

Sean Leary added 3 commits September 10, 2023 21:32

Unit tests for JSONArray stream()

22ac808

streams - jsonobject stream implementation and unit tests

4d0bb3a

streams - cleanup unneeded test cases, refactor to remove warnings in…

6e670d5

… ParserConfiguration

johnjaylward suggested changes Sep 19, 2023

View reviewed changes

streams - code review action items

9dac039

stleary closed this Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streams support #768

Streams support #768

stleary commented Sep 4, 2023 •

edited

Loading

johnjaylward commented Sep 5, 2023

johnjaylward left a comment

johnjaylward Sep 5, 2023

stleary Sep 16, 2023

johnjaylward Sep 19, 2023

stleary Sep 20, 2023 •

edited

Loading

johnjaylward Sep 5, 2023

stleary Sep 16, 2023

johnjaylward Sep 5, 2023

stleary Sep 20, 2023 •

edited

Loading

stleary commented Sep 16, 2023

johnjaylward Sep 19, 2023

stleary Sep 20, 2023

johnjaylward Sep 19, 2023

stleary Sep 20, 2023

johnjaylward Sep 19, 2023

stleary commented Sep 20, 2023

stleary commented Oct 5, 2023

Streams support #768

Streams support #768

Conversation

stleary commented Sep 4, 2023 • edited Loading

johnjaylward commented Sep 5, 2023

johnjaylward left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stleary Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stleary Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

stleary commented Sep 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stleary commented Sep 20, 2023

stleary commented Oct 5, 2023

stleary commented Sep 4, 2023 •

edited

Loading

stleary Sep 20, 2023 •

edited

Loading

stleary Sep 20, 2023 •

edited

Loading