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

[Java] toString() shouldn't disturb repeating groups #890

Closed
spoerri opened this issue Feb 24, 2022 · 3 comments
Closed

[Java] toString() shouldn't disturb repeating groups #890

spoerri opened this issue Feb 24, 2022 · 3 comments

Comments

@spoerri
Copy link
Contributor

spoerri commented Feb 24, 2022

Calling toString() in the middle of decoding a repeating group breaks the decoding of that group. For example, subsequent entries are skipped:

for (MyGroupDecoder member : decoder.myGroup()) {
    logger.debug(decoder);
    handle(member.myValue());
}

Only the first member of the repeating group will be handled.

The fix should be pretty simple: generate code into the appendTo method to save and restore index and offset when handling each group, similar to how it already saves and restores limit.

spoerri added a commit to spoerri/simple-binary-encoding that referenced this issue Mar 4, 2022
mjpt777 pushed a commit that referenced this issue Mar 7, 2022
* toString() shouldn't disturb repeating groups #890

* Remove trailing whitespace

* Ugh, removed more whitespace
@mjpt777 mjpt777 closed this as completed Mar 7, 2022
@krisso-rtb
Copy link

Is this fix merged to master?
I'm looking at sbe-all-1.27.0 and it seems to missing the fix.

@krisso-rtb
Copy link

It looks like it's either partially merged or partially rolled back :)

append(sb, indent, "int " + groupName + "OriginalOffset = " + groupName + ".offset;");
append(sb, indent, "int " + groupName + "OriginalIndex = " + groupName + ".index;");

Above is missing, below is present in 1.27.0:

append(sb, indent, groupName + ".offset = " + groupName + "OriginalOffset;");
append(sb, indent, groupName + ".index = " + groupName + "OriginalIndex;");

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 7, 2023

@krisso-rtb If you think there is a remaining problem then please create a new issue with a test that demonstrates it.

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

3 participants