Skip to content

Conversation

@johnjaylward
Copy link
Contributor

There is some code in the XML converter that tries to wrap arrays into a JSONArray, but does so inconsistently. This causes some disconnect between the handling of a native Java Array and the JSONArray object when conversion to XML occurs.

This PR also updates the loops over the JSONArrays to use the new iterable for loops introduced in Java5 as well as updates some comment formatting to be more consistent with other files in the project.

@stleary
Copy link
Owner

stleary commented Oct 27, 2015

Thanks, investigating.

@stleary
Copy link
Owner

stleary commented Oct 31, 2015

Does not break the unit tests. A bit difficult to isolate the functional change due to all of the style updates. In XML.toString(), Array values found in a JSONObject are converted into JSONArray instances:

New functionality:
XML.toString()
Insert at line 409 of the original code:

} else if (value.getClass().isArray()) {
    value = new JSONArray(value);
}

New behavior:

  • When processing JSONObject values, convert Array to JSONArray.

Unrelated note:
Don't think line 410 of the original code does anything, due to the return on line 466.

            string = value instanceof String ? (String)value : null;

Seems like a good enhancement, but needs more study of how the behavior is changed.

@johnjaylward
Copy link
Contributor Author

I agree with you about line 410. I think that would be outside the scope of this PR though, unless you want me to include it.

@stleary
Copy link
Owner

stleary commented Dec 31, 2015

What problem does this code solve?
XML.toString() does not work as expected when a JSONObject contains Array type values. This results inconsistent handling between raw JSON, JSONArray values and Array values.

Changes to the API?
No.

Changes to how the code behaves?
When processing JSONObject values, Arrays will be converted to JSONArrays before
further processing.
Example

{
    "arr" : [
        "One",
        [
             "Two",
             "Three"
        ],
         "Four",
         []
     ]
}

Produces this XML from raw JSON or internal JSONArrays (correct):

    <arr>One</arr>
    <arr>
        <array>Two</array>
        <array>Three</array>
    </arr>
    <arr>Four</arr>
    <arr></arr>

Currently produces this XML from internal arrays (incorrect):

    <arr>One</arr>
    <arr>Two</arr>
    <arr>Three</arr>
    <arr>Four</arr>

The code fix will bring the Array value in sync with raw JSON and JSONArray.

Does it break the unit tests?
The unit tests will be updated. See stleary/JSON-Java-unit-test#29.

Will this require a new release?
No, this change will be rolled into the next release.

Should the documentation be updated?
No, this is an internal change.

stleary added a commit that referenced this pull request Jan 1, 2016
@stleary stleary merged commit 757fd56 into stleary:master Jan 1, 2016
stleary added a commit that referenced this pull request Jan 1, 2016
@johnjaylward johnjaylward deleted the HandleArraysConsistently branch February 10, 2017 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants