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

nx_azure_iot_json_writer_append_json_text does not take into account required buffer space for commas #47

Closed
hwmaier opened this issue Sep 8, 2021 · 7 comments

Comments

@hwmaier
Copy link

hwmaier commented Sep 8, 2021

When nx_azure_iot_json_writer_append_json_text() is used to write elements of an array, the resulting JSON packet is incomplete. The NX_PACKET's byte count is short of exactly the amount of commas inserted internally by az_json_writer_append_json_text().

This is an issue with the underlaying azure-sdk-for-c and a PR has been submitted here: Azure/azure-sdk-for-c#1905

However as this bug also affects the JSON writing functionality of NetX Duo I believe this should be reported here as well, hoping that a fix can be coordinated between the two Microsoft dev teams and be available with the next NetXDuo release.

@ahsonkhan
Copy link

The current implementation is directly accessing internal fields, which should be avoided where possible. The issue is with the total_bytes_written, but that isn't exposed, and is currently meant for testing purposes only, within azure-sdk-for-c.
https://github.com/azure-rtos/netxduo/blob/d9d7fb05bd6a7a6b50a54c762e1730d8a8054cbb/addons/azure_iot/nx_azure_iot_json_writer.c#L97-L98

@hwmaier
Copy link
Author

hwmaier commented Sep 9, 2021

Well I am only an end user stumbling over a bug which I reported and provided a fix with my PR. Without a bug fix I cannot proceed with my application.

As an outsider I leave the debate about whether internal data structures should be accessed or not within Microsoft products to the relevant dev teams.

However I would prefer a quick pragmatic solution, and my PR does provide this. It fixes the correctness of the internal data structure and then nx_azure_iot_json_writer_append_json_text() works as expected.

In the long run, maybe exposing _internal.total_bytes_written as an API call would be a solution, that way tests can be added and such a bug would have been picked up by a test case.

@ahsonkhan
Copy link

Absolutely! Thank you for flagging the issue and providing the fix. I totally agree that it was the right approach, especially to address customer facing issues like you pointed out. My note above was mainly for contributors to this repo to evaluate current usage of internal-only state. I hadn't realized folks were using and benefiting from the total_bytes_written field in their scenarios
:)

@hwmaier
Copy link
Author

hwmaier commented Sep 9, 2021

@ahsonkhan Thank's for the quick resolution.
Now I hope the change is rolled into the next NetXDuo release soon.

@ahsonkhan
Copy link

@TiejunMS
Copy link
Contributor

cc @hihigupt

@hwmaier
Copy link
Author

hwmaier commented Oct 13, 2021

Fixed in latest 6.1.9 release,

@hwmaier hwmaier closed this as completed Oct 13, 2021
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