Re: [libvirt] [PATCH v2 1/4] util: Introduce virJSONStringCompare for JSON doc comparisons

[Jim Fehlig <jfehlig@suse.com>]

Daniel P. Berrange wrote:
> Comparing JSON docs using strcmp is simple, but is not flexible
> as it is sensitive to whitespace used in the doc generation.
> When comparing objects it may also be desirable to treat the
> existance of keys in the actual object but not expected object
> as non-fatal. Introduce a virJSONStringCompare function which
> takes two strings representing expected and actual JSON docs
> and then does a DOM comparison.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  src/libvirt_private.syms |   1 +
>  src/util/virjson.c       | 185 +++++++++++++++++++++++++++++++++++++++++++++++
>   

I'm not too familiar with this file, but didn't notice any problems with
this patch beyond a little nit below.

>  src/util/virjson.h       |  22 ++++++
>  3 files changed, 208 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 4c1f61c..5f2b9d9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1406,6 +1406,7 @@ virISCSIScanTargets;
>  
>  
>  # util/virjson.h
> +virJSONStringCompare;
>  virJSONValueArrayAppend;
>  virJSONValueArrayGet;
>  virJSONValueArraySize;
> diff --git a/src/util/virjson.c b/src/util/virjson.c
> index 35a8252..0a6d1be 100644
> --- a/src/util/virjson.c
> +++ b/src/util/virjson.c
> @@ -47,6 +47,11 @@
>  
>  VIR_LOG_INIT("util.json");
>  
> +VIR_ENUM_DECL(virJSONType)
> +VIR_ENUM_IMPL(virJSONType, VIR_JSON_TYPE_LAST,
> +              "object", "array", "string",
> +              "number", "boolean", "null")
> +
>  typedef struct _virJSONParserState virJSONParserState;
>  typedef virJSONParserState *virJSONParserStatePtr;
>  struct _virJSONParserState {
> @@ -90,6 +95,7 @@ void virJSONValueFree(virJSONValuePtr value)
>          break;
>      case VIR_JSON_TYPE_BOOLEAN:
>      case VIR_JSON_TYPE_NULL:
> +    case VIR_JSON_TYPE_LAST:
>          break;
>      }
>  
> @@ -1152,3 +1158,182 @@ char *virJSONValueToString(virJSONValuePtr object ATTRIBUTE_UNUSED,
>      return NULL;
>  }
>  #endif
> +
> +
> +static bool
> +virJSONValueCompare(virJSONValuePtr expect,
> +                    virJSONValuePtr actual,
> +                    const char *context,
> +                    unsigned int flags)
> +{
> +    size_t i, j;
> +    if (expect->type != actual->type) {
>   

Super nit: seems most of libvirt code would have a line between local
variable declarations and start of code, but this file is not consistent
in that regard anyhow.

ACK.

Regards,
Jim

> +        if (expect->type == VIR_JSON_TYPE_NULL &&
> +            (flags & VIR_JSON_COMPARE_IGNORE_EXPECTED_NULL))
> +            return true;
> +
> +        const char *expectType = virJSONTypeTypeToString(expect->type);
> +        const char *actualType = virJSONTypeTypeToString(actual->type);
> +
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Expected value type '%s' but got value type '%s' at '%s'"),
> +                       expectType, actualType, context);
> +        return false;
> +    }
> +
> +    switch (expect->type) {
> +    case VIR_JSON_TYPE_OBJECT:
> +        for (i = 0; i < expect->data.object.npairs; i++) {
> +            bool found = false;
> +            char *childcontext;
> +            for (j = 0; j < actual->data.object.npairs; j++) {
> +                if (STREQ(expect->data.object.pairs[i].key,
> +                          actual->data.object.pairs[j].key)) {
> +                    found = true;
> +                    break;
> +                }
> +            }
> +            if (!found) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Expected object key '%s' not found in actual object at '%s'"),
> +                               expect->data.object.pairs[i].key, context);
> +                return false;
> +            }
> +
> +            if (virAsprintf(&childcontext, "%s%s%s",
> +                            context,
> +                            STREQ(context, "/") ? "" : "/",
> +                            expect->data.object.pairs[i].key) < 0)
> +                return false;
> +
> +            if (!virJSONValueCompare(expect->data.object.pairs[i].value,
> +                                     actual->data.object.pairs[j].value,
> +                                     childcontext,
> +                                     flags)) {
> +                VIR_FREE(childcontext);
> +                return false;
> +            }
> +            VIR_FREE(childcontext);
> +        }
> +        if (!(flags & VIR_JSON_COMPARE_IGNORE_EXTRA_OBJECT_KEYS)) {
> +            for (i = 0; i < actual->data.object.npairs; i++) {
> +                bool found = false;
> +                char *childcontext;
> +                for (j = 0; j < expect->data.object.npairs; j++) {
> +                    if (STREQ(actual->data.object.pairs[i].key,
> +                              expect->data.object.pairs[j].key)) {
> +                        found = true;
> +                        break;
> +                    }
> +                }
> +                if (!found) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("Actual object key '%s' not found in expected object at '%s'"),
> +                                   actual->data.object.pairs[i].key, context);
> +                    return false;
> +                }
> +
> +                if (virAsprintf(&childcontext, "%s%s%s",
> +                                STREQ(context, "/") ? "" : "/",
> +                                context, actual->data.object.pairs[i].key) < 0)
> +                    return false;
> +
> +                if (!virJSONValueCompare(actual->data.object.pairs[i].value,
> +                                         expect->data.object.pairs[j].value,
> +                                         childcontext,
> +                                         flags)) {
> +                    VIR_FREE(childcontext);
> +                    return false;
> +                }
> +                VIR_FREE(childcontext);
> +            }
> +        }
> +        break;
> +    case VIR_JSON_TYPE_ARRAY:
> +        if (expect->data.array.nvalues !=
> +            actual->data.array.nvalues) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Expected array size '%zu' but got size '%zu' at '%s'"),
> +                           expect->data.array.nvalues,
> +                           actual->data.array.nvalues,
> +                           context);
> +            return false;
> +        }
> +
> +        for (i = 0; i < expect->data.array.nvalues; i++) {
> +            char *childcontext;
> +            if (virAsprintf(&childcontext, "%s%s[%zu]",
> +                            STREQ(context, "/") ? "" : "/",
> +                            context, i) < 0)
> +                return false;
> +
> +            if (!virJSONValueCompare(expect->data.array.values[i],
> +                                     actual->data.array.values[i],
> +                                     childcontext,
> +                                     flags)) {
> +                VIR_FREE(childcontext);
> +                return false;
> +            }
> +            VIR_FREE(childcontext);
> +        }
> +        break;
> +    case VIR_JSON_TYPE_STRING:
> +        if (STRNEQ(expect->data.string,
> +                   actual->data.string)) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Expected string value '%s' but got '%s' at '%s'"),
> +                           expect->data.string, actual->data.string, context);
> +            return false;
> +        }
> +        break;
> +    case VIR_JSON_TYPE_NUMBER:
> +        if (STRNEQ(expect->data.number,
> +                   actual->data.number)) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Expected number value '%s' but got '%s' at '%s'"),
> +                           expect->data.number, actual->data.number, context);
> +            return false;
> +        }
> +        break;
> +    case VIR_JSON_TYPE_BOOLEAN:
> +        if (expect->data.boolean !=
> +            actual->data.boolean) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Expected bool value '%d' but got '%d' at '%s'"),
> +                           expect->data.boolean, actual->data.boolean, context);
> +            return false;
> +        }
> +        break;
> +    case VIR_JSON_TYPE_NULL:
> +        /* trivially equal */
> +        break;
> +
> +    case VIR_JSON_TYPE_LAST:
> +        /* nothing */
> +        break;
> +    }
> +
> +    return true;
> +}
> +
> +
> +bool virJSONStringCompare(const char *expect,
> +                          const char *actual,
> +                          unsigned int flags)
> +{
> +    virJSONValuePtr expectVal = NULL;
> +    virJSONValuePtr actualVal = NULL;
> +    int ret = false;
> +
> +    if (!(expectVal = virJSONValueFromString(expect)))
> +        goto cleanup;
> +    if (!(actualVal = virJSONValueFromString(actual)))
> +        goto cleanup;
> +
> +    ret = virJSONValueCompare(expectVal, actualVal, "/", flags);
> +
> + cleanup:
> +    virJSONValueFree(expectVal);
> +    virJSONValueFree(actualVal);
> +    return ret;
> +}
> diff --git a/src/util/virjson.h b/src/util/virjson.h
> index db11396..b1f96d3 100644
> --- a/src/util/virjson.h
> +++ b/src/util/virjson.h
> @@ -34,6 +34,8 @@ typedef enum {
>      VIR_JSON_TYPE_NUMBER,
>      VIR_JSON_TYPE_BOOLEAN,
>      VIR_JSON_TYPE_NULL,
> +
> +    VIR_JSON_TYPE_LAST,
>  } virJSONType;
>  
>  typedef struct _virJSONValue virJSONValue;
> @@ -139,4 +141,24 @@ virJSONValuePtr virJSONValueFromString(const char *jsonstring);
>  char *virJSONValueToString(virJSONValuePtr object,
>                             bool pretty);
>  
> +typedef enum {
> +    /*
> +     * When comparing a pair of Objects, if the 'actual' object
> +     * has keys that are not present in the 'expected' object,
> +     * the existance of these extra keys will be non-fatal.
> +     */
> +    VIR_JSON_COMPARE_IGNORE_EXTRA_OBJECT_KEYS = (1 << 0),
> +
> +    /*
> +     * when comparing two values, if their types are different,
> +     * and the 'expected' value type is 'null', then this will
> +     * be considered non-fatal.
> +     */
> +    VIR_JSON_COMPARE_IGNORE_EXPECTED_NULL = (1 << 1),
> +} virJSONCompareFlags;
> +
> +bool virJSONStringCompare(const char *expect,
> +                          const char *actual,
> +                          unsigned int flags);
> +
>  #endif /* __VIR_JSON_H_ */
>