-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix issue in ComplexTypeWriter #1042
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
Fix issue in ComplexTypeWriter #1042
Conversation
…often than setValue
I would really like to have this fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests to verify that the behavior is correct.
} | ||
} | ||
|
||
@Override | ||
public void startNode(String name) { | ||
if (nodeDepth == 1) { | ||
this.fieldNames.add(name); | ||
currentKey = name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to work with nested nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically when the nodes add values after the nested element. E.g.: [value, [value, value], value]
because the current key will be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can take a look at fixing this! Since the current tests all pass I think some more are needed. Would it be possible for you to add one or more failing tests that show the behaviour? I can try adding them myself but I don't have enough knowledge of all the various use cases to support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've send a PR on your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I hope to take a look when I'm back at work on Monday.
* Fixed complex type writer and added tests. * [Core] Add explicit error message to ComplexTypeWriter When dealing with nested complex types, the ComplexTypeWriter would produce unbalanced and misaligned tables. This could be resolved by adding an `@XStreamConverter` to the complex field but this was not obvious. By adding an explicit exception this is resolved. For ease of use this exception is not thrown when acomplex field is empty or not included in the table .
@mpkorstanje I just merged your changes into my PR. It looks fine from my end, I can confirm that your fix works equally well as my fix for our use case. |
Cheers! Thanks for the setup.
And this fortunately is no longer a caveat. We are only including the fields of the root object and not their children. These are guaranteed to be unique. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
It can happen that in ComplexTypeWriter "startNode" is called more often than "setValue". In our case this happened when one of the fields was an empty Set, but perhaps there are other cases too.
In this case, startNode is called, but setValue is not. This causes the fieldNames and fieldValues lists to go out of alignment which causes all sorts of havoc when creating the DataTable.
This fix simply replaces the two separate lists with a Map, so that they are always aligned. One caveat of this approach is that if the same field name is serialized more than once, the previous value would be overwritten. However, seeing how the Writer is used, this does not seem like something that the current DataTables can handle anyway.
All existing tests pass.