Categorized exception to have a better message on table creation failure

cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/BasicDataTypeMapper.java

@@ -179,9 +179,6 @@ public BasicDataTypeMapper() {
     public String getDataType(ColumnDesc columnDesc) {
         TapDataType tt = columnDesc.getDatatype();
         TypePair dbt = findTypePair(tt);
-        if (dbt == null) {
-            throw new UnsupportedOperationException("unsupported database column type: " + tt);
-        }
 
         String ret = dbt.str;
         if (ret.equals("CHAR")) {
@@ -277,7 +274,7 @@ protected TypePair findTypePair(TapDataType tt) {
             dbt = dataTypes.get(tmp);
         }
         if (dbt == null) {
-            throw new UnsupportedOperationException("unexpected datatype: " + tt);
+            throw new UnsupportedOperationException("Unexpected/Unsupported datatype: " + tt.getDatatype());
         }
 
         log.debug("findTypePair: " + tt + " -> " + dbt);

cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableCreator.java

@@ -120,18 +120,27 @@ public void createTable(TableDesc table) {
 
             tm.commitTransaction();
             prof.checkpoint("commit-transaction");
+        } catch (UnsupportedOperationException | IllegalArgumentException rethrow) {
+            try {
+                log.debug("create table failed - rollback", rethrow);
+                tm.rollbackTransaction();
+                prof.checkpoint("rollback-transaction");
+                log.debug("create table failed - rollback: OK");
+            } catch (Exception oops) {
+                log.error("create table failed - rollback : FAIL", oops);
+            }
+            throw rethrow;
         } catch (Exception ex) {
             try {
-                log.error("create table failed - rollback", ex);
+                log.debug("create table failed - rollback", ex);
                 tm.rollbackTransaction();
                 prof.checkpoint("rollback-transaction");
-                log.error("create table failed - rollback: OK");
+                log.debug("create table failed - rollback: OK");
             } catch (Exception oops) {
                 log.error("create table failed - rollback : FAIL", oops);
             }
-            // TODO: categorise failures better
-            throw new RuntimeException("failed to create table " + table.getTableName(), ex);
-        } finally { 
+            throw new RuntimeException("failed to create table " + table.getTableName() + " with cause: " + ex.getMessage(), ex);
+        } finally {
             if (tm.isOpen()) {
                 log.error("BUG: open transaction in finally - trying to rollback");
                 try {
@@ -145,7 +154,7 @@ public void createTable(TableDesc table) {
             }
         }
     }
-    
+
     /**
      * Change a table name. This can also optionally change the schema name.
      * 

cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/PutAction.java

@@ -316,21 +316,30 @@ public TableDesc acceptTargetTableDesc(TableDesc td) {
 
             tm.commitTransaction();
             prof.checkpoint("commit-transaction");
+        } catch (UnsupportedOperationException | IllegalArgumentException | IOException rethrow) {
+            try {
+                log.debug("PUT failed - rollback", rethrow);
+                tm.rollbackTransaction();
+                prof.checkpoint("rollback-transaction");
+                log.debug("PUT failed - rollback: OK");
+            } catch (Exception oops) {
+                log.error("PUT failed - rollback: FAIL", oops);
+            }
+            throw rethrow;
         } catch (Exception ex) {
             try {
-                log.error("PUT failed - rollback", ex);
+                log.debug("PUT failed - rollback", ex);
                 tm.rollbackTransaction();
                 prof.checkpoint("rollback-transaction");
-                log.error("PUT failed - rollback: OK");
+                log.debug("PUT failed - rollback: OK");
             } catch (Exception oops) {
-                log.error("PUT failed - rollback : FAIL", oops);
+                log.error("PUT failed - rollback: FAIL", oops);
             }
-            // TODO: categorise failures better
-            throw new RuntimeException("failed to create/add " + tableName, ex);
-        } finally { 
+            throw new RuntimeException("failed to create/add " + tableName + " with cause: " + ex.getMessage(), ex);
+        } finally {
             if (tm.isOpen()) {
-                log.error("BUG: open transaction in finally - trying to rollback");
                 try {
+                    log.error("BUG: open transaction in finally - trying to rollback");
                     tm.rollbackTransaction();
                     prof.checkpoint("rollback-transaction");
                     log.error("BUG: rollback in finally: OK");

cadc-tap/build.gradle

@@ -14,7 +14,7 @@ apply from: '../opencadc.gradle'
 sourceCompatibility = 11
 
 group = 'org.opencadc'
-version = '1.1.27'
+version = '1.1.28'
 
 description = 'OpenCADC TAP-1.1 tap client library'
 def git_url = 'https://github.com/opencadc/tap'

cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/TapSchemaUtil.java

@@ -75,11 +75,15 @@
 import ca.nrc.cadc.tap.schema.validator.IdentifierValidator;
 import ca.nrc.cadc.tap.schema.validator.ValidatorConfig;
 import ca.nrc.cadc.tap.schema.validator.Violation;
+import ca.nrc.cadc.tap.schema.validator.ViolationType;
 import ca.nrc.cadc.tap.schema.validator.adql.ReservedKeyword;
 import ca.nrc.cadc.tap.schema.validator.ucd.UCDValidator;
 import ca.nrc.cadc.tap.schema.validator.unit.VOUnitValidator;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
+
 import org.apache.log4j.Logger;
 
 /**
@@ -267,6 +271,7 @@ public static VOTableDocument createVOTable(TableDesc tableDesc, ValidatorConfig
     public static String validateTableDesc(TableDesc td, ValidatorConfig config) {
         List<String> errors = new ArrayList<>();
         List<String> warnings = new ArrayList<>();
+        Set<String> names = new HashSet<>();
         IdentifierValidator identifierValidator = new IdentifierValidator(config);
 
         String contextPrefix = td.getSchemaName() + ": ";
@@ -287,6 +292,11 @@ public static String validateTableDesc(TableDesc td, ValidatorConfig config) {
                     identifierValidator.checkValidIdentifier(cd.getColumnName(), IdentifierValidator.IdentifierType.COLUMN_NAME).getViolations(),
                     config, errors, warnings);
 
+            if (!names.add(cd.getColumnName())) {
+                collectViolations(contextPrefix, "column name", cd.getColumnName(),
+                        List.of(new Violation[]{new Violation(ViolationType.DUPLICATE_IDENTIFIER, "Duplicate column name: " + cd.getColumnName())}),
+                        config, errors, warnings);
+            }
             // Ignore UCD and Unit validation for views and non-strict mode(e.g. none).
             if (td.tableType.equals(TableDesc.TableType.VIEW) || config.getConfigType().equals(ValidatorConfig.ConfigType.NONE)) {
                 continue;

cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ValidatorConfig.java

@@ -92,7 +92,8 @@ public enum ConfigType {
             ViolationType.NULL_OR_BLANK,
             ViolationType.STRUCTURAL,
             ViolationType.IDENTIFIER_INVALID_CHAR,
-            ViolationType.IDENTIFIER_RESERVED_KEYWORD // allowing quoted identifiers if config is not "strict"
+            ViolationType.IDENTIFIER_RESERVED_KEYWORD, // allowing quoted identifiers if config is not "strict"
+            ViolationType.DUPLICATE_IDENTIFIER
     ));
 
     private final Set<ViolationType> errorTypes;

cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ViolationType.java

@@ -72,6 +72,7 @@ public enum ViolationType {
     // --- structural (always errors, cannot be downgraded) ---
     NULL_OR_BLANK,
     STRUCTURAL,
+    DUPLICATE_IDENTIFIER,
 
     // --- IDENTIFIER ---
     IDENTIFIER_INVALID_CHAR,

youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java

@@ -478,7 +478,7 @@ public void write(OutputStream out) throws IOException {
             }
         };
         HttpUpload put = new HttpUpload(src, tableURL);
-        put.setContentType(TablesInputHandler.VOTABLE_TYPE);
+        put.setRequestProperty(HttpConstants.HDR_CONTENT_TYPE, TablesInputHandler.VOTABLE_TYPE);
         Subject.doAs(schemaOwner, new RunnableAction(put));
         Assert.assertNull("throwable", put.getThrowable());
         Assert.assertEquals("response code", 200, put.getResponseCode());
@@ -661,4 +661,96 @@ public void write(OutputStream out) throws IOException {
         }
     }
 
+    // Test failures with meaningful messages
+    @Test
+    public void testCreateTableFailures() {
+        try {
+            clearSchemaPerms();
+            TapPermissions tp = new TapPermissions(null, true, null, null);
+            super.setPerms(schemaOwner, testSchemaName, tp, 200);
+
+            String testTable = testSchemaName + ".testCreateTableFailure";
+
+            // cleanup just in case
+            doDelete(schemaOwner, testTable, true);
+
+
+            VOTableTable vtab = new VOTableTable();
+            // Test 1: faulty datatype - expected 400 Bad Request
+            vtab.getFields().add(new VOTableField("c0", TapDataType.STRING.getDatatype(), TapDataType.STRING.arraysize));
+            vtab.getFields().add(new VOTableField("c1", TapDataType.SHORT.getDatatype()));
+            vtab.getFields().add(new VOTableField("c2", TapDataType.INTEGER.getDatatype()));
+            vtab.getFields().add(new VOTableField("c3", "unicodeChar"));
+
+            VOTableResource vres = new VOTableResource("results");
+            vres.setTable(vtab);
+            final VOTableDocument doc = new VOTableDocument();
+            doc.getResources().add(vres);
+
+            // create
+            URL tableURL = new URL(certTablesURL.toExternalForm() + "/" + testTable);
+            OutputStreamWrapper src = new OutputStreamWrapper() {
+                @Override
+                public void write(OutputStream out) throws IOException {
+                    VOTableWriter w = new VOTableWriter(VOTableWriter.SerializationType.TABLEDATA);
+                    w.write(doc, out);
+                }
+            };
+            HttpUpload put = new HttpUpload(src, tableURL);
+            put.setRequestProperty(HttpConstants.HDR_CONTENT_TYPE, TablesInputHandler.VOTABLE_TYPE);
+            Subject.doAs(schemaOwner, new RunnableAction(put));
+            Assert.assertEquals("response code", 400, put.getResponseCode());
+            Assert.assertNotNull("throwable", put.getThrowable());
+
+            // TAP query check (Table is not created)
+            String adql = "SELECT * from " + testTable;
+            Map<String, Object> params = new TreeMap<String, Object>();
+            params.put("LANG", "ADQL");
+            params.put("QUERY", adql);
+            log.info("doQueryCheck: " + testTable + " " + anonQueryURL);
+            ByteArrayOutputStream out = new ByteArrayOutputStream();
+            HttpPost doit = new HttpPost(anonQueryURL, params, out);
+            doit.run();
+            Assert.assertEquals("response code", 400, doit.getResponseCode());
+            Assert.assertNotNull("throwable", doit.getThrowable());
+
+            // Test 2: duplicate names
+            vtab = new VOTableTable();
+            vtab.getFields().add(new VOTableField("c0", TapDataType.STRING.getDatatype(), TapDataType.STRING.arraysize));
+            vtab.getFields().add(new VOTableField("c1", TapDataType.SHORT.getDatatype()));
+            vtab.getFields().add(new VOTableField("c2", TapDataType.INTEGER.getDatatype()));
+            vtab.getFields().add(new VOTableField("c2", TapDataType.STRING.getDatatype(), TapDataType.STRING.arraysize));
+
+            vres = new VOTableResource("results");
+            vres.setTable(vtab);
+            final VOTableDocument doc1 = new VOTableDocument();
+            doc1.getResources().add(vres);
+
+            // create
+            src = new OutputStreamWrapper() {
+                @Override
+                public void write(OutputStream out) throws IOException {
+                    VOTableWriter w = new VOTableWriter(VOTableWriter.SerializationType.TABLEDATA);
+                    w.write(doc1, out);
+                }
+            };
+            put = new HttpUpload(src, tableURL);
+            put.setRequestProperty(HttpConstants.HDR_CONTENT_TYPE, TablesInputHandler.VOTABLE_TYPE);
+            Subject.doAs(schemaOwner, new RunnableAction(put));
+            Assert.assertEquals("response code", 400, put.getResponseCode());
+            Assert.assertNotNull("throwable", put.getThrowable());
+
+            // TAP query check (Table is not created)
+            out = new ByteArrayOutputStream();
+            doit = new HttpPost(anonQueryURL, params, out);
+            doit.run();
+            Assert.assertEquals("response code", 400, doit.getResponseCode());
+            Assert.assertNotNull("throwable", doit.getThrowable());
+
+        } catch (Exception unexpected) {
+            log.error("unexpected exception", unexpected);
+            Assert.fail("unexpected exception: " + unexpected);
+        }
+    }
+
 }