Added a validator for JSON v3 schemas.

chrome/common/json_schema/json_schema_validator.cc

Index: chrome/common/json_schema/json_schema_validator.cc
diff --git a/chrome/common/json_schema/json_schema_validator.cc b/chrome/common/json_schema/json_schema_validator.cc
index 6e5e070b40ddfd6033461171d4f962b1b2ed401e..96d48124354c4152b1213273e7d17723a07b89cd 100644
--- a/chrome/common/json_schema/json_schema_validator.cc
+++ b/chrome/common/json_schema/json_schema_validator.cc
@@ -7,7 +7,9 @@
 #include <cfloat>
 #include <cmath>
 
+#include "base/json/json_reader.h"
 #include "base/string_util.h"
+#include "base/stringprintf.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "chrome/common/json_schema/json_schema_constants.h"
@@ -24,6 +26,166 @@ double GetNumberValue(const Value* value) {
   return result;
 }
 
+bool IsValidType(const std::string& type) {
+  static const char* kValidTypes[] = {
+    schema::kAny,
+    schema::kArray,
+    schema::kBoolean,
+    schema::kInteger,
+    schema::kNull,
+    schema::kNumber,
+    schema::kObject,
+    schema::kString,
+  };
+  const char** end = kValidTypes + arraysize(kValidTypes);
+  return std::find(kValidTypes, end, type) != end;
+}
+
+bool IsValidSchema(const base::DictionaryValue* dict, std::string* error) {
+  static const struct {
+    const char* key;
+    base::Value::Type type;
+  } kExpectedTypes[] = {
+    { schema::kAdditionalProperties,    base::Value::TYPE_DICTIONARY },
+    { schema::kProperties,              base::Value::TYPE_DICTIONARY },
+
+    { schema::kMinimum,                 base::Value::TYPE_DOUBLE },
+    { schema::kMaximum,                 base::Value::TYPE_DOUBLE },
+
+    // All of these must be >= 0.
+    { schema::kMinItems,                base::Value::TYPE_INTEGER },
+    { schema::kMaxItems,                base::Value::TYPE_INTEGER },
+    { schema::kMinLength,               base::Value::TYPE_INTEGER },
+    { schema::kMaxLength,               base::Value::TYPE_INTEGER },
+
+    { schema::kEnum,                    base::Value::TYPE_LIST },
+    { schema::kChoices,                 base::Value::TYPE_LIST },
+
+    { schema::kId,                      base::Value::TYPE_STRING },
+    { schema::kRef,                     base::Value::TYPE_STRING },
+    { schema::kSchema,                  base::Value::TYPE_STRING },
+
+    { schema::kOptional,                base::Value::TYPE_BOOLEAN },
+  };
+
+  const base::Value* value = NULL;
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kExpectedTypes); ++i) {
+    if (!dict->Get(kExpectedTypes[i].key, &value))
+      continue;

[not at google - send to devlin, 2013/05/16 17:08:20]

the problem with this approach is that it won't inform of unsupported
properties, which is asking for typos. Could you invert this; have
kExpectedTypes be a map of schema value -> type and loop over |dict| instead?
That may also be more efficient.

Of course, making kExpectedTypes a map would require some kind of lazy
initialisation. You could do that, or do a trick and make kExpectedTypes sorted
and use binary_search on it, or something.

[Joao da Silva, 2013/05/19 13:16:29]

Good idea, done. "type" and "items" is still special-cased, since those
attributes have two legal types.

The flip-side of doing this is forward-compatibility, in case we want to support
other keys in the future. I've decided to do the stricter checks, since JSON
schema v3 is set in stone and the version is declared in the managed storage
schema; if we support a new format in the future then we can gate the checks on
the version.

Added two more attributes that were ignored before, but that we expect to see in
the schema ("title" and "description") to generate better UI and documentation
from the schema.
An std::map is usually a red-black tree, so doing a binary search over a static
array yields the same logn lookup time. I find that simpler than adding a lazily
initialized std::map.

+
+    if (!value->IsType(kExpectedTypes[i].type)) {
+      *error = base::StringPrintf("Invalid value for %s attribute",
+                                  kExpectedTypes[i].key);
+      return false;
+    }
+
+    // This applies to "minItems", "maxItems", "minLength" and "maxLength".
+    int integer_value;
+    if (value->GetAsInteger(&integer_value) && integer_value < 0) {
+      *error =
+          base::StringPrintf("Value of %s must be >= 0", kExpectedTypes[i].key);

[not at google - send to devlin, 2013/05/16 17:08:20]

might as well include integer_value in here.

[Joao da Silva, 2013/05/19 13:16:29]

Done.

+      return false;
+    }
+  }
+
+  // Validate "type" attribute.
+  std::string string_value;
+  const base::ListValue* list_value = NULL;
+  if (!dict->Get(schema::kType, &value)) {
+    *error = "Schema must have a type attribute";
+    return false;
+  } else if (value->GetAsString(&string_value)) {

[not at google - send to devlin, 2013/05/16 17:08:20]

else unnecessary after a return.

[Joao da Silva, 2013/05/19 13:16:29]

Done.

+    if (!IsValidType(string_value)) {
+      *error = "Invalid value for type attribute";
+      return false;
+    }
+  } else if (value->GetAsList(&list_value)) {

[not at google - send to devlin, 2013/05/16 17:08:20]

a switch might be nicer

[Joao da Silva, 2013/05/19 13:16:29]

Done.

+    for (size_t i = 0; i < list_value->GetSize(); ++i) {
+      if (!list_value->GetString(i, &string_value) ||
+          !IsValidType(string_value)) {
+        *error = "Invalid value for type attribute";
+        return false;
+      }
+    }
+  } else {
+    *error = "Invalid value for type attribute";
+    return false;
+  }
+
+  // Validate "properties" attribute. Each entry maps a key to a schema.
+  const base::DictionaryValue* dictionary_value = NULL;
+  if (dict->GetDictionary(schema::kProperties, &dictionary_value)) {
+    for (base::DictionaryValue::Iterator it(*dictionary_value);
+         !it.IsAtEnd(); it.Advance()) {
+      if (!it.value().GetAsDictionary(&dictionary_value) ||
+          !IsValidSchema(dictionary_value, error)) {
+        return false;

[not at google - send to devlin, 2013/05/16 17:08:20]

all these times you validate IsValidSchema and then return without setting error
make sense - but it may be nice to DCHECK_NE("", *error) each time as well. Or
that may be overkill.

[Joao da Silva, 2013/05/19 13:16:29]

That's a good idea, and there's a bug here too: if |it| is not a dictionary then
this fails without setting |error|. Fixed.

+      }
+    }
+  }
+
+  // Validate "additionalProperties" attribute, which is a schema.
+  if (dict->GetDictionary(schema::kAdditionalProperties, &dictionary_value) &&
+      !IsValidSchema(dictionary_value, error)) {
+    return false;
+  }
+
+  // Validate "items" attribute, which is a schema or a list of schemas.
+  if (dict->Get(schema::kItems, &value)) {
+    if (value->GetAsDictionary(&dictionary_value)) {
+      if (!IsValidSchema(dictionary_value, error))
+        return false;
+    } else if (value->GetAsList(&list_value)) {
+      for (size_t i = 0; i < list_value->GetSize(); ++i) {
+        if (!list_value->GetDictionary(i, &dictionary_value) ||
+            !IsValidSchema(dictionary_value, error)) {
+          return false;
+        }
+      }
+    } else {
+      *error = "Invalid value for items attribute";
+      return false;
+    }
+  }
+
+  // Validate the values contained in an "enum" attribute.
+  if (dict->GetList(schema::kEnum, &list_value)) {
+    for (size_t i = 0; i < list_value->GetSize(); ++i) {
+      list_value->Get(i, &value);
+      switch (value->GetType()) {
+        case base::Value::TYPE_NULL:    case base::Value::TYPE_BOOLEAN:
+        case base::Value::TYPE_STRING:  case base::Value::TYPE_INTEGER:
+        case base::Value::TYPE_DOUBLE:

[not at google - send to devlin, 2013/05/16 17:08:20]

nit: 1 case per line, this looks pretty odd.

[Joao da Silva, 2013/05/19 13:16:29]

Done.

+          break;
+        default:
+          *error = "Invalid value in enum attribute";
+          return false;
+      }
+    }
+  }
+
+  // Validate the schemas contained in a "choices" attribute.
+  if (dict->GetList(schema::kChoices, &list_value)) {
+    for (size_t i = 0; i < list_value->GetSize(); ++i) {
+      if (!list_value->GetDictionary(i, &dictionary_value)) {
+        *error = "Invalid choices attribute";
+        return false;
+      } else if (!IsValidSchema(dictionary_value, error)) {

[not at google - send to devlin, 2013/05/16 17:08:20]

else unnecessary after a return

[Joao da Silva, 2013/05/19 13:16:29]

Done.

+        return false;
+      }
+    }
+  }
+
+
+  // Unsupported attributes.

[not at google - send to devlin, 2013/05/16 17:08:20]

if you do the map thing right at the top then this wouldn't be necessary

[Joao da Silva, 2013/05/19 13:16:29]

Yep, done.

+  if (dict->HasKey(schema::kPattern)) {
+    *error = "pattern attribute is not supported";
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace
 
 
@@ -121,6 +283,26 @@ std::string JSONSchemaValidator::FormatErrorMessage(const std::string& format,
   return ret_val;
 }
 
+// static
+scoped_ptr<base::DictionaryValue> JSONSchemaValidator::IsValidSchema(
+    const std::string& schema,
+    std::string* error) {
+  base::JSONParserOptions options = base::JSON_PARSE_RFC;
+  scoped_ptr<base::Value> json(
+      base::JSONReader::ReadAndReturnError(schema, options, NULL, error));
+  if (!json)
+    return scoped_ptr<base::DictionaryValue>();
+  base::DictionaryValue* dict = NULL;
+  if (!json->GetAsDictionary(&dict)) {
+    *error = "Schema must be a JSON object";
+    return scoped_ptr<base::DictionaryValue>();
+  }
+  if (!::IsValidSchema(dict, error))
+    return scoped_ptr<base::DictionaryValue>();
+  ignore_result(json.release());
+  return make_scoped_ptr(dict);

[not at google - send to devlin, 2013/05/16 17:08:20]

does just

return json.PassAs<base::DictionaryValue>() work?

[Joao da Silva, 2013/05/19 13:16:29]

It doesn't; PassAs<>() can only do upcasts.

+}
+
 JSONSchemaValidator::JSONSchemaValidator(DictionaryValue* schema)
     : schema_root_(schema), default_allow_additional_properties_(false) {
 }