Group metadata doesn't get serialized (#3147)

As @snagles reported, the group metadata seems to be written correctly, but it's empty when you try to read it. I managed to reproduce it locally by opening a TileDB Cloud group, then created a test for it by serializing-deserializing a group with metadata locally. The serialization code is calling `Group::metadata()` to get the md object, but the `Metadata` content is not brought up from disk, so the group looks like it has no metadata. `Group::load_metadata` needs to be called to make sure we serialize a synced version of `Metadata`. --- TYPE: BUG DESC: Group metadata doesn't get serialized --------- Co-authored-by: KiterLuc <[email protected]> (cherry picked from commit 71a2cc4)
TileDB-Inc · Oct 31, 2023 · f5a3e5e · f5a3e5e
1 parent 3d6a0d0
commit f5a3e5e
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 31 deletions.
diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc
@@ -1762,4 +1762,72 @@ TEST_CASE_METHOD(
   tiledb_group_free(&group4_read);
   remove_temp_dir(temp_dir);
 }
+
+TEST_CASE_METHOD(
+    GroupFx,
+    "C API: Group metadata serialization",
+    "[capi][group][metadata][serialization]") {
+  std::string temp_dir = fs_vec_[0]->temp_dir();
+  create_temp_dir(temp_dir);
+
+  tiledb::sm::URI group_uri(temp_dir + "group");
+  REQUIRE(tiledb_group_create(ctx_, group_uri.c_str()) == TILEDB_OK);
+  tiledb::sm::URI group_deserialized_uri(temp_dir + "group_deserialized");
+  REQUIRE(
+      tiledb_group_create(ctx_, group_deserialized_uri.c_str()) == TILEDB_OK);
+
+  tiledb_group_t* group = nullptr;
+  REQUIRE(tiledb_group_alloc(ctx_, group_uri.c_str(), &group) == TILEDB_OK);
+  REQUIRE(tiledb_group_open(ctx_, group, TILEDB_WRITE) == TILEDB_OK);
+
+  int value = 123;
+  REQUIRE(
+      tiledb_group_put_metadata(
+          ctx_, group, "testmetadata", TILEDB_INT32, 1, &value) == TILEDB_OK);
+
+  REQUIRE(tiledb_group_close(ctx_, group) == TILEDB_OK);
+  tiledb_group_free(&group);
+
+  // Reopen in read mode
+  REQUIRE(tiledb_group_alloc(ctx_, group_uri.c_str(), &group) == TILEDB_OK);
+  REQUIRE(tiledb_group_open(ctx_, group, TILEDB_READ) == TILEDB_OK);
+
+  tiledb_group_t* group_deserialized = nullptr;
+  REQUIRE(
+      tiledb_group_alloc(
+          ctx_, group_deserialized_uri.c_str(), &group_deserialized) ==
+      TILEDB_OK);
+  REQUIRE(
+      tiledb_group_open(ctx_, group_deserialized, TILEDB_WRITE) == TILEDB_OK);
+
+  REQUIRE(
+      tiledb_group_serialize(ctx_, group, group_deserialized, TILEDB_CAPNP) ==
+      TILEDB_OK);
+
+  REQUIRE(tiledb_group_close(ctx_, group_deserialized) == TILEDB_OK);
+  tiledb_group_free(&group_deserialized);
+
+  // Reopen in read mode
+  REQUIRE(
+      tiledb_group_alloc(
+          ctx_, group_deserialized_uri.c_str(), &group_deserialized) ==
+      TILEDB_OK);
+  REQUIRE(
+      tiledb_group_open(ctx_, group_deserialized, TILEDB_READ) == TILEDB_OK);
+
+  tiledb_datatype_t dtype;
+  uint32_t num;
+  const void* val;
+  tiledb_group_get_metadata(
+      ctx_, group_deserialized, "testmetadata", &dtype, &num, &val);
+
+  REQUIRE(val != nullptr);
+  CHECK(*static_cast<const int32_t*>(val) == 123);
+
+  REQUIRE(tiledb_group_close(ctx_, group) == TILEDB_OK);
+  tiledb_group_free(&group);
+  REQUIRE(tiledb_group_close(ctx_, group_deserialized) == TILEDB_OK);
+  tiledb_group_free(&group_deserialized);
+  remove_temp_dir(temp_dir);
+}
 #endif
diff --git a/tiledb/sm/rest/rest_client.cc b/tiledb/sm/rest/rest_client.cc
@@ -1276,7 +1276,7 @@ Status RestClient::post_group_metadata_from_rest(const URI& uri, Group* group) {
 
   Buffer buff;
   RETURN_NOT_OK(serialization::group_metadata_serialize(
-      group, serialization_type_, &buff));
+      group, serialization_type_, &buff, false));
   // Wrap in a list
   BufferList serialized;
   RETURN_NOT_OK(serialized.add_buffer(std::move(buff)));
@@ -1319,7 +1319,7 @@ Status RestClient::put_group_metadata_to_rest(const URI& uri, Group* group) {
 
   Buffer buff;
   RETURN_NOT_OK(serialization::group_metadata_serialize(
-      group, serialization_type_, &buff));
+      group, serialization_type_, &buff, true));
   // Wrap in a list
   BufferList serialized;
   RETURN_NOT_OK(serialized.add_buffer(std::move(buff)));

diff --git a/tiledb/sm/serialization/group.cc b/tiledb/sm/serialization/group.cc
@@ -65,12 +65,20 @@ namespace serialization {
 #ifdef TILEDB_SERIALIZATION
 
 Status group_metadata_to_capnp(
-    const Group* group, capnp::GroupMetadata::Builder* group_metadata_builder) {
+    Group* group,
+    capnp::GroupMetadata::Builder* group_metadata_builder,
+    bool load) {
   // Set config
   auto config_builder = group_metadata_builder->initConfig();
   RETURN_NOT_OK(config_to_capnp(group->config(), &config_builder));
 
-  const Metadata* metadata = group->metadata();
+  Metadata* metadata;
+  if (load) {
+    RETURN_NOT_OK(group->metadata(&metadata));
+  } else {
+    metadata = const_cast<Metadata*>(group->metadata());
+  }
+
   if (metadata->num()) {
     auto metadata_builder = group_metadata_builder->initMetadata();
     RETURN_NOT_OK(metadata_to_capnp(metadata, &metadata_builder));
@@ -135,13 +143,12 @@ group_member_from_capnp(capnp::GroupMember::Reader* group_member_reader) {
 }
 
 Status group_details_to_capnp(
-    const Group* group,
-    capnp::Group::GroupDetails::Builder* group_details_builder) {
+    Group* group, capnp::Group::GroupDetails::Builder* group_details_builder) {
   if (group == nullptr)
     return LOG_STATUS(
         Status_SerializationError("Error serializing group; group is null."));
 
-  auto& group_details = group->group_details();
+  auto group_details = group->group_details();
 
   if (group_details != nullptr) {
     const auto& group_members = group->members();
@@ -158,7 +165,8 @@ Status group_details_to_capnp(
     }
   }
 
-  const Metadata* metadata = group->metadata();
+  Metadata* metadata;
+  RETURN_NOT_OK(group->metadata(&metadata));
   if (metadata->num()) {
     auto group_metadata_builder = group_details_builder->initMetadata();
     RETURN_NOT_OK(metadata_to_capnp(metadata, &group_metadata_builder));
@@ -189,8 +197,7 @@ Status group_details_from_capnp(
   return Status::Ok();
 }
 
-Status group_to_capnp(
-    const Group* group, capnp::Group::Builder* group_builder) {
+Status group_to_capnp(Group* group, capnp::Group::Builder* group_builder) {
   if (group == nullptr)
     return LOG_STATUS(
         Status_SerializationError("Error serializing group; group is null."));
@@ -364,9 +371,7 @@ Status group_create_to_capnp(
 }
 
 Status group_serialize(
-    const Group* group,
-    SerializationType serialize_type,
-    Buffer* serialized_buffer) {
+    Group* group, SerializationType serialize_type, Buffer* serialized_buffer) {
   try {
     ::capnp::MallocMessageBuilder message;
     capnp::Group::Builder groupBuilder = message.initRoot<capnp::Group>();
@@ -464,9 +469,7 @@ Status group_deserialize(
 }
 
 Status group_details_serialize(
-    const Group* group,
-    SerializationType serialize_type,
-    Buffer* serialized_buffer) {
+    Group* group, SerializationType serialize_type, Buffer* serialized_buffer) {
   try {
     ::capnp::MallocMessageBuilder message;
     capnp::Group::GroupDetails::Builder groupDetailsBuilder =
@@ -724,14 +727,16 @@ Status group_create_serialize(
 }
 
 Status group_metadata_serialize(
-    const Group* group,
+    Group* group,
     SerializationType serialize_type,
-    Buffer* serialized_buffer) {
+    Buffer* serialized_buffer,
+    bool load) {
   try {
     ::capnp::MallocMessageBuilder message;
     capnp::GroupMetadata::Builder group_metadata_builder =
         message.initRoot<capnp::GroupMetadata>();
-    RETURN_NOT_OK(group_metadata_to_capnp(group, &group_metadata_builder));
+    RETURN_NOT_OK(
+        group_metadata_to_capnp(group, &group_metadata_builder, load));
 
     serialized_buffer->reset_size();
     serialized_buffer->reset_offset();
@@ -779,7 +784,7 @@ Status group_metadata_serialize(
 
 #else
 
-Status group_serialize(const Group*, SerializationType, Buffer*) {
+Status group_serialize(Group*, SerializationType, Buffer*) {
   return LOG_STATUS(Status_SerializationError(
       "Cannot serialize; serialization not enabled."));
 }
@@ -789,7 +794,7 @@ Status group_deserialize(Group*, SerializationType, const Buffer&) {
       "Cannot deserialize; serialization not enabled."));
 }
 
-Status group_details_serialize(const Group*, SerializationType, Buffer*) {
+Status group_details_serialize(Group*, SerializationType, Buffer*) {
   return LOG_STATUS(Status_SerializationError(
       "Cannot serialize; serialization not enabled."));
 }
@@ -814,7 +819,7 @@ Status group_create_serialize(const Group*, SerializationType, Buffer*) {
       "Cannot serialize; serialization not enabled."));
 }
 
-Status group_metadata_serialize(const Group*, SerializationType, Buffer*) {
+Status group_metadata_serialize(Group*, SerializationType, Buffer*, bool) {
   return LOG_STATUS(Status_SerializationError(
       "Cannot serialize; serialization not enabled."));
 }

diff --git a/tiledb/sm/serialization/group.h b/tiledb/sm/serialization/group.h
@@ -57,9 +57,7 @@ namespace serialization {
  * @return Status
  */
 Status group_serialize(
-    const Group* group,
-    SerializationType serialize_type,
-    Buffer* serialized_buffer);
+    Group* group, SerializationType serialize_type, Buffer* serialized_buffer);
 
 /**
  * Deserialize a group via Cap'n proto
@@ -84,9 +82,7 @@ Status group_deserialize(
  * @return Status
  */
 Status group_details_serialize(
-    const Group* group,
-    SerializationType serialize_type,
-    Buffer* serialized_buffer);
+    Group* group, SerializationType serialize_type, Buffer* serialized_buffer);
 
 /**
  * Deserialize a group details via Cap'n proto
@@ -149,15 +145,18 @@ Status group_create_serialize(
  * @param serialize_type format to serialize into Cap'n Proto or JSON
  * @param serialized_buffer buffer to store serialized bytes in
  * serialize the array URI
+ * @param load flag signaling whether or not metadata should be fetched from
+ * storage
  * @return Status
  */
 Status group_metadata_serialize(
-    const Group* group,
+    Group* group,
     SerializationType serialize_type,
-    Buffer* serialized_buffer);
+    Buffer* serialized_buffer,
+    bool load);
 
 }  // namespace serialization
 }  // namespace sm
 }  // namespace tiledb
 
-#endif  // TILEDB_SERIALIZATION_GROUP_H
+#endif  // TILEDB_SERIALIZATION_GROUP_H