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

az_json_writer_append_json_text does not take into account required buffer space for commas #1905

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

hwmaier
Copy link
Contributor

@hwmaier hwmaier commented Sep 8, 2021

The az_json_writer_append_json_text() function does not take into account required buffer space for commas when tracking buffer size with _az_update_json_writer_state().

So for example if 5 array elements are written using az_json_writer_append_json_text(), the resulting JSON span is short of 4 (5-1) characters and the resulting JSON string is incomplete, typically the closing braces and a few last characters show missing in the stream.

This patch fixes this issue.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

Thank you for your contribution hwmaier! We will review the pull request and get back to you soon.

@@ -799,7 +801,7 @@ az_json_writer_append_json_text(az_json_writer* ref_json_writer, az_span json_te
// Therefore, need_comma must be true after appending the json_text.

// We already tracked and updated bytes_written while writing, so no need to update it here.
_az_update_json_writer_state(ref_json_writer, 0, az_span_size(json_text), true, last_token_kind);
_az_update_json_writer_state(ref_json_writer, 0, required_size, true, last_token_kind);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this fixes up _internal.total_bytes_written, this seems to be incrementing _internal.bytes_written by one more than necessary, because it is already being incremented on line 789.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I see the bytes_written increment parameter continues to be 0.

@ahsonkhan ahsonkhan added this to the [2021] October milestone Sep 8, 2021
Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch and fix!

@ahsonkhan ahsonkhan merged commit 9b5ef3d into Azure:main Sep 8, 2021
@ahsonkhan ahsonkhan added the blocking-release Blocks release label Sep 8, 2021
@hwmaier hwmaier deleted the patch/comma_not_counted branch October 13, 2021 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-release Blocks release customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants