Merge bitcoin#26887: RPC: make RPCResult::MatchesType return useful errors

3d1a4d8 RPC: make RPCResult::MatchesType return useful errors (Anthony Towns)

Pull request description:

  Currently if you don't correctly update the description of the return value for an RPC call, you essentially just get an assertion failure with no useful information; this generates a description of the problems instead.

ACKs for top commit:
  MarcoFalke:
    re-ACK 3d1a4d8 🌷

Tree-SHA512: cf0580b7046faab0128672a74f8cc5a1655dfdca6646a2e38b51f0fb5f672c98aad6cd4c5769454a2d644a67da639ccb1c8ff5d24d3d6b4446a082398a643722

Author: MarcoFalke

src/rpc/util.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparamsbase.h>
+#include <clientversion.h>
 #include <consensus/amount.h>
 #include <key_io.h>
 #include <outputtype.h>
@@ -527,7 +528,26 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
     }
     UniValue ret = m_fun(*this, request);
     if (gArgs.GetBoolArg("-rpcdoccheck", DEFAULT_RPC_DOC_CHECK)) {
-        CHECK_NONFATAL(std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); }));
+        UniValue mismatch{UniValue::VARR};
+        for (const auto& res : m_results.m_results) {
+            UniValue match{res.MatchesType(ret)};
+            if (match.isTrue()) {
+                mismatch.setNull();
+                break;
+            }
+            mismatch.push_back(match);
+        }
+        if (!mismatch.isNull()) {
+            std::string explain{
+                mismatch.empty() ? "no possible results defined" :
+                mismatch.size() == 1 ? mismatch[0].write(4) :
+                mismatch.write(4)};
+            throw std::runtime_error{
+                strprintf("Internal bug detected: RPC call \"%s\" returned incorrect type:\n%s\n%s %s\nPlease report this issue here: %s\n",
+                          m_name, explain,
+                          PACKAGE_NAME, FormatFullVersion(),
+                          PACKAGE_BUGREPORT)};
+        }
     }
     return ret;
 }
@@ -804,53 +824,77 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
     NONFATAL_UNREACHABLE();
 }
 
-bool RPCResult::MatchesType(const UniValue& result) const
+static const std::optional<UniValue::VType> ExpectedType(RPCResult::Type type)
 {
-    if (m_skip_type_check) {
-        return true;
-    }
-    switch (m_type) {
+    using Type = RPCResult::Type;
+    switch (type) {
     case Type::ELISION:
     case Type::ANY: {
-        return true;
+        return std::nullopt;
     }
     case Type::NONE: {
-        return UniValue::VNULL == result.getType();
+        return UniValue::VNULL;
     }
     case Type::STR:
     case Type::STR_HEX: {
-        return UniValue::VSTR == result.getType();
+        return UniValue::VSTR;
     }
     case Type::NUM:
     case Type::STR_AMOUNT:
     case Type::NUM_TIME: {
-        return UniValue::VNUM == result.getType();
+        return UniValue::VNUM;
     }
     case Type::BOOL: {
-        return UniValue::VBOOL == result.getType();
+        return UniValue::VBOOL;
     }
     case Type::ARR_FIXED:
     case Type::ARR: {
-        if (UniValue::VARR != result.getType()) return false;
+        return UniValue::VARR;
+    }
+    case Type::OBJ_DYN:
+    case Type::OBJ: {
+        return UniValue::VOBJ;
+    }
+    } // no default case, so the compiler can warn about missing cases
+    NONFATAL_UNREACHABLE();
+}
+
+UniValue RPCResult::MatchesType(const UniValue& result) const
+{
+    if (m_skip_type_check) {
+        return true;
+    }
+
+    const auto exp_type = ExpectedType(m_type);
+    if (!exp_type) return true; // can be any type, so nothing to check
+
+    if (*exp_type != result.getType()) {
+        return strprintf("returned type is %s, but declared as %s in doc", uvTypeName(result.getType()), uvTypeName(*exp_type));
+    }
+
+    if (UniValue::VARR == result.getType()) {
+        UniValue errors(UniValue::VOBJ);
         for (size_t i{0}; i < result.get_array().size(); ++i) {
             // If there are more results than documented, re-use the last doc_inner.
             const RPCResult& doc_inner{m_inner.at(std::min(m_inner.size() - 1, i))};
-            if (!doc_inner.MatchesType(result.get_array()[i])) return false;
+            UniValue match{doc_inner.MatchesType(result.get_array()[i])};
+            if (!match.isTrue()) errors.pushKV(strprintf("%d", i), match);
         }
-        return true; // empty result array is valid
+        if (errors.empty()) return true; // empty result array is valid
+        return errors;
     }
-    case Type::OBJ_DYN:
-    case Type::OBJ: {
-        if (UniValue::VOBJ != result.getType()) return false;
+
+    if (UniValue::VOBJ == result.getType()) {
         if (!m_inner.empty() && m_inner.at(0).m_type == Type::ELISION) return true;
+        UniValue errors(UniValue::VOBJ);
         if (m_type == Type::OBJ_DYN) {
             const RPCResult& doc_inner{m_inner.at(0)}; // Assume all types are the same, randomly pick the first
             for (size_t i{0}; i < result.get_obj().size(); ++i) {
-                if (!doc_inner.MatchesType(result.get_obj()[i])) {
-                    return false;
-                }
+                UniValue match{doc_inner.MatchesType(result.get_obj()[i])};
+                if (!match.isTrue()) errors.pushKV(result.getKeys()[i], match);
             }
-            return true; // empty result obj is valid
+            if (errors.empty()) return true; // empty result obj is valid
+            return errors;
         }
         std::set<std::string> doc_keys;
         for (const auto& doc_entry : m_inner) {
@@ -860,26 +904,26 @@ bool RPCResult::MatchesType(const UniValue& result) const
         result.getObjMap(result_obj);
         for (const auto& result_entry : result_obj) {
             if (doc_keys.find(result_entry.first) == doc_keys.end()) {
-                return false; // missing documentation
+                errors.pushKV(result_entry.first, "key returned that was not in doc");
             }
         }
 
         for (const auto& doc_entry : m_inner) {
             const auto result_it{result_obj.find(doc_entry.m_key_name)};
             if (result_it == result_obj.end()) {
                 if (!doc_entry.m_optional) {
-                    return false; // result is missing a required key
+                    errors.pushKV(doc_entry.m_key_name, "key missing, despite not being optional in doc");
                 }
                 continue;
             }
-            if (!doc_entry.MatchesType(result_it->second)) {
-                return false; // wrong type
-            }
+            UniValue match{doc_entry.MatchesType(result_it->second)};
+            if (!match.isTrue()) errors.pushKV(doc_entry.m_key_name, match);
         }
-        return true;
+        if (errors.empty()) return true;
+        return errors;
     }
-    } // no default case, so the compiler can warn about missing cases
-    NONFATAL_UNREACHABLE();
+
+    return true;
 }
 
 void RPCResult::CheckInnerDoc() const

src/rpc/util.h

@@ -326,8 +326,10 @@ struct RPCResult {
     std::string ToStringObj() const;
     /** Return the description string, including the result type. */
     std::string ToDescriptionString() const;
-    /** Check whether the result JSON type matches. */
-    bool MatchesType(const UniValue& result) const;
+    /** Check whether the result JSON type matches.
+     * Returns true if type matches, or object describing error(s) if not.
+     */
+    UniValue MatchesType(const UniValue& result) const;
 
 private:
     void CheckInnerDoc() const;