From 2737ecec00abd51b796e13375f2ebdfbf8e1b201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Wed, 11 Oct 2023 14:26:22 +0200 Subject: [PATCH] feat: support default schema and catalog for PostgreSQL databases (#1375) * feat: support default schema and catalog for PostgreSQL databases The JDBC driver would return the empty string as the current catalog and schema for PostgreSQL databases. This is incorrect, as the default catalog and schema for PostgreSQL is the database name and the 'public' schema. * test: add additional testing for getSchema and getCatalog * fix: column name should be TABLE_CAT * fix: emulator now supports spanner_sys --- .../cloud/spanner/jdbc/JdbcConnection.java | 50 ++++++++++++++++--- .../spanner/jdbc/JdbcDatabaseMetaData.java | 12 +++-- .../spanner/jdbc/JdbcConnectionTest.java | 38 +++++++------- .../jdbc/JdbcDatabaseMetaDataTest.java | 17 +++++-- .../jdbc/it/ITJdbcDatabaseMetaDataTest.java | 22 ++++++-- .../jdbc/it/ITJdbcPgDatabaseMetaDataTest.java | 14 ++++++ 6 files changed, 114 insertions(+), 39 deletions(-) diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java index 6031afe9..9248dace 100644 --- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java +++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java @@ -47,6 +47,7 @@ import java.util.Map; import java.util.function.BiConsumer; import java.util.function.Function; +import javax.annotation.Nonnull; /** Jdbc Connection class for Google Cloud Spanner */ class JdbcConnection extends AbstractJdbcConnection { @@ -394,31 +395,66 @@ public Array createArrayOf(String typeName, Object[] elements) throws SQLExcepti @Override public void setCatalog(String catalog) throws SQLException { // This method could be changed to allow the user to change to another database. - // For now we only support setting an empty string in order to support frameworks + // For now, we only support setting the default catalog in order to support frameworks // and applications that set this when no catalog has been specified in the connection // URL. checkClosed(); - JdbcPreconditions.checkArgument("".equals(catalog), "Only catalog \"\" is supported"); + checkValidCatalog(catalog); + } + + void checkValidCatalog(String catalog) throws SQLException { + String defaultCatalog = getDefaultCatalog(); + JdbcPreconditions.checkArgument( + defaultCatalog.equals(catalog), + String.format("Only catalog %s is supported", defaultCatalog)); } @Override public String getCatalog() throws SQLException { checkClosed(); - return ""; + return getDefaultCatalog(); + } + + @Nonnull + String getDefaultCatalog() { + switch (getDialect()) { + case POSTGRESQL: + String database = getConnectionOptions().getDatabaseName(); + // It should not be possible that database is null, but it's better to be safe than sorry. + return database == null ? "" : database; + case GOOGLE_STANDARD_SQL: + default: + return ""; + } } @Override public void setSchema(String schema) throws SQLException { checkClosed(); - // Cloud Spanner does not support schemas, but does contain a pseudo 'empty string' schema that - // might be set by frameworks and applications that read the database metadata. - JdbcPreconditions.checkArgument("".equals(schema), "Only schema \"\" is supported"); + checkValidSchema(schema); + } + + void checkValidSchema(String schema) throws SQLException { + String defaultSchema = getDefaultSchema(); + JdbcPreconditions.checkArgument( + defaultSchema.equals(schema), String.format("Only schema %s is supported", defaultSchema)); } @Override public String getSchema() throws SQLException { checkClosed(); - return ""; + return getDefaultSchema(); + } + + @Nonnull + String getDefaultSchema() { + switch (getDialect()) { + case POSTGRESQL: + return "public"; + case GOOGLE_STANDARD_SQL: + default: + return ""; + } } @Override diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java index b9c7cb86..44633958 100644 --- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java +++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaData.java @@ -777,11 +777,12 @@ public ResultSet getSchemas() throws SQLException { } @Override - public ResultSet getCatalogs() { + public ResultSet getCatalogs() throws SQLException { return JdbcResultSet.of( ResultSets.forRows( Type.struct(StructField.of("TABLE_CAT", Type.string())), - Collections.singletonList(Struct.newBuilder().set("TABLE_CAT").to("").build()))); + Collections.singletonList( + Struct.newBuilder().set("TABLE_CAT").to(getConnection().getCatalog()).build()))); } @Override @@ -1524,9 +1525,10 @@ public RowIdLifetime getRowIdLifetime() { @Override public ResultSet getSchemas(String catalog, String schemaPattern) throws SQLException { String sql = readSqlFromFile("DatabaseMetaData_GetSchemas.sql", connection.getDialect()); - JdbcPreparedStatement statement = - prepareStatementReplaceNullWithAnyString(sql, catalog, schemaPattern); - return statement.executeQueryWithOptions(InternalMetadataQuery.INSTANCE); + try (JdbcPreparedStatement statement = + prepareStatementReplaceNullWithAnyString(sql, catalog, schemaPattern)) { + return statement.executeQueryWithOptions(InternalMetadataQuery.INSTANCE); + } } @Override diff --git a/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java b/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java index 592d4040..ff811f37 100644 --- a/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java +++ b/src/test/java/com/google/cloud/spanner/jdbc/JdbcConnectionTest.java @@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.any; @@ -700,34 +701,33 @@ public void testCatalog() throws SQLException { ConnectionOptions options = mockOptions(); when(options.getDatabaseName()).thenReturn("test"); try (JdbcConnection connection = createConnection(options)) { - // The connection should always return the empty string as the current catalog, as no other + // The connection should always return the default catalog as the current catalog, as no other // catalogs exist in the INFORMATION_SCHEMA. - assertThat(connection.getCatalog()).isEqualTo(""); + // The default catalog is the empty string for GoogleSQL databases. + // The default catalog is the database name for PostgreSQL databases. + assertEquals(connection.getDefaultCatalog(), connection.getCatalog()); // This should be allowed. - connection.setCatalog(""); - try { - // This should cause an exception. - connection.setCatalog("other"); - fail("missing expected exception"); - } catch (JdbcSqlExceptionImpl e) { - assertThat(e.getCode()).isEqualTo(Code.INVALID_ARGUMENT); - } + connection.setCatalog(connection.getDefaultCatalog()); + // This should cause an exception. + JdbcSqlExceptionImpl exception = + assertThrows(JdbcSqlExceptionImpl.class, () -> connection.setCatalog("other")); + assertEquals(Code.INVALID_ARGUMENT, exception.getCode()); } } @Test public void testSchema() throws SQLException { try (JdbcConnection connection = createConnection(mockOptions())) { - assertThat(connection.getSchema()).isEqualTo(""); + // The connection should always return the default schema as the current schema, as we + // currently do not support setting the connection to a different schema. + // The default schema is the empty string for GoogleSQL databases. + // The default schema is 'public' for PostgreSQL databases. + assertEquals(connection.getDefaultSchema(), connection.getSchema()); // This should be allowed. - connection.setSchema(""); - try { - // This should cause an exception. - connection.setSchema("other"); - fail("missing expected exception"); - } catch (JdbcSqlExceptionImpl e) { - assertThat(e.getCode()).isEqualTo(Code.INVALID_ARGUMENT); - } + connection.setSchema(connection.getDefaultSchema()); + JdbcSqlExceptionImpl exception = + assertThrows(JdbcSqlExceptionImpl.class, () -> connection.setSchema("other")); + assertEquals(Code.INVALID_ARGUMENT, exception.getCode()); } } diff --git a/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java b/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java index 6a2b0cac..9782e964 100644 --- a/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java +++ b/src/test/java/com/google/cloud/spanner/jdbc/JdbcDatabaseMetaDataTest.java @@ -41,10 +41,19 @@ import java.util.Objects; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; -@RunWith(JUnit4.class) +@RunWith(Parameterized.class) public class JdbcDatabaseMetaDataTest { + @Parameter public Dialect dialect; + + @Parameters(name = "dialect = {0}") + public static Object[] data() { + return Dialect.values(); + } + private static final String DEFAULT_CATALOG = ""; private static final String DEFAULT_SCHEMA = ""; private static final String TEST_TABLE = "FOO"; @@ -314,10 +323,12 @@ public void testGetBestRowIdentifier() throws SQLException { @Test public void testGetCatalogs() throws SQLException { JdbcConnection connection = mock(JdbcConnection.class); + when(connection.getDialect()).thenReturn(dialect); + when(connection.getCatalog()).thenCallRealMethod(); DatabaseMetaData meta = new JdbcDatabaseMetaData(connection); try (ResultSet rs = meta.getCatalogs()) { assertThat(rs.next(), is(true)); - assertThat(rs.getString("TABLE_CAT"), is(equalTo(""))); + assertThat(rs.getString("TABLE_CAT"), is(equalTo(connection.getDefaultCatalog()))); assertThat(rs.next(), is(false)); ResultSetMetaData rsmd = rs.getMetaData(); assertThat(rsmd.getColumnCount(), is(equalTo(1))); diff --git a/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcDatabaseMetaDataTest.java b/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcDatabaseMetaDataTest.java index 7e2481a2..820bdfac 100644 --- a/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcDatabaseMetaDataTest.java +++ b/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcDatabaseMetaDataTest.java @@ -822,6 +822,7 @@ public void testGetViews() throws SQLException { @Test public void testGetSchemas() throws SQLException { try (Connection connection = createConnection(env, database)) { + assertEquals("", connection.getSchema()); try (ResultSet rs = connection.getMetaData().getSchemas()) { assertThat(rs.next(), is(true)); assertThat(rs.getString("TABLE_SCHEM"), is(equalTo(DEFAULT_SCHEMA))); @@ -829,11 +830,22 @@ public void testGetSchemas() throws SQLException { assertThat(rs.next(), is(true)); assertThat(rs.getString("TABLE_SCHEM"), is(equalTo("INFORMATION_SCHEMA"))); assertThat(rs.getString("TABLE_CATALOG"), is(equalTo(DEFAULT_CATALOG))); - if (!EmulatorSpannerHelper.isUsingEmulator()) { - assertThat(rs.next(), is(true)); - assertThat(rs.getString("TABLE_SCHEM"), is(equalTo("SPANNER_SYS"))); - assertThat(rs.getString("TABLE_CATALOG"), is(equalTo(DEFAULT_CATALOG))); - } + assertThat(rs.next(), is(true)); + assertThat(rs.getString("TABLE_SCHEM"), is(equalTo("SPANNER_SYS"))); + assertThat(rs.getString("TABLE_CATALOG"), is(equalTo(DEFAULT_CATALOG))); + assertFalse(rs.next()); + } + } + } + + @Test + public void testGetCatalogs() throws SQLException { + try (Connection connection = createConnection(env, database)) { + assertEquals("", connection.getCatalog()); + try (ResultSet rs = connection.getMetaData().getCatalogs()) { + assertTrue(rs.next()); + assertEquals("", rs.getString("TABLE_CAT")); + assertFalse(rs.next()); } } diff --git a/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcPgDatabaseMetaDataTest.java b/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcPgDatabaseMetaDataTest.java index ff0bff7a..56d75db1 100644 --- a/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcPgDatabaseMetaDataTest.java +++ b/src/test/java/com/google/cloud/spanner/jdbc/it/ITJdbcPgDatabaseMetaDataTest.java @@ -752,6 +752,7 @@ public void testGetViews() throws SQLException { @Test public void testGetSchemas() throws SQLException { try (Connection connection = createConnection(env, database)) { + assertEquals("public", connection.getSchema()); try (ResultSet rs = connection.getMetaData().getSchemas()) { assertTrue(rs.next()); assertEquals(getDefaultCatalog(database), rs.getString("TABLE_CATALOG")); @@ -774,6 +775,19 @@ public void testGetSchemas() throws SQLException { } } + @Test + public void testGetCatalogs() throws SQLException { + try (Connection connection = createConnection(env, database)) { + assertEquals(database.getId().getDatabase(), connection.getCatalog()); + try (ResultSet rs = connection.getMetaData().getCatalogs()) { + assertTrue(rs.next()); + assertEquals(database.getId().getDatabase(), rs.getString("TABLE_CAT")); + + assertFalse(rs.next()); + } + } + } + private static final class Table { private final String name; private final String type;