From 9f35a6b829aff968d506239b0945266fc7866f65 Mon Sep 17 00:00:00 2001
From: Steve Rowe <sarowe@apache.org>
Date: Tue, 27 Sep 2016 18:16:55 -0400
Subject: [PATCH] SOLR-9411: Better validation for Schema API add-field

---
 solr/CHANGES.txt                              |  2 +-
 .../org/apache/solr/schema/SchemaManager.java | 12 +----
 .../solr/rest/schema/TestBulkSchemaAPI.java   | 47 +++++++++++++++++--
 3 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 2f81d0a83df..9073e8f5bf6 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -129,7 +129,7 @@ Bug Fixes
 
 * SOLR-9330: Fix AlreadyClosedException on admin/mbeans?stats=true (Mikhail Khludnev)
 
-* SOLR-9411: Better validation for Schema REST API add-dynamic-field (janhoy)
+* SOLR-9411: Better validation for Schema API add-field and add-dynamic-field (janhoy, Steve Rowe)
 
 Optimizations
 ----------------------
diff --git a/solr/core/src/java/org/apache/solr/schema/SchemaManager.java b/solr/core/src/java/org/apache/solr/schema/SchemaManager.java
index 0a83db23321..4b0ea546fe4 100644
--- a/solr/core/src/java/org/apache/solr/schema/SchemaManager.java
+++ b/solr/core/src/java/org/apache/solr/schema/SchemaManager.java
@@ -226,13 +226,8 @@ public class SchemaManager {
         String type = op.getStr(TYPE);
         if (op.hasError())
           return false;
-        FieldType ft = mgr.managedIndexSchema.getFieldTypeByName(type);
-        if (ft == null) {
-          op.addError("No such field type '" + type + "'");
-          return false;
-        }
         try {
-          SchemaField field = SchemaField.create(name, ft, op.getValuesExcluding(NAME, TYPE));
+          SchemaField field = mgr.managedIndexSchema.newField(name, type, op.getValuesExcluding(NAME, TYPE));
           mgr.managedIndexSchema
               = mgr.managedIndexSchema.addFields(singletonList(field), Collections.emptyMap(), false);
           return true;
@@ -248,11 +243,6 @@ public class SchemaManager {
         String type = op.getStr(TYPE);
         if (op.hasError())
           return false;
-        FieldType ft = mgr.managedIndexSchema.getFieldTypeByName(type);
-        if (ft == null) {
-          op.addError("No such field type '" + type + "'");
-          return  false;
-        }
         try {
           SchemaField field = mgr.managedIndexSchema.newDynamicField(name, type, op.getValuesExcluding(NAME, TYPE));
           mgr.managedIndexSchema
diff --git a/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java b/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java
index 8335bc0db6a..d5db82e2a5a 100644
--- a/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java
+++ b/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java
@@ -92,14 +92,13 @@ public class TestBulkSchemaAPI extends RestTestBase {
     String response = restTestHarness.post("/schema?wt=json", json(payload));
     Map map = (Map) ObjectBuilder.getVal(new JSONParser(new StringReader(response)));
     List l = (List) map.get("errors");
-
+    assertNotNull("No errors", l);
     List errorList = (List) ((Map) l.get(0)).get("errorMessages");
     assertEquals(1, errorList.size());
-    assertTrue (((String)errorList.get(0)).contains("No such field type"));
+    assertTrue (((String)errorList.get(0)).contains("Field 'a1': Field type 'string1' not found.\n"));
     errorList = (List) ((Map) l.get(1)).get("errorMessages");
     assertEquals(1, errorList.size());
     assertTrue (((String)errorList.get(0)).contains("is a required field"));
-
   }
   
   public void testAnalyzerClass() throws Exception {
@@ -217,6 +216,48 @@ public class TestBulkSchemaAPI extends RestTestBase {
     assertNull(newFieldName + " illegal dynamic field should not have been added to schema", map);
   }
 
+  public void testAddIllegalFields() throws Exception {
+    RestTestHarness harness = restTestHarness;
+
+    // 1. Make sure you can't create a new field with an asterisk in its name
+    String newFieldName = "asterisk*";
+
+    String payload = "{\n" +
+        "    'add-field' : {\n" +
+        "         'name':'" + newFieldName + "',\n" +
+        "         'type':'string',\n" +
+        "         'stored':true,\n" +
+        "         'indexed':true\n" +
+        "     }\n" +
+        "}";
+
+    String response = harness.post("/schema?wt=json", json(payload));
+    Map map = (Map)ObjectBuilder.getVal(new JSONParser(new StringReader(response)));
+    assertNotNull(response, map.get("errors"));
+
+    map = getObj(harness, newFieldName, "fields");
+    assertNull(newFieldName + " illegal dynamic field should not have been added to schema", map);
+
+    // 2. Make sure you get an error when you try to create a field that already exists
+    // Make sure 'wdf_nocase' field exists
+    newFieldName = "wdf_nocase";
+    Map m = getObj(harness, newFieldName, "fields");
+    assertNotNull("'" + newFieldName + "' field does not exist in the schema", m);
+
+    payload = "{\n" +
+        "    'add-field' : {\n" +
+        "         'name':'" + newFieldName + "',\n" +
+        "         'type':'string',\n" +
+        "         'stored':true,\n" +
+        "         'indexed':true\n" +
+        "     }\n" +
+        "}";
+
+    response = harness.post("/schema?wt=json", json(payload));
+    map = (Map)ObjectBuilder.getVal(new JSONParser(new StringReader(response)));
+    assertNotNull(response, map.get("errors"));
+  }
+
   public void testAddFieldWithExistingCatchallDynamicField() throws Exception {
     RestTestHarness harness = restTestHarness;