From a82021c3d03a51abff582cf5a009c0a289df7059 Mon Sep 17 00:00:00 2001
From: Levi Tamasi <ltamasi@fb.com>
Date: Tue, 31 Jan 2023 09:59:25 -0800
Subject: [PATCH] Fix a bug where GetEntity would expose a blob reference
 (#11162)

Summary:
The patch fixes a feature interaction bug between BlobDB and the `GetEntity` API:
without the patch, `GetEntity` would return the blob reference (wrapped into a
single-column entity) instead of the actual blob value.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/11162

Test Plan: `make check`

Reviewed By: akankshamahajan15

Differential Revision: D42854092

Pulled By: ltamasi

fbshipit-source-id: f750d0ff57def107da16f545077ddce9860ff21a
---
 HISTORY.md                    |  1 +
 db/blob/db_blob_basic_test.cc | 22 ++++++++++++++++
 db/version_set.cc             | 47 +++++++++++++++++++++++------------
 3 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/HISTORY.md b/HISTORY.md
index 0513248d0..78bf465bf 100644
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -8,6 +8,7 @@
 * Fixed a data race on `ColumnFamilyData::flush_reason` caused by concurrent flushes.
 * Fixed an issue in `Get` and `MultiGet` when user-defined timestamps is enabled in combination with BlobDB.
 * Fixed some atypical behaviors for `LockWAL()` such as allowing concurrent/recursive use and not expecting `UnlockWAL()` after non-OK result. See API comments.
+* Fixed a feature interaction bug where for blobs `GetEntity` would expose the blob reference instead of the blob value.
 
 ### Feature Removal
 * Remove RocksDB Lite.
diff --git a/db/blob/db_blob_basic_test.cc b/db/blob/db_blob_basic_test.cc
index 60d9fbdcd..d4b95f7f0 100644
--- a/db/blob/db_blob_basic_test.cc
+++ b/db/blob/db_blob_basic_test.cc
@@ -1772,6 +1772,28 @@ TEST_F(DBBlobBasicTest, WarmCacheWithBlobsSecondary) {
             1);
 }
 
+TEST_F(DBBlobBasicTest, GetEntityBlob) {
+  Options options = GetDefaultOptions();
+  options.enable_blob_files = true;
+  options.min_blob_size = 0;
+
+  Reopen(options);
+
+  constexpr char key[] = "key";
+  constexpr char blob_value[] = "blob_value";
+
+  ASSERT_OK(Put(key, blob_value));
+
+  ASSERT_OK(Flush());
+
+  PinnableWideColumns result;
+  ASSERT_OK(
+      db_->GetEntity(ReadOptions(), db_->DefaultColumnFamily(), key, &result));
+
+  WideColumns expected_columns{{kDefaultWideColumnName, blob_value}};
+  ASSERT_EQ(result.columns(), expected_columns);
+}
+
 class DBBlobWithTimestampTest : public DBBasicTestWithTimestampBase {
  protected:
   DBBlobWithTimestampTest()
diff --git a/db/version_set.cc b/db/version_set.cc
index 81dbf1944..402e51721 100644
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -2339,23 +2339,38 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k,
         PERF_COUNTER_BY_LEVEL_ADD(user_key_return_count, 1,
                                   fp.GetHitFileLevel());
 
-        if (is_blob_index) {
-          if (do_merge && value) {
-            TEST_SYNC_POINT_CALLBACK("Version::Get::TamperWithBlobIndex",
-                                     value);
-
-            constexpr FilePrefetchBuffer* prefetch_buffer = nullptr;
-            constexpr uint64_t* bytes_read = nullptr;
-
-            *status =
-                GetBlob(read_options, get_context.ukey_to_get_blob_value(),
-                        *value, prefetch_buffer, value, bytes_read);
-            if (!status->ok()) {
-              if (status->IsIncomplete()) {
-                get_context.MarkKeyMayExist();
-              }
-              return;
+        if (is_blob_index && do_merge && (value || columns)) {
+          assert(!columns ||
+                 (!columns->columns().empty() &&
+                  columns->columns().front().name() == kDefaultWideColumnName));
+
+          Slice blob_index =
+              value ? *value : columns->columns().front().value();
+
+          TEST_SYNC_POINT_CALLBACK("Version::Get::TamperWithBlobIndex",
+                                   &blob_index);
+
+          constexpr FilePrefetchBuffer* prefetch_buffer = nullptr;
+
+          PinnableSlice result;
+
+          constexpr uint64_t* bytes_read = nullptr;
+
+          *status = GetBlob(read_options, get_context.ukey_to_get_blob_value(),
+                            blob_index, prefetch_buffer, &result, bytes_read);
+          if (!status->ok()) {
+            if (status->IsIncomplete()) {
+              get_context.MarkKeyMayExist();
             }
+            return;
+          }
+
+          if (value) {
+            *value = std::move(result);
+          } else {
+            assert(columns);
+            columns->Reset();
+            columns->SetPlainValue(result);
           }
         }
 
-- 
GitLab