JKISoftware / JKI-JSON-Serialization

JSON Serialization & Deserialization Library for LabVIEW
http://jki.net/tools#json
BSD 3-Clause "New" or "Revised" License
25 stars 9 forks source link

Error 1527 when parsing an array with cluster containing empty cluster and scalar #51

Open jimkring opened 1 week ago

jimkring commented 1 week ago

Reported here.

Here is the reproducer:

Error 1527 JKI JSON.vi

image

{
    "array": [
        {
            "cluster": {
                "empty_cluster": {}
            },
            "scalar": 0
        }
    ]
}
jimkring commented 1 week ago

@nate-moehring would you (or anyone else) be interested in looking into this issue? 1) Create fork/branch for a Pull Request 2) Add reproducer as a Caraya test with Issue 51 in VI name 3) Fix the bug and keep all unit tests passing :)

nate-moehring commented 1 week ago

Sure, I can help with this, but first help me understand what the expected behavior would be. If you wired this to a Variant to Data node, the type cluster constant would need to have both 'empty_cluster' and 'cluster' elements omitted, otherwise the conversion would fail. Does SlippinJimmy know that these elements will always be empty? If not, then this error seems appropriate (though vague). If it's sometimes populated in jsonText, then the _type_cluster would need to be fully defined. Is there a way to specify optional (non-critical elements) of a json cluster such that isn't doesn't break the unflatten? If he does know that 'empty_cluster' is always empty, which therefore implies that 'cluster' will also be empty, then that seems like a pretty tough case to handle.

Workaround: Using the Path input on Native JSON Unflatten from String nodes to decode select items from the JSON is super easy and powerful.

jimkring commented 1 week ago

@nate-moehring - that’s a great question.

I noticed that parsing this JSON (snippet below) seems to work OK (meaning: No Error and Variant data is returned successfully).

        {
            "cluster": {
                "empty_cluster": {}
            },
            "scalar": 0
        }

The error condition seems to occur in the specific case where the above snippet is an element of an array.

I’m not in front of my computer now or for most of today, but I’m thinking we should look at what the variant structure looks like for the similar, “working” cases.

A first/incremental step forward probably looks like “fixing” the inconsistency by getting the error to go away so that the test case outputs a variant with an array with one element that’s equivalent to the data produced from that second example JSON snippet.

Of course, the devil is in the details around how flattened data is handled. LabVIEW gets a bit finicky with zero element clusters since they aren’t valid edit-time/static data (and only make sense in dynamically-typed data (variants).

Where we may end up with this is: returning a null variant in liu of empty clusters ({}).

I’ll think of a bit more about this once I have a chance to get in front of the computer later this weekend.

jimkring commented 1 week ago

@nate-moehring here's an example showing the desired output. The upper/working code shows how if we remove the outer array, we can successfully convert it to variant and then use the OpenG Variant library to make it an element of a 1D Array.

Example VI: Issue 51 - Array of Cluster with Element of Empty Cluster.vi

Example Screenshot: image

nate-moehring commented 1 week ago

I made some progress tonight. I used your repro to create a test VI (see my branch), and then found that I could fix this particular bug by removing a single node, but this then breaks other tests: image

nate-moehring commented 1 week ago

Without this change, this is the Variant data string that is breaking Array of VData to VArray, if you're fluent in such things: 'Value': Variant <Variant : {{{...}}, 0}>

2000 8000 0000 0004 0014 4050 0000 0D65 6D70 7479 5F63 6C75 7374 6572 0010 4050 0001 0000 0763 6C75 7374 6572 000D 4008 0006 7363 616C 6172 0000 0A00 5000 0200 0100 0200 0100 0300 0000 0000 0000 0000 0000 00

nate-moehring commented 6 days ago

On the left side of the To Variant it has all the type-info, but on the right side it's like the type info gets lost, even though the data is preserved. (Yes, Show Type is enabled) image

Perhaps this To Variant is essentially wrapping the variant in another variant and the Type Info display doesn't decode nested variants?

A: Yes, it does appear to be wrapping a fresh variant around it. When I use Variant to Data with a Type input of Variant, I get the original readable variant back. I always thought that wiring a variant into To Variant was a noop.

nate-moehring commented 6 days ago

This repro shows the problem, any ideas? Report to NI? Errored Variant.zip image

jimkring commented 6 days ago

@nate-moehring Yes, I would call this a LabVIEW bug -- nice work catching it!

Yes, let's report it to NI. Would you be willing to do that and report back here with an NI Bug ID #?

Regarding a short-term work-around, I'd be curious to see which cases now fail. Maybe the extra layer of variant wrapping in Parse Value could be removed and (whatever it was attempting to address could be) done in a different way.

Also, please create a pull request from your own fork/branch, since that'll make it easy for me to check it out and fiddle in there with you, if needed.

jimkring commented 6 days ago

@nate-moehring actually, I'll ping LabVIEW R&D abound this, directly, and see if they can get it into the system.

nate-moehring commented 6 days ago

@jimkring Pull request: https://github.com/JKISoftware/JKI-JSON-Serialization/pull/52

francois-normandin commented 3 days ago

Keeping an interested eye on this. I used to rely on this library because jsonText does not accept deserialization without specifying type, but have since made a jsonText wrapper that supports it.

I'm attaching it here for anyone wanting to reverse engineer it and see how it could fix this Parser in the 0-element Cluster use case.

(VI depends on jsonText and LVOS' Data Manipulation) JSON text to Variant (Void).zip

edit: Just to be clear, OpenVariant.lvlib is 0-BSD licensed. If anything makes sense, just extract it and use. No dependency or attribution needed.

nate-moehring commented 2 days ago

Thanks @francois-normandin . I've poked around with the libraries you provided, man that's a lot of code. In some ways it's more simple code, which is good, and it seems like it might handle more cases than the JKI library, but I'd have a hard time creating a side-by-side feature comparison list.

It's true that your library parses the problematic json just fine. It's also true that the array of variants that gets created just prior to being reshaped into a variant array works just fine. And an element of that array can be flattened and unflattened just fine, unless I force it through a To Variant like what is happening in Parse Value, then I see the same Error 1527.

My conclusion thus far is that I need to remove the To Variant from the JKI Parse Value begin-array case, and then look to the non-array cases of JSON text to Variant (Void) to help address the many resulting unit test errors.