Merge pull request #5468 from vespa-engine/toregge/add-extra-check-for-update-operations-when-document-type-changed

Add extra check for update operations when document types can be different

4 files changed, 149 insertions, 9 deletions

diff --git a/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp b/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp
index 10718508d33..5c0edce0b94 100644
--- a/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include <vespa/persistence/spi/result.h>
+#include <vespa/document/update/assignvalueupdate.h>
 #include <vespa/searchcore/proton/bucketdb/bucketdbhandler.h>
 #include <vespa/searchcore/proton/test/bucketfactory.h>
 #include <vespa/searchcore/proton/common/feedtoken.h>
@@ -26,6 +27,7 @@
 #include <vespa/vespalib/testkit/testapp.h>
 #include <vespa/vespalib/util/closuretask.h>
 #include <vespa/vespalib/util/exceptions.h>
+#include <vespa/vespalib/io/fileutil.h>
 
 #include <vespa/log/log.h>
 LOG_SETUP("feedhandler_test");
@@ -227,6 +229,12 @@ struct MyFeedView : public test::DummyFeedView {
     const ISimpleDocumentMetaStore *getDocumentMetaStorePtr() const override {
         return NULL;
     }
+    void checkCounts(int exp_update_count, SerialNum exp_update_serial, int exp_put_count, SerialNum exp_put_serial) {
+        EXPECT_EQUAL(exp_update_count, update_count);
+        EXPECT_EQUAL(exp_update_serial, update_serial);
+        EXPECT_EQUAL(exp_put_count, put_count);
+        EXPECT_EQUAL(exp_put_serial, put_serial);
+    }
 };
 
 MyFeedView::MyFeedView(const std::shared_ptr<const DocumentTypeRepo> &dtr)
@@ -250,19 +258,31 @@ MyFeedView::~MyFeedView() {}
 struct SchemaContext {
     Schema::SP                schema;
     std::unique_ptr<DocBuilder> builder;
-    SchemaContext() :
-        schema(new Schema()),
-        builder()
-    {
-        schema->addIndexField(Schema::IndexField("i1", DataType::STRING, CollectionType::SINGLE));
-        builder.reset(new DocBuilder(*schema));
-    }
+    SchemaContext();
+    ~SchemaContext();
     DocTypeName getDocType() const {
         return DocTypeName(builder->getDocumentType().getName());
     }
     const std::shared_ptr<const document::DocumentTypeRepo> &getRepo() const { return builder->getDocumentTypeRepo(); }
+    void addField(vespalib::stringref fieldName);
 };
 
+SchemaContext::SchemaContext()
+    : schema(new Schema()),
+      builder()
+{
+    addField("i1");
+}
+
+
+SchemaContext::~SchemaContext() = default;
+
+void
+SchemaContext::addField(vespalib::stringref fieldName)
+{
+    schema->addIndexField(Schema::IndexField(fieldName, DataType::STRING, CollectionType::SINGLE));
+    builder = std::make_unique<DocBuilder>(*schema);
+}
 
 struct DocumentContext {
     Document::SP  doc;
@@ -283,6 +303,16 @@ struct UpdateContext {
         bucketId(BucketFactory::getBucketId(update->getId()))
     {
     }
+    void addFieldUpdate(const vespalib::string &fieldName) {
+        const auto &docType = update->getType();
+        const auto &field = docType.getField(fieldName);
+        auto fieldValue = field.createValue();
+        fieldValue->assign(document::StringFieldValue("new value"));
+        document::AssignValueUpdate assignValueUpdate(*fieldValue);
+        document::FieldUpdate fieldUpdate(field);
+        fieldUpdate.addUpdate(assignValueUpdate);
+        update->addUpdate(fieldUpdate);
+    }
 };
 
 
@@ -644,10 +674,82 @@ TEST_F("require that remove is NOT rejected if resource limit is reached", FeedH
     EXPECT_EQUAL("", token.getResult()->getErrorMessage());
 }
 
+void
+checkUpdate(FeedHandlerFixture &f, SchemaContext &schemaContext,
+            const vespalib::string &fieldName, bool expectReject, bool existing)
+{
+    f.handler.setSerialNum(15);
+    UpdateContext updCtx("id:test:searchdocument::foo", *schemaContext.builder);
+    updCtx.addFieldUpdate(fieldName);
+    if (existing) {
+        f.feedView.metaStore.insert(updCtx.update->getId().getGlobalId(), MyDocumentMetaStore::Entry(5, 5, Timestamp(9)));
+        f.feedView.metaStore.allocate(updCtx.update->getId().getGlobalId());
+    } else {
+        updCtx.update->setCreateIfNonExistent(true);
+    }
+    FeedOperation::UP op = std::make_unique<UpdateOperation>(updCtx.bucketId, Timestamp(10), updCtx.update);
+    FeedTokenContext token;
+    f.handler.performOperation(std::move(token.token), std::move(op));
+    EXPECT_TRUE(dynamic_cast<const UpdateResult *>(token.getResult()));
+    if (expectReject) {
+        TEST_DO(f.feedView.checkCounts(0, 0u, 0, 0u));
+        EXPECT_EQUAL(Result::TRANSIENT_ERROR, token.getResult()->getErrorCode());
+        EXPECT_EQUAL("Update operation rejected for document 'id:test:searchdocument::foo' of type 'searchdocument': 'Field not found'",
+                     token.getResult()->getErrorMessage());
+    } else {
+        if (existing) {
+            TEST_DO(f.feedView.checkCounts(1, 16u, 0, 0u));
+        } else {
+            TEST_DO(f.feedView.checkCounts(0, 0u, 1, 16u));
+        }
+        EXPECT_EQUAL(Result::NONE, token.getResult()->getErrorCode());
+        EXPECT_EQUAL("", token.getResult()->getErrorMessage());
+    }
+}
+
+TEST_F("require that update with same document type repo is ok", FeedHandlerFixture)
+{
+    checkUpdate(f, f.schema, "i1", false, true);
+}
+
+TEST_F("require that update with different document type repo can be ok", FeedHandlerFixture)
+{
+    SchemaContext schema;
+    schema.addField("i2");
+    checkUpdate(f, schema, "i1", false, true);
+}
+
+TEST_F("require that update with different document type repo can be rejected", FeedHandlerFixture)
+{
+    SchemaContext schema;
+    schema.addField("i2");
+    checkUpdate(f, schema, "i2", true, true);
+}
+
+TEST_F("require that update with same document type repo is ok, fallback to create document", FeedHandlerFixture)
+{
+    checkUpdate(f, f.schema, "i1", false, false);
+}
+
+TEST_F("require that update with different document type repo can be ok, fallback to create document", FeedHandlerFixture)
+{
+    SchemaContext schema;
+    schema.addField("i2");
+    checkUpdate(f, schema, "i1", false, false);
+}
+
+TEST_F("require that update with different document type repo can be rejected, preventing fallback to create document", FeedHandlerFixture)
+{
+    SchemaContext schema;
+    schema.addField("i2");
+    checkUpdate(f, schema, "i2", true, false);
+}
+
 }  // namespace
 
 TEST_MAIN()
 {
     DummyFileHeaderContext::setCreator("feedhandler_test");
     TEST_RUN_ALL();
+    vespalib::rmdir("mytlsdir", true);
 }
diff --git a/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.sh b/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.sh
index 574234d1153..ddb94964d77 100755
--- a/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.sh
+++ b/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.sh
@@ -2,5 +2,3 @@
 # Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 set -e
 $VALGRIND ./searchcore_feedhandler_test_app
-rm -rf mytlsdir
-rm -rf myfilecfg
diff --git a/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp b/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp
index c9b3df5a6ed..ad5372b4af6 100644
--- a/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp
@@ -7,7 +7,9 @@
 #include "ifeedview.h"
 #include "tlcproxy.h"
 #include "configstore.h"
+#include <vespa/document/base/exceptions.h>
 #include <vespa/document/datatype/documenttype.h>
+#include <vespa/document/repo/documenttyperepo.h>
 #include <vespa/searchcore/proton/bucketdb/ibucketdbhandler.h>
 #include <vespa/searchcore/proton/persistenceengine/i_resource_write_filter.h>
 #include <vespa/searchcore/proton/persistenceengine/transport_latch.h>
@@ -107,8 +109,14 @@ FeedHandler::performUpdate(FeedToken token, UpdateOperation &op)
 {
     _activeFeedView->prepareUpdate(op);
     if (op.getPrevDbDocumentId().valid() && !op.getPrevMarkedAsRemoved()) {
+        if (considerUpdateOperationForRejection(token, op)) {
+            return;
+        }
         performInternalUpdate(std::move(token), op);
     } else if (op.getUpdate()->getCreateIfNonExistent()) {
+        if (considerUpdateOperationForRejection(token, op)) {
+            return;
+        }
         createNonExistingDocument(std::move(token), op);
     } else {
         if (token) {
@@ -483,6 +491,37 @@ FeedHandler::considerWriteOperationForRejection(FeedToken & token, const FeedOpe
     return false;
 }
 
+bool
+FeedHandler::considerUpdateOperationForRejection(FeedToken &token, const UpdateOperation &op)
+{
+    const auto *repo = _activeFeedView->getDocumentTypeRepo().get();
+    const auto &update = *op.getUpdate();
+    /*
+     * Check if document types are equal. DocumentTypeRepoFactory::make returns
+     * the same document type repo if document type configs are equal, thus we
+     * can just perform a cheaper identity check here.
+     */
+    if (repo->getDocumentType(_docTypeName.getName()) != &update.getType()) {
+        try {
+            vespalib::nbostream stream;
+            op.serialize(stream);
+            UpdateOperation checkOp(op.getType());
+            vespalib::nbostream checkStream(stream.peek(), stream.size());
+            checkOp.deserialize(stream, *repo);
+        } catch (document::FieldNotFoundException &e) {
+            if (token) {
+                auto message = make_string("Update operation rejected for document '%s' of type '%s': 'Field not found'",
+                                           update.getId().toString().c_str(), _docTypeName.toString().c_str());
+                token->setResult(make_unique<UpdateResult>(Result::TRANSIENT_ERROR, message), false);
+                token->fail();
+            }
+            return true;
+        }
+    }
+    return false;
+}
+
+
 void
 FeedHandler::performOperation(FeedToken token, FeedOperation::UP op)
 {
diff --git a/searchcore/src/vespa/searchcore/proton/server/feedhandler.h b/searchcore/src/vespa/searchcore/proton/server/feedhandler.h
index 171462a6c13..faf909080d9 100644
--- a/searchcore/src/vespa/searchcore/proton/server/feedhandler.h
+++ b/searchcore/src/vespa/searchcore/proton/server/feedhandler.h
@@ -102,6 +102,7 @@ private:
     void doHandleOperation(FeedToken token, FeedOperationUP op);
 
     bool considerWriteOperationForRejection(FeedToken & token, const FeedOperation &op);
+    bool considerUpdateOperationForRejection(FeedToken &token, const UpdateOperation &op);
 
     /**
      * Delayed execution of feed operations against feed view, in