[PATCH v2 4/9] json: add support for array iteration

[PATCH v2 4/9] json: add support for array iteration

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

Arrays can now be parsed using the JSON_ARRAY type (stored in
a struct json_iter) then iterated using json_iter_next. When
iterating the type can be checked with json_iter_get_type. For
each iteration the value can be obtained using any of the type
getters (int/uint/boolean/null).
---
 src/json.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/json.h |  4 ++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/src/json.c b/src/json.c
index 6c88ed3a..cf233e56 100644
--- a/src/json.c
+++ b/src/json.c
@@ -88,7 +88,17 @@ static void iter_recurse(struct json_iter *iter, jsmntok_t *token,
 
 	child->contents = c;
 	child->start = token - c->tokens;
+	child->current = child->start;
 	child->count = count_tokens_in_container(iter, token);
+
+	/*
+	 * Add one to include the object/array token itself. This is required
+	 * since 'current' points to the container initially. Only after a call
+	 * to json_iter_next() will 'current' point to the first token in the
+	 * container.
+	 */
+	if (token->type == JSMN_OBJECT || token->type == JSMN_ARRAY)
+		child->count++;
 }
 
 struct json_contents *json_contents_new(const char *json, size_t json_len)
@@ -160,6 +170,7 @@ static void assign_arg(void *data, void *user_data)
 		break;
 	case JSON_OBJECT:
 	case JSON_PRIMITIVE:
+	case JSON_ARRAY:
 		iter_val = arg->value;
 
 		if (!arg->v)
@@ -207,6 +218,7 @@ bool json_iter_parse(struct json_iter *iter, enum json_type type, ...)
 		case JSON_STRING:
 		case JSON_OBJECT:
 		case JSON_PRIMITIVE:
+		case JSON_ARRAY:
 			break;
 		default:
 			goto error;
@@ -271,7 +283,7 @@ static bool iter_get_primitive_data(struct json_iter *iter, void **ptr,
 					size_t *len)
 {
 	struct json_contents *c = iter->contents;
-	jsmntok_t *t = c->tokens + iter->start;
+	jsmntok_t *t = c->tokens + iter->current;
 
 	if (t->type != JSMN_PRIMITIVE)
 		return false;
@@ -370,3 +382,45 @@ bool json_iter_get_null(struct json_iter *iter)
 
 	return false;
 }
+
+enum json_type json_iter_get_type(struct json_iter *iter)
+{
+	struct json_contents *c = iter->contents;
+	jsmntok_t *t = c->tokens + iter->current;
+
+	return (enum json_type) t->type;
+}
+
+bool json_iter_next(struct json_iter *iter)
+{
+	struct json_contents *c = iter->contents;
+	jsmntok_t *t = c->tokens + iter->current;
+	int inc = 1;
+
+	/*
+	 * If this is the initial iteration skip this and just increment
+	 * current by 1 since this iterator points to a container which needs to
+	 * be advanced to the first token..
+	 *
+	 * In addition primitive types and empty containers will have a size
+	 * of 1, so no special handling is needed there.
+	 *
+	 * For non-empty containers 'current' needs to be advanced by all the
+	 * containers child tokens, plus the container itself.
+	 *
+	 * This check ensures:
+	 *    1. It is not the initial iteration
+	 *    2. This is a container
+	 *    3. The container is not empty
+	 */
+	if (iter->current != iter->start && ((t->type == JSMN_OBJECT ||
+					t->type == JSMN_ARRAY) && t->size))
+		inc = count_tokens_in_container(iter, t) + 1;
+
+	if (c->tokens + iter->current + inc >= ITER_END(iter))
+		return false;
+
+	iter->current += inc;
+
+	return true;
+}
diff --git a/src/json.h b/src/json.h
index 9f00a9d5..458deb93 100644
--- a/src/json.h
+++ b/src/json.h
@@ -42,6 +42,7 @@ struct json_iter {
 	struct json_contents *contents;
 	int start;
 	int count;
+	int current;
 };
 
 #define JSON_MANDATORY(key, type, out) \
@@ -92,3 +93,6 @@ bool json_iter_get_int(struct json_iter *iter, int *i);
 bool json_iter_get_uint(struct json_iter *iter, unsigned int *i);
 bool json_iter_get_boolean(struct json_iter *iter, bool *b);
 bool json_iter_get_null(struct json_iter *iter);
+
+enum json_type json_iter_get_type(struct json_iter *iter);
+bool json_iter_next(struct json_iter *iter);
-- 
2.31.1

Re: [PATCH v2 4/9] json: add support for array iteration

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3599 bytes --]

Hi James,

On 1/6/22 13:50, James Prestwood wrote:
> Arrays can now be parsed using the JSON_ARRAY type (stored in
> a struct json_iter) then iterated using json_iter_next. When
> iterating the type can be checked with json_iter_get_type. For
> each iteration the value can be obtained using any of the type
> getters (int/uint/boolean/null).
> ---
>   src/json.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   src/json.h |  4 ++++
>   2 files changed, 59 insertions(+), 1 deletion(-)
> 


> diff --git a/src/json.c b/src/json.c
> index 6c88ed3a..cf233e56 100644
> --- a/src/json.c
> +++ b/src/json.c
> @@ -88,7 +88,17 @@ static void iter_recurse(struct json_iter *iter, jsmntok_t *token,
>   
>   	child->contents = c;
>   	child->start = token - c->tokens;
> +	child->current = child->start;
>   	child->count = count_tokens_in_container(iter, token);
> +
> +	/*
> +	 * Add one to include the object/array token itself. This is required
> +	 * since 'current' points to the container initially. Only after a call
> +	 * to json_iter_next() will 'current' point to the first token in the
> +	 * container.
> +	 */
> +	if (token->type == JSMN_OBJECT || token->type == JSMN_ARRAY)
> +		child->count++;
>   }

This seems a bit dangerous since count is set based on ITER_END in patch 1.  Is 
there a chance this ends up allowing going past the allocated tokens?

I'm guessing you are trying to account for a slight difference in behavior of 
json_iter depending on what type it points to:
- if json_iter points to JSMN_PRIMITIVE,
then json_iter_next should not be called (and should fail)
- if json_iter points to JSMN_OBJECT/ARRAY, then json_iter_next has to be called 
prior to obtaining the first element

Am I right so far?

>   
>   struct json_contents *json_contents_new(const char *json, size_t json_len)

<snip>

> @@ -370,3 +382,45 @@ bool json_iter_get_null(struct json_iter *iter)
>   
>   	return false;
>   }
> +
> +enum json_type json_iter_get_type(struct json_iter *iter)
> +{
> +	struct json_contents *c = iter->contents;
> +	jsmntok_t *t = c->tokens + iter->current;
> +
> +	return (enum json_type) t->type;
> +}
> +
> +bool json_iter_next(struct json_iter *iter)
> +{
> +	struct json_contents *c = iter->contents;
> +	jsmntok_t *t = c->tokens + iter->current;
> +	int inc = 1;
> +
> +	/*
> +	 * If this is the initial iteration skip this and just increment
> +	 * current by 1 since this iterator points to a container which needs to
> +	 * be advanced to the first token..
> +	 *
> +	 * In addition primitive types and empty containers will have a size
> +	 * of 1, so no special handling is needed there.
> +	 *
> +	 * For non-empty containers 'current' needs to be advanced by all the
> +	 * containers child tokens, plus the container itself.

One idea might be to use the 'size' variable of the start token to know whether 
the iterator can be advanced or not.  Alternatively, look up the base type of 
the iterator at the start token and modify the behavior here accordingly.

> +	 *
> +	 * This check ensures:
> +	 *    1. It is not the initial iteration
> +	 *    2. This is a container
> +	 *    3. The container is not empty
> +	 */
> +	if (iter->current != iter->start && ((t->type == JSMN_OBJECT ||
> +					t->type == JSMN_ARRAY) && t->size))
> +		inc = count_tokens_in_container(iter, t) + 1;
> +
> +	if (c->tokens + iter->current + inc >= ITER_END(iter))
> +		return false;
> +
> +	iter->current += inc;
> +
> +	return true;
> +}

Regards,
-Denis

Re: [PATCH v2 4/9] json: add support for array iteration

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 5915 bytes --]

On Thu, 2022-01-06 at 14:25 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 1/6/22 13:50, James Prestwood wrote:
> > Arrays can now be parsed using the JSON_ARRAY type (stored in
> > a struct json_iter) then iterated using json_iter_next. When
> > iterating the type can be checked with json_iter_get_type. For
> > each iteration the value can be obtained using any of the type
> > getters (int/uint/boolean/null).
> > ---
> >   src/json.c | 56
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   src/json.h |  4 ++++
> >   2 files changed, 59 insertions(+), 1 deletion(-)
> > 
> 
> 
> > diff --git a/src/json.c b/src/json.c
> > index 6c88ed3a..cf233e56 100644
> > --- a/src/json.c
> > +++ b/src/json.c
> > @@ -88,7 +88,17 @@ static void iter_recurse(struct json_iter *iter,
> > jsmntok_t *token,
> >   
> >         child->contents = c;
> >         child->start = token - c->tokens;
> > +       child->current = child->start;
> >         child->count = count_tokens_in_container(iter, token);
> > +
> > +       /*
> > +        * Add one to include the object/array token itself. This
> > is required
> > +        * since 'current' points to the container initially. Only
> > after a call
> > +        * to json_iter_next() will 'current' point to the first
> > token in the
> > +        * container.
> > +        */
> > +       if (token->type == JSMN_OBJECT || token->type ==
> > JSMN_ARRAY)
> > +               child->count++;
> >   }
> 
> This seems a bit dangerous since count is set based on ITER_END in
> patch 1.  Is 
> there a chance this ends up allowing going past the allocated tokens?
> 
> I'm guessing you are trying to account for a slight difference in
> behavior of 
> json_iter depending on what type it points to:
> - if json_iter points to JSMN_PRIMITIVE,
> then json_iter_next should not be called (and should fail)
> - if json_iter points to JSMN_OBJECT/ARRAY, then json_iter_next has
> to be called 
> prior to obtaining the first element
> 
> Am I right so far?

Yes that is exactly it. Consider this array:

["foo", "bar"]

count_tokens_in_container() will return 2 tokens. count++ gives us 3
and 'start' points to the array itself.

Call json_iter_next():
	'start' is advanced by 1, pointing to "foo"

Call json_iter_next():
	'start' is advanced by 1, pointing to "bar"

Call json_iter_next():
	'start' is advanced by 1, pointing to some token after "bar".

	We then check:

	if (c->tokens + iter->current + inc >= ITER_END(iter))
		return false;

	And in this case iter->current + inc will equal ITER_END

I do see your point, and accessing a token at [count] would indeed be
bad. But that never actually happens, we bail out once we see current
equal count.

> 
> >   
> >   struct json_contents *json_contents_new(const char *json, size_t
> > json_len)
> 
> <snip>
> 
> > @@ -370,3 +382,45 @@ bool json_iter_get_null(struct json_iter
> > *iter)
> >   
> >         return false;
> >   }
> > +
> > +enum json_type json_iter_get_type(struct json_iter *iter)
> > +{
> > +       struct json_contents *c = iter->contents;
> > +       jsmntok_t *t = c->tokens + iter->current;
> > +
> > +       return (enum json_type) t->type;
> > +}
> > +
> > +bool json_iter_next(struct json_iter *iter)
> > +{
> > +       struct json_contents *c = iter->contents;
> > +       jsmntok_t *t = c->tokens + iter->current;
> > +       int inc = 1;
> > +
> > +       /*
> > +        * If this is the initial iteration skip this and just
> > increment
> > +        * current by 1 since this iterator points to a container
> > which needs to
> > +        * be advanced to the first token..
> > +        *
> > +        * In addition primitive types and empty containers will
> > have a size
> > +        * of 1, so no special handling is needed there.
> > +        *
> > +        * For non-empty containers 'current' needs to be advanced
> > by all the
> > +        * containers child tokens, plus the container itself.
> 
> One idea might be to use the 'size' variable of the start token to
> know whether 
> the iterator can be advanced or not.  Alternatively, look up the base
> type of 
> the iterator at the start token and modify the behavior here
> accordingly.

Are you talking about preventing primitive iteration? Or just
altertering how iteration is done in general?

The check below ensures the token is an iterable container, and size is
used in that check if only to detect empty objects/arrays, which would
then be treated exactly like a primitive/string.

I'm not sure 'size' can be trusted soley to know if its an iterable
object. For example a key's size is 1. I'm not sure how an iterator
could end up pointing there, but checking OBJECT/ARRAY explicitly seems
like the safest bet to me.

> 
> > +        *
> > +        * This check ensures:
> > +        *    1. It is not the initial iteration
> > +        *    2. This is a container
> > +        *    3. The container is not empty
> > +        */
> > +       if (iter->current != iter->start && ((t->type ==
> > JSMN_OBJECT ||
> > +                                       t->type == JSMN_ARRAY) &&
> > t->size))
> > +               inc = count_tokens_in_container(iter, t) + 1;
> > +
> > +       if (c->tokens + iter->current + inc >= ITER_END(iter))
> > +               return false;
> > +
> > +       iter->current += inc;
> > +
> > +       return true;
> > +}
> 
> Regards,
> -Denis

Re: [PATCH v2 4/9] json: add support for array iteration

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2532 bytes --]

Hi James,

>> Am I right so far?
> 
> Yes that is exactly it. Consider this array:
> 
> ["foo", "bar"]
> 
> count_tokens_in_container() will return 2 tokens. count++ gives us 3
> and 'start' points to the array itself.
> 

Ok, I think I managed to convince myself this is indeed okay, particularly since 
it is consistent with json_iter_init() behavior.

One thing you may to do is add a check to json_iter_parse to make sure it is 
operating on iterators of type JSMN_OBJECT and nothing else.  That function's 
use of ITER_END is what made me suspicious.

> Call json_iter_next():
> 	'start' is advanced by 1, pointing to "foo"
> 
> Call json_iter_next():
> 	'start' is advanced by 1, pointing to "bar"
> 

So in theory, you could use 'size' as a limit counter.  So for primitives, set 
pos to 1 and have iter_next check whether pos + 1 <= size.

For arrays/objects, set size to 0 and don't allow getters to work if pos is 0. 
Or something to that effect.  Not sure if this makes things easier or not.

> Call json_iter_next():
> 	'start' is advanced by 1, pointing to some token after "bar".
> 
> 	We then check:
> 
> 	if (c->tokens + iter->current + inc >= ITER_END(iter))
> 		return false;
> 
> 	And in this case iter->current + inc will equal ITER_END
> 

Yes, this all makes sense.  Why do we increment by count_tokens_in_container() + 
1 though?

<snip>

>> One idea might be to use the 'size' variable of the start token to
>> know whether
>> the iterator can be advanced or not.  Alternatively, look up the base
>> type of
>> the iterator at the start token and modify the behavior here
>> accordingly.
> 
> Are you talking about preventing primitive iteration? Or just
> altertering how iteration is done in general?

I think both.

> 
> The check below ensures the token is an iterable container, and size is
> used in that check if only to detect empty objects/arrays, which would
> then be treated exactly like a primitive/string.
> 
> I'm not sure 'size' can be trusted soley to know if its an iterable
> object. For example a key's size is 1. I'm not sure how an iterator
> could end up pointing there, but checking OBJECT/ARRAY explicitly seems
> like the safest bet to me.

I would assume (hope?) that size can be trusted.  I could be wrong, but seems to 
me like it would make some checks easier.

Are empty objects/arrays really of size 1?  How does one distinguish between an 
array with 1 element and an empty array?

Regards,
-Denis

Re: [PATCH v2 4/9] json: add support for array iteration

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 203 bytes --]

Hi James,

> Yes, this all makes sense.  Why do we increment by count_tokens_in_container() + 
> 1 though?

Ah nevermind, I gotcha.

Anyhow, I applied the rest of this series.

Regards,
-Denis