os/json: Skip json null value when parsing json content

[ebrinette]

Accept (by skipping) null values in JSON parser instead of stopping the parser and returning an error.

Signed-off-by: Eric Brinette [email protected]

[andyross]

JSON doesn't define a null. I'm not clear on what this is trying to do? What is the encoded syntax you're trying to produce from a NULL provided by the user? Or what encoded syntax do you expect to produce a NULL in the resulting parsed output?

[ebrinette]

JSON doesn't define a null. I'm not clear on what this is trying to do? What is the encoded syntax you're trying to produce from a NULL provided by the user? Or what encoded syntax do you expect to produce a NULL in the resulting parsed output?

Hello Andy,

When I look at this it seems that JSON does define null.

https://datatracker.ietf.org/doc/html/rfc8259

This commit simply skip the key/value parsed when "key":null is found.

I can shortly describe the case where I need it:

We are connecting to Azure IoT Hub. the cloud side modifies the device twin values. We receive this modification in a MQTT message (json format) which contains "key":null when the key has to be removed from the device twin.
This is a standard behavior from Microsoft's Azure IoT hub.

Since the payload does contain null we fail to parse the json.

I noticed that the test are not passing which is expected.

Now, before changing the tests, I would need to know if this modification can be accepted.

Maybe, I could/should add a boolean option with default value on the json_obj_parse function in order to not break the current API/behavior?

Thank you for your review =)

[ebrinette]

Up please. I would love to get an answer when you can find time. Thank you.

[mrfuchs]

Given the fact that this is a recurring request (cf. #27600, #28905), I'd love to hear @lpereira's opinion on it. Although he left the Zephyr project, it seems like he's still working on his json library.

[lpereira]

The reason I never supported null in this parser is because I could never figure out a way to communicate that a nested object was incomplete with the return value being what it is (nth bit set if nth struct member -- in descriptor array order -- was decoded). It's pretty tricky. I suppose a good way to avoid problems with this would be 0-initializing values when null is encountered, so that at least values are predictable when nested objects are parsed (and won't have whatever garbage was in the struct). This won't solve the nested object vs. int return issue, but seems like a good compromise that won't break the API. Not setting the bit (like the patch does) on null seems like a good approach. Another thing that might be worth considering is to have a "nullable" flag encoded in the descriptor somehow. This way you can say if a value is absolutely necessary, and fail early if null is found where it shouldn't occur. Since JSON is used for network stuff, it's a good idea to be strict by default to not catch anyone by surprise. (The main reason the library is anal about types is because of this; I originally wrote the parser for the NATS protocol implementation.) (Also, yes, there has been some improvements in the JSON library over the years; some of which could be backported to Zephyr.)

…

On Wed, Jul 7, 2021, 07:02 Markus Fuchs ***@***.***> wrote: Given the fact that this is a recurring request (cf. #27600 <#27600>, #28905 <#28905>), I'd love to hear @lpereira <https://github.com/lpereira>'s opinion on it. Although he left the Zephyr project, it seems like he's still working on his json library <https://github.com/lpereira/lwan/commits/master/src/samples/techempower/json.c> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36332 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADVGNQ7EN7OOFW25GVXT3TWRM6BANCNFSM46ZVQGKQ> .

[lpereira]

I don't think however that this will help with the case where null is used to signal that a key should be removed, like in the API you're using. You need to know that the key is there in the JSON but has a null value; the nth bit not being set communicates that the key wasn't there, which is different.

…

On Wed, Jul 7, 2021, 07:36 Leandro Pereira ***@***.***> wrote: The reason I never supported null in this parser is because I could never figure out a way to communicate that a nested object was incomplete with the return value being what it is (nth bit set if nth struct member -- in descriptor array order -- was decoded). It's pretty tricky. I suppose a good way to avoid problems with this would be 0-initializing values when null is encountered, so that at least values are predictable when nested objects are parsed (and won't have whatever garbage was in the struct). This won't solve the nested object vs. int return issue, but seems like a good compromise that won't break the API. Not setting the bit (like the patch does) on null seems like a good approach. Another thing that might be worth considering is to have a "nullable" flag encoded in the descriptor somehow. This way you can say if a value is absolutely necessary, and fail early if null is found where it shouldn't occur. Since JSON is used for network stuff, it's a good idea to be strict by default to not catch anyone by surprise. (The main reason the library is anal about types is because of this; I originally wrote the parser for the NATS protocol implementation.) (Also, yes, there has been some improvements in the JSON library over the years; some of which could be backported to Zephyr.) On Wed, Jul 7, 2021, 07:02 Markus Fuchs ***@***.***> wrote: > Given the fact that this is a recurring request (cf. #27600 > <#27600>, #28905 > <#28905>), I'd love to > hear @lpereira <https://github.com/lpereira>'s opinion on it. Although > he left the Zephyr project, it seems like he's still working on his json > library > <https://github.com/lpereira/lwan/commits/master/src/samples/techempower/json.c> > . > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#36332 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAADVGNQ7EN7OOFW25GVXT3TWRM6BANCNFSM46ZVQGKQ> > . >

[mrfuchs]

The reason I never supported null in this parser is because I could never figure out a way to communicate that a nested object was incomplete with the return value being what it is (nth bit set if nth struct member -- in descriptor array order -- was decoded). It's pretty tricky.

Given the fact there is a JSON_TOK_NULL, we could at least support something like this:

struct object_t {
  const char * string;
} object;

const struct json_obj_descr descriptor[] = {
  JSON_OBJ_DESCR_PRIM(object_t, string, JSON_TOK_NULL),
};

const char in[] = "{\"string\":null}";
char out[strlen(in)+1];
const int ret1 = json_obj_parse((char *) in, strlen(in), descriptor, ARRAY_SIZE(descriptor), &object);
const int ret2 = json_obj_encode_buf(descriptor, ARRAY_SIZE(descriptor), &object, out, sizeof(out));

With the current version of the json library, this sample will fail with EINVAL (-22).

Clearly, this doesn't really solve the original issue (where one wants to distinguish between field is missing, null, empty ("") or set ("...")), but this way you could make the parsing of simple objects double-stage by first parsing against a descriptor defining the field as JSON_TOK_STRING and - if that fails - parsing against a second fall-back descriptor defining the field as JSON_TOK_NULL...

I suppose a good way to avoid problems with this would be 0-initializing values when null is encountered, so that at least values are predictable when nested objects are parsed (and won't have whatever garbage was in the struct).

+1. The patch does not do that a the moment. With the sample code shown above, json_obj_parse() will succeed with return code 0, but object.string will not be touched and therefore stay unitialized.

This won't solve the nested object vs. int return issue, but seems like a good compromise that won't break the API. Not setting the bit (like the patch does) on null seems like a good approach.

I don't think however that this will help with the case where null is used to signal that a key should be removed, like in the API you're using. You need to know that the key is there in the JSON but has a null value;

That could be determined by the application via initializing the field prior parsing (to an invalid value) and checking if it has been set to null by the parser after the function returns. That wouldn't be the cleanest solution, but would at least give you a choice...

[ebrinette]

According to what you suggested.
Now the null value is only handled when a string or a null was expected (JSON_TOK_STRING and JSON_TOK_NULL)
If a string was expected, the char * is zero- initialized.
The n-th bit is set if the n-th value is null and null was expected (defined in the descriptor).

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.