diff --git a/processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java b/processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java index bcd9eee5bdd..b542fc23d10 100644 --- a/processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java +++ b/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.indexer.Property; +import org.apache.druid.java.util.common.IAE; 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( - ImmutableList.of( - new Property<>("hasHeaderRow", hasHeaderRow), - new Property<>("findColumnsFromHeader", findColumnsFromHeader) - ) - ).getValue(); + if (hasHeaderRow != null && findColumnsFromHeader != null) { + // User provided both hasHeaderRow and findColumnsFromHeader. + throw new IAE("Cannot accept both [findColumnsFromHeader] and [hasHeaderRow]"); + } else if (hasHeaderRow == null && findColumnsFromHeader == null) { + // User provided neither columns, nor one of the header-related parameters. + 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" - + " and hasHeaderRow must be set to true." + "If [columns] is not set, the first row of your data must have your header" + + " and [findColumnsFromHeader] must be set to true." ); } } diff --git a/processing/src/main/java/org/apache/druid/indexer/Checks.java b/processing/src/main/java/org/apache/druid/indexer/Checks.java index 7153b747637..e287c2b4f71 100644 --- a/processing/src/main/java/org/apache/druid/indexer/Checks.java +++ b/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; } diff --git a/processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java b/processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java index 348b3eaa4bb..5f5a52ef15a 100644 --- a/processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java +++ b/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); } diff --git a/processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java b/processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java index a21e34d8cc6..35f35ab9ccd 100644 --- a/processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java +++ b/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); } diff --git a/processing/src/test/java/org/apache/druid/indexer/ChecksTest.java b/processing/src/test/java/org/apache/druid/indexer/ChecksTest.java index 8a745435405..9f10ad58ee9 100644 --- a/processing/src/test/java/org/apache/druid/indexer/ChecksTest.java +++ b/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( - "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" - ); + exception.expectMessage("At most one of properties[[p1, p2, p3, p4]] 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( - "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" - ); + exception.expectMessage("At most one of properties[[p1, p2, p3, p4]] must be present"); Checks.checkOneNotNullOrEmpty(properties); } @@ -139,10 +133,7 @@ public class ChecksTest new Property<>("p4", null) ); exception.expect(IllegalArgumentException.class); - exception.expectMessage( - "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" - ); + exception.expectMessage("At least one of properties[[p1, p2, p3, p4]] must be present"); Checks.checkOneNotNullOrEmpty(properties); } }