Reverse iteration over selection result in an attempt to

avoid stale array indexes being used as part of
applying a remove field path update.

This change assumes that variables stored in result list
have increasing values.

Add unit test for removal of multiple array elements
using remove field path update.

3 files changed, 32 insertions, 1 deletions

diff --git a/document/src/tests/fieldpathupdatetestcase.cpp b/document/src/tests/fieldpathupdatetestcase.cpp
index f5db5831912..35b851ca895 100644
--- a/document/src/tests/fieldpathupdatetestcase.cpp
+++ b/document/src/tests/fieldpathupdatetestcase.cpp
@@ -38,6 +38,7 @@ struct FieldPathUpdateTestCase : public CppUnit::TestFixture {
     void testRemoveField();
     void testApplyRemoveEntireListField();
     void testApplyRemoveMultiList();
+    void testApplyRemoveMultiList2();
     void testApplyRemoveMultiWset();
     void testApplyAssignSingle();
     void testApplyAssignMath();
@@ -79,6 +80,7 @@ struct FieldPathUpdateTestCase : public CppUnit::TestFixture {
     CPPUNIT_TEST(testRemoveField);
     CPPUNIT_TEST(testApplyRemoveEntireListField);
     CPPUNIT_TEST(testApplyRemoveMultiList);
+    CPPUNIT_TEST(testApplyRemoveMultiList2);
     CPPUNIT_TEST(testApplyRemoveMultiWset);
     CPPUNIT_TEST(testApplyAssignSingle);
     CPPUNIT_TEST(testApplyAssignMath);
@@ -420,6 +422,32 @@ FieldPathUpdateTestCase::testApplyRemoveMultiList()
 }
 
 void
+FieldPathUpdateTestCase::testApplyRemoveMultiList2()
+{
+    Document::UP doc(new Document(_foobar_type, DocumentId("doc:things:thangs")));
+    doc->setRepo(*_repo);
+    CPPUNIT_ASSERT(doc->hasValue("strarray") == false);
+    {
+        ArrayFieldValue strArray(doc->getType().getField("strarray").getDataType());
+        strArray.add(StringFieldValue("remove val 1"));
+        strArray.add(StringFieldValue("remove val 1"));
+        strArray.add(StringFieldValue("hello hello"));
+        doc->setValue("strarray", strArray);
+    }
+    CPPUNIT_ASSERT(doc->hasValue("strarray"));
+    //doc->print(std::cerr, true, "");
+    DocumentUpdate docUp(*_repo, _foobar_type, DocumentId("doc:barbar:foofoo"));
+    docUp.addFieldPathUpdate(FieldPathUpdate::CP(
+            new RemoveFieldPathUpdate("strarray[$x]", "foobar.strarray[$x] == \"remove val 1\"")));
+    docUp.applyTo(*doc);
+    {
+        std::unique_ptr<ArrayFieldValue> strArray = doc->getAs<ArrayFieldValue>(doc->getField("strarray"));
+        CPPUNIT_ASSERT_EQUAL(std::size_t(1), strArray->size());
+        CPPUNIT_ASSERT_EQUAL(vespalib::string("hello hello"), (*strArray)[0].getAsString());
+    }
+}
+
+void
 FieldPathUpdateTestCase::testApplyRemoveEntireListField()
 {
     Document::UP doc(new Document(_foobar_type, DocumentId("doc:things:thangs")));
diff --git a/document/src/vespa/document/select/resultlist.h b/document/src/vespa/document/select/resultlist.h
index f571106f95b..cd019828ce1 100644
--- a/document/src/vespa/document/select/resultlist.h
+++ b/document/src/vespa/document/select/resultlist.h
@@ -14,6 +14,7 @@ public:
     typedef std::vector<ResultPair> Results;
     typedef Results::iterator iterator;
     typedef Results::const_iterator const_iterator;
+    using const_reverse_iterator = Results::const_reverse_iterator;
 
     ResultList();
     ResultList(ResultList &&) = default;
@@ -45,6 +46,8 @@ public:
     const_iterator end() const { return _results.end(); }
     iterator begin() { return _results.begin(); }
     iterator end() { return _results.end(); }
+    const_reverse_iterator rbegin() const { return _results.rbegin(); }
+    const_reverse_iterator rend() const { return _results.rend(); }
 
 private:
     Results _results;
diff --git a/document/src/vespa/document/update/fieldpathupdate.cpp b/document/src/vespa/document/update/fieldpathupdate.cpp
index fa7a8b38aba..cef5dc21360 100644
--- a/document/src/vespa/document/update/fieldpathupdate.cpp
+++ b/document/src/vespa/document/update/fieldpathupdate.cpp
@@ -74,7 +74,7 @@ FieldPathUpdate::applyTo(Document& doc) const
     } else {
         std::unique_ptr<select::Node> whereClause = parseDocumentSelection(_originalWhereClause, *doc.getRepo());
         select::ResultList results = whereClause->contains(doc);
-        for (select::ResultList::const_iterator i = results.begin(); i != results.end(); ++i) {
+        for (select::ResultList::const_reverse_iterator i = results.rbegin(); i != results.rend(); ++i) {
             LOG(spam, "vars = %s", handler->getVariables().toString().c_str());
             if (*i->second == select::Result::True) {
                 handler->setVariables(i->first);