* The {@link Builder} settings are: @@ -770,13 +785,13 @@ public CSVFormat getFormat() { *

* Note: This is currently like {@link #RFC4180} plus {@link Builder#setAllowMissingColumnNames(boolean) Builder#setAllowMissingColumnNames(true)} and @@ -1220,7 +1235,7 @@ private static boolean isLineBreak(final Character c) { */ public static CSVFormat newFormat(final char delimiter) { return new CSVFormat(String.valueOf(delimiter), null, null, null, null, false, false, null, null, null, null, false, false, false, false, false, false, - true); + DuplicateHeaderMode.ALLOW_ALL); } static String[] toStringArray(final Object[] values) { @@ -1262,7 +1277,7 @@ public static CSVFormat valueOf(final String format) { return CSVFormat.Predefined.valueOf(format).getFormat(); } - private final boolean allowDuplicateHeaderNames; + private final DuplicateHeaderMode duplicateHeaderMode; private final boolean allowMissingColumnNames; @@ -1319,7 +1334,7 @@ private CSVFormat(final Builder builder) { this.trim = builder.trim; this.autoFlush = builder.autoFlush; this.quotedNullString = builder.quotedNullString; - this.allowDuplicateHeaderNames = builder.allowDuplicateHeaderNames; + this.duplicateHeaderMode = builder.duplicateHeaderMode; validate(); } @@ -1343,14 +1358,14 @@ private CSVFormat(final Builder builder) { * @param trim TODO Doc me. * @param trailingDelimiter TODO Doc me. * @param autoFlush TODO Doc me. - * @param allowDuplicateHeaderNames TODO Doc me. + * @param duplicateHeaderMode the behavior when handling duplicate headers * @throws IllegalArgumentException if the delimiter is a line break character. */ private CSVFormat(final String delimiter, final Character quoteChar, final QuoteMode quoteMode, final Character commentStart, final Character escape, final boolean ignoreSurroundingSpaces, final boolean ignoreEmptyLines, final String recordSeparator, final String nullString, final Object[] headerComments, final String[] header, final boolean skipHeaderRecord, final boolean allowMissingColumnNames, final boolean ignoreHeaderCase, final boolean trim, final boolean trailingDelimiter, final boolean autoFlush, - final boolean allowDuplicateHeaderNames) { + final DuplicateHeaderMode duplicateHeaderMode) { this.delimiter = delimiter; this.quoteCharacter = quoteChar; this.quoteMode = quoteMode; @@ -1369,7 +1384,7 @@ private CSVFormat(final String delimiter, final Character quoteChar, final Quote this.trim = trim; this.autoFlush = autoFlush; this.quotedNullString = quoteCharacter + nullString + quoteCharacter; - this.allowDuplicateHeaderNames = allowDuplicateHeaderNames; + this.duplicateHeaderMode = duplicateHeaderMode; validate(); } @@ -1416,7 +1431,7 @@ public boolean equals(final Object obj) { return false; } final CSVFormat other = (CSVFormat) obj; - return allowDuplicateHeaderNames == other.allowDuplicateHeaderNames && allowMissingColumnNames == other.allowMissingColumnNames && + return duplicateHeaderMode == other.duplicateHeaderMode && allowMissingColumnNames == other.allowMissingColumnNames && autoFlush == other.autoFlush && Objects.equals(commentMarker, other.commentMarker) && Objects.equals(delimiter, other.delimiter) && Objects.equals(escapeCharacter, other.escapeCharacter) && Arrays.equals(header, other.header) && Arrays.equals(headerComments, other.headerComments) && ignoreEmptyLines == other.ignoreEmptyLines && @@ -1450,9 +1465,21 @@ public String format(final Object... values) { * * @return whether duplicate header names are allowed * @since 1.7 + * @deprecated Use {@link #getDuplicateHeaderMode()}. */ + @Deprecated public boolean getAllowDuplicateHeaderNames() { - return allowDuplicateHeaderNames; + return duplicateHeaderMode == DuplicateHeaderMode.ALLOW_ALL; + } + + /** + * Returns how duplicate headers are handled. + * + * @return if duplicate header values are allowed, allowed conditionally, or disallowed. + * @since 1.9 + */ + public DuplicateHeaderMode getDuplicateHeaderMode() { + return duplicateHeaderMode; } /** @@ -1633,7 +1660,7 @@ public int hashCode() { int result = 1; result = prime * result + Arrays.hashCode(header); result = prime * result + Arrays.hashCode(headerComments); - return prime * result + Objects.hash(allowDuplicateHeaderNames, allowMissingColumnNames, autoFlush, commentMarker, delimiter, escapeCharacter, + return prime * result + Objects.hash(duplicateHeaderMode, allowMissingColumnNames, autoFlush, commentMarker, delimiter, escapeCharacter, ignoreEmptyLines, ignoreHeaderCase, ignoreSurroundingSpaces, nullString, quoteCharacter, quoteMode, quotedNullString, recordSeparator, skipHeaderRecord, trailingDelimiter, trim); } @@ -2235,7 +2262,7 @@ private void validate() throws IllegalArgumentException { } // validate header - if (header != null && !allowDuplicateHeaderNames) { + if (header != null && duplicateHeaderMode != DuplicateHeaderMode.ALLOW_ALL) { final Set dupCheck = new HashSet<>(); for (final String hdr : header) { if (!dupCheck.add(hdr)) { @@ -2254,7 +2281,7 @@ private void validate() throws IllegalArgumentException { */ @Deprecated public CSVFormat withAllowDuplicateHeaderNames() { - return builder().setAllowDuplicateHeaderNames(true).build(); + return builder().setDuplicateHeaderMode(DuplicateHeaderMode.ALLOW_ALL).build(); } /** @@ -2267,7 +2294,8 @@ public CSVFormat withAllowDuplicateHeaderNames() { */ @Deprecated public CSVFormat withAllowDuplicateHeaderNames(final boolean allowDuplicateHeaderNames) { - return builder().setAllowDuplicateHeaderNames(allowDuplicateHeaderNames).build(); + final DuplicateHeaderMode mode = allowDuplicateHeaderNames ? DuplicateHeaderMode.ALLOW_ALL : DuplicateHeaderMode.ALLOW_EMPTY; + return builder().setDuplicateHeaderMode(mode).build(); } /** diff --git a/src/main/java/org/apache/commons/csv/CSVParser.java b/src/main/java/org/apache/commons/csv/CSVParser.java index 60ecc7374a..af1100d391 100644 --- a/src/main/java/org/apache/commons/csv/CSVParser.java +++ b/src/main/java/org/apache/commons/csv/CSVParser.java @@ -17,8 +17,6 @@ package org.apache.commons.csv; -import static org.apache.commons.csv.Token.Type.TOKEN; - import java.io.Closeable; import java.io.File; import java.io.IOException; @@ -45,6 +43,8 @@ import java.util.stream.Stream; import java.util.stream.StreamSupport; +import static org.apache.commons.csv.Token.Type.TOKEN; + /** * Parses CSV files according to the specified format. * @@ -497,12 +497,16 @@ private Headers createHeaders() throws IOException { throw new IllegalArgumentException( "A header name is missing in " + Arrays.toString(headerRecord)); } - // Note: This will always allow a duplicate header if the header is empty + final boolean containsHeader = header != null && hdrMap.containsKey(header); - if (containsHeader && !emptyHeader && !this.format.getAllowDuplicateHeaderNames()) { + final DuplicateHeaderMode headerMode = this.format.getDuplicateHeaderMode(); + final boolean duplicatesAllowed = headerMode == DuplicateHeaderMode.ALLOW_ALL; + final boolean emptyDuplicatesAllowed = headerMode == DuplicateHeaderMode.ALLOW_EMPTY; + + if (containsHeader && !duplicatesAllowed && !(emptyHeader && emptyDuplicatesAllowed)) { throw new IllegalArgumentException( String.format( - "The header contains a duplicate name: \"%s\" in %s. If this is valid then use CSVFormat.withAllowDuplicateHeaderNames().", + "The header contains a duplicate name: \"%s\" in %s. If this is valid then use CSVFormat.Builder.setDuplicateHeaderMode().", header, Arrays.toString(headerRecord))); } if (header != null) { diff --git a/src/main/java/org/apache/commons/csv/DuplicateHeaderMode.java b/src/main/java/org/apache/commons/csv/DuplicateHeaderMode.java new file mode 100644 index 0000000000..1f42dd500d --- /dev/null +++ b/src/main/java/org/apache/commons/csv/DuplicateHeaderMode.java @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.csv; + +/** + * Determines how duplicate header fields should be handled + * if {@link CSVFormat#withHeader(String...)} is not null. + * + * @since 1.9 + */ +public enum DuplicateHeaderMode { + + /** + * Allows all duplicate headings. + */ + ALLOW_ALL, + + /** + * Allows duplicate headings only if they're empty + * strings or null. + */ + ALLOW_EMPTY, + + /** + * Disallows duplicate headings entirely. + */ + DISALLOW +} diff --git a/src/site/resources/checkstyle/checkstyle-suppressions.xml b/src/site/resources/checkstyle/checkstyle-suppressions.xml index 402525fd84..abff74c8c5 100644 --- a/src/site/resources/checkstyle/checkstyle-suppressions.xml +++ b/src/site/resources/checkstyle/checkstyle-suppressions.xml @@ -19,5 +19,5 @@ "-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN" "https://checkstyle.org/dtds/suppressions_1_2.dtd"> - + diff --git a/src/test/java/org/apache/commons/csv/CSVFormatTest.java b/src/test/java/org/apache/commons/csv/CSVFormatTest.java index 682764f947..f7c32bd6d4 100644 --- a/src/test/java/org/apache/commons/csv/CSVFormatTest.java +++ b/src/test/java/org/apache/commons/csv/CSVFormatTest.java @@ -260,6 +260,10 @@ public void testEqualsHash() throws Exception { final Object a = method.invoke(CSVFormat.DEFAULT, QuoteMode.MINIMAL); final Object b = method.invoke(CSVFormat.DEFAULT, QuoteMode.ALL); assertNotEquals(name, type, a, b); + } else if ("org.apache.commons.csv.DuplicateHeaderMode".equals(type)) { + final Object a = method.invoke(CSVFormat.DEFAULT, new Object[] {DuplicateHeaderMode.ALLOW_ALL}); + final Object b = method.invoke(CSVFormat.DEFAULT, new Object[] {DuplicateHeaderMode.DISALLOW}); + assertNotEquals(name, type, a, b); } else if ("java.lang.Object[]".equals(type)){ final Object a = method.invoke(CSVFormat.DEFAULT, new Object[] {new Object[] {null, null}}); final Object b = method.invoke(CSVFormat.DEFAULT, new Object[] {new Object[] {new Object(), new Object()}}); @@ -1295,6 +1299,15 @@ public void testWithEscape() { } + @Test + public void testWithEmptyDuplicates() { + final CSVFormat formatWithEmptyDuplicates = + CSVFormat.DEFAULT.builder().setDuplicateHeaderMode(DuplicateHeaderMode.ALLOW_EMPTY).build(); + + assertEquals(DuplicateHeaderMode.ALLOW_EMPTY, formatWithEmptyDuplicates.getDuplicateHeaderMode()); + assertFalse(formatWithEmptyDuplicates.getAllowDuplicateHeaderNames()); + } + @Test public void testWithEscapeCRThrowsExceptions() { assertThrows(IllegalArgumentException.class, () -> CSVFormat.DEFAULT.withEscape(CR)); diff --git a/src/test/java/org/apache/commons/csv/issues/JiraCsv264Test.java b/src/test/java/org/apache/commons/csv/issues/JiraCsv264Test.java new file mode 100644 index 0000000000..a2e22aedf9 --- /dev/null +++ b/src/test/java/org/apache/commons/csv/issues/JiraCsv264Test.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.csv.issues; + +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.DuplicateHeaderMode; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.io.StringReader; + +/** + * When {@link CSVFormat#withHeader(String...)} is not null; + * duplicate headers with empty strings should not be allowed. + * + * @see Jira Ticker + */ +public class JiraCsv264Test { + + private static final String CSV_STRING = "\"\",\"B\",\"\"\n" + + "\"1\",\"2\",\"3\"\n" + + "\"4\",\"5\",\"6\""; + + /** + * A CSV file with a random gap in the middle. + */ + private static final String CSV_STRING_GAP = "\"A\",\"B\",\"\",\"\",\"E\"\n" + + "\"1\",\"2\",\"\",\"\",\"5\"\n" + + "\"6\",\"7\",\"\",\"\",\"10\""; + + @Test + public void testJiraCsv264() throws IOException { + final CSVFormat csvFormat = CSVFormat.DEFAULT + .builder() + .setHeader() + .setDuplicateHeaderMode(DuplicateHeaderMode.DISALLOW) + .setAllowMissingColumnNames(true) + .build(); + + try (StringReader reader = new StringReader(CSV_STRING)) { + try { + csvFormat.parse(reader); + Assertions.fail("This shouldn't parse the CSV string successfully."); + } catch (IllegalArgumentException ex) { + // Test is successful! + } + } + } + + @Test + public void testJiraCsv264WithGapAllowEmpty() throws IOException { + final CSVFormat csvFormat = CSVFormat.DEFAULT + .builder() + .setHeader() + .setDuplicateHeaderMode(DuplicateHeaderMode.ALLOW_EMPTY) + .setAllowMissingColumnNames(true) + .build(); + + try (StringReader reader = new StringReader(CSV_STRING_GAP)) { + csvFormat.parse(reader); + } + } + + @Test + public void testJiraCsv264WithGapDisallow() throws IOException { + final CSVFormat csvFormat = CSVFormat.DEFAULT + .builder() + .setHeader() + .setDuplicateHeaderMode(DuplicateHeaderMode.DISALLOW) + .setAllowMissingColumnNames(true) + .build(); + + try (StringReader reader = new StringReader(CSV_STRING_GAP)) { + try { + csvFormat.parse(reader); + Assertions.fail("This shouldn't parse the CSV string successfully."); + } catch (IllegalArgumentException ex) { + // Test is successful! + } + } + } +} From 511f2b49f5db1d60866f08664cc09f85427f2c81 Mon Sep 17 00:00:00 2001 From: Seth Falco Date: Fri, 9 Jul 2021 16:23:51 +0200 Subject: [PATCH 2/2] fix: use assertthrows and update docs --- .../org/apache/commons/csv/CSVFormat.java | 28 +++++++++---------- .../org/apache/commons/csv/CSVParser.java | 4 +-- .../commons/csv/DuplicateHeaderMode.java | 9 +++--- .../commons/csv/issues/JiraCsv264Test.java | 25 ++++++----------- 4 files changed, 28 insertions(+), 38 deletions(-) diff --git a/src/main/java/org/apache/commons/csv/CSVFormat.java b/src/main/java/org/apache/commons/csv/CSVFormat.java index 324829f6b5..77a009dc13 100644 --- a/src/main/java/org/apache/commons/csv/CSVFormat.java +++ b/src/main/java/org/apache/commons/csv/CSVFormat.java @@ -17,6 +17,18 @@ package org.apache.commons.csv; +import static org.apache.commons.csv.Constants.BACKSLASH; +import static org.apache.commons.csv.Constants.COMMA; +import static org.apache.commons.csv.Constants.COMMENT; +import static org.apache.commons.csv.Constants.CR; +import static org.apache.commons.csv.Constants.CRLF; +import static org.apache.commons.csv.Constants.DOUBLE_QUOTE_CHAR; +import static org.apache.commons.csv.Constants.EMPTY; +import static org.apache.commons.csv.Constants.LF; +import static org.apache.commons.csv.Constants.PIPE; +import static org.apache.commons.csv.Constants.SP; +import static org.apache.commons.csv.Constants.TAB; + import java.io.File; import java.io.FileOutputStream; import java.io.IOException; @@ -36,18 +48,6 @@ import java.util.Objects; import java.util.Set; -import static org.apache.commons.csv.Constants.BACKSLASH; -import static org.apache.commons.csv.Constants.COMMA; -import static org.apache.commons.csv.Constants.COMMENT; -import static org.apache.commons.csv.Constants.CR; -import static org.apache.commons.csv.Constants.CRLF; -import static org.apache.commons.csv.Constants.DOUBLE_QUOTE_CHAR; -import static org.apache.commons.csv.Constants.EMPTY; -import static org.apache.commons.csv.Constants.LF; -import static org.apache.commons.csv.Constants.PIPE; -import static org.apache.commons.csv.Constants.SP; -import static org.apache.commons.csv.Constants.TAB; - /** * Specifies the format of a CSV file and parses input. * @@ -1473,10 +1473,10 @@ public boolean getAllowDuplicateHeaderNames() { } /** - * Returns how duplicate headers are handled. + * Gets how duplicate headers are handled. * * @return if duplicate header values are allowed, allowed conditionally, or disallowed. - * @since 1.9 + * @since 1.9.0 */ public DuplicateHeaderMode getDuplicateHeaderMode() { return duplicateHeaderMode; diff --git a/src/main/java/org/apache/commons/csv/CSVParser.java b/src/main/java/org/apache/commons/csv/CSVParser.java index af1100d391..58cdb146b2 100644 --- a/src/main/java/org/apache/commons/csv/CSVParser.java +++ b/src/main/java/org/apache/commons/csv/CSVParser.java @@ -17,6 +17,8 @@ package org.apache.commons.csv; +import static org.apache.commons.csv.Token.Type.TOKEN; + import java.io.Closeable; import java.io.File; import java.io.IOException; @@ -43,8 +45,6 @@ import java.util.stream.Stream; import java.util.stream.StreamSupport; -import static org.apache.commons.csv.Token.Type.TOKEN; - /** * Parses CSV files according to the specified format. * diff --git a/src/main/java/org/apache/commons/csv/DuplicateHeaderMode.java b/src/main/java/org/apache/commons/csv/DuplicateHeaderMode.java index 1f42dd500d..e623adaa40 100644 --- a/src/main/java/org/apache/commons/csv/DuplicateHeaderMode.java +++ b/src/main/java/org/apache/commons/csv/DuplicateHeaderMode.java @@ -21,23 +21,22 @@ * Determines how duplicate header fields should be handled * if {@link CSVFormat#withHeader(String...)} is not null. * - * @since 1.9 + * @since 1.9.0 */ public enum DuplicateHeaderMode { /** - * Allows all duplicate headings. + * Allows all duplicate headers. */ ALLOW_ALL, /** - * Allows duplicate headings only if they're empty - * strings or null. + * Allows duplicate headers only if they're empty strings or null. */ ALLOW_EMPTY, /** - * Disallows duplicate headings entirely. + * Disallows duplicate headers entirely. */ DISALLOW } diff --git a/src/test/java/org/apache/commons/csv/issues/JiraCsv264Test.java b/src/test/java/org/apache/commons/csv/issues/JiraCsv264Test.java index a2e22aedf9..bbbb262454 100644 --- a/src/test/java/org/apache/commons/csv/issues/JiraCsv264Test.java +++ b/src/test/java/org/apache/commons/csv/issues/JiraCsv264Test.java @@ -19,20 +19,21 @@ import org.apache.commons.csv.CSVFormat; import org.apache.commons.csv.DuplicateHeaderMode; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertThrows; + import java.io.IOException; import java.io.StringReader; /** - * When {@link CSVFormat#withHeader(String...)} is not null; - * duplicate headers with empty strings should not be allowed. + * When {@link CSVFormat#withHeader(String...)} is not null; duplicate headers + * with empty strings should not be allowed. * - * @see Jira Ticker + * @see Jira Ticker */ public class JiraCsv264Test { - + private static final String CSV_STRING = "\"\",\"B\",\"\"\n" + "\"1\",\"2\",\"3\"\n" + "\"4\",\"5\",\"6\""; @@ -54,12 +55,7 @@ public void testJiraCsv264() throws IOException { .build(); try (StringReader reader = new StringReader(CSV_STRING)) { - try { - csvFormat.parse(reader); - Assertions.fail("This shouldn't parse the CSV string successfully."); - } catch (IllegalArgumentException ex) { - // Test is successful! - } + assertThrows(IllegalArgumentException.class, () -> csvFormat.parse(reader)); } } @@ -87,12 +83,7 @@ public void testJiraCsv264WithGapDisallow() throws IOException { .build(); try (StringReader reader = new StringReader(CSV_STRING_GAP)) { - try { - csvFormat.parse(reader); - Assertions.fail("This shouldn't parse the CSV string successfully."); - } catch (IllegalArgumentException ex) { - // Test is successful! - } + assertThrows(IllegalArgumentException.class, () -> csvFormat.parse(reader)); } } }