[DRAFT] What if we always emitted optional fields?

CHANGELOG.md

@@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 - The `cchost` configuration file now includes an `idle_connection_timeout` option. This controls how long the node will keep idle connections (for user TLS sessions) before automatically closing them. This may be set to `null` to restore the previous behaviour, where idle connections are never closed. By default connections will be closed after 60s of idle time.
 
+### Changed
+
+- Serialisation of C++ types to JSON has changed. Fields which are marked as optional in the CCF JSON serdes macros (ie - those in `DECLARE_JSON_OPTIONAL_FIELDS`) will now always be present in the resulting JSON object. Previously they would be omitted from the object if they matched the default value. They are still optional on deserialisation (ie - if these fields are missing, the object can still be deserialised).
+
 ## [5.0.0-rc0]
 
 [5.0.0-rc0]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.0-rc0

include/ccf/crypto/pem.h

@@ -76,7 +76,14 @@ namespace ccf::crypto
 
   inline void to_json(nlohmann::json& j, const Pem& p)
   {
-    j = p.str();
+    if (p.empty())
+    {
+      j = nullptr;
+    }
+    else
+    {
+      j = p.str();
+    }
   }
 
   inline void from_json(const nlohmann::json& j, Pem& p)
@@ -89,6 +96,10 @@ namespace ccf::crypto
     {
       p = Pem(j.get<std::vector<uint8_t>>());
     }
+    else if (j.is_null())
+    {
+      p = Pem();
+    }
     else
     {
       throw std::runtime_error(

include/ccf/ds/json.h

@@ -383,7 +383,6 @@ namespace std
 
 #define WRITE_OPTIONAL_WITH_RENAMES_FOR_JSON_NEXT(TYPE, C_FIELD, JSON_FIELD) \
   { \
-    if (t.C_FIELD != t_default.C_FIELD) \
     { \
       j[JSON_FIELD] = t.C_FIELD; \
     } \

include/ccf/odata_error.h

@@ -37,7 +37,7 @@ namespace ccf
   {
     std::string code;
     std::string message;
-    std::vector<nlohmann::json> details = {};
+    std::optional<std::vector<nlohmann::json>> details = std::nullopt;
   };
 
   DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(ODataError);

include/ccf/rpc_context.h

@@ -160,7 +160,8 @@ namespace ccf
       http_status status,
       const std::string& code,
       std::string&& msg,
-      const std::vector<nlohmann::json>& details = {}) = 0;
+      const std::optional<std::vector<nlohmann::json>>& details =
+        std::nullopt) = 0;
 
     /// Construct error response, formatted according to the request content
     /// type (either JSON OData-formatted or gRPC error)

python/ccf/ledger.py

@@ -421,8 +421,9 @@ def add_transaction(self, transaction):
                 node_info = json.loads(node_info)
                 # Add the self-signed node certificate (only available in 1.x,
                 # refer to node endorsed certificates table otherwise)
-                if "cert" in node_info:
-                    node_certs[node_id] = node_info["cert"].encode()
+                cert = node_info.get("cert", None)
+                if cert:
+                    node_certs[node_id] = cert.encode()
                     self.node_certificates[node_id] = node_certs[node_id]
                 # Update node trust status
                 # Also record the seqno at which the node status changed to

samples/apps/logging/logging.cpp

@@ -1658,6 +1658,13 @@ namespace loggingapp
 
         // Construct the HTTP response
         nlohmann::json j_response = response;
+
+        // Prefer missing field to null, in this case
+        if (!response.next_link.has_value())
+        {
+          j_response.erase("@nextLink");
+        }
+
         ctx.rpc_ctx->set_response_status(HTTP_STATUS_OK);
         ctx.rpc_ctx->set_response_header(
           ccf::http::headers::CONTENT_TYPE,

src/endpoints/common_endpoint_registry.cpp

@@ -164,7 +164,7 @@ namespace ccf
         }
         else if (view_history.value() == "false")
         {
-          out.view_history.clear();
+          out.view_history = std::nullopt;
         }
         else
         {

src/node/history.h

@@ -773,7 +773,7 @@ namespace ccf
         return;
       }
 
-      if (!endorsed_cert.has_value())
+      if (!endorsed_cert.has_value() || endorsed_cert->empty())
       {
         throw std::logic_error(
           fmt::format("No endorsed certificate set to emit signature"));
@@ -837,6 +837,11 @@ namespace ccf
 
     void set_endorsed_certificate(const ccf::crypto::Pem& cert) override
     {
+      if (cert.empty())
+      {
+        throw std::logic_error(fmt::format("Cannot set empty endorsed cert"));
+      }
+
       endorsed_cert = cert;
     }
   };

src/node/rpc/call_types.h

@@ -24,7 +24,7 @@ namespace ccf
     struct Out
     {
       ccf::TxID transaction_id;
-      std::vector<ccf::TxID> view_history;
+      std::optional<std::vector<ccf::TxID>> view_history;
     };
   };
 

src/node/rpc_context_impl.h

@@ -76,7 +76,8 @@ namespace ccf
       http_status status,
       const std::string& code,
       std::string&& msg,
-      const std::vector<nlohmann::json>& details = {}) override
+      const std::optional<std::vector<nlohmann::json>>& details =
+        std::nullopt) override
     {
       auto content_type = get_request_header(ccf::http::headers::CONTENT_TYPE);
       if (

src/node/test/cert_utils.h

@@ -0,0 +1,18 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the Apache 2.0 License.
+#pragma once
+
+#include "crypto/certs.h"
+
+ccf::crypto::Pem make_self_signed_cert(ccf::crypto::KeyPairPtr kp)
+{
+  constexpr size_t certificate_validity_period_days = 365;
+  using namespace std::literals;
+  const auto valid_from =
+    ::ds::to_x509_time_string(std::chrono::system_clock::now() - 24h);
+
+  const auto valid_to = ccf::crypto::compute_cert_valid_to_string(
+    valid_from, certificate_validity_period_days);
+
+  return kp->self_sign("CN=Node", valid_from, valid_to);
+}

src/node/test/historical_queries.cpp

@@ -57,7 +57,9 @@ TestState create_and_init_state(bool initialise_ledger_rekey = true)
   const ccf::NodeId node_id = std::string("node_id");
   auto h =
     std::make_shared<ccf::MerkleTxHistory>(*ts.kv_store, node_id, *ts.node_kp);
-  h->set_endorsed_certificate({});
+  const auto self_signed =
+    ts.node_kp->self_sign("CN=Node", valid_from, valid_to);
+  h->set_endorsed_certificate(self_signed);
   ts.kv_store->set_history(h);
   ts.kv_store->initialise_term(2);
 

src/node/test/history.cpp

@@ -5,7 +5,7 @@
 #include "ccf/app_interface.h"
 #include "ccf/ds/logger.h"
 #include "ccf/service/tables/nodes.h"
-#include "crypto/certs.h"
+#include "cert_utils.h"
 #include "crypto/openssl/hash.h"
 #include "ds/x509_time_fmt.h"
 #include "kv/kv_types.h"
@@ -23,14 +23,6 @@ std::unique_ptr<threading::ThreadMessaging>
 
 using MapT = ccf::kv::Map<size_t, size_t>;
 
-constexpr size_t certificate_validity_period_days = 365;
-using namespace std::literals;
-auto valid_from =
-  ::ds::to_x509_time_string(std::chrono::system_clock::now() - 24h);
-
-auto valid_to = ccf::crypto::compute_cert_valid_to_string(
-  valid_from, certificate_validity_period_days);
-
 class DummyConsensus : public ccf::kv::test::StubConsensus
 {
 public:
@@ -75,7 +67,7 @@ TEST_CASE("Check signature verification")
   auto encryptor = std::make_shared<ccf::kv::NullTxEncryptor>();
 
   auto kp = ccf::crypto::make_key_pair();
-  const auto self_signed = kp->self_sign("CN=Node", valid_from, valid_to);
+  const auto self_signed = make_self_signed_cert(kp);
 
   ccf::kv::Store primary_store;
   primary_store.set_encryptor(encryptor);
@@ -140,7 +132,7 @@ TEST_CASE("Check signing works across rollback")
   auto encryptor = std::make_shared<ccf::kv::NullTxEncryptor>();
 
   auto kp = ccf::crypto::make_key_pair();
-  const auto self_signed = kp->self_sign("CN=Node", valid_from, valid_to);
+  const auto self_signed = make_self_signed_cert(kp);
 
   ccf::kv::Store primary_store;
   primary_store.set_encryptor(encryptor);

src/node/test/snapshot.cpp

@@ -3,6 +3,7 @@
 
 #include "ccf/crypto/key_pair.h"
 #include "ccf/service/tables/nodes.h"
+#include "cert_utils.h"
 #include "crypto/openssl/hash.h"
 #include "kv/test/null_encryptor.h"
 #include "kv/test/stub_consensus.h"
@@ -30,7 +31,8 @@ TEST_CASE("Snapshot with merkle tree" * doctest::test_suite("snapshot"))
 
   auto source_history = std::make_shared<ccf::MerkleTxHistory>(
     source_store, source_node_id, *source_node_kp);
-  source_history->set_endorsed_certificate({});
+  source_history->set_endorsed_certificate(
+    make_self_signed_cert(source_node_kp));
   source_store.set_history(source_history);
   source_store.initialise_term(2);
 
@@ -97,7 +99,8 @@ TEST_CASE("Snapshot with merkle tree" * doctest::test_suite("snapshot"))
 
       auto target_history = std::make_shared<ccf::MerkleTxHistory>(
         target_store, ccf::kv::test::PrimaryNodeId, *target_node_kp);
-      target_history->set_endorsed_certificate({});
+      target_history->set_endorsed_certificate(
+        make_self_signed_cert(target_node_kp));
       target_store.set_history(target_history);
     }
 

tests/e2e_logging.py

@@ -36,7 +36,6 @@
 import threading
 import copy
 import programmability
-import e2e_common_endpoints
 
 from loguru import logger as LOG
 
@@ -1625,12 +1624,12 @@ def test_post_local_commit_failure(network, args):
         # check we can parse the txid from the header
         # this gets set since the post-commit handler threw
         TxID.from_str(r.headers[txid_header_key])
-        assert r.body.json() == {
-            "error": {
-                "code": "InternalError",
-                "message": "Failed to execute local commit handler func: didn't set user_data!",
-            }
-        }, r.body.json()
+        error = r.body.json()["error"]
+        assert error["code"] == "InternalError"
+        assert (
+            error["message"]
+            == "Failed to execute local commit handler func: didn't set user_data!"
+        )
 
 
 @reqs.description(
@@ -2061,14 +2060,14 @@ def run_parsing_errors(args):
 if __name__ == "__main__":
     cr = ConcurrentRunner()
 
-    cr.add(
-        "js",
-        run,
-        package="libjs_generic",
-        nodes=infra.e2e_args.max_nodes(cr.args, f=0),
-        initial_user_count=4,
-        initial_member_count=2,
-    )
+    # cr.add(
+    #     "js",
+    #     run,
+    #     package="libjs_generic",
+    #     nodes=infra.e2e_args.max_nodes(cr.args, f=0),
+    #     initial_user_count=4,
+    #     initial_member_count=2,
+    # )
 
     cr.add(
         "app_space_js",
@@ -2089,34 +2088,34 @@ def run_parsing_errors(args):
         initial_member_count=2,
     )
 
-    cr.add(
-        "common",
-        e2e_common_endpoints.run,
-        package="samples/apps/logging/liblogging",
-        nodes=infra.e2e_args.max_nodes(cr.args, f=0),
-    )
-
-    # Run illegal traffic tests in separate runners, to reduce total serial runtime
-    cr.add(
-        "js_illegal",
-        run_parsing_errors,
-        package="libjs_generic",
-        nodes=infra.e2e_args.max_nodes(cr.args, f=0),
-    )
-
-    cr.add(
-        "cpp_illegal",
-        run_parsing_errors,
-        package="samples/apps/logging/liblogging",
-        nodes=infra.e2e_args.max_nodes(cr.args, f=0),
-    )
-
-    # This is just for the UDP echo test for now
-    cr.add(
-        "udp",
-        run_udp_tests,
-        package="samples/apps/logging/liblogging",
-        nodes=infra.e2e_args.max_nodes(cr.args, f=0),
-    )
+    # cr.add(
+    #     "common",
+    #     e2e_common_endpoints.run,
+    #     package="samples/apps/logging/liblogging",
+    #     nodes=infra.e2e_args.max_nodes(cr.args, f=0),
+    # )
+
+    # # Run illegal traffic tests in separate runners, to reduce total serial runtime
+    # cr.add(
+    #     "js_illegal",
+    #     run_parsing_errors,
+    #     package="libjs_generic",
+    #     nodes=infra.e2e_args.max_nodes(cr.args, f=0),
+    # )
+
+    # cr.add(
+    #     "cpp_illegal",
+    #     run_parsing_errors,
+    #     package="samples/apps/logging/liblogging",
+    #     nodes=infra.e2e_args.max_nodes(cr.args, f=0),
+    # )
+
+    # # This is just for the UDP echo test for now
+    # cr.add(
+    #     "udp",
+    #     run_udp_tests,
+    #     package="samples/apps/logging/liblogging",
+    #     nodes=infra.e2e_args.max_nodes(cr.args, f=0),
+    # )
 
     cr.run()