Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added type conversion support #540

Merged
merged 7 commits into from
Sep 17, 2020
Merged

Added type conversion support #540

merged 7 commits into from
Sep 17, 2020

Conversation

kumar529
Copy link

@kumar529 kumar529 commented Jul 19, 2020

The lib doesn't support type conversion for a specific value.
I have added the feature to support type conversion for a specific tag.
Example:

//XML String
<root><id1 xsi:type="java.lang.String">1234</id1><id2 xsi:type="java.lang.Integer">1234</id2></root>

//Corresponding JSON String
{"root":{"id2":1234,"id1":"1234"}}

@kumar529 kumar529 force-pushed the master branch 2 times, most recently from bba8246 to e828c11 Compare July 19, 2020 13:21
@stleary
Copy link
Owner

stleary commented Jul 20, 2020

Sounds like you are trying to make the case that Java-specific XML xsi:type strings should be supported. If so, is this commonly used by projects other than yours? Otherwise, no real objection since we allow a bit more latitude for supporting XML to JSON transformations, which will always be imperfect.

We already have too many fixed parameters for XMLParserConfiguration. This needs to be fixed first. The existing constructors need to be deprecated, the properties made non-final, and setters added to set up the config manually. I will add an issue to address this.

@johnjaylward
Copy link
Contributor

johnjaylward commented Jul 20, 2020

@kumar529 would it make sense to have mappings for xsi:type as a HashMap instead of a boolean?

i.e.

/**
Interface to support conversions of XML xsl:type values to Java objects
*/
public interface XMLXsiTypeConverter<T> {
    public T convert(String value);
}

public void someFunc() {
Map<string, XsiTypeConverter<?>> XsiTypeMap = new HashMap<>();
XsiTypeMap.put("string", new XMLXsiTypeConverter<String>(){ public String convert(String value){return value;}});
XsiTypeMap.put("int", new XMLXsiTypeConverter<Long>(){ public Long convert(String value){return Long.valueOf(value);}});
XsiTypeMap.put("bool", new XMLXsiTypeConverter<Long>(){ public Boolean convert(String value){return Boolean.FALSE;}});

XMLParserConfiguration config = new XMLParserConfiguration(false, "content", true, XsiTypeMap);

JSONObject jo = XML.toJSONObject(myInput, config);

}

@stleary
Copy link
Owner

stleary commented Jul 20, 2020

Let's try the simplest solution first, and see if it works to everyone's satisfaction.

Sorry, ignore, thought that question was addressed to me.

@kumar529
Copy link
Author

@johnjaylward I think HashMap will not serve the purpose.
Because I want to add data type support on an element.

@stleary I am facing the problem of data conversion. Currently, the lib supports two types of config for data conversion:

  1. Convert all values to the respective data types: If the value is an integer in XML, it will convert it to an integer in JSON. So sometimes the conversion changes the values. It will convert the value to scientific notation.

  2. Convert all values to a string: In this case, if want some integer as an actual integer in JSON. Then using this config, we will not get it.

So I want to add type support on specific elements.

For example:
<root><id1 xsi:type="java.lang.String">1234</id1><id2 xsi:type="java.lang.Integer">1234</id2><id3>1234</id3></root>

Here the value for id1 will be converted to a string, id2 will be converted to an integer, id3 has no type information so default behavior will be applicable.

We can do without adding any configuration change. If xsi:type is present, it will try to convert only. @stleary Please suggest what is your opinion.

We can also use "string", "number", "boolean" for type representation and we can handle these cases from code.

@johnjaylward
Copy link
Contributor

johnjaylward commented Jul 21, 2020

@kumar529, that was why I suggested the hash map. The hashmap is to map the xsi:type to the java type.

The xsi:type would then be configurable per application. You don't need to pick how the type is represented in the xml now. Instead configure it at runtime with converters. I don't think we should presume that all providers of XML will have the same xsi:type schema, nor that all consumers will want conversions done the same way.

@stleary
Copy link
Owner

stleary commented Jul 21, 2020

Hi @johnjaylward

>>>If the value is an integer in XML, it will convert it to an integer in JSON. So sometimes the conversion changes the values.
>>>It will convert the value to scientific notation.

Can it be confirmed that this is not a regression introduced in #453 or one of the subsequent BigNumber/Decimal/Integer commits?

@kumar529
Copy link
Author

@johnjaylward Yes, HashMap can work. The client will provide the HashMap at the time of initialization.

@stleary
Copy link
Owner

stleary commented Jul 23, 2020

@kumar529 This PR has several issues -

  • XMLTest has changed due to a recent commit, merge is needed.
  • Additional XMLParserConfiguration, XML, and XMLTest changes are pending in Refactor XMLConfiguration to use Builder Pattern #543, merge will probably be needed in a few days.
    - We still don't know if this observation is due to an underlying bug: sometimes the conversion changes the values.

@johnjaylward
Copy link
Contributor

johnjaylward commented Jul 23, 2020

@stleary I think the "changing values" is due to not using the "keep strings" option (default parsing). The XSI processing as proposed here would basically be another flavor of "keep strings" but lets generators of XML specify the actual types instead of the tokener always using a string value.

Copy link
Owner

@stleary stleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See conversation comments.

@kumar529
Copy link
Author

@stleary Sure, I will make the change.
I think the idea of passing HashMap as configurations suggested by @johnjaylward looks good.
So I will try to do this.

Copy link
Contributor

@johnjaylward johnjaylward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case in that tests an XML structure that looks something like this:

<root>
<asString xsi:type="string">12345</asString>
<asInt>54321</asInt>
</root>

I would expect a resulting JSON that looks similar to this:

{
"asString":"12345",
"asInt":54321
}

this.keepStrings = keepStrings;
this.cDataTagName = cDataTagName;
this.convertNilAttributeToNull = false;
this(keepStrings, cDataTagName, false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not modify deprecated functions, or use them in your tests.

/**
* This will allow type conversion for values in XML if xsi:type attribute is defined
*/
public Map<String, XMLXsiTypeConverter<?>> xsiTypeMap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I finally had time for a closer look at the configuration changes. With the new changes in #543, this should be private with a Getter and no Setter. Preferably the getter should return an "Unmodifiable Map". To prevent wrapping the collection every time the getter is called, storing the unmodifiable map in this variable would be acceptable.

Note in the Getter javadoc that the map is "unmodifiable"

* xsi:type="integer" as integer, xsi:type="string" as string
* <code>null</code> to use default behaviour.
*/
public XMLParserConfiguration (final boolean keepStrings, final String cDataTagName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be marked private instead of public.
Also, please change:

this.xsiTypeMap = xsiTypeMap

to:

this.xsiTypeMap = Collections.unmodifiableMap(new HashMap<String, XMLXsiTypeConverter<?>>(xsiTypeMap));

Our expected configuration should look something like this:

XMLParserConfiguration xmlConfig = new XMLParserConfiguration().withKeepStrings(false).WithXsiTypeMap(myXsiMap);

// use the config

After marking this private, also update the "clone" method (currently line 165). Be sure to follow the comment that is included in the "clone" method. If changing the constructor above to do the shallow clone/unmodifiable map wrapping, you can probably just update the clone to call this private constructor.

Lastly, be sure to create the new "with" method that will take the XSI:Type conversion map.

@stleary
Copy link
Owner

stleary commented Sep 4, 2020

Manual gradlew build is failing due to javadoc problems, please fix.

@johnjaylward Do you have any more un-addressed issues?
Otherwise, after this PR is merged, I would like to follow up with a PR that removes clone() and the new XMLParserConfiguration ctor that @kumar529 just added. Any objections?

@johnjaylward
Copy link
Contributor

@stleary
The test case already there.

    @Test
    public void testToJsonWithPartialXSITypeWhenTypeConversionEnabled() {
        String originalXml = "<root><asString xsi:type=\"string\">12345</asString><asInt>54321</asInt></root>";
        String expectedJsonString = "{\"root\":{\"asString\":\"12345\",\"asInt\":54321}}";
        JSONObject expectedJson = new JSONObject(expectedJsonString);
        Map<String, XMLXsiTypeConverter<?>> xsiTypeMap = new HashMap<String, XMLXsiTypeConverter<?>>();
        xsiTypeMap.put("string", new XMLXsiTypeConverter<String>() {
            @Override public String convert(final String value) {
                return value;
            }
        });
        JSONObject actualJson = XML.toJSONObject(originalXml, new XMLParserConfiguration().withXsiTypeMap(xsiTypeMap));
        Util.compareActualVsExpectedJsonObjects(actualJson,expectedJson);
    }

I do not see this test case in the change set. The XML I see tested is:

<root><asString xsi:type="string">12345</asString><asInt xsi:type="integer">54321</asInt></root>

NOT

<root><asString xsi:type="string">12345</asString><asInt>54321</asInt></root>

@kumar529
Copy link
Author

@stleary @johnjaylward Please let me know if there is anything pending in this PR.

@johnjaylward
Copy link
Contributor

@kumar529, I believe that my concerns have been addressed. Sorry I did not notice your most recent commit until you commented.

@stleary
Copy link
Owner

stleary commented Sep 14, 2020

What problem does this code solve?
Provides additional support for XML / JSON transformation via xsitype, by adding an xsiTypeMap to XMLParserConfiguration. This is an enhancement, not a bug fix. Allowed because additional flexibility is allowed for XML / JSON code.

Risks
Low

Changes to the API?
No

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
No

Review status
APPROVED
Starting 3 day comment window.

@stleary stleary merged commit 7abd89b into stleary:master Sep 17, 2020
This was referenced Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants