Commit b3e5cfd5 authored by Heikki Linnakangas's avatar Heikki Linnakangas

Jsonb comparison bug fixes.

Fix an over-zealous assertion, which didn't take into account that sometimes
a scalar element can be compared against an array/object element.

Avoid comparing possibly-uninitialized local variables when end-of-array or
end-of-object is reached. Also fix and enhance comments a bit.

Peter Geoghegan, per reports by Pavel Stehule and me.
parent 45b7abe5
...@@ -62,13 +62,13 @@ static void uniqueifyJsonbObject(JsonbValue *object); ...@@ -62,13 +62,13 @@ static void uniqueifyJsonbObject(JsonbValue *object);
* *
* There isn't a JsonbToJsonbValue(), because generally we find it more * There isn't a JsonbToJsonbValue(), because generally we find it more
* convenient to directly iterate through the Jsonb representation and only * convenient to directly iterate through the Jsonb representation and only
* really convert nested scalar values. formIterIsContainer() does this, so * really convert nested scalar values. JsonbIteratorNext() does this, so that
* that clients of the iteration code don't have to directly deal with the * clients of the iteration code don't have to directly deal with the binary
* binary representation (JsonbDeepContains() is a notable exception, although * representation (JsonbDeepContains() is a notable exception, although all
* all exceptions are internal to this module). In general, functions that * exceptions are internal to this module). In general, functions that accept
* accept a JsonbValue argument are concerned with the manipulation of scalar * a JsonbValue argument are concerned with the manipulation of scalar values,
* values, or simple containers of scalar values, where it would be * or simple containers of scalar values, where it would be inconvenient to
* inconvenient to deal with a great amount of other state. * deal with a great amount of other state.
*/ */
Jsonb * Jsonb *
JsonbValueToJsonb(JsonbValue *val) JsonbValueToJsonb(JsonbValue *val)
...@@ -137,13 +137,6 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b) ...@@ -137,13 +137,6 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b)
ra = JsonbIteratorNext(&ita, &va, false); ra = JsonbIteratorNext(&ita, &va, false);
rb = JsonbIteratorNext(&itb, &vb, false); rb = JsonbIteratorNext(&itb, &vb, false);
/*
* To a limited extent we'll redundantly iterate over an array/object
* while re-performing the same test without any reasonable
* expectation of the same container types having differing lengths
* (as when we process a WJB_BEGIN_OBJECT, and later the corresponding
* WJB_END_OBJECT), but no matter.
*/
if (ra == rb) if (ra == rb)
{ {
if (ra == WJB_DONE) if (ra == WJB_DONE)
...@@ -152,6 +145,17 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b) ...@@ -152,6 +145,17 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b)
break; break;
} }
if (ra == WJB_END_ARRAY || ra == WJB_END_OBJECT)
{
/*
* There is no array or object to compare at this stage of
* processing. jbvArray/jbvObject values are compared
* initially, at the WJB_BEGIN_ARRAY and WJB_BEGIN_OBJECT
* tokens.
*/
continue;
}
if (va.type == vb.type) if (va.type == vb.type)
{ {
switch (va.type) switch (va.type)
...@@ -194,19 +198,26 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b) ...@@ -194,19 +198,26 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b)
else else
{ {
/* /*
* It's safe to assume that the types differed. * It's safe to assume that the types differed, and that the va
* and vb values passed were set.
* *
* If the two values were the same container type, then there'd * If the two values were of the same container type, then there'd
* have been a chance to observe the variation in the number of * have been a chance to observe the variation in the number of
* elements/pairs (when processing WJB_BEGIN_OBJECT, say). They * elements/pairs (when processing WJB_BEGIN_OBJECT, say). They're
* can't be scalar types either, because then they'd have to be * either two heterogeneously-typed containers, or a container and
* contained in containers already ruled unequal due to differing * some scalar type.
* numbers of pairs/elements, or already directly ruled unequal *
* with a call to the underlying type's comparator. * We don't have to consider the WJB_END_ARRAY and WJB_END_OBJECT
* cases here, because we would have seen the corresponding
* WJB_BEGIN_ARRAY and WJB_BEGIN_OBJECT tokens first, and
* concluded that they don't match.
*/ */
Assert(ra != WJB_END_ARRAY && ra != WJB_END_OBJECT);
Assert(rb != WJB_END_ARRAY && rb != WJB_END_OBJECT);
Assert(va.type != vb.type); Assert(va.type != vb.type);
Assert(va.type == jbvArray || va.type == jbvObject); Assert(va.type != jbvBinary);
Assert(vb.type == jbvArray || vb.type == jbvObject); Assert(vb.type != jbvBinary);
/* Type-defined order */ /* Type-defined order */
res = (va.type > vb.type) ? 1 : -1; res = (va.type > vb.type) ? 1 : -1;
} }
...@@ -630,7 +641,9 @@ JsonbIteratorInit(JsonbContainer *container) ...@@ -630,7 +641,9 @@ JsonbIteratorInit(JsonbContainer *container)
* It is our job to expand the jbvBinary representation without bothering them * It is our job to expand the jbvBinary representation without bothering them
* with it. However, clients should not take it upon themselves to touch array * with it. However, clients should not take it upon themselves to touch array
* or Object element/pair buffers, since their element/pair pointers are * or Object element/pair buffers, since their element/pair pointers are
* garbage. * garbage. Also, *val will not be set when returning WJB_END_ARRAY or
* WJB_END_OBJECT, on the assumption that it's only useful to access values
* when recursing in.
*/ */
JsonbIteratorToken JsonbIteratorToken
JsonbIteratorNext(JsonbIterator **it, JsonbValue *val, bool skipNested) JsonbIteratorNext(JsonbIterator **it, JsonbValue *val, bool skipNested)
...@@ -686,7 +699,7 @@ recurse: ...@@ -686,7 +699,7 @@ recurse:
else else
{ {
/* /*
* Scalar item in array (or a container and caller didn't * Scalar item in array, or a container and caller didn't
* want us to recurse into it. * want us to recurse into it.
*/ */
return WJB_ELEM; return WJB_ELEM;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment