Improve error message display for Schema::Validate()

components/policy/core/common/schema.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/policy/core/common/schema.h"
 
 #include <algorithm>
 #include <climits>
 #include <map>
 #include <utility>
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/memory/scoped_vector.h"
+#include "base/strings/stringprintf.h"
 #include "components/json_schema/json_schema_constants.h"
 #include "components/json_schema/json_schema_validator.h"
 #include "components/policy/core/common/schema_internal.h"
 
 namespace schema = json_schema_constants;
 
 namespace policy {
 
 using internal::PropertiesNode;
 using internal::PropertyNode;
@@ skipping 71 matching lines @@
     SCHEMA_STRICT,         // SCHEMA_STRICT
     SCHEMA_STRICT,         // SCHEMA_ALLOW_UNKNOWN_TOPLEVEL
     SCHEMA_ALLOW_UNKNOWN,  // SCHEMA_ALLOW_UNKNOWN
     SCHEMA_STRICT,         // SCHEMA_ALLOW_INVALID_TOPLEVEL
     SCHEMA_ALLOW_UNKNOWN,  // SCHEMA_ALLOW_INVALID_TOPLEVEL_AND_ALLOW_UNKNOWN
     SCHEMA_ALLOW_INVALID,  // SCHEMA_ALLOW_INVALID
   };
   return next_level_strategy[(int)strategy];
 }
 
+void SchemaFindError(std::string* error_path,std::string *error,

[Joao da Silva, 2014/01/23 20:59:31]

A better name for this is ErrorFound

[binjin, 2014/01/23 21:21:58]

Done.

+    const std::string& msg) {

[Joao da Silva, 2014/01/23 20:59:31]

Format this. One argument per line:

void SchemaFindError(std::string* error_path,
                     std::string* error,
                     const std::string& msg) {

[binjin, 2014/01/23 21:21:58]

Done.

+  if (error_path)
+    *error_path = "";
+  *error = msg;
+}
+
+void AddListIndexPrefixToPath(int index, std::string* path) {
+  if (path) {
+    if (path->size() == 0)

[Joao da Silva, 2014/01/23 20:59:31]

path->empty

[binjin, 2014/01/23 21:21:58]

Done.

+      *path = base::StringPrintf("items[%d]", index);
+    else
+      *path = base::StringPrintf("items[%d].", index) + *path;

[Joao da Silva, 2014/01/23 20:59:31]

Maybe just "[%d]" without the "items" looks OK?

E.g. "URLBlacklist[4].filter" instead of "URLBlacklist.items[4].filter"

[binjin, 2014/01/23 21:21:58]

root schema can be a list, and things like "[4].first" looks odd to me. 

+  }
+}
+
+void AddDictKeyPrefixToPath(const std::string& key, std::string* path) {
+  if (path) {
+    if (path->size() == 0)

[Joao da Silva, 2014/01/23 20:59:31]

path->empty

[binjin, 2014/01/23 21:21:58]

Done.

+      *path = key;
+    else
+      *path = key + "." + *path;
+  }
+}
+
 }  // namespace
 
 // Contains the internal data representation of a Schema. This can either wrap
 // a SchemaData owned elsewhere (currently used to wrap the Chrome schema, which
 // is generated at compile time), or it can own its own SchemaData.
 class Schema::InternalStorage
     : public base::RefCountedThreadSafe<InternalStorage> {
  public:
   static scoped_refptr<const InternalStorage> Wrap(const SchemaData* data);
 
@@ skipping 509 matching lines @@
 }
 
 // static
 Schema Schema::Wrap(const SchemaData* data) {
   scoped_refptr<const InternalStorage> storage = InternalStorage::Wrap(data);
   return Schema(storage, storage->root_node());
 }
 
 bool Schema::Validate(const base::Value& value,
                       SchemaOnErrorStrategy strategy,
+                      std::string* error_path,
                       std::string* error) const {
   if (!valid()) {
-    *error = "The schema is invalid.";
+    SchemaFindError(error_path, error, "The schema is invalid.");
     return false;
   }
 
   if (!value.IsType(type())) {
     // Allow the integer to double promition. Note that range restriction on
     // double is not supported now.
     if (value.IsType(base::Value::TYPE_INTEGER) &&
         type() == base::Value::TYPE_DOUBLE)
       return true;
 
-    *error = "The value type doesn't match the schema type.";
+    SchemaFindError(error_path, error,
+        "The value type doesn't match the schema type.");

[Joao da Silva, 2014/01/23 20:59:31]

Format this line.

[binjin, 2014/01/23 21:21:58]

Done.

     return false;
   }
 
   const base::DictionaryValue* dict = NULL;
   const base::ListValue* list = NULL;
   int int_value;
   std::string str_value;
   if (value.GetAsDictionary(&dict)) {
     for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd();
          it.Advance()) {
       Schema subschema = GetProperty(it.key());
       if (!subschema.valid()) {
         // Unknown property was detected.
-        if (!StrategyAllowUnknownOnTopLevel(strategy)) {
-          *error = "Unknown property: " + it.key();
+        SchemaFindError(error_path, error, "Unknown property: " + it.key());
+        if (!StrategyAllowUnknownOnTopLevel(strategy))
           return false;
-        }
-      } else {
-        std::string sub_error;
-        if (!subschema.Validate(
-                 it.value(), StrategyForNextLevel(strategy), &sub_error)) {
-          // Invalid property was detected.
-          if (!StrategyAllowInvalidOnTopLevel(strategy)) {
-            *error = sub_error;
-            return false;
-          }
-        }
+      } else if (!subschema.Validate(it.value(),
+          StrategyForNextLevel(strategy), error_path, error)) {

[Joao da Silva, 2014/01/23 20:59:31]

Format this line

[binjin, 2014/01/23 21:21:58]

Done.

+        // Invalid property was detected.
+        AddDictKeyPrefixToPath(it.key(), error_path);
+        if (!StrategyAllowInvalidOnTopLevel(strategy))
+          return false;
       }
     }
   } else if (value.GetAsList(&list)) {
     for (base::ListValue::const_iterator it = list->begin(); it != list->end();
          ++it) {
       if (!*it ||
-          !GetItems().Validate(**it, StrategyForNextLevel(strategy), error)) {
+          !GetItems().Validate(
+               **it, StrategyForNextLevel(strategy), error_path, error)) {
         // Invalid list item was detected.
+        AddListIndexPrefixToPath(it - list->begin(), error_path);
         if (!StrategyAllowInvalidOnTopLevel(strategy))
           return false;
       }
     }
   } else if (value.GetAsInteger(&int_value)) {
     if (node_->extra != kInvalid &&
         !ValidateIntegerRestriction(node_->extra, int_value)) {
-      *error = "Invalid value for integer";
+      SchemaFindError(error_path, error, "Invalid value for integer");
       return false;
     }
   } else if (value.GetAsString(&str_value)) {
     if (node_->extra != kInvalid &&
         !ValidateStringRestriction(node_->extra, str_value.c_str())) {
-      *error = "Invalid value for string";
+      SchemaFindError(error_path, error, "Invalid value for string");
       return false;
     }
   }
 
   return true;
 }
 
 bool Schema::Normalize(base::Value* value,
                        SchemaOnErrorStrategy strategy,
+                       std::string* error_path,
                        std::string* error) const {
   if (!valid()) {
-    *error = "The schema is invalid.";
+    SchemaFindError(error_path, error, "The schema is invalid.");
     return false;
   }
 
 
   if (!value->IsType(type())) {
     // Allow the integer to double promition. Note that range restriction on
     // double is not supported now.
     if (value->IsType(base::Value::TYPE_INTEGER) &&
         type() == base::Value::TYPE_DOUBLE)
       return true;
 
-    *error = "The value type doesn't match the schema type.";
+    SchemaFindError(error_path, error,
+        "The value type doesn't match the schema type.");

[Joao da Silva, 2014/01/23 20:59:31]

Format this line

[binjin, 2014/01/23 21:21:58]

Done.

     return false;
   }
 
   base::DictionaryValue* dict = NULL;
   base::ListValue* list = NULL;
   if (value->GetAsDictionary(&dict)) {
     std::vector<std::string> drop_list;  // Contains the keys to drop.
     for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd();
          it.Advance()) {
       Schema subschema = GetProperty(it.key());
       if (!subschema.valid()) {
         // Unknown property was detected.
-        if (StrategyAllowUnknownOnTopLevel(strategy)) {
+        SchemaFindError(error_path, error, "Unknown property: " + it.key());
+        if (StrategyAllowUnknownOnTopLevel(strategy))
           drop_list.push_back(it.key());
-        } else {
-          *error = "Unknown property: " + it.key();
+        else
           return false;
-        }
       } else {
         base::Value* sub_value = NULL;
         dict->GetWithoutPathExpansion(it.key(), &sub_value);
-        std::string sub_error;
-        if (!subschema.Normalize(
-                 sub_value, StrategyForNextLevel(strategy), &sub_error)) {
+        if (!subschema.Normalize(sub_value,
+                                 StrategyForNextLevel(strategy),
+                                 error_path,
+                                 error)) {
           // Invalid property was detected.
-          if (StrategyAllowInvalidOnTopLevel(strategy)) {
+          AddDictKeyPrefixToPath(it.key(), error_path);
+          if (StrategyAllowInvalidOnTopLevel(strategy))
             drop_list.push_back(it.key());
-          } else {
-            *error = sub_error;
+          else
             return false;
-          }
         }
       }
     }
     for (std::vector<std::string>::const_iterator it = drop_list.begin();
          it != drop_list.end();
          ++it) {
       dict->RemoveWithoutPathExpansion(*it, NULL);
     }
     return true;
   } else if (value->GetAsList(&list)) {
     std::vector<size_t> drop_list;  // Contains the indexes to drop.
     for (size_t index = 0; index < list->GetSize(); index++) {
       base::Value* sub_value = NULL;
-      std::string sub_error;
       list->Get(index, &sub_value);
       if (!sub_value ||
           !GetItems().Normalize(
-               sub_value, StrategyForNextLevel(strategy), &sub_error)) {
+               sub_value, StrategyForNextLevel(strategy), error_path, error)) {
         // Invalid list item was detected.
-        if (StrategyAllowInvalidOnTopLevel(strategy)) {
+        AddListIndexPrefixToPath(index, error_path);
+        if (StrategyAllowInvalidOnTopLevel(strategy))
           drop_list.push_back(index);
-        } else {
-          *error = sub_error;
+        else
           return false;
-        }
       }
     }
     for (std::vector<size_t>::reverse_iterator it = drop_list.rbegin();
          it != drop_list.rend(); ++it) {
       list->Remove(*it, NULL);
     }
     return true;
   }
 
-  return Validate(*value, strategy, error);
+  return Validate(*value, strategy, error_path, error);
 }
 
 // static
 Schema Schema::Parse(const std::string& content, std::string* error) {
   // Validate as a generic JSON schema, and ignore unknown attributes; they
   // may become used in a future version of the schema format.
   scoped_ptr<base::DictionaryValue> dict = JSONSchemaValidator::IsValidSchema(
       content, JSONSchemaValidator::OPTIONS_IGNORE_UNKNOWN_ATTRIBUTES, error);
   if (!dict)
     return Schema();
@@ skipping 84 matching lines @@
   } else {
     for (int i = rnode->enumeration_restriction.offset_begin;
          i < rnode->enumeration_restriction.offset_end; i++) {
       if (*storage_->int_enums(i) == value)
         return true;
     }
     return false;
   }
 }
 
-bool Schema::ValidateStringRestriction(int index, const char *str) const {
+bool Schema::ValidateStringRestriction(int index, const char* str) const {
   const RestrictionNode* rnode = storage_->restriction(index);
   for (int i = rnode->enumeration_restriction.offset_begin;
        i < rnode->enumeration_restriction.offset_end; i++) {
     if (strcmp(*storage_->string_enums(i), str) == 0)
       return true;
   }
   return false;
 }
 
 }  // namespace policy