Merge pull request #20875 from vespa-engine/vekterli/disallow-none-fieldset-for-visitors

Expicitly reject "none"-fieldset for external visitors

3 files changed, 28 insertions, 0 deletions

diff --git a/storage/src/tests/distributor/visitoroperationtest.cpp b/storage/src/tests/distributor/visitoroperationtest.cpp
index ea99186a434..6c597b620dd 100644
--- a/storage/src/tests/distributor/visitoroperationtest.cpp
+++ b/storage/src/tests/distributor/visitoroperationtest.cpp
@@ -252,6 +252,20 @@ TEST_F(VisitorOperationTest, no_bucket) {
               runEmptyVisitor(msg));
 }
 
+TEST_F(VisitorOperationTest, none_fieldset_is_rejected) {
+    enable_cluster_state("distributor:1 storage:1");
+    auto msg = std::make_shared<api::CreateVisitorCommand>(
+            makeBucketSpace(), "dumpvisitor", "instance", "");
+    msg->addBucketToBeVisited(document::BucketId(16, 1));
+    msg->addBucketToBeVisited(nullId);
+    msg->setFieldSet("[none]");
+
+    EXPECT_EQ("CreateVisitorReply(last=BucketId(0x0000000000000000)) "
+              "ReturnCode(ILLEGAL_PARAMETERS, Field set '[none]' is not supported "
+              "for external visitor operations. Use '[id]' to return documents with no fields set.)",
+              runEmptyVisitor(msg));
+}
+
 TEST_F(VisitorOperationTest, only_super_bucket_and_progress_allowed) {
     enable_cluster_state("distributor:1 storage:1");
 
diff --git a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp
index 42411d53d52..5abaad6ef9f 100644
--- a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "visitoroperation.h"
+#include <vespa/document/fieldset/fieldsets.h>
 #include <vespa/storage/common/reindexing_constants.h>
 #include <vespa/storage/storageserver/storagemetricsset.h>
 #include <vespa/storage/distributor/top_level_distributor.h>
@@ -347,6 +348,17 @@ VisitorOperation::verifyOperationSentToCorrectDistributor()
     verifyDistributorOwnsBucket(_superBucket.bid);
 }
 
+void
+VisitorOperation::verify_fieldset_makes_sense_for_visiting()
+{
+    if (_msg->getFieldSet() == document::NoFields::NAME) {
+        throw VisitorVerificationException(
+                api::ReturnCode::ILLEGAL_PARAMETERS,
+                "Field set '[none]' is not supported for external visitor operations. "
+                "Use '[id]' to return documents with no fields set.");
+    }
+}
+
 bool
 VisitorOperation::verifyCreateVisitorCommand(DistributorStripeMessageSender& sender)
 {
@@ -354,6 +366,7 @@ VisitorOperation::verifyCreateVisitorCommand(DistributorStripeMessageSender& sen
         verifyOperationContainsBuckets();
         verifyOperationHasSuperbucketAndProgress();
         verifyOperationSentToCorrectDistributor();
+        verify_fieldset_makes_sense_for_visiting();
         // TODO wrap and test
         if (is_read_for_write() && (_msg->getMaxBucketsPerVisitor() != 1)) {
             throw VisitorVerificationException(
diff --git a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
index c38c463a313..ce10ea87c11 100644
--- a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
+++ b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
@@ -106,6 +106,7 @@ private:
     void verifyOperationContainsBuckets();
     void verifyOperationHasSuperbucketAndProgress();
     void verifyOperationSentToCorrectDistributor();
+    void verify_fieldset_makes_sense_for_visiting();
     bool verifyCreateVisitorCommand(DistributorStripeMessageSender& sender);
     bool pickBucketsToVisit(const std::vector<BucketDatabase::Entry>& buckets);
     bool expandBucketContaining();