PlainText Serializer incorrectly unmarshalls certain nested clusters/arrays

[IlyaVino]

I believe I have found a fix and will submit an incomplete pull request shortly, but an additional bug surfaces that prevents all the unit tests I added from passing:

If the datatype contains a nested numeric array, then the numeric values are not preserved. As far as I can tell, this second bug occurs inside OpenVariant.lvlib:Variant_KVP to Strict Variant.vi. I've submitted a separate issue inside that repository.

[francois-normandin]

Can you provide a test vector for this issue? What is an example of an input that does not deserialize correctly? And what do you think is the expected output of this deserialization?

VI or screenshots would help. Thanks.

[IlyaVino]

Here with a modified set of tests demonstrating the issue.

Test Serializer.Text.zip

[IlyaVino]

Some more context as requested in the pull request:

I'm interested in serializing/deserializing an array of clusters that contain a name string and numeric array. These are phase cycling schemes for a spectrometer. I was using the open source configuration manager to save/read these to/from a text file when I ran into the issue.

If I use the cluster array described above I get the serialized string: [{"A",[1,2,3]},{"B",[4,5,6]}], which throws an invalid type error upon deserializing. This error is thrown by Serializer.PlainText.lvclass:PlainTextToVariant.vi. My understanding is that this occurs because PlainTextToVariant.vi encounters '{' and switches to parsing the remaining string as a cluster, whereas it should be parsed as an array and then recursively call PlainTextToVariant.vi to parse the array's contents.

The proposed fix is to remove the shift register switch for '{' and '[' if already parsing the string as a cluster or array. That way, the contents are correctly passed to the recursive call of PlainTextToVariant.vi

I've added unit tests to Test Serializer.Text in the zip file in the preceding comment. These unit tests include a variant cast test and tests to see if the values are preserved for the following cases:

1. Array of clusters [{"A",[1,2,3]},{"B",[4,5,6]}]
2. Array of clusters no array [{"A",{"B",1}},{"C",{"D",2}}]
3. Cluster with array {"A",[1,2,3]}
4. Cluster with cluster array {"A",[{"B",1},{"C",2}]}

Before the proposed fix, all cases except number 2 failed. After the fix, all cases pass for cast but fail (except number 2) for preserving numeric array values.

The failure for preserving numeric array values appears to be a separate bug in https://github.com/LabVIEW-Open-Source/DataManipulation/issues/5. I am unsure on how to fix this bug. I'll add a test vector when I get a chance tomorrow as a comment to that issue.

Finally, the most recent commit in the pull request contains the proposed fix and the added unit tests described above. Note that, as described above, not all tests pass. Therefore, I am calling this an incomplete fix.

[francois-normandin]

Thanks for the test vectors. I don't have much time right now to look into this problem with the PlainText Serializer, but the test vectors are useful.

Note: Comparing Array of Floats through the text serializer will always fail... you will need to compare individual elements with the "Almost Equal" method... unless serializing the byte array representation, there is simply no way to preserve the full 64-bit resolution.

[IlyaVino]

No worries, the object oriented framework makes it easy to find a workaround by using a different serializer.

Good to know about array of floats. The failing tests still fail with any numeric type except I64. I've attached a modified I16 version of the tests. Note that now "cluster with cluster array - nested string" also fails...

Test Serializer.Text.zip

[francois-normandin]

I might have a fix @IlyaVino. Question for you... Is your system configured to use "dot" or "comma" for decimals? The unit tests were failing on my Canadian computer... where we use comma for decimal point. When I turned the flag "Use System Decimal Point" to false across the board, the test suite passes!

Found two related issues: #7 and #8

[francois-normandin]

I have a fix for them all, which should effectively solve the current reported bug.

[francois-normandin]

version 1.2.0 is fixing this bug

[IlyaVino]

Hi,

Thanks for getting back to this so quickly!

I tried your update, but unfortunately it still fails for arrays of clusters that contain arrays in them, which is the case I'm most interested in (test file re-linked from previous comment):

I also ran the test that shipped with your update, and I get all passing, which I hope rules out possible differences between our computers/configuration settings.

I didn't have time yet to fully play around with your new update to narrow down the issue, but I'll take a more careful look tomorrow morning.

Thanks again for all your help!

[IlyaVino]

I made some headway on the issue. It looks like the changes I made to PlainTextToVariant.vi were not completely merged in the most recent version, probably because they did not pass all tests/required intervention in the DataManipulation library for a full fix.

If I recreate those changes and ( important ) keep the type input to unmarshall.vi unwired, everything works! I'm not sure why keeping the type input wired fails.

My tests:

Your tests:

In addition, I took a second more careful look at that vi (PlainTextToVariant (standard).vi in v1.2.0) and discovered an additional but closely related bug for deeply nested clusters and arrays. This bug represents a fairly rare case but has a simple fix. I'll open a new issue for it for documentation.

[IlyaVino]

Also uploaded in issue #10, here is the version of PlainTextToVariant.vi that contains the changes to pass the tests above:

PlainTextToVariant (Standard).zip