From 3c71ac3b6a48e363aca4c928e744d307eabc8ca0 Mon Sep 17 00:00:00 2001 From: Patrick Dowler Date: Fri, 1 May 2026 14:19:07 -0700 Subject: [PATCH] cadc-tap-server: update BasicUploadManager to use table validation from cadc-tap improve tests of invalid input --- .../ca/nrc/cadc/tap/db/TableCreatorTest.java | 39 ------- cadc-tap-server/build.gradle | 2 +- .../nrc/cadc/tap/BasicUploadManagerTest.java | 100 +++++++----------- .../ca/nrc/cadc/tap/BasicUploadManager.java | 25 ++++- .../nrc/cadc/tap/upload/UploadParameters.java | 10 +- 5 files changed, 69 insertions(+), 107 deletions(-) diff --git a/cadc-tap-schema/src/intTest/java/ca/nrc/cadc/tap/db/TableCreatorTest.java b/cadc-tap-schema/src/intTest/java/ca/nrc/cadc/tap/db/TableCreatorTest.java index 970476a7..673c7165 100644 --- a/cadc-tap-schema/src/intTest/java/ca/nrc/cadc/tap/db/TableCreatorTest.java +++ b/cadc-tap-schema/src/intTest/java/ca/nrc/cadc/tap/db/TableCreatorTest.java @@ -361,45 +361,6 @@ public void testCreateUniqueIndex() { } } - - @Test - public void testInvalidTableName() { - try { - String testTable = testSchemaName + ".testInvalidTableName;drop table tap_schema.tables"; - TableDesc orig = new TableDesc(testSchemaName, testTable); - orig.tableType = TableDesc.TableType.TABLE; - orig.getColumnDescs().add(new ColumnDesc(testTable, "c0", TapDataType.STRING)); - - TableCreator tc = new TableCreator(dataSource); - tc.createTable(orig); - Assert.fail("expected IllegalArgumentException - createTable returned"); - } catch (IllegalArgumentException expected) { - log.info("caught expected exception: " + expected); - } catch (Exception unexpected) { - log.error("unexpected exception", unexpected); - Assert.fail("unexpected exception: " + unexpected); - } - } - - @Test - public void testInvalidColumnName() { - try { - String testTable = testSchemaName + ".testInvalidColumnName;drop table tap_schema.tables"; - TableDesc orig = new TableDesc(testSchemaName, testTable); - orig.tableType = TableDesc.TableType.TABLE; - orig.getColumnDescs().add(new ColumnDesc(testTable, "c0", TapDataType.STRING)); - - TableCreator tc = new TableCreator(dataSource); - tc.createTable(orig); - Assert.fail("expected IllegalArgumentException - createTable returned"); - } catch (IllegalArgumentException expected) { - log.info("caught expected exception: " + expected); - } catch (Exception unexpected) { - log.error("unexpected exception", unexpected); - Assert.fail("unexpected exception: " + unexpected); - } - } - public static class SimpleRowMapper implements RowMapper { @Override diff --git a/cadc-tap-server/build.gradle b/cadc-tap-server/build.gradle index 484aec8c..b0e45374 100644 --- a/cadc-tap-server/build.gradle +++ b/cadc-tap-server/build.gradle @@ -15,7 +15,7 @@ sourceCompatibility = 11 group = 'org.opencadc' -version = '1.1.38' +version = '1.1.39' description = 'OpenCADC TAP-1.1 tap server library' def git_url = 'https://github.com/opencadc/tap' diff --git a/cadc-tap-server/src/intTest/java/ca/nrc/cadc/tap/BasicUploadManagerTest.java b/cadc-tap-server/src/intTest/java/ca/nrc/cadc/tap/BasicUploadManagerTest.java index 37b6b816..0fdc48bb 100644 --- a/cadc-tap-server/src/intTest/java/ca/nrc/cadc/tap/BasicUploadManagerTest.java +++ b/cadc-tap-server/src/intTest/java/ca/nrc/cadc/tap/BasicUploadManagerTest.java @@ -353,7 +353,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("empty table name didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } // table names with a spaces paramList.clear(); @@ -363,7 +365,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name with a space didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } paramList.clear(); paramList.add(new Parameter("UPLOAD", "table name,http://localhost/foo")); @@ -372,7 +376,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name with a space didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } paramList.clear(); paramList.add(new Parameter("UPLOAD", "tablename ,http://localhost/foo")); @@ -381,7 +387,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name with a space didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } // Invalid table names paramList.clear(); @@ -391,7 +399,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name starting with a digit didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } paramList.clear(); paramList.add(new Parameter("UPLOAD", "_tablename,http://localhost/foo")); @@ -400,7 +410,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name starting with an underscore didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } paramList.clear(); paramList.add(new Parameter("UPLOAD", "tablename!,http://localhost/foo")); @@ -409,7 +421,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name with a exclaimation didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } paramList.clear(); paramList.add(new Parameter("UPLOAD", "?tablename,http://localhost/foo")); @@ -418,7 +432,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name with a question mark didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } paramList.clear(); paramList.add(new Parameter("UPLOAD", "&tablename,http://localhost/foo")); @@ -427,7 +443,9 @@ public void testInvalidTableNames() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("table name with an ampersand didn't cause an Exception"); } - catch (RuntimeException ignore) { } + catch (IllegalArgumentException expected) { + log.info("caught expected: " + expected); + } log.debug("testInvalidTableNames passed."); } @@ -461,15 +479,8 @@ public void testColumnNameStartsWithDigit() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("column name starting with a digit didn't cause an Exception"); } - catch (RuntimeException e) - { - Throwable cause = e.getCause(); - while(cause.getCause() != null) - cause = cause.getCause(); - if (cause instanceof ADQLIdentifierException) - log.debug("caught expected: " + cause); - else - Assert.fail(e.getCause().getMessage()); + catch (IllegalArgumentException expected) { + log.info("expected exception: " + expected); } log.debug("testColumnNameStartsWithDigit passed."); @@ -504,15 +515,8 @@ public void testColumnNameStartsWithUnderscore() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("column name starting with an underscore didn't cause an Exception"); } - catch (RuntimeException e) - { - Throwable cause = e.getCause(); - while(cause.getCause() != null) - cause = cause.getCause(); - if (cause instanceof ADQLIdentifierException) - log.debug("caught expected: " + cause); - else - Assert.fail(e.getCause().getMessage()); + catch (IllegalArgumentException expected) { + log.info("expected exception: " + expected); } log.debug("testColumnNameStartsWithUnderscore passed."); @@ -594,15 +598,8 @@ public void testColumnNameContainsSpace() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("column name containing a space didn't cause an Exception"); } - catch (RuntimeException e) - { - Throwable cause = e.getCause(); - while(cause.getCause() != null) - cause = cause.getCause(); - if (cause instanceof ADQLIdentifierException) - log.debug("caught expected: " + cause); - else - Assert.fail(e.getCause().getMessage()); + catch (IllegalArgumentException expected) { + log.info("expected exception: " + expected); } log.debug("testColumnNameContainsSpace passed."); @@ -681,18 +678,8 @@ public void testColumnNameStartsWithInvalidLetter() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("column name starting with an invalid letter didn't cause an Exception"); } - catch (RuntimeException e) - { - Throwable cause = e.getCause(); - while(cause.getCause() != null) - cause = cause.getCause(); - if (cause instanceof ADQLIdentifierException) - log.debug("caught expected: " + cause); - else - { - log.error("unexpected exception", e); - Assert.fail("unexpected exception: " + e); - } + catch (IllegalArgumentException expected) { + log.info("expected exception: " + expected); } log.debug("testColumnNameStartsWithInvalidLetter passed."); @@ -727,15 +714,8 @@ public void testColumnNameContainsInvalidLetter() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("column name containing an invalid letter didn't cause an Exception"); } - catch (RuntimeException e) - { - Throwable cause = e.getCause(); - while(cause.getCause() != null) - cause = cause.getCause(); - if (cause instanceof ADQLIdentifierException) - log.debug("caught expected: " + cause); - else - Assert.fail(e.getCause().getMessage()); + catch (IllegalArgumentException expected) { + log.info("expected exception: " + expected); } log.debug("testColumnNameContainsInvalidLetter passed."); @@ -770,10 +750,8 @@ public void testColumnNameEndsWithInvalidLetter() Map tableDescs = uploadManager.upload(paramList, jobID); Assert.fail("column name ending with an invalid letter didn't cause an Exception"); } - catch (RuntimeException e) - { - if (e.getCause() instanceof MissingResourceException) - Assert.fail(e.getCause().getMessage()); + catch (IllegalArgumentException expected) { + log.info("expected exception: " + expected); } log.debug("testColumnNameEndsWithInvalidLetter passed."); diff --git a/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/BasicUploadManager.java b/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/BasicUploadManager.java index a7d42ee0..8e3e89a2 100644 --- a/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/BasicUploadManager.java +++ b/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/BasicUploadManager.java @@ -70,6 +70,8 @@ package ca.nrc.cadc.tap; +import ca.nrc.cadc.dali.tables.ListTableData; +import ca.nrc.cadc.dali.tables.TableData; import ca.nrc.cadc.dali.tables.votable.VOTableTable; import ca.nrc.cadc.date.DateUtil; import ca.nrc.cadc.io.ResourceIterator; @@ -78,6 +80,8 @@ import ca.nrc.cadc.tap.db.TapConstants; import ca.nrc.cadc.tap.schema.ColumnDesc; import ca.nrc.cadc.tap.schema.TableDesc; +import ca.nrc.cadc.tap.schema.TapSchemaUtil; +import ca.nrc.cadc.tap.schema.validator.ValidatorConfig; import ca.nrc.cadc.tap.upload.JDOMVOTableParser; import ca.nrc.cadc.tap.upload.UploadLimits; import ca.nrc.cadc.tap.upload.UploadParameters; @@ -225,6 +229,7 @@ public Map upload(List paramList, String jobID) { final String tableName = tableDesc.getTableName(); try { tableDesc.setTableName(databaseTableName); + validateTable(tableDesc); storeTable(tableDesc, parser.getVOTable()); } finally { tableDesc.setTableName(tableName); @@ -241,6 +246,20 @@ public Map upload(List paramList, String jobID) { return metadata; } + /** + * Use TapSchemaUtil to validate the table structure, including names of schema, table, + * and all columns. + * @param td input table descriptor + */ + protected void validateTable(TableDesc td) { + // none() includes identifier validation, just not UCDs, units, etc + ValidatorConfig vc = ValidatorConfig.none(); + String report = TapSchemaUtil.validateTableDesc(td, vc); + if (report != null) { + throw new IllegalArgumentException("invalid input table: " + report); + } + } + /** * Use TableCreator and TableLoader to create and load the table using the dataSource. * @@ -261,7 +280,11 @@ public void close() { @Override public ResourceIterator> iterator() throws IOException { - return vot.getTableData().iterator(); + TableData tdata = vot.getTableData(); + if (tdata == null) { + tdata = new ListTableData(); + } + return tdata.iterator(); } @Override diff --git a/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/upload/UploadParameters.java b/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/upload/UploadParameters.java index fed89823..22cc3e55 100644 --- a/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/upload/UploadParameters.java +++ b/cadc-tap-server/src/main/java/ca/nrc/cadc/tap/upload/UploadParameters.java @@ -165,7 +165,7 @@ protected String validateTableName(Parameter parameter, String[] tableNameUri) try { tableName = tableNameUri[0]; } catch (IndexOutOfBoundsException e) { - throw new UnsupportedOperationException("UPLOAD table name is missing: " + parameter); + throw new IllegalArgumentException("UPLOAD table name is missing: " + parameter); } // Does table name start with a valid schema. @@ -173,10 +173,10 @@ protected String validateTableName(Parameter parameter, String[] tableNameUri) if (index >= 0) { String schema = tableName.substring(0, index); if (!schema.equalsIgnoreCase(UploadManager.SCHEMA)) { - throw new UnsupportedOperationException("UPLOAD table schema name is invalid: " + tableName); + throw new IllegalArgumentException("UPLOAD table schema name is invalid: " + tableName); } if (tableName.length() == index + 1) { - throw new UnsupportedOperationException("UPLOAD table name is missing: " + tableName); + throw new IllegalArgumentException("UPLOAD table name is missing: " + tableName); } tableName = tableName.substring(index + 1); } @@ -184,13 +184,13 @@ protected String validateTableName(Parameter parameter, String[] tableNameUri) try { TapSchemaUtil.checkValidIdentifier(tableName); } catch (ADQLIdentifierException e) { - throw new UnsupportedOperationException("UPLOAD table name not a valid ADQL identifier: " + tableName, e); + throw new IllegalArgumentException("UPLOAD table name not a valid ADQL identifier: " + tableName, e); } // duplicate table names are not allowed. for (UploadTable uploadTable : uploadTables) { if (uploadTable.tableName.equals(tableName)) { - throw new UnsupportedOperationException("UPLOAD table name is a duplicate: " + tableName); + throw new IllegalArgumentException("UPLOAD table name is a duplicate: " + tableName); } } return tableName;