You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2019/10/07 17:06:15 UTC

[commons-csv] branch master updated: CSV-247: CSVParser to check an empty header before checking duplicates. (#47)

This is an automated email from the ASF dual-hosted git repository.

ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-csv.git


The following commit(s) were added to refs/heads/master by this push:
     new 0596491  CSV-247: CSVParser to check an empty header before checking duplicates. (#47)
0596491 is described below

commit 059649167c5045dfd40ebf0f97def33a9235c3d1
Author: Alex Herbert <ah...@apache.org>
AuthorDate: Mon Oct 7 18:06:11 2019 +0100

    CSV-247: CSVParser to check an empty header before checking duplicates. (#47)
    
    This updates the issues test for CSV-247 and adds tests to the
    CSVParserTest.
---
 .../java/org/apache/commons/csv/CSVParser.java     | 23 ++++++++-------
 .../checkstyle/checkstyle-suppressions.xml         |  2 +-
 .../java/org/apache/commons/csv/CSVParserTest.java | 14 ++++++---
 .../apache/commons/csv/issues/JiraCsv247Test.java  | 33 ++++++++++++++++------
 4 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/src/main/java/org/apache/commons/csv/CSVParser.java b/src/main/java/org/apache/commons/csv/CSVParser.java
index 3b3b3e7..3803d96 100644
--- a/src/main/java/org/apache/commons/csv/CSVParser.java
+++ b/src/main/java/org/apache/commons/csv/CSVParser.java
@@ -495,19 +495,18 @@ public final class CSVParser implements Iterable<CSVRecord>, Closeable {
             if (headerRecord != null) {
                 for (int i = 0; i < headerRecord.length; i++) {
                     final String header = headerRecord[i];
-                    final boolean containsHeader = header != null && hdrMap.containsKey(header);
                     final boolean emptyHeader = header == null || header.trim().isEmpty();
-                    if (containsHeader) {
-                        if (!emptyHeader && !this.format.getAllowDuplicateHeaderNames()) {
-                            throw new IllegalArgumentException(
-                                String.format(
-                                    "The header contains a duplicate name: \"%s\" in %s. If this is valid then use CSVFormat.withAllowDuplicateHeaderNames().",
-                                    header, Arrays.toString(headerRecord)));
-                        }
-                        if (emptyHeader && !this.format.getAllowMissingColumnNames()) {
-                            throw new IllegalArgumentException(
-                                    "A header name is missing in " + Arrays.toString(headerRecord));
-                        }
+                    if (emptyHeader && !this.format.getAllowMissingColumnNames()) {
+                        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()) {
+                        throw new IllegalArgumentException(
+                            String.format(
+                                "The header contains a duplicate name: \"%s\" in %s. If this is valid then use CSVFormat.withAllowDuplicateHeaderNames().",
+                                header, Arrays.toString(headerRecord)));
                     }
                     if (header != null) {
                         hdrMap.put(header, Integer.valueOf(i));
diff --git a/src/main/resources/checkstyle/checkstyle-suppressions.xml b/src/main/resources/checkstyle/checkstyle-suppressions.xml
index edc6783..d6b6e9c 100644
--- a/src/main/resources/checkstyle/checkstyle-suppressions.xml
+++ b/src/main/resources/checkstyle/checkstyle-suppressions.xml
@@ -19,5 +19,5 @@
     "-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
     "https://checkstyle.org/dtds/suppressions_1_2.dtd">
 <suppressions>
-  <suppress checks="LineLength" files="[\\/]CSVParser\.java$" lines="504"/>
+  <suppress checks="LineLength" files="[\\/]CSVParser\.java$" lines="508"/>
 </suppressions>
diff --git a/src/test/java/org/apache/commons/csv/CSVParserTest.java b/src/test/java/org/apache/commons/csv/CSVParserTest.java
index eeb3526..310574a 100644
--- a/src/test/java/org/apache/commons/csv/CSVParserTest.java
+++ b/src/test/java/org/apache/commons/csv/CSVParserTest.java
@@ -688,7 +688,7 @@ public class CSVParserTest {
     public void testHeaderMissing() throws Exception {
         final Reader in = new StringReader("a,,c\n1,2,3\nx,y,z");
 
-        final Iterator<CSVRecord> records = CSVFormat.DEFAULT.withHeader().parse(in).iterator();
+        final Iterator<CSVRecord> records = CSVFormat.DEFAULT.withHeader().withAllowMissingColumnNames().parse(in).iterator();
 
         for (int i = 0; i < 2; i++) {
             assertTrue(records.hasNext());
@@ -702,23 +702,29 @@ public class CSVParserTest {
 
     @Test
     public void testHeaderMissingWithNull() throws Exception {
-        final Reader in = new StringReader("a,,c,,d\n1,2,3,4\nx,y,z,zz");
+        final Reader in = new StringReader("a,,c,,e\n1,2,3,4,5\nv,w,x,y,z");
         CSVFormat.DEFAULT.withHeader().withNullString("").withAllowMissingColumnNames().parse(in).iterator();
     }
 
     @Test
     public void testHeadersMissing() throws Exception {
-        final Reader in = new StringReader("a,,c,,d\n1,2,3,4\nx,y,z,zz");
+        final Reader in = new StringReader("a,,c,,e\n1,2,3,4,5\nv,w,x,y,z");
         CSVFormat.DEFAULT.withHeader().withAllowMissingColumnNames().parse(in).iterator();
     }
 
     @Test
     public void testHeadersMissingException() {
-        final Reader in = new StringReader("a,,c,,d\n1,2,3,4\nx,y,z,zz");
+        final Reader in = new StringReader("a,,c,,e\n1,2,3,4,5\nv,w,x,y,z");
         assertThrows(IllegalArgumentException.class, () -> CSVFormat.DEFAULT.withHeader().parse(in).iterator());
     }
 
     @Test
+    public void testHeadersMissingOneColumnException() throws Exception {
+       final Reader in = new StringReader("a,,c,d,e\n1,2,3,4,5\nv,w,x,y,z");
+       assertThrows(IllegalArgumentException.class, () -> CSVFormat.DEFAULT.withHeader().parse(in).iterator());
+    }
+
+    @Test
     public void testIgnoreCaseHeaderMapping() throws Exception {
         final Reader reader = new StringReader("1,2,3");
         final Iterator<CSVRecord> records = CSVFormat.DEFAULT.withHeader("One", "TWO", "three").withIgnoreHeaderCase()
diff --git a/src/test/java/org/apache/commons/csv/issues/JiraCsv247Test.java b/src/test/java/org/apache/commons/csv/issues/JiraCsv247Test.java
index 129c3a4..daa66be 100644
--- a/src/test/java/org/apache/commons/csv/issues/JiraCsv247Test.java
+++ b/src/test/java/org/apache/commons/csv/issues/JiraCsv247Test.java
@@ -19,6 +19,9 @@ package org.apache.commons.csv.issues;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 import java.io.Reader;
 import java.io.StringReader;
 import java.util.Arrays;
@@ -32,9 +35,14 @@ import org.junit.jupiter.api.Test;
 public class JiraCsv247Test {
 
     @Test
-    public void testHeadersMissingOneColumn() throws Exception {
+    public void testHeadersMissingOneColumnWhenAllowingMissingColumnNames() throws Exception {
+        final CSVFormat format = CSVFormat.DEFAULT.withHeader().withAllowMissingColumnNames(true);
+
+        assertTrue(format.getAllowMissingColumnNames(),
+            "We should allow missing column names");
+
         final Reader in = new StringReader("a,,c,d,e\n1,2,3,4,5\nv,w,x,y,z");
-        try (final CSVParser parser = CSVFormat.DEFAULT.withHeader().parse(in)) {
+        try (final CSVParser parser = format.parse(in)) {
             assertEquals(Arrays.asList("a", "", "c", "d", "e"), parser.getHeaderNames());
             final Iterator<CSVRecord> iterator = parser.iterator();
             CSVRecord record = iterator.next();
@@ -54,11 +62,20 @@ public class JiraCsv247Test {
     }
 
     @Test
-    public void testJiraDescription() throws Exception {
-        final Reader in = new StringReader("a,,c,d\n1,2,3,4\nx,y,z,zz");
-        try (final CSVParser parser = CSVFormat.DEFAULT.withHeader().parse(in)) {
-            parser.iterator();
-        }
-    }
+    public void testHeadersMissingThrowsWhenNotAllowingMissingColumnNames() throws Exception {
+        final CSVFormat format = CSVFormat.DEFAULT.withHeader();
+
+        assertFalse(format.getAllowMissingColumnNames(),
+            "By default we should not allow missing column names");
 
+        assertThrows(IllegalArgumentException.class, () -> {
+            final Reader in = new StringReader("a,,c,d,e\n1,2,3,4,5\nv,w,x,y,z");
+            format.parse(in);
+        }, "1 missing column header is not allowed");
+
+        assertThrows(IllegalArgumentException.class, () -> {
+            final Reader in = new StringReader("a,,c,d,\n1,2,3,4,5\nv,w,x,y,z");
+            format.parse(in);
+        }, "2+ missing column headers is not allowed!");
+    }
 }
\ No newline at end of file