Merge pull request #721 from capnproto/json-annotations

More JSON annotation fixes/improvements

c++/src/capnp/compat/json-test.c++

@@ -844,10 +844,14 @@ R"({ "names-can_contain!anything Really": "foo",
   "testBase64": "ZnJlZA==",
   "testHex": "706c756768",
   "bUnion": "renamed-bar",
-  "bValue": 678 })"_kj;
+  "bValue": 678,
+  "externalUnion": {"type": "bar", "value": "cba"},
+  "unionWithVoid": {"type": "voidValue"} })"_kj;
 
 static constexpr kj::StringPtr GOLDEN_ANNOTATED_REVERSE =
 R"({
+  "unionWithVoid": {"type": "voidValue"},
+  "externalUnion": {"type": "bar", "value": "cba"},
   "bValue": 678,
   "bUnion": "renamed-bar",
   "testHex": "706c756768",
@@ -940,6 +944,10 @@ KJ_TEST("rename fields") {
 
     root.getBUnion().setBar(678);
 
+    root.initExternalUnion().initBar().setValue("cba");
+
+    root.initUnionWithVoid().setVoidValue();
+
     auto encoded = json.encode(root.asReader());
     KJ_EXPECT(encoded == GOLDEN_ANNOTATED, encoded);
 

c++/src/capnp/compat/json-test.capnp

@@ -82,6 +82,14 @@ struct TestJsonAnnotations {
     foo @20 :Text;
     bar @21 :UInt32 $Json.name("renamed-bar");
   }
+
+  externalUnion @22 :TestJsonAnnotations3;
+
+  unionWithVoid :union $Json.discriminator(name = "type") {
+    intValue @23 :UInt32;
+    voidValue @24 :Void;
+    textValue @25 :Text;
+  }
 }
 
 struct TestJsonAnnotations2 {
@@ -89,6 +97,17 @@ struct TestJsonAnnotations2 {
   cycle @1 :TestJsonAnnotations;
 }
 
+struct TestJsonAnnotations3 $Json.discriminator(name = "type") {
+  union {
+    foo @0 :UInt32;
+    bar @1 :TestFlattenedStruct $Json.flatten();
+  }
+}
+
+struct TestFlattenedStruct {
+  value @0 :Text;
+}
+
 enum TestJsonAnnotatedEnum {
   foo @0;
   bar @1 $Json.name("renamed-bar");

c++/src/capnp/compat/json.c++

@@ -860,13 +860,17 @@ void JsonCodec::HandlerBase::decodeStructBase(
 }
 
 void JsonCodec::addTypeHandlerImpl(Type type, HandlerBase& handler) {
-  impl->typeHandlers.insert(type, &handler);
+  impl->typeHandlers.upsert(type, &handler, [](HandlerBase*& existing, HandlerBase* replacement) {
+    KJ_REQUIRE(existing == replacement, "type already has a different registered handler");
+  });
 }
 
 void JsonCodec::addFieldHandlerImpl(StructSchema::Field field, Type type, HandlerBase& handler) {
   KJ_REQUIRE(type == field.getType(),
       "handler type did not match field type for addFieldHandler()");
-  impl->fieldHandlers.insert(field, &handler);
+  impl->fieldHandlers.upsert(field, &handler, [](HandlerBase*& existing, HandlerBase* replacement) {
+    KJ_REQUIRE(existing == replacement, "field already has a different registered handler");
+  });
 }
 
 // =======================================================================================
@@ -1010,6 +1014,11 @@ public:
         if (flattened) {
           info.flattenHandler = subHandler;
         }
+      } else if (type.isStruct()) {
+        if (flattened) {
+          info.flattenHandler = codec.loadAnnotatedHandler(
+              type.asStruct(), nullptr, nullptr, dependencies);
+        }
       }
 
       bool isUnionMember = fieldProto.getDiscriminantValue() != schema::Field::NO_DISCRIMINANT;
@@ -1107,7 +1116,7 @@ public:
   void decode(const JsonCodec& codec, JsonValue::Reader input,
               DynamicStruct::Builder output) const override {
     KJ_REQUIRE(input.isObject());
-    kj::HashMap<const void*, StructSchema::Field> unionsSeen;
+    kj::HashSet<const void*> unionsSeen;
     kj::Vector<JsonValue::Field::Reader> retries;
     for (auto field: input.getObject()) {
       if (!decodeField(codec, field.getName(), field.getValue(), output, unionsSeen)) {
@@ -1235,15 +1244,19 @@ private:
       KJ_IF_MAYBE(handler, info.flattenHandler) {
         handler->gatherForEncode(codec, reader.get(*which), prefix, info.prefix, flattenedFields);
       } else {
-        flattenedFields.add(FlattenedField {
-            prefix, info.name, which->getType(), reader.get(*which) });
+        auto type = which->getType();
+        if (type.which() == schema::Type::VOID && unionTagName != nullptr) {
+          // When we have an explicit union discriminant, we don't need to encode void fields.
+        } else {
+          flattenedFields.add(FlattenedField {
+              prefix, info.name, which->getType(), reader.get(*which) });
+        }
       }
     }
   }
 
   bool decodeField(const JsonCodec& codec, kj::StringPtr name, JsonValue::Reader value,
-                   DynamicStruct::Builder output,
-                   kj::HashMap<const void*, StructSchema::Field>& unionsSeen) const {
+                   DynamicStruct::Builder output, kj::HashSet<const void*>& unionsSeen) const {
     KJ_ASSERT(output.getSchema() == schema);
 
     KJ_IF_MAYBE(info, fieldsByName.find(name)) {
@@ -1264,20 +1277,20 @@ private:
           // Mark that we've seen a union tag for this struct.
           const void* ptr = getUnionInstanceIdentifier(output);
           KJ_IF_MAYBE(field, unionTagValues.find(value.getString())) {
-            unionsSeen.insert(ptr, *field);
+            // clear() has the side-effect of activating this member of the union, without
+            // allocating any objects.
+            output.clear(*field);
+            unionsSeen.insert(ptr);
           }
           return true;
         }
         case FieldNameInfo::FLATTENED_FROM_UNION: {
           const void* ptr = getUnionInstanceIdentifier(output);
-          KJ_IF_MAYBE(variant, unionsSeen.find(ptr)) {
-            bool alreadyInitialized = output.which()
-                .map([&](auto f) { return f == *variant; })
-                .orDefault(false);
-            auto child = alreadyInitialized ? output.get(*variant) : output.init(*variant);
-            return KJ_ASSERT_NONNULL(fields[variant->getIndex()].flattenHandler)
+          if (unionsSeen.contains(ptr)) {
+            auto variant = KJ_ASSERT_NONNULL(output.which());
+            return KJ_ASSERT_NONNULL(fields[variant.getIndex()].flattenHandler)
                 .decodeField(codec, name.slice(info->prefixLength), value,
-                    child.as<DynamicStruct>(), unionsSeen);
+                    output.get(variant).as<DynamicStruct>(), unionsSeen);
           } else {
             // We haven't seen the union tag yet, so we can't parse this field yet. Try again later.
             return false;
@@ -1285,8 +1298,9 @@ private:
         }
         case FieldNameInfo::UNION_VALUE: {
           const void* ptr = getUnionInstanceIdentifier(output);
-          KJ_IF_MAYBE(variant, unionsSeen.find(ptr)) {
-            codec.decodeField(*variant, value, Orphanage::getForMessageContaining(output), output);
+          if (unionsSeen.contains(ptr)) {
+            auto variant = KJ_ASSERT_NONNULL(output.which());
+            codec.decodeField(variant, value, Orphanage::getForMessageContaining(output), output);
             return true;
           } else {
             // We haven't seen the union tag yet, so we can't parse this field yet. Try again later.

c++/src/capnp/testdata/annotated.json

@@ -17,4 +17,6 @@
   "testBase64": "ZnJlZA==",
   "testHex": "706c756768",
   "bUnion": "renamed-bar",
-  "bValue": 678 }
+  "bValue": 678,
+  "externalUnion": {"type": "bar", "value": "cba"},
+  "unionWithVoid": {"type": "voidValue"} }

c++/src/kj/map.h

@@ -44,8 +44,8 @@ public:
   void reserve(size_t size);
   // Pre-allocates space for a map of the given size.
 
-  size_t size();
-  size_t capacity();
+  size_t size() const;
+  size_t capacity() const;
   void clear();
 
   struct Entry {
@@ -136,8 +136,8 @@ public:
   void reserve(size_t size);
   // Pre-allocates space for a map of the given size.
 
-  size_t size();
-  size_t capacity();
+  size_t size() const;
+  size_t capacity() const;
   void clear();
 
   struct Entry {
@@ -262,6 +262,11 @@ class HashSet: public Table<Element, HashIndex<_::HashSetCallbacks>> {
 
 public:
   // Everything is inherited.
+
+  template <typename... Params>
+  inline bool contains(Params&&... params) const {
+    return this->find(kj::fwd<Params>(params)...) != nullptr;
+  }
 };
 
 template <typename Element>
@@ -281,11 +286,11 @@ void HashMap<Key, Value>::reserve(size_t size) {
 }
 
 template <typename Key, typename Value>
-size_t HashMap<Key, Value>::size() {
+size_t HashMap<Key, Value>::size() const {
   return table.size();
 }
 template <typename Key, typename Value>
-size_t HashMap<Key, Value>::capacity() {
+size_t HashMap<Key, Value>::capacity() const {
   return table.capacity();
 }
 template <typename Key, typename Value>
@@ -373,11 +378,11 @@ void TreeMap<Key, Value>::reserve(size_t size) {
 }
 
 template <typename Key, typename Value>
-size_t TreeMap<Key, Value>::size() {
+size_t TreeMap<Key, Value>::size() const {
   return table.size();
 }
 template <typename Key, typename Value>
-size_t TreeMap<Key, Value>::capacity() {
+size_t TreeMap<Key, Value>::capacity() const {
   return table.capacity();
 }
 template <typename Key, typename Value>