refactor: cleanup, formatting, annotations

include/jvalidate/validation_result.h

@@ -3,6 +3,7 @@
 #include <map>
 #include <ostream>
 #include <unordered_set>
+#include <utility>
 #include <variant>
 #include <vector>
 
@@ -17,7 +18,7 @@ public:
   using SchemaPointer = detail::Pointer;
   using Annotation = std::variant<std::string, std::vector<std::string>>;
   struct LocalResult {
-    /* bool valid; */
+    bool valid;
     std::map<std::string, Annotation> errors;
     std::map<std::string, Annotation> annotations;
   };
@@ -35,22 +36,26 @@ public:
   };
 
 private:
+  bool valid_;
   std::map<DocPointer, std::map<SchemaPointer, LocalResult>> results_;
 
 public:
   friend std::ostream & operator<<(std::ostream & os, ValidationResult const & result) {
-    os << "{\n" << indent(1) << R"("details": [)" << '\n';
+    char const * div = "\n";
+    os << "{\n" << indent(1) << R"("valid": )" << (result.valid_ ? "true" : "false") << ',' << '\n';
+    os << indent(1) << R"("details": [)";
     for (auto const & [doc_path, by_schema] : result.results_) {
       for (auto const & [schema_path, local] : by_schema) {
-        os << indent(2) << '{' << '\n';
+        os << std::exchange(div, ",\n") << indent(2) << '{' << '\n';
+        os << indent(3) << R"("valid": )" << (local.valid ? "true" : "false") << ',' << '\n';
         os << indent(3) << R"("evaluationPath": ")" << schema_path << '"' << ',' << '\n';
         os << indent(3) << R"("instanceLocation": ")" << doc_path << '"';
         print(os, local.annotations, "annotations", 3);
         print(os, local.errors, "errors", 3);
-        os << '\n' << indent(2) << '}' << '\n';
+        os << '\n' << indent(2) << '}';
       }
     }
-    return os << indent(1) << ']' << '\n' << '}';
+    return os << '\n' << indent(1) << ']' << '\n' << '}';
   }
 
   static void print(std::ostream & os, std::map<std::string, Annotation> const & named,
@@ -67,10 +72,10 @@ public:
       } else if (auto const * vec = std::get_if<1>(&anno)) {
         os << '[';
         char const * div = "\n";
-        for (size_t i = 0; i < vec->size(); ++i) {
-          os << std::exchange(div, ",\n") << indent(i + 2) << '"' << vec->at(i) << '"';
+        for (size_t n = 0; n < vec->size(); ++n) {
+          os << std::exchange(div, ",\n") << indent(i + 2) << '"' << vec->at(n) << '"';
         }
-        os << indent(i + 1) << ']';
+        os << '\n' << indent(i + 1) << ']';
       }
       os << '\n';
     }
@@ -125,13 +130,28 @@ private:
     }
   }
 
+  void valid(detail::Pointer const & where, detail::Pointer const & schema_path, bool valid) {
+    if (has(where, schema_path)) {
+      results_[where][schema_path].valid = valid;
+    }
+    if (where.empty() && schema_path.empty()) {
+      valid_ = valid;
+    }
+  }
+
   void error(detail::Pointer const & where, detail::Pointer const & schema_path,
              std::string const & name, Annotation message) {
+    if (std::visit([](auto const & v) { return v.empty(); }, message)) {
+      return;
+    }
     results_[where][schema_path].errors.emplace(name, std::move(message));
   }
 
   void annotate(detail::Pointer const & where, detail::Pointer const & schema_path,
                 std::string const & name, Annotation message) {
+    if (std::visit([](auto const & v) { return v.empty(); }, message)) {
+      return;
+    }
     results_[where][schema_path].annotations.emplace(name, std::move(message));
   }
 };

include/jvalidate/validation_visitor.h

@@ -3,6 +3,7 @@
 #include <tuple>
 #include <type_traits>
 #include <unordered_map>
+#include <vector>
 
 #include <jvalidate/constraint/array_constraint.h>
 #include <jvalidate/constraint/general_constraint.h>
@@ -24,6 +25,15 @@
 
 #define VISITED(type) std::get<std::unordered_set<type>>(*visited_)
 
+#define VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(subschema, subinstance, path, local_visited)       \
+  do {                                                                                             \
+    Status const partial = validate_subschema_on(subschema, subinstance, path);                    \
+    rval &= partial;                                                                               \
+    if (result_ and partial != Status::Noop) {                                                     \
+      local_visited.insert(local_visited.end(), path);                                             \
+    }                                                                                              \
+  } while (false)
+
 #define NOOP_UNLESS_TYPE(etype)                                                                    \
   RETURN_UNLESS(adapter::Type::etype == document_.type(), Status::Noop)
 
@@ -159,6 +169,8 @@ public:
       scoped_state(tracking_, StoreResults::ForAnything);
       return validate_subschema(cons.if_constraint);
     }();
+
+    annotate(if_true ? "valid" : "invalid");
     if (if_true) {
       return validate_subschema(cons.then_constraint, detail::parent, "then");
     }
@@ -218,9 +230,9 @@ public:
     NOOP_UNLESS_TYPE(String);
     std::string const str = document_.as_string();
     if (int64_t len = detail::length(str); len > cons.value) {
-      return result(Status::Reject, "'", str, "' of length ", len, " is >", cons.value);
+      return result(Status::Reject, "string of length ", len, " is >", cons.value);
     } else {
-      return result(Status::Accept, "'", str, "' of length ", len, " is <=", cons.value);
+      return result(Status::Accept, "string of length ", len, " is <=", cons.value);
     }
   }
 
@@ -228,9 +240,9 @@ public:
     NOOP_UNLESS_TYPE(String);
     std::string const str = document_.as_string();
     if (int64_t len = detail::length(str); len < cons.value) {
-      return result(Status::Reject, "'", str, "' of length ", len, " is <", cons.value);
+      return result(Status::Reject, "string of length ", len, " is <", cons.value);
     } else {
-      return result(Status::Accept, "'", str, "' of length ", len, " is >=", cons.value);
+      return result(Status::Accept, "string of length ", len, " is >=", cons.value);
     }
   }
 
@@ -240,9 +252,9 @@ public:
     RE const & regex = regex_cache_.try_emplace(cons.regex, cons.regex).first->second;
     std::string const str = document_.as_string();
     if (regex.search(str)) {
-      return result(Status::Accept, "'", str, "' matches pattern /", cons.regex, "/");
+      return result(Status::Accept, "string matches pattern /", cons.regex, "/");
     }
-    return result(Status::Reject, "'", str, "' does not match pattern /", cons.regex, "/");
+    return result(Status::Reject, "string does not match pattern /", cons.regex, "/");
   }
 
   Status visit(constraint::FormatConstraint const & cons) const {
@@ -264,11 +276,13 @@ public:
     auto array = document_.as_array();
 
     Status rval = Status::Accept;
+    std::vector<size_t> items;
     for (size_t i = cons.applies_after_nth; i < array.size(); ++i) {
-      rval &= validate_subschema_on(cons.subschema, array[i], i);
+      VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(cons.subschema, array[i], i, items);
       BREAK_EARLY_IF_NO_RESULT_TREE();
     }
 
+    annotate_list(items);
     return rval;
   }
 
@@ -286,12 +300,12 @@ public:
     }
 
     if (matches < minimum) {
-      return result(Status::Reject, "array contains <", minimum, " matching elements");
+      return result(Status::Reject, "array contains <", minimum, " matching items");
     }
     if (matches > maximum) {
-      return result(Status::Reject, "array contains >", maximum, " matching elements");
+      return result(Status::Reject, "array contains >", maximum, " matching items");
     }
-    return Status::Accept;
+    return result(Status::Accept, "array contains ", matches, " matching items");
   }
 
   Status visit(constraint::MaxItemsConstraint const & cons) const {
@@ -317,13 +331,16 @@ public:
 
     Status rval = Status::Accept;
 
-    auto array = document_.as_array();
-    size_t const n = std::min(cons.items.size(), array.size());
-    for (size_t i = 0; i < n; ++i) {
-      rval &= validate_subschema_on(cons.items[i], array[i], i);
+    std::vector<size_t> items;
+    for (auto const & [index, item] : detail::enumerate(document_.as_array())) {
+      if (index >= cons.items.size()) {
+        break;
+      }
+      VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(cons.items[index], item, index, items);
       BREAK_EARLY_IF_NO_RESULT_TREE();
     }
 
+    annotate_list(items);
     return rval;
   }
 
@@ -365,13 +382,15 @@ public:
     };
 
     Status rval = Status::Accept;
+    std::vector<std::string> properties;
     for (auto const & [key, elem] : document_.as_object()) {
       if (not cons.properties.contains(key) && not matches_any_pattern(key)) {
-        rval &= validate_subschema_on(cons.subschema, elem, key);
+        VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(cons.subschema, elem, key, properties);
       }
       BREAK_EARLY_IF_NO_RESULT_TREE();
     }
 
+    annotate_list(properties);
     return rval;
   }
 
@@ -426,17 +445,20 @@ public:
   Status visit(constraint::PatternPropertiesConstraint const & cons) const {
     NOOP_UNLESS_TYPE(Object);
 
+    std::vector<std::string> properties;
     Status rval = Status::Accept;
     for (auto const & [pattern, subschema] : cons.properties) {
       RE const & regex = regex_cache_.try_emplace(pattern, pattern).first->second;
       for (auto const & [key, elem] : document_.as_object()) {
-        if (regex.search(key)) {
-          rval &= validate_subschema_on(subschema, elem, key);
+        if (not regex.search(key)) {
+          continue;
         }
+        VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(subschema, elem, key, properties);
         BREAK_EARLY_IF_NO_RESULT_TREE();
       }
     }
 
+    annotate_list(properties);
     return rval;
   }
 
@@ -455,13 +477,15 @@ public:
       }
     }
 
+    std::vector<std::string> properties;
     for (auto const & [key, elem] : object) {
       if (auto it = cons.properties.find(key); it != cons.properties.end()) {
-        rval &= validate_subschema_on(it->second, elem, key);
+        VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(it->second, elem, key, properties);
       }
       BREAK_EARLY_IF_NO_RESULT_TREE();
     }
 
+    annotate_list(properties);
     return rval;
   }
 
@@ -499,14 +523,15 @@ public:
     }
 
     Status rval = Status::Accept;
-    auto array = document_.as_array();
-    for (size_t i = 0; i < array.size(); ++i) {
-      if (not VISITED(size_t).contains(i)) {
-        rval &= validate_subschema_on(cons.subschema, array[i], i);
+    std::vector<size_t> items;
+    for (auto const & [index, item] : detail::enumerate(document_.as_array())) {
+      if (not VISITED(size_t).contains(index)) {
+        VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(cons.subschema, item, index, items);
       }
       BREAK_EARLY_IF_NO_RESULT_TREE();
     }
 
+    annotate_list(items);
     return rval;
   }
 
@@ -517,26 +542,30 @@ public:
     }
 
     Status rval = Status::Accept;
+    std::vector<std::string> properties;
     for (auto const & [key, elem] : document_.as_object()) {
       if (not VISITED(std::string).contains(key)) {
-        rval &= validate_subschema_on(cons.subschema, elem, key);
+        VALIDATE_SUBSCHEMA_AND_MARK_LOCAL_VISIT(cons.subschema, elem, key, properties);
       }
       BREAK_EARLY_IF_NO_RESULT_TREE();
     }
 
+    annotate_list(properties);
     return rval;
   }
 
   Status validate() {
     if (std::optional<std::string> const & reject = schema_->rejects_all()) {
-      if (should_annotate(Status::Reject) && result_) {
+      if (should_annotate(Status::Reject)) {
         result_->error(where_, schema_path_, "", *reject);
       }
+      (result_ ? result_->valid(where_, schema_path_, false) : void());
       return Status::Reject;
     }
 
     if (schema_->accepts_all()) {
       // An accept-all schema is not No-Op for the purpose of unevaluated*
+      (result_ ? result_->valid(where_, schema_path_, true) : void());
       return Status::Accept;
     }
 
@@ -563,6 +592,7 @@ public:
       rval &= p_constraint->accept(*this);
     }
 
+    (result_ ? result_->valid(where_, schema_path_, rval) : void());
     return rval;
   }
 
@@ -588,6 +618,9 @@ private:
   }
 
   bool should_annotate(Status stat) const {
+    if (not result_) {
+      return false;
+    }
     switch (tracking_) {
     case StoreResults::ForAnything:
       return stat != Status::Noop;
@@ -601,9 +634,8 @@ private:
 #define ANNOTATION_HELPER(name, ADD, FMT)                                                          \
   void name(auto const &... args) const {                                                          \
     if (not result_) {                                                                             \
-      return;                                                                                      \
-    }                                                                                              \
-    if (schema_path_.empty()) {                                                                    \
+      /* do nothing if there's no result object to append to */                                    \
+    } else if (schema_path_.empty()) {                                                             \
       result_->ADD(where_, schema_path_, "", FMT(args...));                                        \
     } else {                                                                                       \
       result_->ADD(where_, schema_path_.parent(), schema_path_.back(), FMT(args...));              \
@@ -618,10 +650,6 @@ private:
     return (should_annotate(stat) ? error(args...) : void(), stat);
   }
 
-  template <typename C> static void merge_visited(C & to, C const & from) {
-    to.insert(from.begin(), from.end());
-  }
-
   template <typename... K>
   Status validate_subschema(constraint::Constraint::SubConstraint const & subschema,
                             K const &... keys) const {
@@ -644,8 +672,8 @@ private:
     Status rval = next.validate();
 
     if (rval == Status::Accept and visited_) {
-      merge_visited(std::get<0>(*visited_), std::get<0>(annotate));
-      merge_visited(std::get<1>(*visited_), std::get<1>(annotate));
+      std::get<0>(*visited_).merge(std::get<0>(annotate));
+      std::get<1>(*visited_).merge(std::get<1>(annotate));
     }
     return rval;
   }

tests/annotation_test.cxx

@@ -161,6 +161,21 @@ TEST(Annotation, SomeConstraintsAnnotateBothValidAndInvalid) {
   EXPECT_THAT(result, ErrorAt(""_jptr, "/oneOf/1"_jptr, "maximum", "-1 <= 0"));
 }
 
+TEST(Annotation, AttachesAlwaysFalseSensibly) {
+  auto const schema = R"({
+    "properties": {
+      "A": false
+    }
+  })"_json;
+
+  auto const instance = R"({
+    "A": null
+  })"_json;
+  jvalidate::ValidationResult result = validate(schema, instance);
+
+  EXPECT_THAT(result, ErrorAt("/A"_jptr, "/properties"_jptr, "", "always false"));
+}
+
 int main(int argc, char ** argv) {
   testing::InitGoogleMock(&argc, argv);
   return RUN_ALL_TESTS();