diff --git a/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/BasicDataTypeMapper.java b/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/BasicDataTypeMapper.java index bafec40e..5e586607 100644 --- a/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/BasicDataTypeMapper.java +++ b/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); diff --git a/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableCreator.java b/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableCreator.java index e82e22bb..4c8f53d3 100644 --- a/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableCreator.java +++ b/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. * diff --git a/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/PutAction.java b/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/PutAction.java index 00be368a..777c34b0 100644 --- a/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/PutAction.java +++ b/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"); diff --git a/cadc-tap/build.gradle b/cadc-tap/build.gradle index 3495c821..c70fdfd3 100644 --- a/cadc-tap/build.gradle +++ b/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' diff --git a/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/TapSchemaUtil.java b/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/TapSchemaUtil.java index ff283edb..cdee2d85 100644 --- a/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/TapSchemaUtil.java +++ b/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 errors = new ArrayList<>(); List warnings = new ArrayList<>(); + Set 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; diff --git a/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ValidatorConfig.java b/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ValidatorConfig.java index bb7f18b4..bf7c0bda 100644 --- a/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ValidatorConfig.java +++ b/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 errorTypes; diff --git a/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ViolationType.java b/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ViolationType.java index e114e3b6..312f7bd8 100644 --- a/cadc-tap/src/main/java/ca/nrc/cadc/tap/schema/validator/ViolationType.java +++ b/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, diff --git a/youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java b/youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java index 4d9b1f9e..0364a3fa 100644 --- a/youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java +++ b/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 params = new TreeMap(); + 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); + } + } + }