Merge pull request #24100 from vespa-engine/geirst/unify-error-handling-in-docsum-field-writer-factor

Unify how errors during setup is handled in DocsumFieldWriterFactory.

7 files changed, 101 insertions, 76 deletions

diff --git a/searchcore/src/tests/proton/docsummary/docsummary.cpp b/searchcore/src/tests/proton/docsummary/docsummary.cpp
index 72f32ec8a29..e8710a00789 100644
--- a/searchcore/src/tests/proton/docsummary/docsummary.cpp
+++ b/searchcore/src/tests/proton/docsummary/docsummary.cpp
@@ -82,11 +82,7 @@ namespace proton {
 class MockDocsumFieldWriterFactory : public search::docsummary::IDocsumFieldWriterFactory
 {
 public:
-    std::unique_ptr<DocsumFieldWriter> create_docsum_field_writer(const vespalib::string& fieldName, const vespalib::string& overrideName, const vespalib::string& argument, bool& rc) override {
-        (void) fieldName;
-        (void) overrideName;
-        (void) argument;
-        (void) rc;
+    std::unique_ptr<DocsumFieldWriter> create_docsum_field_writer(const vespalib::string&, const vespalib::string&, const vespalib::string&) override {
         return {};
     }
 
diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.cpp b/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.cpp
index b3fa6c68b87..fb00b759574 100644
--- a/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.cpp
+++ b/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.cpp
@@ -34,65 +34,83 @@ DocsumFieldWriterFactory::has_attribute_manager() const noexcept
     return getEnvironment().getAttributeManager() != nullptr;
 }
 
+namespace {
+
+void
+throw_if_nullptr(const std::unique_ptr<DocsumFieldWriter>& writer,
+                 const vespalib::string& command)
+{
+    if (writer.get() == nullptr) {
+        throw IllegalArgumentException("Failed to create docsum field writer for command '" + command + "'.");
+    }
+}
+
+void
+throw_missing_source(const vespalib::string& command)
+{
+    throw IllegalArgumentException("Missing source for command '" + command + "'.");
+}
+
+}
+
 std::unique_ptr<DocsumFieldWriter>
-DocsumFieldWriterFactory::create_docsum_field_writer(const vespalib::string& fieldName, const vespalib::string& overrideName, const vespalib::string& argument, bool& rc)
+DocsumFieldWriterFactory::create_docsum_field_writer(const vespalib::string& field_name,
+                                                     const vespalib::string& command,
+                                                     const vespalib::string& source)
 {
-    rc = false;
     std::unique_ptr<DocsumFieldWriter> fieldWriter;
-    if (overrideName == "dynamicteaser") {
-        if ( ! argument.empty() ) {
+    if (command == "dynamicteaser") {
+        if ( ! source.empty() ) {
             auto fw = std::make_unique<DynamicTeaserDFW>(getEnvironment().getJuniper());
             auto fw_ptr = fw.get();
             fieldWriter = std::move(fw);
-            rc = fw_ptr->Init(fieldName.c_str(), argument);
+            if (!fw_ptr->Init(field_name.c_str(), source)) {
+                throw IllegalArgumentException("Failed to initialize DynamicTeaserDFW.");
+            }
         } else {
-            throw IllegalArgumentException("Missing argument");
+            throw_missing_source(command);
         }
-    } else if (overrideName == "summaryfeatures") {
+    } else if (command == "summaryfeatures") {
         fieldWriter = std::make_unique<SummaryFeaturesDFW>();
-        rc = true;
-    } else if (overrideName == "rankfeatures") {
+    } else if (command == "rankfeatures") {
         fieldWriter = std::make_unique<RankFeaturesDFW>();
-        rc = true;
-    } else if (overrideName == "empty") {
+    } else if (command == "empty") {
         fieldWriter = std::make_unique<EmptyDFW>();
-        rc = true;
-    } else if (overrideName == "copy") {
-        if ( ! argument.empty() ) {
-            fieldWriter = std::make_unique<CopyDFW>(argument);
-            rc = true;
+    } else if (command == "copy") {
+        if ( ! source.empty() ) {
+            fieldWriter = std::make_unique<CopyDFW>(source);
         } else {
-            throw IllegalArgumentException("Missing argument");
+            throw_missing_source(command);
         }
-    } else if (overrideName == "absdist") {
+    } else if (command == "absdist") {
         if (has_attribute_manager()) {
-            fieldWriter = AbsDistanceDFW::create(argument.c_str(), getEnvironment().getAttributeManager());
-            rc = static_cast<bool>(fieldWriter);
+            fieldWriter = AbsDistanceDFW::create(source.c_str(), getEnvironment().getAttributeManager());
+            throw_if_nullptr(fieldWriter, command);
         }
-    } else if (overrideName == "positions") {
+    } else if (command == "positions") {
         if (has_attribute_manager()) {
-            fieldWriter = PositionsDFW::create(argument.c_str(), getEnvironment().getAttributeManager(), _use_v8_geo_positions);
-            rc = static_cast<bool>(fieldWriter);
+            fieldWriter = PositionsDFW::create(source.c_str(), getEnvironment().getAttributeManager(), _use_v8_geo_positions);
+            throw_if_nullptr(fieldWriter, command);
         }
-    } else if (overrideName == "geopos") {
+    } else if (command == "geopos") {
         if (has_attribute_manager()) {
-            fieldWriter = GeoPositionDFW::create(argument.c_str(), getEnvironment().getAttributeManager(), _use_v8_geo_positions);
-            rc = static_cast<bool>(fieldWriter);
+            fieldWriter = GeoPositionDFW::create(source.c_str(), getEnvironment().getAttributeManager(), _use_v8_geo_positions);
+            throw_if_nullptr(fieldWriter, command);
         }
-    } else if (overrideName == "attribute") {
+    } else if (command == "attribute") {
         if (has_attribute_manager()) {
-            fieldWriter = AttributeDFWFactory::create(*getEnvironment().getAttributeManager(), argument);
-            rc = true; // Allow missing attribute vector
+            fieldWriter = AttributeDFWFactory::create(*getEnvironment().getAttributeManager(), source);
+            // Missing attribute vector is allowed, so throw_if_nullptr() is NOT used.
         }
-    } else if (overrideName == "attributecombiner") {
+    } else if (command == "attributecombiner") {
         if (has_attribute_manager()) {
             auto attr_ctx = getEnvironment().getAttributeManager()->createContext();
-            const vespalib::string& source_field = argument.empty() ? fieldName : argument;
+            const vespalib::string& source_field = source.empty() ? field_name : source;
             fieldWriter = AttributeCombinerDFW::create(source_field, *attr_ctx, false, std::shared_ptr<MatchingElementsFields>());
-            rc = static_cast<bool>(fieldWriter);
+            throw_if_nullptr(fieldWriter, command);
         }
-    } else if (overrideName == "matchedattributeelementsfilter") {
-        const vespalib::string& source_field = argument.empty() ? fieldName : argument;
+    } else if (command == "matchedattributeelementsfilter") {
+        const vespalib::string& source_field = source.empty() ? field_name : source;
         if (has_attribute_manager()) {
             auto attr_ctx = getEnvironment().getAttributeManager()->createContext();
             if (attr_ctx->getAttribute(source_field) != nullptr) {
@@ -100,20 +118,19 @@ DocsumFieldWriterFactory::create_docsum_field_writer(const vespalib::string& fie
             } else {
                 fieldWriter = AttributeCombinerDFW::create(source_field, *attr_ctx, true, _matching_elems_fields);
             }
-            rc = static_cast<bool>(fieldWriter);
+            throw_if_nullptr(fieldWriter, command);
         }
-    } else if (overrideName == "matchedelementsfilter") {
-        const vespalib::string& source_field = argument.empty() ? fieldName : argument;
+    } else if (command == "matchedelementsfilter") {
+        const vespalib::string& source_field = source.empty() ? field_name : source;
         if (has_attribute_manager()) {
             auto attr_ctx = getEnvironment().getAttributeManager()->createContext();
             fieldWriter = MatchedElementsFilterDFW::create(source_field,*attr_ctx, _matching_elems_fields);
-            rc = static_cast<bool>(fieldWriter);
+            throw_if_nullptr(fieldWriter, command);
         }
-    } else if (overrideName == "documentid") {
+    } else if (command == "documentid") {
         fieldWriter = std::make_unique<DocumentIdDFW>();
-        rc = true;
     } else {
-        throw IllegalArgumentException("unknown override operation '" + overrideName + "' for field '" + fieldName + "'.");
+        throw IllegalArgumentException("Unknown command '" + command + "'.");
     }
     return fieldWriter;
 }
diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.h b/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.h
index bab7153009d..e341f49c25b 100644
--- a/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.h
+++ b/searchsummary/src/vespa/searchsummary/docsummary/docsum_field_writer_factory.h
@@ -24,7 +24,9 @@ protected:
 public:
     DocsumFieldWriterFactory(bool use_v8_geo_positions, const IDocsumEnvironment& env);
     ~DocsumFieldWriterFactory() override;
-    std::unique_ptr<DocsumFieldWriter> create_docsum_field_writer(const vespalib::string& fieldName, const vespalib::string& overrideName, const vespalib::string& argument, bool& rc) override;
+    std::unique_ptr<DocsumFieldWriter> create_docsum_field_writer(const vespalib::string& field_name,
+                                                                  const vespalib::string& command,
+                                                                  const vespalib::string& source) override;
 };
 
 }
diff --git a/searchsummary/src/vespa/searchsummary/docsummary/i_docsum_field_writer_factory.h b/searchsummary/src/vespa/searchsummary/docsummary/i_docsum_field_writer_factory.h
index 927fef26d1a..6a5cd691857 100644
--- a/searchsummary/src/vespa/searchsummary/docsummary/i_docsum_field_writer_factory.h
+++ b/searchsummary/src/vespa/searchsummary/docsummary/i_docsum_field_writer_factory.h
@@ -16,7 +16,12 @@ class IDocsumFieldWriterFactory
 {
 public:
     virtual ~IDocsumFieldWriterFactory() = default;
-    virtual std::unique_ptr<DocsumFieldWriter> create_docsum_field_writer(const vespalib::string& fieldName, const vespalib::string& overrideName, const vespalib::string& argument, bool& rc) = 0;
+    /**
+     * Implementations can throw vespalib::IllegalArgumentException if setup of field writer fails.
+     */
+    virtual std::unique_ptr<DocsumFieldWriter> create_docsum_field_writer(const vespalib::string& field_name,
+                                                                          const vespalib::string& command,
+                                                                          const vespalib::string& source) = 0;
 };
 
 }
diff --git a/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp b/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp
index 65b11a2b66f..4f5b5db841c 100644
--- a/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp
+++ b/searchsummary/src/vespa/searchsummary/docsummary/resultconfig.cpp
@@ -4,8 +4,9 @@
 #include "docsum_field_writer.h"
 #include "docsum_field_writer_factory.h"
 #include "resultclass.h"
-#include <vespa/vespalib/stllike/hash_map.hpp>
 #include <vespa/config-summary.h>
+#include <vespa/vespalib/stllike/hash_map.hpp>
+#include <vespa/vespalib/util/exceptions.h>
 #include <atomic>
 
 #include <vespa/log/log.h>
@@ -126,16 +127,20 @@ ResultConfig::ReadConfig(const SummaryConfig &cfg, const char *configId, IDocsum
         for (unsigned int j = 0; rc && (j < cfg_class.fields.size()); j++) {
             const char *fieldtype = cfg_class.fields[j].type.c_str();
             const char *fieldname = cfg_class.fields[j].name.c_str();
-            vespalib::string override_name = cfg_class.fields[j].command;
+            vespalib::string command = cfg_class.fields[j].command;
             vespalib::string source_name = cfg_class.fields[j].source;
             auto res_type = ResTypeUtils::get_res_type(fieldtype);
             LOG(debug, "Reconfiguring class '%s' field '%s' of type '%s'", cfg_class.name.c_str(), fieldname, fieldtype);
             if (res_type != RES_BAD) {
                 std::unique_ptr<DocsumFieldWriter> docsum_field_writer;
-                if (!override_name.empty()) {
-                    docsum_field_writer = docsum_field_writer_factory.create_docsum_field_writer(fieldname, override_name, source_name, rc);
-                    if (!rc) {
-                        LOG(error, "%s override operation failed during initialization", override_name.c_str());
+                if (!command.empty()) {
+                    try {
+                        docsum_field_writer = docsum_field_writer_factory.create_docsum_field_writer(fieldname,
+                                                                                                     command,
+                                                                                                     source_name);
+                    } catch (const vespalib::IllegalArgumentException& ex) {
+                        LOG(error, "Exception during setup of summary result class '%s': field='%s', command='%s', source='%s': %s",
+                            cfg_class.name.c_str(), fieldname, command.c_str(), source_name.c_str(), ex.getMessage().c_str());
                         break;
                     }
                 }
diff --git a/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.cpp b/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.cpp
index 35410f3ec67..d5ecb158edb 100644
--- a/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.cpp
+++ b/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.cpp
@@ -44,35 +44,33 @@ DocsumFieldWriterFactory::DocsumFieldWriterFactory(bool use_v8_geo_positions, co
 DocsumFieldWriterFactory::~DocsumFieldWriterFactory() = default;
 
 std::unique_ptr<DocsumFieldWriter>
-DocsumFieldWriterFactory::create_docsum_field_writer(const vespalib::string& fieldName, const vespalib::string& overrideName, const vespalib::string& argument, bool& rc)
+DocsumFieldWriterFactory::create_docsum_field_writer(const vespalib::string& field_name,
+                                                     const vespalib::string& command,
+                                                     const vespalib::string& source)
 {
     std::unique_ptr<DocsumFieldWriter> fieldWriter;
-    if ((overrideName == "staticrank") ||
-        (overrideName == "ranklog") ||
-        (overrideName == "label") ||
-        (overrideName == "project") ||
-        (overrideName == "positions") ||
-        (overrideName == "absdist") ||
-        (overrideName == "subproject"))
+    if ((command == "staticrank") ||
+        (command == "ranklog") ||
+        (command == "label") ||
+        (command == "project") ||
+        (command == "positions") ||
+        (command == "absdist") ||
+        (command == "subproject"))
     {
         fieldWriter = std::make_unique<EmptyDFW>();
-        rc = true;
-    } else if ((overrideName == "attribute") ||
-               (overrideName == "attributecombiner")) {
-        if (!argument.empty() && argument != fieldName) {
-            fieldWriter = std::make_unique<CopyDFW>(argument);
+    } else if ((command == "attribute") ||
+               (command == "attributecombiner")) {
+        if (!source.empty() && source != field_name) {
+            fieldWriter = std::make_unique<CopyDFW>(source);
         }
-        rc = true;
-    } else if (overrideName == "geopos") {
-        rc = true;
-    } else if ((overrideName == "matchedattributeelementsfilter") ||
-               (overrideName == "matchedelementsfilter")) {
-        vespalib::string source_field = argument.empty() ? fieldName : argument;
+    } else if (command == "geopos") {
+    } else if ((command == "matchedattributeelementsfilter") ||
+               (command == "matchedelementsfilter")) {
+        vespalib::string source_field = source.empty() ? field_name : source;
         populate_fields(*_matching_elems_fields, _vsm_fields_config, source_field);
         fieldWriter = MatchedElementsFilterDFW::create(source_field, _matching_elems_fields);
-        rc = static_cast<bool>(fieldWriter);
     } else {
-        return search::docsummary::DocsumFieldWriterFactory::create_docsum_field_writer(fieldName, overrideName, argument, rc);
+        return search::docsummary::DocsumFieldWriterFactory::create_docsum_field_writer(field_name, command, source);
     }
     return fieldWriter;
 }
diff --git a/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.h b/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.h
index c06cb0454b3..17c270292e2 100644
--- a/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.h
+++ b/streamingvisitors/src/vespa/vsm/vsm/docsum_field_writer_factory.h
@@ -19,7 +19,9 @@ public:
     DocsumFieldWriterFactory(bool use_v8_geo_positions, const search::docsummary::IDocsumEnvironment& env, const vespa::config::search::vsm::VsmfieldsConfig& vsm_fields_config);
     ~DocsumFieldWriterFactory() override;
     std::unique_ptr<search::docsummary::DocsumFieldWriter>
-    create_docsum_field_writer(const vespalib::string& fieldName, const vespalib::string& overrideName, const vespalib::string& argument, bool& rc) override;
+    create_docsum_field_writer(const vespalib::string& field_name,
+                               const vespalib::string& command,
+                               const vespalib::string& source) override;
 };
 
 }