Merge pull request #15921 from vespa-engine/balder/let-updates-that-does-not-add-2-corpus-through

Allow updates with only remove/clear updates and operations to single…

5 files changed, 182 insertions, 10 deletions

diff --git a/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp b/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp
index d06da31b415..d466e5a00d2 100644
--- a/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/feedhandler/feedhandler_test.cpp
@@ -5,6 +5,17 @@
 #include <vespa/document/update/assignvalueupdate.h>
 #include <vespa/document/repo/documenttyperepo.h>
 #include <vespa/document/update/documentupdate.h>
+#include <vespa/document/update/clearvalueupdate.h>
+#include <vespa/document/update/removevalueupdate.h>
+#include <vespa/document/update/addvalueupdate.h>
+#include <vespa/document/update/tensor_remove_update.h>
+#include <vespa/document/update/tensor_modify_update.h>
+#include <vespa/document/update/tensor_add_update.h>
+#include <vespa/document/update/tensor_partial_update.h>
+#include <vespa/document/update/arithmeticvalueupdate.h>
+#include <vespa/document/update/mapvalueupdate.h>
+#include <vespa/document/update/removefieldpathupdate.h>
+#include <vespa/document/fieldvalue/referencefieldvalue.h>
 #include <vespa/eval/eval/simple_value.h>
 #include <vespa/eval/eval/tensor_spec.h>
 #include <vespa/eval/eval/value.h>
@@ -18,6 +29,7 @@
 #include <vespa/searchcore/proton/feedoperation/updateoperation.h>
 #include <vespa/searchcore/proton/persistenceengine/i_resource_write_filter.h>
 #include <vespa/searchcore/proton/server/configstore.h>
+#include <vespa/searchcore/proton/server/feed_reject_helper.h>
 #include <vespa/searchcore/proton/server/ddbstate.h>
 #include <vespa/searchcore/proton/server/executorthreadingservice.h>
 #include <vespa/searchcore/proton/server/feedhandler.h>
@@ -278,7 +290,7 @@ struct SchemaContext {
 };
 
 SchemaContext::SchemaContext()
-    : schema(new Schema()),
+    : schema(std::make_shared<Schema>()),
       builder()
 {
     schema->addAttributeField(Schema::AttributeField("tensor", DataType::TENSOR, CollectionType::SINGLE, "tensor(x{},y{})"));
@@ -320,7 +332,7 @@ struct UpdateContext {
     DocumentUpdate::SP update;
     BucketId           bucketId;
     UpdateContext(const vespalib::string &docId, DocBuilder &builder) :
-        update(new DocumentUpdate(*builder.getDocumentTypeRepo(), builder.getDocumentType(), DocumentId(docId))),
+        update(std::make_shared<DocumentUpdate>(*builder.getDocumentTypeRepo(), builder.getDocumentType(), DocumentId(docId))),
         bucketId(BucketFactory::getBucketId(update->getId()))
     {
     }
@@ -683,6 +695,7 @@ TEST_F("require that update is rejected if resource limit is reached", FeedHandl
     f.writeFilter._message = "Attribute resource limit reached";
 
     UpdateContext updCtx("id:test:searchdocument::foo", *f.schema.builder);
+    updCtx.addFieldUpdate("tensor");
     auto op = std::make_unique<UpdateOperation>(updCtx.bucketId, Timestamp(10), updCtx.update);
     FeedTokenContext token;
     f.handler.performOperation(std::move(token.token), std::move(op));
@@ -806,6 +819,63 @@ TEST_F("require that put with different document type repo is ok", FeedHandlerFi
     EXPECT_EQUAL(1, f.tls_writer.store_count);
 }
 
+TEST("require that fixed size field values are detected") {
+    EXPECT_TRUE(FeedRejectHelper::isFixedSizeSingleValue(document::BoolFieldValue()));
+    EXPECT_TRUE(FeedRejectHelper::isFixedSizeSingleValue(document::ByteFieldValue()));
+    EXPECT_TRUE(FeedRejectHelper::isFixedSizeSingleValue(document::ShortFieldValue()));
+    EXPECT_TRUE(FeedRejectHelper::isFixedSizeSingleValue(document::IntFieldValue()));
+    EXPECT_TRUE(FeedRejectHelper::isFixedSizeSingleValue(document::LongFieldValue()));
+    EXPECT_TRUE(FeedRejectHelper::isFixedSizeSingleValue(document::FloatFieldValue()));
+    EXPECT_TRUE(FeedRejectHelper::isFixedSizeSingleValue(document::DoubleFieldValue()));
+
+    EXPECT_FALSE(FeedRejectHelper::isFixedSizeSingleValue(document::StringFieldValue()));
+    EXPECT_FALSE(FeedRejectHelper::isFixedSizeSingleValue(document::RawFieldValue()));
+    EXPECT_FALSE(FeedRejectHelper::isFixedSizeSingleValue(document::PredicateFieldValue()));
+    EXPECT_FALSE(FeedRejectHelper::isFixedSizeSingleValue(document::ReferenceFieldValue()));
+
+    document::ArrayDataType intArrayType(*document::DataType::INT);
+    EXPECT_FALSE(FeedRejectHelper::isFixedSizeSingleValue(document::ArrayFieldValue(intArrayType)));
+}
+
+using namespace document;
+
+TEST("require that clear, remove, tensor_remove and artithmetic updates ignore feed rejection") {
+    EXPECT_FALSE(FeedRejectHelper::mustReject(ClearValueUpdate()));
+    EXPECT_FALSE(FeedRejectHelper::mustReject(RemoveValueUpdate(StringFieldValue())));
+    EXPECT_FALSE(FeedRejectHelper::mustReject(ArithmeticValueUpdate(ArithmeticValueUpdate::Add, 5.0)));
+    EXPECT_FALSE(FeedRejectHelper::mustReject(TensorRemoveUpdate(std::make_unique<TensorFieldValue>())));
+}
+
+TEST("require that add, map, tensor_modify and tensor_add updates will be rejected") {
+    EXPECT_TRUE(FeedRejectHelper::mustReject(AddValueUpdate(IntFieldValue())));
+    EXPECT_TRUE(FeedRejectHelper::mustReject(MapValueUpdate(IntFieldValue(), ClearValueUpdate())));
+    EXPECT_TRUE(FeedRejectHelper::mustReject(TensorModifyUpdate(TensorModifyUpdate::Operation::REPLACE,
+                                                                std::make_unique<TensorFieldValue>())));
+    EXPECT_TRUE(FeedRejectHelper::mustReject(TensorAddUpdate(std::make_unique<TensorFieldValue>())));
+}
+TEST("require that assign updates will be rejected based on their content") {
+    EXPECT_FALSE(FeedRejectHelper::mustReject(AssignValueUpdate(IntFieldValue())));
+    EXPECT_TRUE(FeedRejectHelper::mustReject(AssignValueUpdate(StringFieldValue())));
+}
+
+TEST_F("require that update with a fieldpath update will be rejected", SchemaContext) {
+    const DocumentType *docType = f.getRepo()->getDocumentType(f.getDocType().getName());
+    auto docUpdate = std::make_unique<DocumentUpdate>(*f.getRepo(), *docType, DocumentId("id:ns:" + docType->getName() + "::1"));
+    docUpdate->addFieldPathUpdate(FieldPathUpdate::CP(std::make_unique<RemoveFieldPathUpdate>()));
+    EXPECT_TRUE(FeedRejectHelper::mustReject(*docUpdate));
+}
+
+TEST_F("require that all value updates will be inspected before rejected", SchemaContext) {
+    const DocumentType *docType = f.getRepo()->getDocumentType(f.getDocType().getName());
+    auto docUpdate = std::make_unique<DocumentUpdate>(*f.getRepo(), *docType, DocumentId("id:ns:" + docType->getName() + "::1"));
+    docUpdate->addUpdate(FieldUpdate(docType->getField("i1")).addUpdate(ClearValueUpdate()));
+    EXPECT_FALSE(FeedRejectHelper::mustReject(*docUpdate));
+    docUpdate->addUpdate(FieldUpdate(docType->getField("i1")).addUpdate(ClearValueUpdate()));
+    EXPECT_FALSE(FeedRejectHelper::mustReject(*docUpdate));
+    docUpdate->addUpdate(FieldUpdate(docType->getField("i1")).addUpdate(AssignValueUpdate(StringFieldValue())));
+    EXPECT_TRUE(FeedRejectHelper::mustReject(*docUpdate));
+}
+
 }  // namespace
 
 TEST_MAIN()
diff --git a/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt b/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt
index 73b7404ce31..85e01f9a8ed 100644
--- a/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt
+++ b/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt
@@ -46,6 +46,7 @@ vespa_add_library(searchcore_server STATIC
     feedhandler.cpp
     feedstate.cpp
     feedstates.cpp
+    feed_reject_helper.cpp
     fileconfigmanager.cpp
     flushhandlerproxy.cpp
     forcecommitcontext.cpp
diff --git a/searchcore/src/vespa/searchcore/proton/server/feed_reject_helper.cpp b/searchcore/src/vespa/searchcore/proton/server/feed_reject_helper.cpp
new file mode 100644
index 00000000000..43cc975acc2
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/server/feed_reject_helper.cpp
@@ -0,0 +1,75 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "feed_reject_helper.h"
+#include <vespa/searchcore/proton/feedoperation/operations.h>
+#include <vespa/document/update/documentupdate.h>
+#include <vespa/document/update/assignvalueupdate.h>
+#include <vespa/document/fieldvalue/boolfieldvalue.h>
+#include <vespa/document/fieldvalue/numericfieldvalue.h>
+
+using namespace document;
+
+namespace proton {
+
+bool
+FeedRejectHelper::isFixedSizeSingleValue(const document::FieldValue & fv) {
+    return fv.inherits(BoolFieldValue::classId) || fv.inherits(NumericFieldValueBase::classId);
+}
+
+bool
+FeedRejectHelper::mustReject(const document::ValueUpdate & valueUpdate) {
+    using namespace document;
+    switch (valueUpdate.getType()) {
+        case ValueUpdate::Add:
+        case ValueUpdate::TensorAddUpdate:
+        case ValueUpdate::TensorModifyUpdate:
+        case ValueUpdate::Map:
+            return true;
+        case ValueUpdate::Assign: {
+            const auto & assign = dynamic_cast<const AssignValueUpdate &>(valueUpdate);
+            if (assign.hasValue()) {
+                if ( ! isFixedSizeSingleValue(assign.getValue())) {
+                    return true;
+                }
+            }
+        }
+        default:
+            break;
+    }
+    return false;
+}
+
+bool
+FeedRejectHelper::mustReject(const DocumentUpdate & documentUpdate) {
+    for (const auto & update : documentUpdate.getUpdates()) {
+        for (const auto & valueUpdate : update.getUpdates()) {
+            if (mustReject(*valueUpdate)) {
+                return true;
+            }
+        }
+    }
+    return ! documentUpdate.getFieldPathUpdates().empty();
+}
+
+bool
+FeedRejectHelper::mustReject(const UpdateOperation & updateOperation) {
+    using namespace document;
+    if (updateOperation.getUpdate()) {
+        return mustReject(*updateOperation.getUpdate());
+    }
+    return false;
+}
+
+bool
+FeedRejectHelper::isRejectableFeedOperation(const FeedOperation & op)
+{
+    FeedOperation::Type type = op.getType();
+    if (type == FeedOperation::PUT) {
+        return true;
+    } else if (type == FeedOperation::UPDATE_42 || type == FeedOperation::UPDATE) {
+        return mustReject(dynamic_cast<const UpdateOperation &>(op));
+    }
+    return false;
+}
+
+}
diff --git a/searchcore/src/vespa/searchcore/proton/server/feed_reject_helper.h b/searchcore/src/vespa/searchcore/proton/server/feed_reject_helper.h
new file mode 100644
index 00000000000..00a8b3d77ad
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/server/feed_reject_helper.h
@@ -0,0 +1,30 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+namespace document {
+    class FieldValue;
+    class DocumentUpdate;
+    class ValueUpdate;
+}
+
+namespace proton {
+
+class FeedOperation;
+class UpdateOperation;
+
+/**
+ * Tells wether an operation should be blocked when resourcelimits have been reached.
+ * It looks at the operation type and also the content if it is an 'update' operation.
+ */
+class FeedRejectHelper {
+public:
+    static bool isRejectableFeedOperation(const FeedOperation & op);
+    // Public only for testing
+    static bool isFixedSizeSingleValue(const document::FieldValue & fv);
+    static bool mustReject(const document::ValueUpdate & valueUpdate);
+    static bool mustReject(const document::DocumentUpdate & documentUpdate);
+    static bool mustReject(const UpdateOperation & updateOperation);
+};
+
+}
diff --git a/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp b/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp
index 441b1745616..132fdf56b90 100644
--- a/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/feedhandler.cpp
@@ -6,6 +6,7 @@
 #include "i_feed_handler_owner.h"
 #include "ifeedview.h"
 #include "configstore.h"
+#include "feed_reject_helper.h"
 #include <vespa/document/base/exceptions.h>
 #include <vespa/document/datatype/documenttype.h>
 #include <vespa/document/repo/documenttyperepo.h>
@@ -583,14 +584,9 @@ FeedHandler::tlsPrune(SerialNum oldest_to_keep) {
 
 namespace {
 
-bool
-isRejectableFeedOperation(FeedOperation::Type type)
-{
-    return type == FeedOperation::PUT || type == FeedOperation::UPDATE_42 || type == FeedOperation::UPDATE;
-}
-
 template <typename ResultType>
-void feedOperationRejected(FeedToken & token, const vespalib::string &opType, const vespalib::string &docId,
+void
+feedOperationRejected(FeedToken & token, const vespalib::string &opType, const vespalib::string &docId,
                            const DocTypeName & docTypeName, const vespalib::string &rejectMessage)
 {
     if (token) {
@@ -621,7 +617,7 @@ notifyFeedOperationRejected(FeedToken & token, const FeedOperation &op,
 bool
 FeedHandler::considerWriteOperationForRejection(FeedToken & token, const FeedOperation &op)
 {
-    if (!_writeFilter.acceptWriteOperation() && isRejectableFeedOperation(op.getType())) {
+    if (!_writeFilter.acceptWriteOperation() && FeedRejectHelper::isRejectableFeedOperation(op)) {
         IResourceWriteFilter::State state = _writeFilter.getAcceptState();
         if (!state.acceptWriteOperation()) {
             notifyFeedOperationRejected(token, op, _docTypeName, state.message());