Propagate provider tombstone metadata to internal SPI GetResult

4 files changed, 59 insertions, 16 deletions

diff --git a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
index b45a4131e7c..036a2b87692 100644
--- a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
+++ b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
@@ -625,6 +625,7 @@ TEST_F(ConformanceTest, testPutNewDocumentVersion)
 
     EXPECT_EQ(Result::ErrorType::NONE, gr.getErrorCode());
     EXPECT_EQ(Timestamp(4), gr.getTimestamp());
+    EXPECT_FALSE(gr.is_tombstone());
 
     if (!((*doc2)==gr.getDocument())) {
         std::cerr << "Document returned is not the expected one: \n"
@@ -676,6 +677,7 @@ TEST_F(ConformanceTest, testPutOlderDocumentVersion)
     EXPECT_EQ(Result::ErrorType::NONE, gr.getErrorCode());
     EXPECT_EQ(Timestamp(5), gr.getTimestamp());
     EXPECT_EQ(*doc1, gr.getDocument());
+    EXPECT_FALSE(gr.is_tombstone());
 }
 
 TEST_F(ConformanceTest, testPutDuplicate)
@@ -798,8 +800,9 @@ TEST_F(ConformanceTest, testRemove)
                                 context);
 
     EXPECT_EQ(Result::ErrorType::NONE, getResult.getErrorCode());
-    EXPECT_EQ(Timestamp(0), getResult.getTimestamp());
-    EXPECT_TRUE(!getResult.hasDocument());
+    EXPECT_EQ(Timestamp(9), getResult.getTimestamp());
+    EXPECT_TRUE(getResult.is_tombstone());
+    EXPECT_FALSE(getResult.hasDocument());
 }
 
 TEST_F(ConformanceTest, testRemoveMerge)
@@ -939,6 +942,7 @@ TEST_F(ConformanceTest, testUpdate)
 
         EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode());
         EXPECT_EQ(Timestamp(4), result.getTimestamp());
+        EXPECT_FALSE(result.is_tombstone());
         EXPECT_EQ(document::IntFieldValue(42),
                              static_cast<document::IntFieldValue&>(
                                      *result.getDocument().getValue("headerval")));
@@ -953,8 +957,9 @@ TEST_F(ConformanceTest, testUpdate)
                                     context);
 
         EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode());
-        EXPECT_EQ(Timestamp(0), result.getTimestamp());
-        EXPECT_TRUE(!result.hasDocument());
+        EXPECT_EQ(Timestamp(5), result.getTimestamp());
+        EXPECT_FALSE(result.hasDocument());
+        EXPECT_TRUE(result.is_tombstone());
     }
 
     {
@@ -968,8 +973,9 @@ TEST_F(ConformanceTest, testUpdate)
     {
         GetResult result = spi->get(bucket, document::AllFields(), doc1->getId(), context);
         EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode());
-        EXPECT_EQ(Timestamp(0), result.getTimestamp());
-        EXPECT_TRUE(!result.hasDocument());
+        EXPECT_EQ(Timestamp(5), result.getTimestamp());
+        EXPECT_FALSE(result.hasDocument());
+        EXPECT_TRUE(result.is_tombstone());
     }
 
     update->setCreateIfNonExistent(true);
@@ -985,6 +991,7 @@ TEST_F(ConformanceTest, testUpdate)
         GetResult result = spi->get(bucket, document::AllFields(), doc1->getId(), context);
         EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode());
         EXPECT_EQ(Timestamp(7), result.getTimestamp());
+        EXPECT_FALSE(result.is_tombstone());
         EXPECT_EQ(document::IntFieldValue(42),
                              reinterpret_cast<document::IntFieldValue&>(
                                      *result.getDocument().getValue("headerval")));
@@ -1008,6 +1015,7 @@ TEST_F(ConformanceTest, testGet)
 
         EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode());
         EXPECT_EQ(Timestamp(0), result.getTimestamp());
+        EXPECT_FALSE(result.is_tombstone());
     }
 
     spi->put(bucket, Timestamp(3), doc1, context);
@@ -1017,6 +1025,7 @@ TEST_F(ConformanceTest, testGet)
                                     doc1->getId(), context);
         EXPECT_EQ(*doc1, result.getDocument());
         EXPECT_EQ(Timestamp(3), result.getTimestamp());
+        EXPECT_FALSE(result.is_tombstone());
     }
 
     spi->remove(bucket, Timestamp(4), doc1->getId(), context);
@@ -1026,7 +1035,8 @@ TEST_F(ConformanceTest, testGet)
                                     doc1->getId(), context);
 
         EXPECT_EQ(Result::ErrorType::NONE, result.getErrorCode());
-        EXPECT_EQ(Timestamp(0), result.getTimestamp());
+        EXPECT_EQ(Timestamp(4), result.getTimestamp());
+        EXPECT_TRUE(result.is_tombstone());
     }
 }
 
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
index 5720a0ba662..92fea5ff6e4 100644
--- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
+++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
@@ -533,7 +533,10 @@ DummyPersistence::get(const Bucket& b, const document::FieldSet& fieldSet, const
     if (!bc.get()) {
     } else {
         DocEntry::SP entry((*bc)->getEntry(did));
-        if (entry.get() == 0 || entry->isRemove()) {
+        if (!entry) {
+            return GetResult();
+        } else if (entry->isRemove()) {
+            return GetResult::make_for_tombstone(entry->getTimestamp());
         } else {
             Document::UP doc(entry->getDocument()->clone());
             if (fieldSet.getType() != document::FieldSet::ALL) {
diff --git a/persistence/src/vespa/persistence/spi/result.cpp b/persistence/src/vespa/persistence/spi/result.cpp
index f5131f08b1f..8de6245a8d2 100644
--- a/persistence/src/vespa/persistence/spi/result.cpp
+++ b/persistence/src/vespa/persistence/spi/result.cpp
@@ -30,8 +30,17 @@ std::ostream & operator << (std::ostream & os, const Result::ErrorType &errorCod
 GetResult::GetResult(Document::UP doc, Timestamp timestamp)
     : Result(),
       _timestamp(timestamp),
-      _doc(std::move(doc))
-{ }
+      _doc(std::move(doc)),
+      _is_tombstone(false)
+{
+}
+
+GetResult::GetResult(Timestamp removed_at_ts)
+    : _timestamp(removed_at_ts),
+      _doc(),
+      _is_tombstone(true)
+{
+}
 
 GetResult::~GetResult() = default;
 BucketIdListResult::~BucketIdListResult() = default;
diff --git a/persistence/src/vespa/persistence/spi/result.h b/persistence/src/vespa/persistence/spi/result.h
index fe8e74706bb..714d71e37ee 100644
--- a/persistence/src/vespa/persistence/spi/result.h
+++ b/persistence/src/vespa/persistence/spi/result.h
@@ -156,13 +156,20 @@ public:
      */
     GetResult(ErrorType error, const vespalib::string& errorMessage)
         : Result(error, errorMessage),
-          _timestamp(0) { }
+          _timestamp(0),
+          _is_tombstone(false)
+    {
+    }
 
     /**
      * Constructor to use when we didn't find the document in question.
      */
     GetResult()
-        : _timestamp(0) { }
+        : _timestamp(0),
+          _doc(),
+          _is_tombstone(false)
+    {
+    }
 
     /**
      * Constructor to use when we found the document asked for.
@@ -172,12 +179,22 @@ public:
      */
     GetResult(DocumentUP doc, Timestamp timestamp);
 
-    ~GetResult();
+    static GetResult make_for_tombstone(Timestamp removed_at_ts) {
+        return GetResult(removed_at_ts);
+    }
+
+    ~GetResult() override;
 
-    Timestamp getTimestamp() const { return _timestamp; }
+    [[nodiscard]] Timestamp getTimestamp() const {
+        return _timestamp;
+    }
 
-    bool hasDocument() const {
-        return _doc.get() != NULL;
+    [[nodiscard]] bool hasDocument() const {
+        return (_doc.get() != nullptr);
+    }
+
+    [[nodiscard]] bool is_tombstone() const noexcept {
+        return _is_tombstone;
     }
 
     const Document& getDocument() const {
@@ -193,8 +210,12 @@ public:
     }
 
 private:
+    // Explicitly creates a tombstone (remove entry) GetResult with no document.
+    explicit GetResult(Timestamp removed_at_ts);
+
     Timestamp  _timestamp;
     DocumentSP _doc;
+    bool _is_tombstone;
 };
 
 class BucketIdListResult : public Result {