From 9215c375b9ae2708d38be6a62839ecd41a05d7fe Mon Sep 17 00:00:00 2001 From: FANNG Date: Tue, 30 Jan 2024 21:12:32 +0800 Subject: [PATCH] [#1717] fix(jdbc-mysql): throw an exception if setting comment on MySQL database (#1784) ### What changes were proposed in this pull request? throw a exception when user. set comment on MySQL database ### Why are the changes needed? Fix: #1717 #1767 ### Does this PR introduce _any_ user-facing change? yes, if user set comment on MySQL database, will throw a exception. ### How was this patch tested? add UT --- .../operation/MysqlDatabaseOperations.java | 8 ++++--- .../jdbc/mysql/AuditCatalogMysqlIT.java | 5 ++-- .../catalog/jdbc/mysql/CatalogMysqlIT.java | 24 +++++++++++++++++-- .../jdbc/postgresql/CatalogPostgreSqlIT.java | 2 +- .../test/trino/TrinoConnectorIT.java | 2 +- 5 files changed, 31 insertions(+), 10 deletions(-) diff --git a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java index 2e5032a7925..8542d264e1c 100644 --- a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java +++ b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java @@ -4,6 +4,7 @@ */ package com.datastrato.gravitino.catalog.mysql.operation; +import com.datastrato.gravitino.StringIdentifier; import com.datastrato.gravitino.catalog.jdbc.JdbcSchema; import com.datastrato.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations; import com.datastrato.gravitino.exceptions.NoSuchSchemaException; @@ -39,9 +40,10 @@ public class MysqlDatabaseOperations extends JdbcDatabaseOperations { @Override public String generateCreateDatabaseSql( String databaseName, String comment, Map properties) { - if (StringUtils.isNotEmpty(comment)) { - LOG.warn( - "Ignoring comment option on database create. mysql does not support comment option on database create."); + String originComment = StringIdentifier.removeIdFromComment(comment); + if (StringUtils.isNotEmpty(originComment)) { + throw new UnsupportedOperationException( + "MySQL doesn't support set schema comment: " + originComment); } StringBuilder sqlBuilder = new StringBuilder("CREATE DATABASE "); diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/AuditCatalogMysqlIT.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/AuditCatalogMysqlIT.java index 30587e82295..5d666dc92b6 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/AuditCatalogMysqlIT.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/AuditCatalogMysqlIT.java @@ -104,7 +104,7 @@ public void testAuditSchema() throws Exception { Catalog catalog = createCatalog(catalogName); NameIdentifier ident = NameIdentifier.of(metalakeName, catalogName, schemaName); Map prop = Maps.newHashMap(); - Schema schema = catalog.asSchemas().createSchema(ident, "comment", prop); + Schema schema = catalog.asSchemas().createSchema(ident, null, prop); Assertions.assertEquals(expectUser, schema.auditInfo().creator()); Assertions.assertNull(schema.auditInfo().lastModifier()); } @@ -126,8 +126,7 @@ public void testAuditTable() throws Exception { catalog .asSchemas() - .createSchema( - NameIdentifier.of(metalakeName, catalogName, schemaName), "comment", properties); + .createSchema(NameIdentifier.of(metalakeName, catalogName, schemaName), null, properties); Table table = catalog .asTableCatalog() diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/CatalogMysqlIT.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/CatalogMysqlIT.java index 20bf89208c1..7a7d43a093d 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/CatalogMysqlIT.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/CatalogMysqlIT.java @@ -71,7 +71,8 @@ public class CatalogMysqlIT extends AbstractIT { public String alertTableName = "alert_table_name"; public String table_comment = "table_comment"; - public String schema_comment = "schema_comment"; + // MySQL doesn't support schema comment + public String schema_comment = null; public String MYSQL_COL_NAME1 = "mysql_col_name1"; public String MYSQL_COL_NAME2 = "mysql_col_name2"; public String MYSQL_COL_NAME3 = "mysql_col_name3"; @@ -545,7 +546,7 @@ void testDropMySQLDatabase() { .asSchemas() .createSchema( NameIdentifier.of(metalakeName, catalogName, schemaName), - "Created by gravitino client", + null, ImmutableMap.builder().build()); catalog @@ -805,4 +806,23 @@ public void testAutoIncrement() { runtimeException.getMessage(), "Only one column can be auto-incremented. There are multiple auto-increment columns in your table: [col_1,col_6]")); } + + void testSchemaComment() { + String testSchemaName = "test"; + NameIdentifier identer = NameIdentifier.of(metalakeName, catalogName, testSchemaName); + RuntimeException exception = + Assertions.assertThrowsExactly( + RuntimeException.class, + () -> catalog.asSchemas().createSchema(identer, "comment", null)); + Assertions.assertTrue( + exception.getMessage().contains("MySQL doesn't support set schema comment: comment")); + + // test null comment + testSchemaName = "test2"; + NameIdentifier ident = NameIdentifier.of(metalakeName, catalogName, testSchemaName); + Schema schema = catalog.asSchemas().createSchema(ident, "", null); + Assertions.assertTrue(StringUtils.isEmpty(schema.comment())); + schema = catalog.asSchemas().loadSchema(ident); + Assertions.assertTrue(StringUtils.isEmpty(schema.comment())); + } } diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/CatalogPostgreSqlIT.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/CatalogPostgreSqlIT.java index 95603d0969d..aa5b0562e82 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/CatalogPostgreSqlIT.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/CatalogPostgreSqlIT.java @@ -43,7 +43,7 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; -import org.datanucleus.util.StringUtils; +import org.apache.commons.lang3.StringUtils; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java index fe8aad0310a..6d52cc682dd 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoConnectorIT.java @@ -1146,7 +1146,7 @@ void testMySQLTableCreatedByGravitino() throws InterruptedException { .asSchemas() .createSchema( NameIdentifier.of(metalakeName, catalogName, schemaName), - "Created by gravitino client", + null, ImmutableMap.builder().build()); Assertions.assertNotNull(schema);