Do not expose fieldupdates as a list. Hide implementation details instead.

4 files changed, 81 insertions, 49 deletions

diff --git a/document/src/main/java/com/yahoo/document/DocumentUpdate.java b/document/src/main/java/com/yahoo/document/DocumentUpdate.java
index ad93942c1c0..b33568f2ff8 100644
--- a/document/src/main/java/com/yahoo/document/DocumentUpdate.java
+++ b/document/src/main/java/com/yahoo/document/DocumentUpdate.java
@@ -13,9 +13,12 @@ import com.yahoo.document.update.ValueUpdate;
 import com.yahoo.io.GrowableByteBuffer;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 
 /**
@@ -43,7 +46,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
     public static final int CLASSID = 0x1000 + 6;
 
     private DocumentId docId;
-    private List<FieldUpdate> fieldUpdates;
+    private Map<Integer, FieldUpdate> fieldUpdates;
     private List<FieldPathUpdate> fieldPathUpdates;
     private DocumentType documentType;
     private Optional<Boolean> createIfNonExistent = Optional.empty();
@@ -55,7 +58,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
      * @param docType the document type that this update is valid for
      */
     public DocumentUpdate(DocumentType docType, DocumentId docId) {
-        this(docType, docId, new ArrayList<FieldUpdate>());
+        this(docType, docId, new HashMap<Integer, FieldUpdate>());
     }
 
     /**
@@ -64,7 +67,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
     public DocumentUpdate(DocumentUpdateReader reader) {
         docId = null;
         documentType = null;
-        fieldUpdates = new ArrayList<>();
+        fieldUpdates = new HashMap<>();
         fieldPathUpdates = new ArrayList<>();
         reader.read(this);
     }
@@ -79,7 +82,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
         this(docType, new DocumentId(docId));
     }
 
-    private DocumentUpdate(DocumentType docType, DocumentId docId, List<FieldUpdate> fieldUpdates) {
+    private DocumentUpdate(DocumentType docType, DocumentId docId, Map<Integer, FieldUpdate> fieldUpdates) {
         this.docId = docId;
         this.documentType = docType;
         this.fieldUpdates = fieldUpdates;
@@ -114,7 +117,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
     public DocumentUpdate applyTo(Document doc) {
         verifyType(doc);
 
-        for (FieldUpdate fieldUpdate : fieldUpdates) {
+        for (FieldUpdate fieldUpdate : fieldUpdates.values()) {
             fieldUpdate.applyTo(doc);
         }
         for (FieldPathUpdate fieldPathUpdate : fieldPathUpdates) {
@@ -132,8 +135,9 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
     public DocumentUpdate prune(Document doc) {
         verifyType(doc);
 
-        for (Iterator<FieldUpdate> iter = fieldUpdates.iterator(); iter.hasNext();) {
-            FieldUpdate update = iter.next();
+        for (Iterator<Map.Entry<Integer, FieldUpdate>> iter = fieldUpdates.entrySet().iterator(); iter.hasNext();) {
+            Map.Entry<Integer, FieldUpdate> entry = iter.next();
+            FieldUpdate update = entry.getValue();
             if (!update.isEmpty()) {
                 ValueUpdate last = update.getValueUpdate(update.size() - 1);
                 if (last instanceof AssignValueUpdate) {
@@ -163,8 +167,18 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
      *
      * @return a list of all FieldUpdates in this DocumentUpdate
      */
+    @Deprecated
     public List<FieldUpdate> getFieldUpdates() {
-        return Collections.unmodifiableList(fieldUpdates);
+        return Collections.unmodifiableList(new ArrayList<>(fieldUpdates.values()));
+    }
+
+    /**
+     * Get an unmodifiable collection of all field updates that this document update specifies.
+     *
+     * @return a list of all FieldUpdates in this DocumentUpdate
+     */
+    public Collection<FieldUpdate> getFieldUpdatesCollection() {
+        return Collections.unmodifiableCollection(fieldUpdates.values());
     }
 
     /**
@@ -199,8 +213,17 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
      * @return the FieldUpdate at the specified index
      * @throws IndexOutOfBoundsException if index is out of range
      */
+    @Deprecated
     public FieldUpdate getFieldUpdate(int index) {
-        return fieldUpdates.get(index);
+        if (index < 0 || index >= fieldUpdates.size()) {
+            throw new IndexOutOfBoundsException("Index " + index + " is outside of [" + 0 + ", " + fieldUpdates.size() + ">");
+        }
+        for (FieldUpdate fieldUpdate : fieldUpdates.values()) {
+            if (index-- == 0) {
+                return fieldUpdate;
+            }
+        }
+        return null;
     }
 
     /**
@@ -211,8 +234,18 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
      * @return the FieldUpdate previously at the specified position
      * @throws IndexOutOfBoundsException if index is out of range
      */
+    @Deprecated
     public FieldUpdate setFieldUpdate(int index, FieldUpdate upd) {
-        return fieldUpdates.set(index, upd);
+        if (index < 0 || index >= fieldUpdates.size()) {
+            throw new IndexOutOfBoundsException("Index " + index + " is outside of [" + 0 + ", " + fieldUpdates.size() + ">");
+        }
+        for (FieldUpdate fieldUpdate : fieldUpdates.values()) {
+            if (index-- == 0) {
+                addFieldUpdateNoCheck(fieldUpdate);
+                return fieldUpdate;
+            }
+        }
+        return null;
     }
 
     /**
@@ -222,7 +255,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
      * @return the update for the field, or null if that field has no update in this
      */
     public FieldUpdate getFieldUpdate(Field field) {
-        return getFieldUpdate(field.getName());
+        return getFieldUpdateById(field.getId());
     }
 
     /** Removes all field updates from the list for field updates. */
@@ -237,27 +270,31 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
      * @return the update for the field, or null if that field has no update in this
      */
     public FieldUpdate getFieldUpdate(String fieldName) {
-        for (FieldUpdate fieldUpdate : fieldUpdates) {
-            if (fieldUpdate.getField().getName().equals(fieldName)) {
+        for (FieldUpdate fieldUpdate : fieldUpdates.values()) {
+            if (fieldUpdate.getField().getName().equals(fieldName))
                 return fieldUpdate;
-            }
         }
         return null;
     }
+    private FieldUpdate getFieldUpdateById(Integer fieldId) {
+        return fieldUpdates.get(fieldId);
+    }
 
     /**
      * Assigns the field updates of this document update.
      * This document update receives ownership of the list - it can not be subsequently used
-     * by the caller. The list may not be unmodifiable.
+     * by the caller.
      *
      * @param fieldUpdates the new list of updates of this
      * @throws NullPointerException if the argument passed is null
      */
-    public void setFieldUpdates(List<FieldUpdate> fieldUpdates) {
+    public void setFieldUpdates(Collection<FieldUpdate> fieldUpdates) {
         if (fieldUpdates == null) {
             throw new NullPointerException("The field updates of a document update can not be null");
         }
-        this.fieldUpdates = fieldUpdates;
+        for (FieldUpdate fieldUpdate : fieldUpdates) {
+            this.fieldUpdates.put(fieldUpdate.getField().getId(), fieldUpdate);
+        }
     }
 
     /**
@@ -281,15 +318,14 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
     public DocumentUpdate addFieldUpdate(FieldUpdate update) {
         String fieldName = update.getField().getName();
         if (!documentType.hasField(fieldName)) {
-            throw new IllegalArgumentException("Document type '" + documentType.getName() + "' does not have field '" +
-                                               fieldName + "'.");
+            throw new IllegalArgumentException("Document type '" + documentType.getName() + "' does not have field '" + fieldName + "'.");
         }
         FieldUpdate prevUpdate = getFieldUpdate(fieldName);
         if (prevUpdate != update) {
             if (prevUpdate != null) {
                 prevUpdate.addAll(update);
             } else {
-                fieldUpdates.add(update);
+                fieldUpdates.put(update.getField().getId(), update);
             }
         }
         return this;
@@ -308,7 +344,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
     // TODO: Remove this when we figure out correct behaviour.
 
     public void addFieldUpdateNoCheck(FieldUpdate fieldUpdate) {
-        fieldUpdates.add(fieldUpdate);
+        fieldUpdates.put(fieldUpdate.getField().getId(), fieldUpdate);
     }
 
     /**
@@ -329,7 +365,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
         if (!documentType.equals(update.documentType)) {
             throw new IllegalArgumentException("Expected " + documentType + ", got " + update.documentType + ".");
         }
-        for (FieldUpdate fieldUpd : update.fieldUpdates) {
+        for (FieldUpdate fieldUpd : update.fieldUpdates.values()) {
             addFieldUpdate(fieldUpd);
         }
         for (FieldPathUpdate pathUpd : update.fieldPathUpdates) {
@@ -344,8 +380,14 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
      * @return the FieldUpdate previously at the specified position
      * @throws IndexOutOfBoundsException if index is out of range
      */
+    @Deprecated
     public FieldUpdate removeFieldUpdate(int index) {
-        return fieldUpdates.remove(index);
+        FieldUpdate prev = getFieldUpdate(index);
+        return removeFieldUpdate(prev.getField());
+    }
+
+    public FieldUpdate removeFieldUpdate(Field field) {
+        return fieldUpdates.remove(field.getId());
     }
 
     /**
@@ -402,7 +444,7 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP
         string.append(": ");
         string.append("[");
 
-        for (Iterator<FieldUpdate> i = fieldUpdates.iterator(); i.hasNext();) {
+        for (Iterator<FieldUpdate> i = fieldUpdates.values().iterator(); i.hasNext();) {
             FieldUpdate fieldUpdate = i.next();
             string.append(fieldUpdate);
             if (i.hasNext()) {
diff --git a/document/src/main/java/com/yahoo/document/serialization/VespaDocumentDeserializer42.java b/document/src/main/java/com/yahoo/document/serialization/VespaDocumentDeserializer42.java
index a048ea349eb..3f885844987 100644
--- a/document/src/main/java/com/yahoo/document/serialization/VespaDocumentDeserializer42.java
+++ b/document/src/main/java/com/yahoo/document/serialization/VespaDocumentDeserializer42.java
@@ -472,7 +472,7 @@ public class VespaDocumentDeserializer42 extends VespaDocumentSerializer42 imple
 
         for (int i = 0; i < size; i++) {
             // TODO: Should use checked method, but doesn't work according to test now.
-            update.addFieldUpdateNoCheck(new FieldUpdate(this, update.getDocumentType(), serializationVersion));
+            update.addFieldUpdate(new FieldUpdate(this, update.getDocumentType(), serializationVersion));
         }
     }
 
diff --git a/document/src/test/java/com/yahoo/document/DocumentUpdateTestCase.java b/document/src/test/java/com/yahoo/document/DocumentUpdateTestCase.java
index 15319985591..b04fece0ef4 100644
--- a/document/src/test/java/com/yahoo/document/DocumentUpdateTestCase.java
+++ b/document/src/test/java/com/yahoo/document/DocumentUpdateTestCase.java
@@ -259,8 +259,8 @@ public class DocumentUpdateTestCase {
         update.addFieldUpdate(FieldUpdate.createAssign(field, new IntegerFieldValue(1)));
         update.addFieldUpdate(FieldUpdate.createAssign(field, new IntegerFieldValue(2)));
 
-        assertEquals(1, update.getFieldUpdates().size());
-        FieldUpdate fieldUpdate = update.getFieldUpdate(0);
+        assertEquals(1, update.getFieldUpdatesCollection().size());
+        FieldUpdate fieldUpdate = update.getFieldUpdate(field);
         assertNotNull(fieldUpdate);
         assertEquals(field, fieldUpdate.getField());
         assertEquals(2, fieldUpdate.getValueUpdates().size());
@@ -342,13 +342,16 @@ public class DocumentUpdateTestCase {
         assertEquals(new DocumentId("doc:update:test"), upd.getId());
         assertEquals(type, upd.getType());
 
-        FieldUpdate serAssignFU = upd.getFieldUpdate(0);
+        FieldUpdate serAssignFU = upd.getFieldUpdate(type.getField("intfield"));
         assertEquals(type.getField("intfield"), serAssignFU.getField());
         ValueUpdate serAssign = serAssignFU.getValueUpdate(0);
         assertEquals(ValueUpdate.ValueUpdateClassID.ASSIGN, serAssign.getValueUpdateClassID());
         assertEquals(new IntegerFieldValue(4), serAssign.getValue());
 
-        FieldUpdate serAddFU = upd.getFieldUpdate(2);
+        ValueUpdate serArith = serAssignFU.getValueUpdate(1);
+        assertEquals(ValueUpdate.ValueUpdateClassID.ARITHMETIC, serArith.getValueUpdateClassID());
+
+        FieldUpdate serAddFU = upd.getFieldUpdate(type.getField("arrayoffloatfield"));
         assertEquals(type.getField("arrayoffloatfield"), serAddFU.getField());
         ValueUpdate serAdd1 = serAddFU.getValueUpdate(0);
         assertEquals(ValueUpdate.ValueUpdateClassID.ADD, serAdd1.getValueUpdateClassID());
@@ -363,12 +366,7 @@ public class DocumentUpdateTestCase {
         FloatFieldValue addparam3 = (FloatFieldValue)serAdd3.getValue();
         assertEquals(new FloatFieldValue(-1.00f), addparam3);
 
-        FieldUpdate arithFU = upd.getFieldUpdate(3);
-        assertEquals(type.getField("intfield"), serAssignFU.getField());
-        ValueUpdate serArith = arithFU.getValueUpdate(0);
-        assertEquals(ValueUpdate.ValueUpdateClassID.ARITHMETIC, serArith.getValueUpdateClassID());
-
-        FieldUpdate wsetFU = upd.getFieldUpdate(4);
+        FieldUpdate wsetFU = upd.getFieldUpdate(type.getField("wsfield"));
         assertEquals(type.getField("wsfield"), wsetFU.getField());
         assertEquals(2, wsetFU.size());
         ValueUpdate mapUpd = wsetFU.getValueUpdate(0);
@@ -420,8 +418,8 @@ public class DocumentUpdateTestCase {
         barUpdate.addFieldUpdate(barField);
 
         fooUpdate.addAll(barUpdate);
-        assertEquals(1, fooUpdate.getFieldUpdates().size());
-        FieldUpdate fieldUpdate = fooUpdate.getFieldUpdate(0);
+        assertEquals(1, fooUpdate.getFieldUpdatesCollection().size());
+        FieldUpdate fieldUpdate = fooUpdate.getFieldUpdate(field);
         assertNotNull(fieldUpdate);
         assertEquals(field, fieldUpdate.getField());
         assertEquals(2, fieldUpdate.getValueUpdates().size());
@@ -493,14 +491,6 @@ public class DocumentUpdateTestCase {
 
 
         assertSame(fu1, documentUpdate.getFieldUpdate(f1));
-        assertSame(fu1, documentUpdate.getFieldUpdate(0));
-        assertSame(fu2, documentUpdate.getFieldUpdate(1));
-
-        documentUpdate.setFieldUpdate(0, fu2);
-        documentUpdate.setFieldUpdate(1, fu1);
-        assertEquals(2, documentUpdate.size());
-        assertSame(fu1, documentUpdate.getFieldUpdate(1));
-        assertSame(fu2, documentUpdate.getFieldUpdate(0));
 
         try {
             documentUpdate.setFieldUpdates(null);
@@ -515,12 +505,12 @@ public class DocumentUpdateTestCase {
 
         documentUpdate.setFieldUpdates(fus);
         assertEquals(2, documentUpdate.size());
-        assertSame(fu1, documentUpdate.getFieldUpdate(0));
-        assertSame(fu2, documentUpdate.getFieldUpdate(1));
+        assertSame(fu1, documentUpdate.getFieldUpdate(fu1.getField()));
+        assertSame(fu2, documentUpdate.getFieldUpdate(fu2.getField()));
 
-        documentUpdate.removeFieldUpdate(1);
+        documentUpdate.removeFieldUpdate(fu2.getField());
         assertEquals(1, documentUpdate.size());
-        assertSame(fu1, documentUpdate.getFieldUpdate(0));
+        assertSame(fu1, documentUpdate.getFieldUpdate(fu1.getField()));
 
 
         documentUpdate.toString();
diff --git a/document/src/tests/data/serializeupdatejava.dat b/document/src/tests/data/serializeupdatejava.dat
index e2a98d42fb1..20c56228bde 100644
--- a/document/src/tests/data/serializeupdatejava.dat
+++ b/document/src/tests/data/serializeupdatejava.dat