Nicer error message for CSV with no properties. (#14093)

* Nicer error message for CSV with no properties.

* Take two.

* Adjustments from review, and test fixes.

* Fix test.

* Fix static check.

processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java

@@ -22,10 +22,8 @@ package org.apache.druid.data.input.impl;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import org.apache.druid.data.input.InputFormat;
-import org.apache.druid.indexer.Checks;
+import org.apache.druid.java.util.common.IAE;
-import org.apache.druid.indexer.Property;
 
 import javax.annotation.Nullable;
 import java.util.Collections;
@@ -52,14 +50,17 @@ public abstract class FlatTextInputFormat implements InputFormat
     this.columns = columns == null ? Collections.emptyList() : columns;
     this.listDelimiter = listDelimiter;
     this.delimiter = Preconditions.checkNotNull(delimiter, "delimiter");
-    //noinspection ConstantConditions
     if (columns == null || columns.isEmpty()) {
-      this.findColumnsFromHeader = Checks.checkOneNotNullOrEmpty(
+      if (hasHeaderRow != null && findColumnsFromHeader != null) {
-          ImmutableList.of(
+        // User provided both hasHeaderRow and findColumnsFromHeader.
-              new Property<>("hasHeaderRow", hasHeaderRow),
+        throw new IAE("Cannot accept both [findColumnsFromHeader] and [hasHeaderRow]");
-              new Property<>("findColumnsFromHeader", findColumnsFromHeader)
+      } else if (hasHeaderRow == null && findColumnsFromHeader == null) {
-          )
+        // User provided neither columns, nor one of the header-related parameters.
-      ).getValue();
+        throw new IAE("Either [columns] or [findColumnsFromHeader] must be set");
+      } else {
+        // User provided one of hasHeaderRow or findColumnsFromHeader. Take the one they provided.
+        this.findColumnsFromHeader = hasHeaderRow != null ? hasHeaderRow : findColumnsFromHeader;
+      }
     } else {
       this.findColumnsFromHeader = findColumnsFromHeader == null ? false : findColumnsFromHeader;
     }
@@ -80,8 +81,8 @@ public abstract class FlatTextInputFormat implements InputFormat
     } else {
       Preconditions.checkArgument(
           this.findColumnsFromHeader,
-          "If columns field is not set, the first row of your data must have your header"
+          "If [columns] is not set, the first row of your data must have your header"
-          + " and hasHeaderRow must be set to true."
+          + " and [findColumnsFromHeader] must be set to true."
       );
     }
   }

processing/src/main/java/org/apache/druid/indexer/Checks.java

@@ -22,6 +22,7 @@ package org.apache.druid.indexer;
 import org.apache.druid.java.util.common.IAE;
 
 import java.util.List;
+import java.util.stream.Collectors;
 
 /**
  * Various helper methods useful for checking the validity of arguments to spec constructors.
@@ -36,12 +37,18 @@ public final class Checks
         if (nonNullProperty == null) {
           nonNullProperty = property;
         } else {
-          throw new IAE("At most one of %s must be present", properties);
+          throw new IAE(
+              "At most one of properties[%s] must be present",
+              properties.stream().map(Property::getName).collect(Collectors.toList())
+          );
         }
       }
     }
     if (nonNullProperty == null) {
-      throw new IAE("At least one of %s must be present", properties);
+      throw new IAE(
+          "At least one of properties[%s] must be present",
+          properties.stream().map(Property::getName).collect(Collectors.toList())
+      );
     }
     return nonNullProperty;
   }

processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java

@@ -19,12 +19,16 @@
 
 package org.apache.druid.data.input.impl;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.druid.data.input.InputFormat;
 import org.apache.druid.testing.InitializedNullHandlingTest;
+import org.hamcrest.CoreMatchers;
+import org.hamcrest.MatcherAssert;
 import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.internal.matchers.ThrowableMessageMatcher;
 import org.junit.rules.ExpectedException;
 
 import java.io.IOException;
@@ -45,6 +49,83 @@ public class CsvInputFormatTest extends InitializedNullHandlingTest
     Assert.assertEquals(format, fromJson);
   }
 
+  @Test
+  public void testDeserializeWithoutColumnsWithHasHeaderRow() throws IOException
+  {
+    final ObjectMapper mapper = new ObjectMapper();
+    final CsvInputFormat inputFormat = (CsvInputFormat) mapper.readValue(
+        "{\"type\":\"csv\",\"hasHeaderRow\":true}",
+        InputFormat.class
+    );
+    Assert.assertTrue(inputFormat.isFindColumnsFromHeader());
+  }
+
+  @Test
+  public void testDeserializeWithoutColumnsWithFindColumnsFromHeaderTrue() throws IOException
+  {
+    final ObjectMapper mapper = new ObjectMapper();
+    final CsvInputFormat inputFormat = (CsvInputFormat) mapper.readValue(
+        "{\"type\":\"csv\",\"findColumnsFromHeader\":true}",
+        InputFormat.class
+    );
+    Assert.assertTrue(inputFormat.isFindColumnsFromHeader());
+  }
+
+  @Test
+  public void testDeserializeWithoutColumnsWithFindColumnsFromHeaderFalse()
+  {
+    final ObjectMapper mapper = new ObjectMapper();
+    final JsonProcessingException e = Assert.assertThrows(
+        JsonProcessingException.class,
+        () -> mapper.readValue(
+            "{\"type\":\"csv\",\"findColumnsFromHeader\":false}",
+            InputFormat.class
+        )
+    );
+    MatcherAssert.assertThat(
+        e,
+        ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith(
+            "Cannot construct instance of `org.apache.druid.data.input.impl.CsvInputFormat`, problem: "
+            + "If [columns] is not set, the first row of your data must have your header and "
+            + "[findColumnsFromHeader] must be set to true."))
+    );
+  }
+
+  @Test
+  public void testDeserializeWithoutColumnsWithBothHeaderProperties()
+  {
+    final ObjectMapper mapper = new ObjectMapper();
+    final JsonProcessingException e = Assert.assertThrows(
+        JsonProcessingException.class,
+        () -> mapper.readValue(
+            "{\"type\":\"csv\",\"findColumnsFromHeader\":true,\"hasHeaderRow\":true}",
+            InputFormat.class
+        )
+    );
+    MatcherAssert.assertThat(
+        e,
+        ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith(
+            "Cannot construct instance of `org.apache.druid.data.input.impl.CsvInputFormat`, problem: "
+            + "Cannot accept both [findColumnsFromHeader] and [hasHeaderRow]"))
+    );
+  }
+
+  @Test
+  public void testDeserializeWithoutAnyProperties()
+  {
+    final ObjectMapper mapper = new ObjectMapper();
+    final JsonProcessingException e = Assert.assertThrows(
+        JsonProcessingException.class,
+        () -> mapper.readValue("{\"type\":\"csv\"}", InputFormat.class)
+    );
+    MatcherAssert.assertThat(
+        e,
+        ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith(
+            "Cannot construct instance of `org.apache.druid.data.input.impl.CsvInputFormat`, problem: "
+            + "Either [columns] or [findColumnsFromHeader] must be set"))
+    );
+  }
+
   @Test
   public void testComma()
   {
@@ -79,7 +160,7 @@ public class CsvInputFormatTest extends InitializedNullHandlingTest
   public void testMissingFindColumnsFromHeaderWithMissingColumnsThrowingError()
   {
     expectedException.expect(IllegalArgumentException.class);
-    expectedException.expectMessage("At least one of [Property{name='hasHeaderRow', value=null}");
+    expectedException.expectMessage("Either [columns] or [findColumnsFromHeader] must be set");
     new CsvInputFormat(null, null, null, null, 0);
   }
 
@@ -94,7 +175,7 @@ public class CsvInputFormatTest extends InitializedNullHandlingTest
   public void testHasHeaderRowWithMissingFindColumnsThrowingError()
   {
     expectedException.expect(IllegalArgumentException.class);
-    expectedException.expectMessage("At most one of [Property{name='hasHeaderRow', value=true}");
+    expectedException.expectMessage("Cannot accept both [findColumnsFromHeader] and [hasHeaderRow]");
     new CsvInputFormat(null, null, true, false, 0);
   }
 

processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java

@@ -107,7 +107,7 @@ public class DelimitedInputFormatTest
   public void testMissingFindColumnsFromHeaderWithMissingColumnsThrowingError()
   {
     expectedException.expect(IllegalArgumentException.class);
-    expectedException.expectMessage("At least one of [Property{name='hasHeaderRow', value=null}");
+    expectedException.expectMessage("Either [columns] or [findColumnsFromHeader] must be set");
     new DelimitedInputFormat(null, null, "delim", null, null, 0);
   }
 
@@ -129,7 +129,7 @@ public class DelimitedInputFormatTest
   public void testHasHeaderRowWithMissingFindColumnsThrowingError()
   {
     expectedException.expect(IllegalArgumentException.class);
-    expectedException.expectMessage("At most one of [Property{name='hasHeaderRow', value=true}");
+    expectedException.expectMessage("Cannot accept both [findColumnsFromHeader] and [hasHeaderRow]");
     new DelimitedInputFormat(null, null, "delim", true, false, 0);
   }
 

processing/src/test/java/org/apache/druid/indexer/ChecksTest.java

@@ -105,10 +105,7 @@ public class ChecksTest
         new Property<>("p4", Collections.emptyList())
     );
     exception.expect(IllegalArgumentException.class);
-    exception.expectMessage(
+    exception.expectMessage("At most one of properties[[p1, p2, p3, p4]] must be present");
-        "At most one of [Property{name='p1', value=null}, Property{name='p2', value=2}, Property{name='p3', value=3}, "
-        + "Property{name='p4', value=[]}] must be present"
-    );
     Checks.checkOneNotNullOrEmpty(properties);
   }
 
@@ -122,10 +119,7 @@ public class ChecksTest
         new Property<>("p4", Lists.newArrayList(1, 2))
     );
     exception.expect(IllegalArgumentException.class);
-    exception.expectMessage(
+    exception.expectMessage("At most one of properties[[p1, p2, p3, p4]] must be present");
-        "At most one of [Property{name='p1', value=null}, Property{name='p2', value=2}, Property{name='p3', value=null}, "
-        + "Property{name='p4', value=[1, 2]}] must be present"
-    );
     Checks.checkOneNotNullOrEmpty(properties);
   }
 
@@ -139,10 +133,7 @@ public class ChecksTest
         new Property<>("p4", null)
     );
     exception.expect(IllegalArgumentException.class);
-    exception.expectMessage(
+    exception.expectMessage("At least one of properties[[p1, p2, p3, p4]] must be present");
-        "At least one of [Property{name='p1', value=null}, Property{name='p2', value=null}, "
-        + "Property{name='p3', value=null}, Property{name='p4', value=null}] must be present"
-    );
     Checks.checkOneNotNullOrEmpty(properties);
   }
 }