SQOOP-91. Empty "columns" clause leads to null pointer exception

From: Jonathan Hsieh <jon@cloudera.com>

git-svn-id: https://svn.apache.org/repos/asf/incubator/sqoop/trunk@1150045 13f79535-47bb-0310-9956-ffa450edef68

src/java/com/cloudera/sqoop/orm/ClassWriter.java

@@ -1005,11 +1005,7 @@ private void generateHadoopWrite(Map<String, Integer> columnTypes,
     return cleanedColNames;
   }
 
-
-  /**
-   * Generate the ORM code for the class.
-   */
-  public void generate() throws IOException {
+  private Map<String, Integer> setupColumnTypes() throws IOException {
     Map<String, Integer> columnTypes;
 
     if (null != tableName) {
@@ -1025,7 +1021,10 @@ public void generate() throws IOException {
 
       columnTypes = connManager.getColumnTypesForQuery(query);
     }
+    return columnTypes;
+  }
 
+  private String[] setupColNames(Map<String, Integer> columnTypes) {
     String [] colNames = options.getColumns();
     if (null == colNames) {
       if (null != tableName) {
@@ -1054,7 +1053,11 @@ public void generate() throws IOException {
         }
       }
     }
+    return colNames;
+  }
 
+  private String[] setupCleanedColNames(Map<String, Integer> columnTypes,
+      String[] colNames) {
     // Translate all the column names into names that are safe to
     // use as identifiers.
     String [] cleanedColNames = cleanColNames(colNames);
@@ -1071,8 +1074,24 @@ public void generate() throws IOException {
       // Make sure the col->type mapping holds for the
       // new identifier name, too.
       String col = colNames[i];
-      columnTypes.put(identifier, columnTypes.get(col));
+      Integer type = columnTypes.get(col);
+      if (type == null) {
+        // column doesn't have a type, means that is illegal column name!
+        throw new IllegalArgumentException("Column name '" + col
+            + "' not in table");
+      }
+      columnTypes.put(identifier, type);
     }
+    return cleanedColNames;
+  }
+
+  /**
+   * Generate the ORM code for the class.
+   */
+  public void generate() throws IOException {
+    Map<String, Integer> columnTypes = setupColumnTypes();
+    String[] colNames = setupColNames(columnTypes);
+    String[] cleanedColNames = setupCleanedColNames(columnTypes, colNames);
 
     // The db write() method may use column names in a different
     // order. If this is set in the options, pull it out here and
@@ -1178,6 +1197,10 @@ public void generate() throws IOException {
   private StringBuilder generateClassForColumns(
       Map<String, Integer> columnTypes,
       String [] colNames, String [] dbWriteColNames) {
+    if (colNames.length ==0) {
+      throw new IllegalArgumentException("Attempted to generate class with "
+          + "no columns!");
+    }
     StringBuilder sb = new StringBuilder();
     sb.append("// ORM class for " + tableName + "\n");
     sb.append("// WARNING: This class is AUTO-GENERATED. "

src/java/com/cloudera/sqoop/tool/ImportTool.java

@@ -421,6 +421,12 @@ public int run(SqoopOptions options) {
 
       // Import a single table (or query) the user specified.
       importTable(options, options.getTableName(), hiveImport);
+    } catch (IllegalArgumentException iea) {
+        LOG.error("Imported Failed: " + iea.getMessage());
+        if (System.getProperty(Sqoop.SQOOP_RETHROW_PROPERTY) != null) {
+          throw iea;
+        }
+        return 1;
     } catch (IOException ioe) {
       LOG.error("Encountered IOException running import job: "
           + StringUtils.stringifyException(ioe));

src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java

@@ -40,6 +40,11 @@
 
 /**
  * Test aspects of the DataDrivenImportJob class' failure reporting.
+ *
+ * These tests have strange error checking because they have different correct
+ * exit conditions when run in "debug mode" when run by ('ant test') and when
+ * run within eclipse.  Debug mode is entered by setting system property
+ * SQOOP_RETHROW_PROPERTY = "sqoop.throwOnError".
  */
 public class TestImportJob extends ImportJobTestCase {
 
@@ -63,8 +68,8 @@ public void testFailedImportDueToIOException() throws IOException {
 
     Sqoop importer = new Sqoop(new ImportTool());
     try {
-      Sqoop.runSqoop(importer, argv);
-      fail("Expected IOException running this job.");
+      int ret = Sqoop.runSqoop(importer, argv);
+      assertTrue("Expected ImportException running this job.", 1==ret);
     } catch (Exception e) {
       // In debug mode, IOException is wrapped in RuntimeException.
       LOG.info("Got exceptional return (expected: ok). msg is: " + e);
@@ -136,14 +141,71 @@ public void testFailedImportDueToJobFail() throws IOException {
 
     Sqoop importer = new Sqoop(new ImportTool(), conf);
     try {
-      Sqoop.runSqoop(importer, argv);
-      fail("Expected ImportException running this job.");
+      int ret = Sqoop.runSqoop(importer, argv);
+      assertTrue("Expected ImportException running this job.", 1==ret);
     } catch (Exception e) {
       // In debug mode, ImportException is wrapped in RuntimeException.
       LOG.info("Got exceptional return (expected: ok). msg is: " + e);
     }
   }
 
+  public void testFailedNoColumns() throws IOException {
+    // Make sure that if a MapReduce job to do the import fails due
+    // to an IOException, we tell the user about it.
+
+    // Create a table to attempt to import.
+    createTableForColType("VARCHAR(32)", "'meep'");
+
+    Configuration conf = new Configuration();
+
+    // Make the output dir exist so we know the job will fail via IOException.
+    Path outputPath = new Path(new Path(getWarehouseDir()), getTableName());
+    FileSystem fs = FileSystem.getLocal(conf);
+    fs.mkdirs(outputPath);
+    assertTrue(fs.exists(outputPath));
+
+    String [] argv = getArgv(true, new String [] { "" }, conf);
+
+    Sqoop importer = new Sqoop(new ImportTool());
+    try {
+      int ret = Sqoop.runSqoop(importer, argv);
+      assertTrue("Expected job to fail due to no colnames.", 1==ret);
+    } catch (Exception e) {
+      // In debug mode, IOException is wrapped in RuntimeException.
+      LOG.info("Got exceptional return (expected: ok). msg is: " + e);
+    }
+  }
+
+  public void testFailedIllegalColumns() throws IOException {
+    // Make sure that if a MapReduce job to do the import fails due
+    // to an IOException, we tell the user about it.
+
+    // Create a table to attempt to import.
+    createTableForColType("VARCHAR(32)", "'meep'");
+
+    Configuration conf = new Configuration();
+
+    // Make the output dir exist so we know the job will fail via IOException.
+    Path outputPath = new Path(new Path(getWarehouseDir()), getTableName());
+    FileSystem fs = FileSystem.getLocal(conf);
+    fs.mkdirs(outputPath);
+
+    assertTrue(fs.exists(outputPath));
+
+    // DATA_COL0 ok, by zyzzyva not good
+    String [] argv = getArgv(true, new String [] { "DATA_COL0", "zyzzyva" },
+        conf);
+
+    Sqoop importer = new Sqoop(new ImportTool());
+    try {
+      int ret = Sqoop.runSqoop(importer, argv);
+      assertTrue("Expected job to fail due bad colname.", 1==ret);
+    } catch (Exception e) {
+      // In debug mode, IOException is wrapped in RuntimeException.
+      LOG.info("Got exceptional return (expected: ok). msg is: " + e);
+    }
+  }
+
   public void testDuplicateColumns() throws IOException {
     // Make sure that if a MapReduce job to do the import fails due
     // to an IOException, we tell the user about it.
@@ -157,7 +219,6 @@ public void testDuplicateColumns() throws IOException {
     Path outputPath = new Path(new Path(getWarehouseDir()), getTableName());
     FileSystem fs = FileSystem.getLocal(conf);
     fs.mkdirs(outputPath);
-
     assertTrue(fs.exists(outputPath));
 
     String[] argv = getArgv(true, new String[] { "DATA_COL0,DATA_COL0" }, conf);