Revamp native code #291

kalaiyarasiganeshalingam · 2021-10-22T05:22:10Z

Purpose

The toJson() has some gaps with spec. When analyzed the spec, the following gaps were identified:

According to the spec, If XML has identical keys, it has to create a JSON array with an identical key. But existing implementation doesn't support XML that has multiple identical keys.

xml x1 = xml `<books>
                      <item>book1</item>
                      <item>book2</item>
                      <item>book3</item>
                      <item1>book1</item1>
                      <item1>book2</item1>
                      <item1>book3</item1>
                  </books>`;

Existing output:

{"books":{"item":"book3","item1":"book3"}}

Expected output:

{
	"books":{
		"item":[
			"book1",
			"book2",
			"book3"
		],
		"item1":[
			"book1",
			"book2",
			"book3"
		]
	}
}

If an XML element has an attribute and value, that has to create this output. But existing implementation doesn't create that format.
Existing output:

{"foo":"5","@key":"value"}

If the key is missing in the XML, it has to take the key value as #content. But this is not implemented.
Following issues have been reported by others
Getting child elements as an array in XML to JSON conversion
Getting the same element name twice in JSON output
Unable to remove all namespace in xml to json

So, this PR contains all these fixes.

Fixes: ballerina-platform/ballerina-library#2126

Examples

Checklist

Linked to an issue
Updated the changelog
Added tests

codecov · 2021-10-22T05:26:40Z

Codecov Report

Merging #291 (d8cdd2f) into master (fe152b5) will increase coverage by 0.85%.
The diff coverage is 93.02%.

@@             Coverage Diff              @@
##             master     #291      +/-   ##
============================================
+ Coverage     87.20%   88.05%   +0.85%     
+ Complexity       89       83       -6     
============================================
  Files             7        7              
  Lines           297      268      -29     
  Branches         64       56       -8     
============================================
- Hits            259      236      -23     
+ Misses           22       18       -4     
+ Partials         16       14       -2

Impacted Files	Coverage Δ
...in/java/io/ballerina/stdlib/xmldata/XmlToJson.java	`90.06% <92.30%> (+1.29%)`	⬆️
.../io/ballerina/stdlib/xmldata/AttributeManager.java	`100.00% <100.00%> (ø)`
ballerina/xmldata.bal	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe152b5...d8cdd2f. Read the comment docs.

niveathika · 2021-10-22T13:31:15Z

Marked as draft since the proposal acceptance is still under review

hasithaa · 2021-10-28T06:12:16Z

native/src/main/java/io/ballerina/stdlib/xmldata/XmlToJson.java

+        List<BXml> newSequence = new ArrayList<BXml>();
+        for (BXml value: sequence) {
+            String textValue = value.getTextValue();
+            if (textValue.isEmpty() || !textValue.trim().isEmpty()) {


What are we checking here ?

When initialising xml with line break between elements, couldn't get the proper BXml list by using xmlSequence.getChildrenList(). This list contain line breaks as element. So I used this to eliminate line break. Now I have created issue for this. Please find the issue for this: ballerina-platform/ballerina-lang#33497

hasithaa · 2021-10-28T06:22:39Z

native/src/main/java/io/ballerina/stdlib/xmldata/XmlToJson.java

+    private static LinkedHashMap<String, String> attributeMap = new LinkedHashMap<>();
+    private static LinkedHashMap<String, String> tempAttributeMap = new LinkedHashMap<>();


Why static variables ? This is not concurrent safe.

This is used in static method. Hence, I am using static variable. Now, I have changed LinkedHashMap to ConcurrentHashMap in 562a4dd.

hasithaa

Code is not concurrent safe.

madhukaw · 2021-10-28T10:56:30Z

ballerina/tests/xml_to_json_test.bal

+    groups: ["toJson"]
+}
+isolated function testComplexXmlWithNamespace() returns error?  {
+    xml x1 = xml `<ns0:bookStore status="online" xmlns:ns0="http://sample.com/test">


AFAIK this is not a valid xml (multiple possible root nodes) and therefore should return an error.

This is in the spec. So, I haven't checked whether this valid or not. Yes, This is not valid XML. We don't need to validate in the xmldata module. it should be validated in the lang lib. Did you create an issue for this?

Created an issue: ballerina-platform/ballerina-lang#33501

…into task

ballerina/tests/xml_to_json_test.bal

hasithaa · 2021-11-01T05:02:39Z

native/src/main/java/io/ballerina/stdlib/xmldata/AttributeManager.java

+ */
+public class AttributeManager {
+
+    private ConcurrentHashMap<String, String> concurrentHashMap;


Use LinkedHashMap here, since AttributeManager is concurrent safe now. Also we need to preserve the order, so replace other usages with LinkedHashMap as well.

Fixed in 3481ab2

Fixed

hasithaa

Overall design looks ok.

madhukaw · 2021-11-01T09:54:23Z

ballerina/tests/xml_to_json_test.bal

-                            "{\"StreetAddress\":\"20, Palm grove, Colombo 3\"}, \"\\n            \", " +
-                            "{\"City\":\"Colombo\"}, \"\\n            \", {\"Zip\":\"00300\"}, \"\\n            \", " +
-                            "{\"Country\":\"LK\"}, \"\\n        \"], \"@xmlns\":\"\"}, \"\\n    \"], " +
+    test:assertEquals(convertToJson(e.toString()),"{\"Invoice\":{\"PurchesedItems\":{\"PLine\":[" +


IINM convertToJson is a test util function. Why are there unit tests for this function?

I had these as these test utils functions are keeping long times. But It seems these are not useful because the toJson() directly calls the convertToJSON(). We can test this by calling ballerina API. So, we don't need to test the convertToJSON() by using these test utils. So I have removed this util module in 873cc8c.

native/src/main/java/io/ballerina/stdlib/xmldata/XmlToJson.java

Revert "Remove testutils module" This reverts commit 2c0e2a0. Revert "Revert "Remove testutils module"" This reverts commit 74a7409.

native/src/main/java/io/ballerina/stdlib/xmldata/XmlToJson.java

ballerina/tests/xml_to_json_test.bal

kalaiyarasiganeshalingam added 3 commits October 4, 2021 22:19

[Automated] Update the native jar versions

cd6c4f5

Refactor native code

b60611d

Merge with master

9ca9048

kalaiyarasiganeshalingam requested a review from daneshk as a code owner October 22, 2021 05:22

Refactor test case

b4a9d20

niveathika marked this pull request as draft October 22, 2021 13:30

Fix an issue with removing namespace

a685af0

kalaiyarasiganeshalingam marked this pull request as ready for review October 26, 2021 04:25

Format the coding

c31ed6a

hasithaa reviewed Oct 28, 2021

View reviewed changes

hasithaa previously requested changes Oct 28, 2021

View reviewed changes

madhukaw reviewed Oct 28, 2021

View reviewed changes

kalaiyarasiganeshalingam added 2 commits October 28, 2021 21:52

Change LinkedHashMap to ConcurrentHashMap

562a4dd

Merge https://github.com/ballerina-platform/module-ballerina-xmldata …

64bbdc9

…into task

madhukaw reviewed Oct 29, 2021

View reviewed changes

ballerina/tests/xml_to_json_test.bal Outdated Show resolved Hide resolved

kalaiyarasiganeshalingam added 3 commits November 1, 2021 09:38

Change global variable to local

cc3a9bb

Update invalid xml

be7d4b6

Change params name

7dfbf35

hasithaa reviewed Nov 1, 2021

View reviewed changes

Change concurrent hash map to linked hash map

3481ab2

hasithaa previously approved these changes Nov 1, 2021

View reviewed changes

madhukaw reviewed Nov 1, 2021

View reviewed changes

niveathika reviewed Nov 1, 2021

View reviewed changes

[Automated] Update the native jar versions

861bb6c

kalaiyarasiganeshalingam dismissed hasithaa’s stale review via 2c0e2a0 November 1, 2021 13:56

Remove testutils module

873cc8c

Revert "Remove testutils module" This reverts commit 2c0e2a0. Revert "Revert "Remove testutils module"" This reverts commit 74a7409.

kalaiyarasiganeshalingam force-pushed the task branch from 5855d71 to 873cc8c Compare November 1, 2021 16:02

Change method nname

ee87e95

niveathika reviewed Nov 2, 2021

View reviewed changes

kalaiyarasiganeshalingam added 2 commits November 2, 2021 12:47

Change mapData to childrenData

84b71bc

Fix reviewed comments

52a8818

niveathika previously approved these changes Nov 3, 2021

View reviewed changes

madhukaw reviewed Nov 3, 2021

View reviewed changes

ballerina/tests/xml_to_json_test.bal Outdated Show resolved Hide resolved

madhukaw reviewed Nov 3, 2021

View reviewed changes

ballerina/tests/xml_to_json_test.bal Outdated Show resolved Hide resolved

Fix typo

d8cdd2f

kalaiyarasiganeshalingam dismissed niveathika’s stale review via d8cdd2f November 4, 2021 04:12

madhukaw approved these changes Nov 4, 2021

View reviewed changes

kalaiyarasiganeshalingam merged commit 3a35a9a into master Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp native code #291

Revamp native code #291

kalaiyarasiganeshalingam commented Oct 22, 2021 •

edited

Loading

codecov bot commented Oct 22, 2021 •

edited

Loading

niveathika commented Oct 22, 2021

hasithaa Oct 28, 2021

kalaiyarasiganeshalingam Oct 28, 2021

hasithaa Oct 28, 2021

kalaiyarasiganeshalingam Oct 28, 2021

hasithaa left a comment

madhukaw Oct 28, 2021

kalaiyarasiganeshalingam Oct 28, 2021

madhukaw Oct 29, 2021

hasithaa Nov 1, 2021

kalaiyarasiganeshalingam Nov 1, 2021

hasithaa left a comment

madhukaw Nov 1, 2021

kalaiyarasiganeshalingam Nov 1, 2021

		private static LinkedHashMap<String, String> attributeMap = new LinkedHashMap<>();
		private static LinkedHashMap<String, String> tempAttributeMap = new LinkedHashMap<>();

Revamp native code #291

Revamp native code #291

Conversation

kalaiyarasiganeshalingam commented Oct 22, 2021 • edited Loading

Purpose

Examples

Checklist

codecov bot commented Oct 22, 2021 • edited Loading

Codecov Report

niveathika commented Oct 22, 2021

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

hasithaa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasithaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalaiyarasiganeshalingam commented Oct 22, 2021 •

edited

Loading

codecov bot commented Oct 22, 2021 •

edited

Loading