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

PlainText Serializer incorrectly unmarshalls deeply nested clusters/arrays that contain multiple ] or } characters #10

Closed
IlyaVino opened this issue Jan 19, 2022 · 5 comments

Comments

@IlyaVino
Copy link

This is a bug I referenced at the end of #5

This bug exists for deeply nested clusters and arrays such as:
deeply nested cluster array

which generate plain text strings:

[{"asdf",[{[1.0,2.0],"asdf"},{[3.0,4.0],"ghjk"}]},{"ghjk",[{[1.0,2.0],"asdf"},{[3.0,4.0],"ghjk"}]}]
{[{[1.0,2.0],"asdf"},{[3.0,4.0],"ghjk"}]}

Whenever PlainTextToVariant encounters a nested ] or }, it may switch the data type it is parsing from "cluster" or "array" to "unknown". This should only happen for the outermost ] or } so that the contents can be parsed recursively.

For example, the second cluster's contents are initially parsed under the "array" flag based on the second [ character but when encountering the first ] character after 2.0, the parsing incorrectly switches to "unknown". Instead, the flag should only be changed when encountering the outermost ] character at the end so that the full contents of the array inside the [ ... ] is parsed recursively.

My suggested fix is to add a counter that keeps track of the bracket depth. For example:
bracket counter inside plainTextToVariant

This counter needs to be added for all cases involved in parsing nested data types, i.e. under the cluster ("{" .. "{~") and array ("[" .. "[~") case. Any subcase that has "[" or "{" needs an increment and any subcase that has "]" or "}" needs a decrement and check to see if the counter is at zero.

I'll clean up my changes to the vi a bit and compile some tests shortly.

@IlyaVino
Copy link
Author

IlyaVino commented Jan 19, 2022

Here with compiled tests, including a test for this issue, issue #5, and the tests that shipped with v1.2.0
Test Serializer.Text.zip

@francois-normandin, I've decided to upload my changes to the VI here since there may still be issues with these deeply nested data types and the data manipulation library. Let me know if you prefer a pull request instead.
PlainTextToVariant (Standard).zip

@francois-normandin
Copy link
Member

Thanks @IlyaVino
This should do it indeed. I usually implement those in a recursive way but the shift register should work great while I develop a real "lexer" class to add to the serializer (but this will probably be at a later date... )

francois-normandin added a commit that referenced this issue Jan 20, 2022
…d clusters/arrays

[Fix #10] PlainText Serializer now correctly unmarshalls deeply nested clusters/arrays that contain multiple ] or } characters
@francois-normandin
Copy link
Member

Marvelous, @IlyaVino
I`ve added your unit tests along the few extra one for Issues 7,8,9 and now the full test suite passes.

@francois-normandin
Copy link
Member

Released 1.2.1 (will be sent to vipm.io shortly)

https://github.com/LabVIEW-Open-Source/Serializer/releases/tag/1.2.1

@IlyaVino
Copy link
Author

Great! Thanks again for all the help with these issues.

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

No branches or pull requests

2 participants