Merge pull request #28850 from vespa-engine/jonmv/concrete-document-field-validation

Jonmv/concrete document field validation

8 files changed, 103 insertions, 45 deletions

diff --git a/document/src/main/java/com/yahoo/document/annotation/ListAnnotationContainer.java b/document/src/main/java/com/yahoo/document/annotation/ListAnnotationContainer.java
index 6ef36f0013a..c2c22558a32 100644
--- a/document/src/main/java/com/yahoo/document/annotation/ListAnnotationContainer.java
+++ b/document/src/main/java/com/yahoo/document/annotation/ListAnnotationContainer.java
@@ -10,10 +10,10 @@ import java.util.ListIterator;
 import java.util.NoSuchElementException;
 
 /**
- * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a>
+ * @author Einar M R Rosenvinge
  */
 public class ListAnnotationContainer extends IteratingAnnotationContainer {
-    private final List<Annotation> annotations = new LinkedList<Annotation>();
+    private final List<Annotation> annotations = new LinkedList<>();
 
     @Override
     void annotateAll(Collection<Annotation> annotations) {
@@ -55,7 +55,7 @@ public class ListAnnotationContainer extends IteratingAnnotationContainer {
         private boolean nextCalled = false;
 
         AnnotationIterator(ListIterator<Annotation> baseIt, IdentityHashMap<SpanNode, SpanNode> nodes) {
-            this.base = new PeekableListIterator<Annotation>(baseIt);
+            this.base = new PeekableListIterator(baseIt);
             this.nodes = nodes;
         }
 
diff --git a/document/src/main/java/com/yahoo/document/annotation/SpanNodeParent.java b/document/src/main/java/com/yahoo/document/annotation/SpanNodeParent.java
index 167ce4589da..a4d178e6925 100644
--- a/document/src/main/java/com/yahoo/document/annotation/SpanNodeParent.java
+++ b/document/src/main/java/com/yahoo/document/annotation/SpanNodeParent.java
@@ -6,7 +6,7 @@ import com.yahoo.document.datatypes.StringFieldValue;
 /**
  * An interface to be implemented by classes that can be parents of SpanNodes.
  *
- * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a>
+ * @author Einar M R Rosenvinge
  * @see SpanNode#getParent()
  */
 public interface SpanNodeParent {
@@ -15,12 +15,12 @@ public interface SpanNodeParent {
      *
      * @return the SpanTree of this, if it belongs to a SpanTree, otherwise null.
      */
-    public SpanTree getSpanTree();
+    SpanTree getSpanTree();
 
     /**
      * Returns the StringFieldValue that this node belongs to, if any.
      *
      * @return the StringFieldValue that this node belongs to, if any, otherwise null.
      */
-    public StringFieldValue getStringFieldValue();
+    StringFieldValue getStringFieldValue();
 }
diff --git a/document/src/main/java/com/yahoo/document/annotation/SpanTree.java b/document/src/main/java/com/yahoo/document/annotation/SpanTree.java
index 3dc27171d8c..f785cf3b3ec 100644
--- a/document/src/main/java/com/yahoo/document/annotation/SpanTree.java
+++ b/document/src/main/java/com/yahoo/document/annotation/SpanTree.java
@@ -22,7 +22,7 @@ import java.util.Map;
 
 /**
  * A SpanTree holds a root node of a tree of SpanNodes, and a List of Annotations pointing to these nodes
- * or each other.&nbsp;It also has a name.
+ * or each other. It also has a name.
  *
  * @author Einar M R Rosenvinge
  * @see com.yahoo.document.annotation.SpanNode
@@ -36,7 +36,7 @@ public class SpanTree implements Iterable<Annotation>, SpanNodeParent, Comparabl
     private StringFieldValue stringFieldValue;
 
     /**
-     * WARNING!&nbsp;Only to be used by deserializers!&nbsp;Creates an empty SpanTree instance.
+     * WARNING! Only to be used by deserializers! Creates an empty SpanTree instance.
      */
     public SpanTree() { }
 
@@ -65,8 +65,8 @@ public class SpanTree implements Iterable<Annotation>, SpanNodeParent, Comparabl
     public SpanTree(SpanTree otherToCopy) {
         name = otherToCopy.name;
         setRoot(copySpan(otherToCopy.root));
-        List<Annotation> annotationsToCopy = new ArrayList<Annotation>(otherToCopy.getAnnotations());
-        List<Annotation> newAnnotations = new ArrayList<Annotation>(annotationsToCopy.size());
+        List<Annotation> annotationsToCopy = new ArrayList<>(otherToCopy.getAnnotations());
+        List<Annotation> newAnnotations = new ArrayList<>(annotationsToCopy.size());
 
         for (Annotation otherAnnotationToCopy : annotationsToCopy) {
             newAnnotations.add(new Annotation(otherAnnotationToCopy));
@@ -153,7 +153,7 @@ public class SpanTree implements Iterable<Annotation>, SpanNodeParent, Comparabl
     }
 
     private IdentityHashMap<Annotation, Integer> getAnnotations(List<Annotation> annotationsToCopy) {
-        IdentityHashMap<Annotation, Integer> map = new IdentityHashMap<Annotation, Integer>();
+        IdentityHashMap<Annotation, Integer> map = new IdentityHashMap<>();
         for (int i = 0; i < annotationsToCopy.size(); i++) {
             map.put(annotationsToCopy.get(i), i);
         }
@@ -162,7 +162,7 @@ public class SpanTree implements Iterable<Annotation>, SpanNodeParent, Comparabl
 
 
     private List<SpanNode> getSpanNodes() {
-        ArrayList<SpanNode> nodes = new ArrayList<SpanNode>();
+        ArrayList<SpanNode> nodes = new ArrayList<>();
         nodes.add(root);
         Iterator<SpanNode> it = root.childIteratorRecursive();
         while (it.hasNext()) {
@@ -172,7 +172,7 @@ public class SpanTree implements Iterable<Annotation>, SpanNodeParent, Comparabl
     }
 
     private static IdentityHashMap<SpanNode, Integer> getSpanNodes(SpanTree otherToCopy) {
-        IdentityHashMap<SpanNode, Integer> map = new IdentityHashMap<SpanNode, Integer>();
+        IdentityHashMap<SpanNode, Integer> map = new IdentityHashMap<>();
         int spanNodeCounter = 0;
         map.put(otherToCopy.getRoot(), spanNodeCounter++);
         Iterator<SpanNode> it = otherToCopy.getRoot().childIteratorRecursive();
diff --git a/document/src/main/java/com/yahoo/document/datatypes/StringFieldValue.java b/document/src/main/java/com/yahoo/document/datatypes/StringFieldValue.java
index 797d89226f3..8b4b94f6bbf 100644
--- a/document/src/main/java/com/yahoo/document/datatypes/StringFieldValue.java
+++ b/document/src/main/java/com/yahoo/document/datatypes/StringFieldValue.java
@@ -106,7 +106,7 @@ public class StringFieldValue extends FieldValue {
     }
 
     /**
-     * Sets a new value for this StringFieldValue.&nbsp;NOTE that doing so will clear all span trees from this value,
+     * Sets a new value for this StringFieldValue. NOTE that doing so will clear all span trees from this value,
      * since they most certainly will not make sense for a new string value.
      *
      * @param o the new String to assign to this. An argument of null is equal to calling clear().
diff --git a/document/src/test/java/com/yahoo/document/datatypes/ArrayTestCase.java b/document/src/test/java/com/yahoo/document/datatypes/ArrayTestCase.java
index 0554b7349c2..7b1e161ef0e 100755
--- a/document/src/test/java/com/yahoo/document/datatypes/ArrayTestCase.java
+++ b/document/src/test/java/com/yahoo/document/datatypes/ArrayTestCase.java
@@ -77,7 +77,7 @@ public class ArrayTestCase {
 
     @Test
     public void testWrappedList() {
-        Array<StringFieldValue> array = new Array<StringFieldValue>(DataType.getArray(DataType.STRING));
+        Array<StringFieldValue> array = new Array<>(DataType.getArray(DataType.STRING));
         List<String> list = new ArrayList<>();
         list.add("foo");
         list.add("bar");
@@ -217,10 +217,10 @@ public class ArrayTestCase {
         assertEquals(new StringFieldValue("apple"), subArray.get(1));
 
 
-        assertEquals(false, array.containsAll(Arrays.<StringFieldValue>asList(new StringFieldValue("bob"))));
-        assertEquals(true, array.containsAll(Arrays.<StringFieldValue>asList(new StringFieldValue("foo"), new StringFieldValue("boo"), new StringFieldValue("apple"))));
+        assertEquals(false, array.containsAll(List.of(new StringFieldValue("bob"))));
+        assertEquals(true, array.containsAll(List.of(new StringFieldValue("foo"), new StringFieldValue("boo"), new StringFieldValue("apple"))));
 
-        array.removeAll(Arrays.<StringFieldValue>asList(new StringFieldValue("foo"), new StringFieldValue("boo")));
+        array.removeAll(List.of(new StringFieldValue("foo"), new StringFieldValue("boo")));
 
         assertEquals(1, array.size());
         assertEquals(1, list.size());
@@ -249,7 +249,7 @@ public class ArrayTestCase {
             assertFalse(it.hasNext());
         }
 
-        array.addAll(Arrays.<StringFieldValue>asList(new StringFieldValue("microsoft"), new StringFieldValue("google")));
+        array.addAll(List.of(new StringFieldValue("microsoft"), new StringFieldValue("google")));
 
         assertEquals(4, array.size());
         assertEquals(4, list.size());
diff --git a/documentgen-test/src/test/java/com/yahoo/vespa/config/DocumentGenPluginTest.java b/documentgen-test/src/test/java/com/yahoo/vespa/config/DocumentGenPluginTest.java
index 8c49ec9ba29..cae11d66d13 100644
--- a/documentgen-test/src/test/java/com/yahoo/vespa/config/DocumentGenPluginTest.java
+++ b/documentgen-test/src/test/java/com/yahoo/vespa/config/DocumentGenPluginTest.java
@@ -79,6 +79,7 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -500,6 +501,29 @@ public class DocumentGenPluginTest {
         assertEquals(book.getFieldValue("isbn"), new StringFieldValue("ISBN YEP"));
     }
 
+    @Test
+    public void testSetterValidation() {
+        Book book = new Book(new DocumentId("id:book:book::0"));
+
+        book.setAuthor("Herman Melville");
+        assertEquals("The string field value contains illegal code point 0x16",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> book.setAuthor("He\u0016rman Malville")).getMessage());
+
+        book.setRef(new DocumentId("id:ns:parent::foo"));
+        assertEquals("Can't assign document ID 'id:ns:common::bar' (of type 'common') to reference of document type 'parent'",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> book.setRef(new DocumentId("id:ns:common::bar"))).getMessage());
+
+        book.setStringmap(Map.of("foo", "bar"));
+        assertEquals("The string field value contains illegal code point 0x16",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> book.setStringmap(Map.of("foo", "bar\u0016"))).getMessage());
+        assertEquals("The string field value contains illegal code point 0x16",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> book.setStringmap(Map.of("bar\u0016", "foo"))).getMessage());
+    }
+
     public static class BookProcessor extends DocumentProcessor {
 
         public Progress process(Processing processing) {
diff --git a/vespa-documentgen-plugin/src/main/java/com/yahoo/vespa/DocumentGenMojo.java b/vespa-documentgen-plugin/src/main/java/com/yahoo/vespa/DocumentGenMojo.java
index 43e74af9c07..5cd1d3bf9b6 100644
--- a/vespa-documentgen-plugin/src/main/java/com/yahoo/vespa/DocumentGenMojo.java
+++ b/vespa-documentgen-plugin/src/main/java/com/yahoo/vespa/DocumentGenMojo.java
@@ -851,8 +851,12 @@ public class DocumentGenMojo extends AbstractMojo {
         for (Field field: fields) {
             DataType dt = field.getDataType();
             out.write(
-            ind(ind)+"public "+toJavaType(dt)+" "+getter(field.getName())+"() { return "+field.getName()+"; }\n"+
-            ind(ind)+"public "+className+" "+setter(field.getName())+"("+toJavaType(dt)+" "+field.getName()+") { this."+field.getName()+"="+field.getName()+"; return this; }\n");
+                    ind(ind) + "public " + toJavaType(dt) + " " + getter(field.getName()) + "() { return " + field.getName() + "; }\n" +
+                    ind(ind) + "public " + className + " " + setter(field.getName()) + "(" + toJavaType(dt) + " " + field.getName() + ") {\n" +
+                    validateArgument(field.getDataType(), field.getName(), ind + 1) +
+                    ind(ind+1) + "this." + field.getName() + "=" + field.getName() + ";\n" +
+                    ind(ind+1) + "return this;\n" +
+                    ind(ind) + "}\n");
             if (spanTrees && dt.equals(DataType.STRING)) {
                 out.write(ind(ind)+"public java.util.Map<java.lang.String,com.yahoo.document.annotation.SpanTree> "+spanTreeGetter(field.getName())+"() { return "+field.getName()+"SpanTrees; }\n" +
                         ind(ind)+"public void "+spanTreeSetter(field.getName())+"(java.util.Map<java.lang.String,com.yahoo.document.annotation.SpanTree> spanTrees) { this."+field.getName()+"SpanTrees=spanTrees; }\n");
@@ -861,6 +865,38 @@ public class DocumentGenMojo extends AbstractMojo {
         out.write("\n");
     }
 
+    private static String validateArgument(DataType type, String variable, int ind) {
+        if (type instanceof MapDataType mdt) {
+            return validateWrapped(mdt.getKeyType(), variable, variable + ".keySet()", ind) +
+                   validateWrapped(mdt.getValueType(), variable, variable + ".values()", ind);
+        }
+        else if (type instanceof CollectionDataType cdt) {
+            String elements = cdt instanceof WeightedSetDataType ? variable + ".keySet()" : variable;
+            return validateWrapped(cdt.getNestedType(), variable, elements, ind);
+        }
+        else if (   DataType.STRING.equals(type)
+                 || DataType.URI.equals(type)
+                 || type instanceof AnnotationReferenceDataType
+                 || type instanceof NewDocumentReferenceDataType) {
+            return ind(ind) + "if (" + variable + " != null) {\n" +
+                   ind(ind+1) + toJavaReference(type) + ".createFieldValue(" + variable + ");\n" +
+                   ind(ind) + "}\n";
+        }
+        else {
+            return "";
+        }
+    }
+
+    private static String validateWrapped(DataType type, String variable, String elements, int ind) {
+        String wrappedValidation = validateArgument(type, variable + "$", ind + 2);
+        if (wrappedValidation.isBlank()) return "";
+        return ind(ind) + "if (" + variable + " != null) {\n" +
+               ind(ind+1) + "for (" + toJavaType(type) + " " + variable + "$ : " + elements + ") {\n" +
+               wrappedValidation +
+               ind(ind+1) + "}\n" +
+               ind(ind) + "}\n";
+    }
+
     private static String spanTreeSetter(String field) {
         return setter(field)+"SpanTrees";
     }
@@ -914,10 +950,10 @@ public class DocumentGenMojo extends AbstractMojo {
         if (DataType.BOOL.equals(dt)) return "java.lang.Boolean";
         if (DataType.TAG.equals(dt)) return "java.lang.String";
         if (dt instanceof StructDataType) return className(dt.getName());
-        if (dt instanceof WeightedSetDataType) return "java.util.Map<"+toJavaType(((WeightedSetDataType)dt).getNestedType())+",java.lang.Integer>";
-        if (dt instanceof ArrayDataType) return "java.util.List<"+toJavaType(((ArrayDataType)dt).getNestedType())+">";
-        if (dt instanceof MapDataType) return "java.util.Map<"+toJavaType(((MapDataType)dt).getKeyType())+","+toJavaType(((MapDataType)dt).getValueType())+">";
-        if (dt instanceof AnnotationReferenceDataType) return className(((AnnotationReferenceDataType) dt).getAnnotationType().getName());
+        if (dt instanceof WeightedSetDataType wdt) return "java.util.Map<"+toJavaType(wdt.getNestedType())+",java.lang.Integer>";
+        if (dt instanceof ArrayDataType adt) return "java.util.List<"+toJavaType(adt.getNestedType())+">";
+        if (dt instanceof MapDataType mdt) return "java.util.Map<"+toJavaType(mdt.getKeyType())+","+toJavaType((mdt).getValueType())+">";
+        if (dt instanceof AnnotationReferenceDataType ardt) return className(ardt.getAnnotationType().getName());
         if (dt instanceof NewDocumentReferenceDataType) {
             return "com.yahoo.document.DocumentId";
         }
@@ -942,22 +978,22 @@ public class DocumentGenMojo extends AbstractMojo {
         if (DataType.BOOL.equals(dt)) return "com.yahoo.document.DataType.BOOL";
         if (DataType.TAG.equals(dt)) return "com.yahoo.document.DataType.TAG";
         if (dt instanceof StructDataType) return  className(dt.getName()) +".type";
-        if (dt instanceof WeightedSetDataType) return "new com.yahoo.document.WeightedSetDataType("+toJavaReference(((WeightedSetDataType)dt).getNestedType())+", "+
-            ((WeightedSetDataType)dt).createIfNonExistent()+", "+ ((WeightedSetDataType)dt).removeIfZero()+","+dt.getId()+")";
-        if (dt instanceof ArrayDataType) return "new com.yahoo.document.ArrayDataType("+toJavaReference(((ArrayDataType)dt).getNestedType())+")";
-        if (dt instanceof MapDataType) return "new com.yahoo.document.MapDataType("+toJavaReference(((MapDataType)dt).getKeyType())+", "+
-            toJavaReference(((MapDataType)dt).getValueType())+", "+dt.getId()+")";
+        if (dt instanceof WeightedSetDataType wdt) return "new com.yahoo.document.WeightedSetDataType("+toJavaReference(wdt.getNestedType())+", "+
+            wdt.createIfNonExistent()+", "+ wdt.removeIfZero()+","+dt.getId()+")";
+        if (dt instanceof ArrayDataType adt) return "new com.yahoo.document.ArrayDataType("+toJavaReference(adt.getNestedType())+")";
+        if (dt instanceof MapDataType mdt) return "new com.yahoo.document.MapDataType("+toJavaReference(mdt.getKeyType())+", "+
+            toJavaReference(mdt.getValueType())+", "+dt.getId()+")";
         // For annotation references and generated types, the references are to the actual objects of the correct types, so most likely this is never needed,
         // but there might be scenarios where we want to look up the AnnotationType in the AnnotationTypeRegistry here instead.
-        if (dt instanceof AnnotationReferenceDataType) return "new com.yahoo.document.annotation.AnnotationReferenceDataType(new com.yahoo.document.annotation.AnnotationType(\""+((AnnotationReferenceDataType)dt).getAnnotationType().getName()+"\"))";
-        if (dt instanceof NewDocumentReferenceDataType) {
+        if (dt instanceof AnnotationReferenceDataType adt) return "new com.yahoo.document.annotation.AnnotationReferenceDataType(new com.yahoo.document.annotation.AnnotationType(\""+adt.getAnnotationType().getName()+"\"))";
+        if (dt instanceof NewDocumentReferenceDataType nrdt) {
             // All concrete document types have a public `type` constant with their DocumentType.
             return String.format("new com.yahoo.document.ReferenceDataType(%s.type, %d)",
-                    className(((NewDocumentReferenceDataType) dt).getTargetType().getName()), dt.getId());
+                    className(nrdt.getTargetType().getName()), dt.getId());
         }
-        if (dt instanceof TensorDataType) {
+        if (dt instanceof TensorDataType tdt) {
             return String.format("new com.yahoo.document.TensorDataType(com.yahoo.tensor.TensorType.fromSpec(\"%s\"))",
-                    ((TensorDataType)dt).getTensorType().toString());
+                    tdt.getTensorType().toString());
         }
         return "com.yahoo.document.DataType.RAW";
     }
diff --git a/vespa-documentgen-plugin/src/test/java/com/yahoo/vespa/DocumentGenTest.java b/vespa-documentgen-plugin/src/test/java/com/yahoo/vespa/DocumentGenTest.java
index a2820e12309..d0315f84272 100644
--- a/vespa-documentgen-plugin/src/test/java/com/yahoo/vespa/DocumentGenTest.java
+++ b/vespa-documentgen-plugin/src/test/java/com/yahoo/vespa/DocumentGenTest.java
@@ -11,8 +11,9 @@ import java.io.File;
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 public class DocumentGenTest {
 
@@ -39,7 +40,7 @@ public class DocumentGenTest {
         assertEquals(searches.get("music3").getDocument("music3").getField("pos").getDataType().getName(), "position");
         assertTrue(searches.get("book").getDocument("book").getField("mystruct").getDataType() instanceof StructDataType);
         assertTrue(searches.get("book").getDocument("book").getField("mywsinteger").getDataType() instanceof WeightedSetDataType);
-        assertTrue(((WeightedSetDataType)(searches.get("book").getDocument("book").getField("mywsinteger").getDataType())).getNestedType() == DataType.INT);
+        assertSame(((WeightedSetDataType) (searches.get("book").getDocument("book").getField("mywsinteger").getDataType())).getNestedType(), DataType.INT);
     }
 
     @Test
@@ -50,18 +51,15 @@ public class DocumentGenTest {
         assertEquals(searches.get("video").getDocument("video").getField("weight").getDataType(), DataType.FLOAT);
         assertTrue(searches.get("book").getDocument("book").getField("mystruct").getDataType() instanceof StructDataType);
         assertTrue(searches.get("book").getDocument("book").getField("mywsinteger").getDataType() instanceof WeightedSetDataType);
-        assertTrue(((WeightedSetDataType)(searches.get("book").getDocument("book").getField("mywsinteger").getDataType())).getNestedType() == DataType.INT);
+        assertSame(((WeightedSetDataType) (searches.get("book").getDocument("book").getField("mywsinteger").getDataType())).getNestedType(), DataType.INT);
     }
 
     @Test
     public void testEmptyPkgNameForbidden() {
-        DocumentGenMojo mojo = new DocumentGenMojo();
-        try {
-            mojo.execute(new File("etc/localapp/"), new File("target/generated-test-sources/vespa-documentgen-plugin/"), "");
-            fail("Didn't throw in empty pkg");
-        } catch (IllegalArgumentException e) {
-
-        }
+        assertThrows(IllegalArgumentException.class,
+                     () -> new DocumentGenMojo().execute(new File("etc/localapp/"),
+                                                         new File("target/generated-test-sources/vespa-documentgen-plugin/"),
+                                                         ""));
     }
 
 }