From ed27b38c0ad7a2a88884fdeb8fcf2a7b8d277751 Mon Sep 17 00:00:00 2001
From: Tor Brede Vekterli <vekterli@verizonmedia.com>
Date: Thu, 7 Nov 2019 14:17:35 +0000
Subject: Let Z-curve attribute be authoritative for position field existence

Avoids inconsistencies caused by concrete position struct fields not
being created or cleared by partial updates, only the Z-curve attribute
fields.

This fixes #11208
---
 .../tests/proton/server/documentretriever_test.cpp | 27 ++++++++++++++++------
 .../searchcore/proton/server/documentretriever.cpp |  4 ++--
 2 files changed, 22 insertions(+), 9 deletions(-)

(limited to 'searchcore')

diff --git a/searchcore/src/tests/proton/server/documentretriever_test.cpp b/searchcore/src/tests/proton/server/documentretriever_test.cpp
index d5e40592b12..fc12cde57af 100644
--- a/searchcore/src/tests/proton/server/documentretriever_test.cpp
+++ b/searchcore/src/tests/proton/server/documentretriever_test.cpp
@@ -134,10 +134,12 @@ const TensorDataType tensorDataType(ValueType::from_spec(tensor_spec));
 
 struct MyDocumentStore : proton::test::DummyDocumentStore {
     mutable std::unique_ptr<Document> _testDoc;
+    bool _set_position_struct_field;
 
     MyDocumentStore()
         : proton::test::DummyDocumentStore(),
-          _testDoc()
+          _testDoc(),
+          _set_position_struct_field(true)
     {
     }
 
@@ -163,11 +165,13 @@ struct MyDocumentStore : proton::test::DummyDocumentStore {
         TensorFieldValue tensorFieldValue(tensorDataType);
         tensorFieldValue = static_tensor->clone();
         doc->setValue(dyn_field_tensor, tensorFieldValue);
-        FieldValue::UP fv = PositionDataType::getInstance().createFieldValue();
-        StructFieldValue &pos = static_cast<StructFieldValue &>(*fv);
-        pos.set(PositionDataType::FIELD_X, 42);
-        pos.set(PositionDataType::FIELD_Y, 21);
-        doc->setValue(doc->getField(position_field), *fv);
+        if (_set_position_struct_field) {
+            FieldValue::UP fv = PositionDataType::getInstance().createFieldValue();
+            StructFieldValue &pos = static_cast<StructFieldValue &>(*fv);
+            pos.set(PositionDataType::FIELD_X, 42);
+            pos.set(PositionDataType::FIELD_Y, 21);
+            doc->setValue(doc->getField(position_field), *fv);
+        }
 
         return doc;
     }
@@ -473,7 +477,7 @@ TEST_F("require that attributes are patched into stored document unless also ind
     checkFieldValue<StringFieldValue>(doc->getValue(dyn_field_s), static_value_s);
 }
 
-TEST_F("require that position fields are regenerated from zcurves", Fixture) {
+void verify_position_field_has_expected_values(Fixture& f) {
     DocumentMetaData meta_data = f._retriever->getDocumentMetaData(doc_id);
     Document::UP doc = f._retriever->getDocument(meta_data.lid);
     ASSERT_TRUE(doc.get());
@@ -490,6 +494,15 @@ TEST_F("require that position fields are regenerated from zcurves", Fixture) {
     checkFieldValue<LongFieldValue>(doc->getValue(zcurve_field), dynamic_zcurve_value);
 }
 
+TEST_F("require that position fields are regenerated from zcurves", Fixture) {
+    verify_position_field_has_expected_values(f);
+}
+
+TEST_F("zcurve attribute is authoritative for position field existence", Fixture) {
+    f.doc_store._set_position_struct_field = false;
+    verify_position_field_has_expected_values(f);
+}
+
 TEST_F("require that non-existing lid returns null pointer", Fixture) {
     Document::UP doc = f._retriever->getDocument(0);
     ASSERT_FALSE(doc.get());
diff --git a/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp b/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp
index 1a1bf1ed000..3fe58075d77 100644
--- a/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp
@@ -81,8 +81,8 @@ FieldValue::UP positionFromZcurve(int64_t zcurve) {
 void fillInPositionFields(Document &doc, DocumentIdT lid, const DocumentRetriever::PositionFields & possiblePositionFields, const IAttributeManager & attr_manager)
 {
     for (const auto & it : possiblePositionFields) {
-        if (doc.hasValue(*it.first)) {
-            AttributeGuard::UP attr = attr_manager.getAttribute(it.second);
+        AttributeGuard::UP attr = attr_manager.getAttribute(it.second);
+        if (!(*attr)->isUndefined(lid)) {
             int64_t zcurve = (*attr)->getInt(lid);
             doc.setValue(*it.first, *positionFromZcurve(zcurve));
         }
-- 
cgit v1.2.3