Simplify DocEntry to get a clean interface with multiple implementations, instead of an mutant.

Also add tests for the different variations a DocEntry can have.

10 files changed, 156 insertions, 90 deletions

diff --git a/persistence/src/tests/spi/clusterstatetest.cpp b/persistence/src/tests/spi/clusterstatetest.cpp
index ac67903244f..9d03cb982c8 100644
--- a/persistence/src/tests/spi/clusterstatetest.cpp
+++ b/persistence/src/tests/spi/clusterstatetest.cpp
@@ -5,6 +5,7 @@
 #include <vespa/vdslib/distribution/distribution.h>
 #include <vespa/vdslib/state/clusterstate.h>
 #include <vespa/config-stor-distribution.h>
+#include <vespa/document/base/testdocman.h>
 #include <gtest/gtest.h>
 
 using storage::spi::test::makeSpiBucket;
@@ -260,4 +261,63 @@ TEST(ClusterStateTest, node_maintenance_state_is_set_independent_of_bucket_space
     EXPECT_FALSE(node_marked_as_maintenance_in_state("distributor:3 storage:3 .0.s:m", d, 0, false));
 }
 
+TEST(DocEntryTest, test_basics) {
+    EXPECT_EQ(40u, sizeof(DocEntry));
+}
+
+TEST(DocEntryTest, test_meta_only) {
+    DocEntry e(Timestamp(9), 0);
+    EXPECT_EQ(9, e.getTimestamp());
+    EXPECT_FALSE(e.isRemove());
+    EXPECT_EQ(40, e.getSize());
+    EXPECT_EQ(0, e.getDocumentSize());
+    EXPECT_EQ(0, e.getPersistedDocumentSize());
+    EXPECT_EQ(nullptr, e.getDocument());
+    EXPECT_EQ(nullptr, e.getDocumentId());
+
+    e.setPersistedDocumentSize(7);
+    EXPECT_EQ(40, e.getSize());
+    EXPECT_EQ(0, e.getDocumentSize());
+    EXPECT_EQ(7, e.getPersistedDocumentSize());
+
+    DocEntry r(Timestamp(666), 1);
+    EXPECT_EQ(666, r.getTimestamp());
+    EXPECT_TRUE(r.isRemove());
+}
+
+TEST(DocEntryTest, test_docid_only) {
+    DocEntry e(Timestamp(9), 0, DocumentId("id:test:test::1"));
+    EXPECT_EQ(9, e.getTimestamp());
+    EXPECT_FALSE(e.isRemove());
+    EXPECT_EQ(56, e.getSize());
+    EXPECT_EQ(16, e.getDocumentSize());
+    EXPECT_EQ(16, e.getPersistedDocumentSize());
+    EXPECT_EQ(nullptr, e.getDocument());
+    EXPECT_NE(nullptr, e.getDocumentId());
+
+    e.setPersistedDocumentSize(7);
+    EXPECT_EQ(56, e.getSize());
+    EXPECT_EQ(16, e.getDocumentSize());
+    EXPECT_EQ(7, e.getPersistedDocumentSize());
+
+}
+
+TEST(DocEntryTest, test_document_only) {
+    document::TestDocMan testDocMan;
+    DocEntry e(Timestamp(9), 0, testDocMan.createRandomDocument(0, 1000));
+    EXPECT_EQ(9, e.getTimestamp());
+    EXPECT_FALSE(e.isRemove());
+    EXPECT_EQ(672, e.getSize());
+    EXPECT_EQ(632, e.getDocumentSize());
+    EXPECT_EQ(632, e.getPersistedDocumentSize());
+    EXPECT_NE(nullptr, e.getDocument());
+    EXPECT_NE(nullptr, e.getDocumentId());
+
+    e.setPersistedDocumentSize(7);
+    EXPECT_EQ(672, e.getSize());
+    EXPECT_EQ(632, e.getDocumentSize());
+    EXPECT_EQ(7, e.getPersistedDocumentSize());
+
+}
+
 }
diff --git a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
index 810f2ad2356..57b6b282e60 100644
--- a/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
+++ b/persistence/src/vespa/persistence/conformancetest/conformancetest.cpp
@@ -5,6 +5,7 @@
 #include <vespa/persistence/spi/test.h>
 #include <vespa/persistence/spi/catchresult.h>
 #include <vespa/persistence/spi/resource_usage_listener.h>
+#include <vespa/persistence/spi/docentry.h>
 #include <vespa/document/fieldset/fieldsets.h>
 #include <vespa/document/update/documentupdate.h>
 #include <vespa/document/update/assignvalueupdate.h>
@@ -25,6 +26,7 @@ using document::BucketId;
 using document::BucketSpace;
 using document::test::makeBucketSpace;
 using storage::spi::test::makeSpiBucket;
+using storage::spi::test::cloneDocEntry;
 
 namespace storage::spi {
 
@@ -183,7 +185,7 @@ getEntriesFromChunks(const std::vector<Chunk>& chunks)
     std::vector<spi::DocEntry::UP> ret;
     for (size_t chunk = 0; chunk < chunks.size(); ++chunk) {
         for (size_t i = 0; i < chunks[chunk]._entries.size(); ++i) {
-            ret.push_back(DocEntry::UP(chunks[chunk]._entries[i]->clone()));
+            ret.push_back(cloneDocEntry(*chunks[chunk]._entries[i]));
         }
     }
     std::sort(ret.begin(),
@@ -238,8 +240,7 @@ verifyDocs(const std::vector<DocAndTimestamp>& wanted,
            const std::vector<Chunk>& chunks,
            const std::set<string>& removes = std::set<string>())
 {
-    std::vector<DocEntry::UP> retrieved(
-            getEntriesFromChunks(chunks));
+    std::vector<DocEntry::UP> retrieved = getEntriesFromChunks(chunks);
     size_t removeCount = getRemoveEntryCount(retrieved);
     // Ensure that we've got the correct number of puts and removes
     EXPECT_EQ(removes.size(), removeCount);
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
index 9d9f31b63a3..142b7faad9d 100644
--- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
+++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp
@@ -9,6 +9,7 @@
 #include <vespa/persistence/spi/i_resource_usage_listener.h>
 #include <vespa/persistence/spi/resource_usage.h>
 #include <vespa/persistence/spi/bucketexecutor.h>
+#include <vespa/persistence/spi/test.h>
 #include <vespa/vespalib/util/crc.h>
 #include <vespa/document/fieldset/fieldsetrepo.h>
 #include <vespa/vespalib/stllike/asciistream.h>
@@ -24,6 +25,8 @@ using vespalib::make_string;
 using std::binary_search;
 using std::lower_bound;
 using document::FixedBucketSpaces;
+using storage::spi::test::cloneDocEntry;
+using storage::spi::test::equal;
 
 namespace storage::spi::dummy {
 
@@ -162,7 +165,7 @@ BucketContent::insert(DocEntry::SP e) {
         auto it = lower_bound(_entries.begin(), _entries.end(), e->getTimestamp(), TimestampLess());
         if (it != _entries.end()) {
             if (it->entry->getTimestamp() == e->getTimestamp()) {
-                if (*it->entry.get() == *e) {
+                if (equal(*it->entry, *e)) {
                     LOG(debug, "Ignoring duplicate put entry %s", e->toString().c_str());
                     return;
                 } else {
@@ -664,7 +667,7 @@ DummyPersistence::iterate(IteratorId id, uint64_t maxByteSize, Context& ctx) con
                 entries.push_back(std::move(ret));
             } else {
                 // Use entry as-is.
-                entries.push_back(DocEntry::UP(entry->clone()));
+                entries.push_back(cloneDocEntry(*entry));
                 ++fastPath;
             }
         }
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
index a7a784d0479..3b6fc9f1449 100644
--- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
+++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h
@@ -9,6 +9,7 @@
 #pragma once
 
 #include <vespa/persistence/spi/abstractpersistenceprovider.h>
+#include <vespa/persistence/spi/docentry.h>
 #include <vespa/document/base/globalid.h>
 #include <vespa/document/fieldset/fieldsets.h>
 #include <vespa/vespalib/stllike/hash_map.h>
diff --git a/persistence/src/vespa/persistence/spi/docentry.cpp b/persistence/src/vespa/persistence/spi/docentry.cpp
index 8669bbf666b..a48f0656b23 100644
--- a/persistence/src/vespa/persistence/spi/docentry.cpp
+++ b/persistence/src/vespa/persistence/spi/docentry.cpp
@@ -31,7 +31,7 @@ DocEntry::DocEntry(Timestamp t, int metaFlags, const DocumentId& docId)
       _metaFlags(metaFlags),
       _persistedDocumentSize(docId.getSerializedSize()),
       _size(_persistedDocumentSize + sizeof(DocEntry)),
-      _documentId(new DocumentId(docId)),
+      _documentId(std::make_unique<DocumentId>(docId)),
       _document()
 { }
 
@@ -46,23 +46,6 @@ DocEntry::DocEntry(Timestamp t, int metaFlags)
 
 DocEntry::~DocEntry() = default;
 
-DocEntry* 
-DocEntry::clone() const {
-    DocEntry* ret;
-    if (_documentId) {
-        ret = new DocEntry(_timestamp, _metaFlags, *_documentId);
-        ret->setPersistedDocumentSize(_persistedDocumentSize);
-    } else if (_document) {
-        ret = new DocEntry(_timestamp, _metaFlags,
-                           std::make_unique<Document>(*_document),
-                           _persistedDocumentSize);
-    } else {
-        ret = new DocEntry(_timestamp, _metaFlags);
-        ret->setPersistedDocumentSize(_persistedDocumentSize);
-    }
-    return ret;
-}
-
 const DocumentId*
 DocEntry::getDocumentId() const {
     return (_document ? &_document->getId() : _documentId.get());
@@ -101,48 +84,4 @@ operator << (std::ostream & os, const DocEntry & r) {
     return os << r.toString();
 }
 
-bool
-DocEntry::operator==(const DocEntry& entry) const {
-    if (_timestamp != entry._timestamp) {
-        return false;
-    }
-
-    if (_metaFlags != entry._metaFlags) {
-        return false;
-    }
-
-    if (_documentId) {
-        if (!entry._documentId) {
-            return false;
-        }
-
-        if (*_documentId != *entry._documentId) {
-            return false;
-        }
-    } else {
-        if (entry._documentId) {
-            return false;
-        }
-    }
-
-    if (_document) {
-        if (!entry._document) {
-            return false;
-        }
-
-        if (*_document != *entry._document) {
-            return false;
-        }
-    } else {
-        if (entry._document) {
-            return false;
-        }
-    }
-    if (_persistedDocumentSize != entry._persistedDocumentSize) {
-        return false;
-    }
-
-    return true;
-}
-
 }
diff --git a/persistence/src/vespa/persistence/spi/docentry.h b/persistence/src/vespa/persistence/spi/docentry.h
index 3374ef6c02d..8109c7f04a5 100644
--- a/persistence/src/vespa/persistence/spi/docentry.h
+++ b/persistence/src/vespa/persistence/spi/docentry.h
@@ -26,12 +26,12 @@ class DocEntry {
 public:
     typedef uint32_t SizeType;
 private:
-    Timestamp _timestamp;
-    int _metaFlags;
-    SizeType _persistedDocumentSize;
-    SizeType _size;
+    Timestamp    _timestamp;
+    int          _metaFlags;
+    SizeType     _persistedDocumentSize;
+    SizeType     _size;
     DocumentIdUP _documentId;
-    DocumentUP _document;
+    DocumentUP   _document;
 public:
     using UP = std::unique_ptr<DocEntry>;
     using SP = std::shared_ptr<DocEntry>;
@@ -47,15 +47,17 @@ public:
     DocEntry(Timestamp t, int metaFlags, const DocumentId& docId);
 
     DocEntry(Timestamp t, int metaFlags);
+    DocEntry(const DocEntry &) = delete;
+    DocEntry & operator=(const DocEntry &) = delete;
+    DocEntry(DocEntry &&) = delete;
+    DocEntry & operator=(DocEntry &&) = delete;
     ~DocEntry();
-    DocEntry* clone() const;
     const Document* getDocument() const { return _document.get(); }
     const DocumentId* getDocumentId() const;
     DocumentUP releaseDocument();
     bool isRemove() const { return (_metaFlags & REMOVE_ENTRY); }
     Timestamp getTimestamp() const { return _timestamp; }
     int getFlags() const { return _metaFlags; }
-    void setFlags(int flags) { _metaFlags = flags; }
     /**
      * @return In-memory size of this doc entry, including document instance.
      *     In essence: serialized size of document + sizeof(DocEntry).
@@ -87,7 +89,6 @@ public:
     }
 
     vespalib::string toString() const;
-    bool operator==(const DocEntry& entry) const;
 };
 
 std::ostream & operator << (std::ostream & os, const DocEntry & r);
diff --git a/persistence/src/vespa/persistence/spi/result.cpp b/persistence/src/vespa/persistence/spi/result.cpp
index e458d58fe69..a728f93e60a 100644
--- a/persistence/src/vespa/persistence/spi/result.cpp
+++ b/persistence/src/vespa/persistence/spi/result.cpp
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "result.h"
+#include "docentry.h"
 #include <vespa/document/fieldvalue/document.h>
 #include <vespa/vespalib/stllike/asciistream.h>
 #include <ostream>
@@ -48,6 +49,24 @@ GetResult::~GetResult() = default;
 BucketIdListResult::~BucketIdListResult() = default;
 
 IterateResult::~IterateResult() = default;
+IterateResult::IterateResult(IterateResult &&) noexcept = default;
+IterateResult & IterateResult::operator=(IterateResult &&) noexcept = default;
+
+IterateResult::IterateResult(ErrorType error, const vespalib::string& errorMessage)
+    : Result(error, errorMessage),
+      _completed(false)
+{ }
+
+
+IterateResult::IterateResult(List entries, bool completed)
+    : _completed(completed),
+      _entries(std::move(entries))
+{ }
+
+IterateResult::List
+IterateResult::steal_entries() {
+    return std::move(_entries);
+}
 
 }
 
diff --git a/persistence/src/vespa/persistence/spi/result.h b/persistence/src/vespa/persistence/spi/result.h
index c734a885b12..10c589307ba 100644
--- a/persistence/src/vespa/persistence/spi/result.h
+++ b/persistence/src/vespa/persistence/spi/result.h
@@ -3,11 +3,12 @@
 
 #include "bucketinfo.h"
 #include "bucket.h"
-#include "docentry.h"
 #include <vespa/document/bucket/bucketidlist.h>
 
 namespace storage::spi {
 
+class DocEntry;
+
 class Result {
 public:
     typedef std::unique_ptr<Result> UP;
@@ -279,15 +280,12 @@ private:
 
 class IterateResult : public Result {
 public:
-    typedef std::vector<DocEntry::UP> List;
+    using List = std::vector<std::unique_ptr<DocEntry>>;
 
     /**
      * Constructor used when there was an error creating the iterator.
      */
-    IterateResult(ErrorType error, const vespalib::string& errorMessage)
-        : Result(error, errorMessage),
-          _completed(false)
-    { }
+    IterateResult(ErrorType error, const vespalib::string& errorMessage);
 
     /**
      * Constructor used when the iteration was successful.
@@ -296,24 +294,21 @@ public:
      *
      * @param completed Set to true if iteration has been completed.
      */
-    IterateResult(List entries, bool completed)
-        : _completed(completed),
-          _entries(std::move(entries))
-    { }
+    IterateResult(List entries, bool completed);
 
     IterateResult(const IterateResult &) = delete;
-    IterateResult(IterateResult &&rhs) noexcept = default;
-    IterateResult &operator=(IterateResult &&rhs) noexcept = default;
+    IterateResult(IterateResult &&rhs) noexcept;
+    IterateResult &operator=(IterateResult &&rhs) noexcept;
 
     ~IterateResult();
 
     const List& getEntries() const { return _entries; }
-    List steal_entries() { return std::move(_entries); }
+    List steal_entries();
     bool isCompleted() const { return _completed; }
 
 private:
     bool _completed;
-    std::vector<DocEntry::UP> _entries;
+    List _entries;
 };
 
 }
diff --git a/persistence/src/vespa/persistence/spi/test.cpp b/persistence/src/vespa/persistence/spi/test.cpp
index 32381110630..c61f21a3893 100644
--- a/persistence/src/vespa/persistence/spi/test.cpp
+++ b/persistence/src/vespa/persistence/spi/test.cpp
@@ -1,7 +1,9 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "test.h"
+#include "docentry.h"
 #include <vespa/document/test/make_bucket_space.h>
+#include <vespa/document/fieldvalue/document.h>
 
 using document::BucketId;
 using document::BucketSpace;
@@ -9,9 +11,49 @@ using document::test::makeBucketSpace;
 
 namespace storage::spi::test {
 
-Bucket makeSpiBucket(BucketId bucketId)
+Bucket
+makeSpiBucket(BucketId bucketId)
 {
     return Bucket(document::Bucket(makeBucketSpace(), bucketId));
 }
 
+std::unique_ptr<DocEntry>
+cloneDocEntry(const DocEntry & e) {
+    std::unique_ptr<DocEntry> ret;
+    if (e.getDocument()) {
+        ret = std::make_unique<DocEntry>(e.getTimestamp(), e.getFlags(),
+                           std::make_unique<Document>(*e.getDocument()),
+                           e.getPersistedDocumentSize());
+    } else if (e.getDocumentId()) {
+        ret = std::make_unique<DocEntry>(e.getTimestamp(), e.getFlags(), *e.getDocumentId());
+        ret->setPersistedDocumentSize(e.getPersistedDocumentSize());
+    } else {
+        ret = std::make_unique<DocEntry>(e.getTimestamp(), e.getFlags());
+        ret->setPersistedDocumentSize(e.getPersistedDocumentSize());
+    }
+    return ret;
+}
+
+bool
+equal(const DocEntry & a, const DocEntry & b) {
+    if (a.getTimestamp() != b.getTimestamp()) return false;
+    if (a.getFlags() != b.getFlags()) return false;
+    if (a.getPersistedDocumentSize() != b.getPersistedDocumentSize()) return false;
+
+    if (a.getDocument()) {
+        if (!b.getDocument()) return false;
+        if (*a.getDocument() != *b.getDocument()) return false;
+    } else {
+        if (b.getDocument()) return false;
+    }
+    if (a.getDocumentId()) {
+        if (!b.getDocumentId()) return false;
+        if (*a.getDocumentId() != *b.getDocumentId()) return false;
+    } else {
+        if (b.getDocumentId()) return false;
+    }
+
+    return true;
+}
+
 }
diff --git a/persistence/src/vespa/persistence/spi/test.h b/persistence/src/vespa/persistence/spi/test.h
index af7109ec80c..1660e5f14fd 100644
--- a/persistence/src/vespa/persistence/spi/test.h
+++ b/persistence/src/vespa/persistence/spi/test.h
@@ -3,11 +3,16 @@
 #pragma once
 
 #include "bucket.h"
+#include <memory>
+
+namespace storage::spi { class DocEntry; }
 
 namespace storage::spi::test {
 
 // Helper functions used by unit tests
 
 Bucket makeSpiBucket(document::BucketId bucketId);
+std::unique_ptr<DocEntry> cloneDocEntry(const DocEntry & entry);
+bool equal(const DocEntry & a, const DocEntry & b);
 
 }