From aa0479f5b6992d068599921290964b904d9f8c2d Mon Sep 17 00:00:00 2001 From: Patrick Dowler <pdowler.cadc@gmail.com> Date: Fri, 25 Oct 2024 12:47:58 -0700 Subject: [PATCH 1/5] youcat: added some safety checks that tests only interact with the test schema --- .../opencadc/youcat/AbstractTablesTest.java | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java b/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java index ba0f5a05..573b0afc 100644 --- a/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java +++ b/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java @@ -180,9 +180,15 @@ abstract class AbstractTablesTest { } } - void doDelete(Subject subject, String testTable, boolean fnf) throws Exception { + void doDelete(Subject subject, String tableName, boolean fnf) throws Exception { + if (!tableName.startsWith(testSchemaName + ".")) { + throw new RuntimeException("TEST BUG: attempt to create table " + tableName + + " Not in test schema " + testSchemaName); + } + + URL tableURL = new URL(certTablesURL.toExternalForm() + "/" + tableName); + // delete - URL tableURL = new URL(certTablesURL.toExternalForm() + "/" + testTable); HttpDelete del = new HttpDelete(tableURL, false); log.info("doDelete: " + tableURL); Subject.doAs(subject, new RunnableAction(del)); @@ -196,13 +202,17 @@ void doDelete(Subject subject, String testTable, boolean fnf) throws Exception { throw (Exception) del.getThrowable(); } - URL getTableURL = new URL(certTablesURL.toExternalForm() + "/" + testTable); - HttpGet check = new HttpGet(getTableURL, new ByteArrayOutputStream()); + HttpGet check = new HttpGet(tableURL, new ByteArrayOutputStream()); Subject.doAs(subject, new RunnableAction(check)); Assert.assertEquals("table deleted", 404, check.getResponseCode()); } TableDesc doCreateTable(Subject subject, String tableName) throws Exception { + if (!tableName.startsWith(testSchemaName + ".")) { + throw new RuntimeException("TEST BUG: attempt to create table " + tableName + + " Not in test schema " + testSchemaName); + } + // cleanup just in case doDelete(subject, tableName, true); @@ -255,6 +265,11 @@ public void write(OutputStream out) throws IOException { } void doCreateIndex(Subject subject, String tableName, String indexCol, boolean unique, ExecutionPhase expected, String emsg) throws Exception { + if (!tableName.startsWith(testSchemaName + ".")) { + throw new RuntimeException("TEST BUG: attempt to create table " + tableName + + " Not in test schema " + testSchemaName); + } + Assert.assertNotNull("found async table-update URL", certUpdateURL); // create job @@ -311,7 +326,10 @@ protected void clearSchemaPerms() throws MalformedURLException { } protected void setPerms(Subject subject, String name, TapPermissions tp, int expectedCode) throws MalformedURLException { - + if (!name.equals(testSchemaName) && !name.startsWith(testSchemaName + ".")) { + throw new RuntimeException("TEST BUG: attempt to change permissions on " + name + + " Not in test schema " + testSchemaName); + } StringBuilder perms = new StringBuilder(); if (tp.isPublic != null) { perms.append("public=").append(Boolean.toString(tp.isPublic)).append("\n"); From 81ac5b22f9a758ebf527ea11ac019b1b00cd8607 Mon Sep 17 00:00:00 2001 From: Patrick Dowler <pdowler.cadc@gmail.com> Date: Wed, 30 Oct 2024 16:25:33 -0700 Subject: [PATCH 2/5] youcat: update dependency improve safety of intTest code --- youcat/build.gradle | 2 +- .../opencadc/youcat/AbstractTablesTest.java | 32 +++++++++---------- .../org/opencadc/youcat/CreateTableTest.java | 13 ++++---- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/youcat/build.gradle b/youcat/build.gradle index 790be437..c2d4f5bc 100644 --- a/youcat/build.gradle +++ b/youcat/build.gradle @@ -36,7 +36,7 @@ dependencies { compile 'org.opencadc:cadc-uws-server:[1.2.22,)' compile 'org.opencadc:cadc-tap:[1.1.17,)' compile 'org.opencadc:cadc-tap-schema:[1.2.2,)' - compile 'org.opencadc:cadc-tap-server:[1.1.25,)' + compile 'org.opencadc:cadc-tap-server:[1.1.26,)' compile 'org.opencadc:cadc-tap-server-pg:[1.1.1,)' compile 'org.opencadc:cadc-adql:[1.1.4,)' compile 'org.opencadc:cadc-vosi:[1.4.3,2.0)' diff --git a/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java b/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java index 573b0afc..f949a2a9 100644 --- a/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java +++ b/youcat/src/intTest/java/org/opencadc/youcat/AbstractTablesTest.java @@ -132,7 +132,8 @@ abstract class AbstractTablesTest { Subject schemaOwner; Subject subjectWithGroups; - protected String testSchemaName = "int_test_schema"; + protected String testSchemaName = "int_test_schema"; + protected final String testCreateSchema = "test_create_schema"; URL anonQueryURL; URL certQueryURL; @@ -180,11 +181,16 @@ abstract class AbstractTablesTest { } } - void doDelete(Subject subject, String tableName, boolean fnf) throws Exception { - if (!tableName.startsWith(testSchemaName + ".")) { - throw new RuntimeException("TEST BUG: attempt to create table " + tableName - + " Not in test schema " + testSchemaName); + protected void checkTestSchema(String name) { + if (name.equals(testCreateSchema) || name.equals(testSchemaName) || name.startsWith(testSchemaName + ".")) { + return; // ok } + throw new RuntimeException("TEST BUG: attempt to use schema|table name " + name + + " not in test schema " + testSchemaName); + } + + void doDelete(Subject subject, String tableName, boolean fnf) throws Exception { + checkTestSchema(tableName); URL tableURL = new URL(certTablesURL.toExternalForm() + "/" + tableName); @@ -208,10 +214,7 @@ void doDelete(Subject subject, String tableName, boolean fnf) throws Exception { } TableDesc doCreateTable(Subject subject, String tableName) throws Exception { - if (!tableName.startsWith(testSchemaName + ".")) { - throw new RuntimeException("TEST BUG: attempt to create table " + tableName - + " Not in test schema " + testSchemaName); - } + checkTestSchema(tableName); // cleanup just in case doDelete(subject, tableName, true); @@ -265,10 +268,7 @@ public void write(OutputStream out) throws IOException { } void doCreateIndex(Subject subject, String tableName, String indexCol, boolean unique, ExecutionPhase expected, String emsg) throws Exception { - if (!tableName.startsWith(testSchemaName + ".")) { - throw new RuntimeException("TEST BUG: attempt to create table " + tableName - + " Not in test schema " + testSchemaName); - } + checkTestSchema(tableName); Assert.assertNotNull("found async table-update URL", certUpdateURL); @@ -326,10 +326,8 @@ protected void clearSchemaPerms() throws MalformedURLException { } protected void setPerms(Subject subject, String name, TapPermissions tp, int expectedCode) throws MalformedURLException { - if (!name.equals(testSchemaName) && !name.startsWith(testSchemaName + ".")) { - throw new RuntimeException("TEST BUG: attempt to change permissions on " + name - + " Not in test schema " + testSchemaName); - } + checkTestSchema(name); + StringBuilder perms = new StringBuilder(); if (tp.isPublic != null) { perms.append("public=").append(Boolean.toString(tp.isPublic)).append("\n"); diff --git a/youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java b/youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java index 8be20b5a..efdb3672 100644 --- a/youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java +++ b/youcat/src/intTest/java/org/opencadc/youcat/CreateTableTest.java @@ -401,17 +401,16 @@ public void write(OutputStream out) throws IOException { @Test public void testCreateUpdateDropSchema() { - String schemaName = "test_create_schema"; // TODO: use schemaOwner subject to determine the user name here final String owner = "cadcauthtest1"; try { - final URL schemaURL = new URL(certTablesURL.toExternalForm() + "/" + schemaName); + final URL schemaURL = new URL(certTablesURL.toExternalForm() + "/" + testCreateSchema); - doDelete(admin, schemaName, true); + doDelete(admin, testCreateSchema, true); - SchemaDesc orig = new SchemaDesc(schemaName); + SchemaDesc orig = new SchemaDesc(testCreateSchema); orig.description = "original description"; TableSetWriter w = new TableSetWriter(); StringWriter sw = new StringWriter(); @@ -427,7 +426,7 @@ public void testCreateUpdateDropSchema() { log.info("update: " + create.getResponseCode() + " " + create.getThrowable()); Assert.assertEquals(200, create.getResponseCode()); - SchemaDesc sd = doVosiSchemaCheck(schemaOwner, schemaName); + SchemaDesc sd = doVosiSchemaCheck(schemaOwner, testCreateSchema); Assert.assertNotNull(sd); Assert.assertEquals(orig.getSchemaName(), sd.getSchemaName()); Assert.assertEquals(orig.description, sd.description); @@ -450,12 +449,12 @@ public void testCreateUpdateDropSchema() { log.info("update: " + update.getResponseCode() + " " + update.getThrowable()); Assert.assertEquals(204, update.getResponseCode()); - SchemaDesc sd2 = doVosiSchemaCheck(schemaOwner, schemaName); + SchemaDesc sd2 = doVosiSchemaCheck(schemaOwner, testCreateSchema); Assert.assertEquals(sd.description, sd2.description); Assert.assertEquals(sd.utype, sd2.utype); // cleanup on success - doDelete(admin, schemaName, false); + doDelete(admin, testCreateSchema, false); } catch (Exception unexpected) { log.error("unexpected exception", unexpected); Assert.fail("unexpected exception: " + unexpected); From 224b040b0736fb3fbd8a7bc0f9f741a12e2e3a77 Mon Sep 17 00:00:00 2001 From: Patrick Dowler <pdowler.cadc@gmail.com> Date: Wed, 30 Oct 2024 16:26:50 -0700 Subject: [PATCH 3/5] youcat 0.7.2 --- youcat/VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/youcat/VERSION b/youcat/VERSION index e7135390..11ceeb7c 100644 --- a/youcat/VERSION +++ b/youcat/VERSION @@ -1,6 +1,6 @@ ## deployable containers have a semantic and build tag # semantic version tag: major.minor # build version tag: timestamp -VER=0.7.1 +VER=0.7.2 TAGS="${VER} ${VER}-$(date --utc +"%Y%m%dT%H%M%S")" unset VER From 1370cdd51e6917bb6b1e9b42c11bba157a2dc194 Mon Sep 17 00:00:00 2001 From: Patrick Dowler <pdowler.cadc@gmail.com> Date: Fri, 29 Nov 2024 13:37:20 -0800 Subject: [PATCH 4/5] cadc-tap-schema: move table ingest outside transaction ensure ingest connection closed before return to avoid leak --- cadc-tap-schema/build.gradle | 2 +- .../ca/nrc/cadc/tap/db/TableIngester.java | 87 +++++++++++-------- .../vosi/actions/TableContentHandler.java | 6 +- .../cadc/vosi/actions/TableUpdateRunner.java | 48 +++++----- 4 files changed, 76 insertions(+), 67 deletions(-) diff --git a/cadc-tap-schema/build.gradle b/cadc-tap-schema/build.gradle index 8f669c24..313b9bd3 100644 --- a/cadc-tap-schema/build.gradle +++ b/cadc-tap-schema/build.gradle @@ -16,7 +16,7 @@ sourceCompatibility = 1.8 group = 'org.opencadc' -version = '1.2.2' +version = '1.2.3' description = 'OpenCADC TAP-1.1 tap schema server library' def git_url = 'https://github.com/opencadc/tap' diff --git a/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableIngester.java b/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableIngester.java index 84ea2d30..866ae467 100644 --- a/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableIngester.java +++ b/cadc-tap-schema/src/main/java/ca/nrc/cadc/tap/db/TableIngester.java @@ -76,6 +76,7 @@ import ca.nrc.cadc.tap.schema.TableDesc; import ca.nrc.cadc.tap.schema.TapDataType; import ca.nrc.cadc.tap.schema.TapPermissions; +import java.sql.Connection; import java.sql.DatabaseMetaData; import java.sql.ResultSet; import java.sql.SQLException; @@ -137,47 +138,59 @@ private TableDesc createTableDesc(String schemaName, String tableName) final String internalTableName = databaseDataType.toInternalDatabaseObjectName(s); final String internalSchemaName = databaseDataType.toInternalDatabaseObjectName(schemaName); log.debug(String.format("creating TableDesc for %s %s aka %s", internalSchemaName, internalTableName, tableName)); - DatabaseMetaData databaseMetaData = dataSource.getConnection().getMetaData(); - ResultSet rs = databaseMetaData.getTables(null, internalSchemaName, internalTableName, null); - if (rs != null && !rs.next()) { - log.debug("table does not exist: " + tableName); - throw new ResourceNotFoundException("database table not found: " + tableName); - } - - log.debug(String.format("querying DatabaseMetadata for schema=%s table=%s", internalSchemaName, internalTableName)); - //TODO too pg specific? table names are stored lower case in the system tables queried for the metadata - ResultSet indexInfo = databaseMetaData.getIndexInfo(null, internalSchemaName, internalTableName, false, false); - // get column names for indexed columns - List<String> indexedColumns = new ArrayList<String>(); - while (indexInfo.next()) { - String indexedColumn = indexInfo.getString("COLUMN_NAME"); - indexedColumns.add(indexedColumn); - log.debug("indexed column: " + indexedColumn); - } + Connection conn = dataSource.getConnection(); + try { + DatabaseMetaData databaseMetaData = conn.getMetaData(); + ResultSet rs = databaseMetaData.getTables(null, internalSchemaName, internalTableName, null); + if (rs != null && !rs.next()) { + log.debug("table does not exist: " + tableName); + throw new ResourceNotFoundException("database table not found: " + tableName); + } + + log.debug(String.format("querying DatabaseMetadata for schema=%s table=%s", internalSchemaName, internalTableName)); + //TODO too pg specific? table names are stored lower case in the system tables queried for the metadata + ResultSet indexInfo = databaseMetaData.getIndexInfo(null, internalSchemaName, internalTableName, false, false); + List<String> indexedColumns = new ArrayList<String>(); + try { + while (indexInfo.next()) { + String indexedColumn = indexInfo.getString("COLUMN_NAME"); + indexedColumns.add(indexedColumn); + log.debug("indexed column: " + indexedColumn); + } + } finally { + indexInfo.close(); + } - // build TableDesc - TableDesc tableDesc = new TableDesc(schemaName, tableName); // as specified by caller - tableDesc.tableType = TableDesc.TableType.TABLE; - log.debug(String.format("creating TableDesc %s %s aka %s", internalSchemaName, internalTableName, tableName)); - //TODO too pg specific? table names are stored lower case in the system tables queried for the metadata - ResultSet columnInfo = databaseMetaData.getColumns(null, internalSchemaName, internalTableName, null); - while (columnInfo.next()) { - String columnName = columnInfo.getString("COLUMN_NAME"); - String columnType = columnInfo.getString("TYPE_NAME"); - TapDataType tapDataType = databaseDataType.toTapDataType(columnType, null); - if (TapDataType.CHAR.getDatatype().equals(tapDataType.getDatatype()) && tapDataType.xtype == null) { - Integer colSize = columnInfo.getInt("COLUMN_SIZE"); // int - if (colSize == 1) { - colSize = null; // length 1 means scalar in TAP + // build TableDesc + TableDesc tableDesc = new TableDesc(schemaName, tableName); // as specified by caller + tableDesc.tableType = TableDesc.TableType.TABLE; + log.debug(String.format("creating TableDesc %s %s aka %s", internalSchemaName, internalTableName, tableName)); + //TODO too pg specific? table names are stored lower case in the system tables queried for the metadata + ResultSet columnInfo = databaseMetaData.getColumns(null, internalSchemaName, internalTableName, null); + try { + while (columnInfo.next()) { + String columnName = columnInfo.getString("COLUMN_NAME"); + String columnType = columnInfo.getString("TYPE_NAME"); + TapDataType tapDataType = databaseDataType.toTapDataType(columnType, null); + if (TapDataType.CHAR.getDatatype().equals(tapDataType.getDatatype()) && tapDataType.xtype == null) { + Integer colSize = columnInfo.getInt("COLUMN_SIZE"); // int + if (colSize == 1) { + colSize = null; // length 1 means scalar in TAP + } + tapDataType = databaseDataType.toTapDataType(columnType, colSize); + } + log.debug(String.format("creating ColumnDesc %s %s %s", tableName, columnName, tapDataType)); + ColumnDesc columnDesc = new ColumnDesc(tableName, columnName, tapDataType); + columnDesc.indexed = indexedColumns.contains(columnName); + tableDesc.getColumnDescs().add(columnDesc); } - tapDataType = databaseDataType.toTapDataType(columnType, colSize); + } finally { + columnInfo.close(); } - log.debug(String.format("creating ColumnDesc %s %s %s", tableName, columnName, tapDataType)); - ColumnDesc columnDesc = new ColumnDesc(tableName, columnName, tapDataType); - columnDesc.indexed = indexedColumns.contains(columnName); - tableDesc.getColumnDescs().add(columnDesc); + return tableDesc; + } finally { + conn.close(); } - return tableDesc; } String getUnqualifiedTableNameFromTable(String tableName) { diff --git a/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableContentHandler.java b/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableContentHandler.java index bcb9ae4c..0d60292d 100644 --- a/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableContentHandler.java +++ b/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableContentHandler.java @@ -114,6 +114,9 @@ public Content accept(String name, String contentType, InputStream inputStream) if (tableName == null) { throw new IllegalArgumentException("Missing table name in path"); } + if (contentType == null) { + throw new IllegalArgumentException("Table Content-Type is required."); + } TapSchemaDAO ts = parent.getTapSchemaDAO(); parent.checkTableWritePermissions(ts, tableName); @@ -125,9 +128,6 @@ public Content accept(String name, String contentType, InputStream inputStream) log.debug("Content-Type: " + contentType); TableDataInputStream tableData = null; - if (contentType == null) { - throw new IllegalArgumentException("Table Content-Type is requried."); - } if (contentType.equals(CONTENT_TYPE_CSV) || contentType.equals(CONTENT_TYPE_TSV)) { tableData = new AsciiTableData(inputStream, contentType); } diff --git a/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableUpdateRunner.java b/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableUpdateRunner.java index 02d9db48..5d2aa231 100644 --- a/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableUpdateRunner.java +++ b/cadc-tap-schema/src/main/java/ca/nrc/cadc/vosi/actions/TableUpdateRunner.java @@ -379,50 +379,46 @@ protected void ingestTable(Map<String, List<String>> params) throw new IllegalArgumentException("ingest schema not found in tap_schema: " + schemaName); } - // check if table already exists in tap_schema + log.debug("check if table already exists in tap_schema"); TableDesc tableDesc = tapSchemaDAO.getTable(tableName); if (tableDesc != null) { throw new IllegalArgumentException("ingest table already exists in tap_schema: " + tableName); } - // add the table to the tap_schema + // note: this is outside the transaction because it uses low-level db to get + // database metadata TableIngester tableIngester = new TableIngester(ds); + log.debug("read table from database"); + TableDesc ingestable = tableIngester.getTableDesc(schemaName, tableName); + // check the table is valid ADQL name + try { + TapSchemaUtil.checkValidTableName(ingestable.getTableName()); + } catch (ADQLIdentifierException ex) { + throw new IllegalArgumentException("invalid table name: " + ingestable.getTableName(), ex); + } + try { + for (ColumnDesc cd : ingestable.getColumnDescs()) { + TapSchemaUtil.checkValidIdentifier(cd.getColumnName()); + } + } catch (ADQLIdentifierException ex) { + throw new IllegalArgumentException(ex.getMessage()); + } + DatabaseTransactionManager tm = new DatabaseTransactionManager(ds); try { + log.debug("start transaction"); tm.startTransaction(); - TableDesc ingestable = tableIngester.getTableDesc(schemaName, tableName); - // check the table is valid ADQL name - try { - TapSchemaUtil.checkValidTableName(ingestable.getTableName()); - } catch (ADQLIdentifierException ex) { - throw new IllegalArgumentException("invalid table name: " + ingestable.getTableName(), ex); - } - try { - for (ColumnDesc cd : ingestable.getColumnDescs()) { - TapSchemaUtil.checkValidIdentifier(cd.getColumnName()); - } - } catch (ADQLIdentifierException ex) { - throw new IllegalArgumentException(ex.getMessage()); - } - // assign owner ingestable.tapPermissions.owner = AuthenticationUtil.getCurrentSubject(); ingestable.apiCreated = false; // pre-existing table + log.debug("put table to tap_schema"); tapSchemaDAO.put(ingestable); log.debug(String.format("added table '%s' to tap_schema", tableName)); + log.debug("commit transaction"); tm.commitTransaction(); - } catch (IllegalArgumentException | ResourceNotFoundException | UnsupportedOperationException ex) { - try { - log.debug("ingest table and update tap_schema failed - rollback", ex); - tm.rollbackTransaction(); - log.debug("ingest table and update tap_schema failed - rollback OK"); - } catch (Exception oops) { - log.error("ingest table and update tap_schema failed - rollback: FAIL", ex); - } - throw ex; } catch (Exception ex) { try { From 8cad4d7a224efe06b7296e757824b489c4b8551f Mon Sep 17 00:00:00 2001 From: Patrick Dowler <pdowler.cadc@gmail.com> Date: Fri, 29 Nov 2024 13:40:38 -0800 Subject: [PATCH 5/5] youcat: update dependency --- youcat/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/youcat/build.gradle b/youcat/build.gradle index c2d4f5bc..733cf1f1 100644 --- a/youcat/build.gradle +++ b/youcat/build.gradle @@ -35,7 +35,7 @@ dependencies { compile 'org.opencadc:cadc-uws:[1.0.2,)' compile 'org.opencadc:cadc-uws-server:[1.2.22,)' compile 'org.opencadc:cadc-tap:[1.1.17,)' - compile 'org.opencadc:cadc-tap-schema:[1.2.2,)' + compile 'org.opencadc:cadc-tap-schema:[1.2.3,)' compile 'org.opencadc:cadc-tap-server:[1.1.26,)' compile 'org.opencadc:cadc-tap-server-pg:[1.1.1,)' compile 'org.opencadc:cadc-adql:[1.1.4,)'