[GitHub] [iceberg] nastra commented on a change in pull request #3668: AWS: Make the error message of GlueCatalog's isValidIdentifier more detailed

[GitBox <gi...@apache.org>]

nastra commented on a change in pull request #3668:
URL: https://github.com/apache/iceberg/pull/3668#discussion_r797816549



##########
File path: aws/src/test/java/org/apache/iceberg/aws/glue/TestGlueCatalog.java
##########
@@ -454,4 +455,43 @@ public void testRemoveProperties() {
         .when(glue).updateDatabase(Mockito.any(UpdateDatabaseRequest.class));
     glueCatalog.removeProperties(Namespace.of("db1"), Sets.newHashSet("key"));
   }
+
+  @Test
+  public void testValidIdentifier() {
+    String databaseNameValidationMessage = "Cannot convert namespace %s to Glue database name, because it must be " +
+        "1-252 chars of lowercase letters, numbers, underscore";
+    String tableNameValidationMessage = "Cannot use %s as Glue table name, because it must be 1-255 chars of " +
+        "lowercase letters, numbers, underscore";
+
+    // Positive Test Cases
+    Assert.assertTrue(glueCatalog.isValidIdentifier(TableIdentifier.of("db", "table")));
+    Assert.assertTrue(glueCatalog.isValidIdentifier(TableIdentifier.of("db0123456789", "table0123456789")));
+    Assert.assertTrue(glueCatalog.isValidIdentifier(TableIdentifier.of("db_0123456789", "table_0123456789")));
+
+    // Negative Test Cases
+    AssertHelpers.assertThrows("Database Name Invalid", ValidationException.class,
+        String.format(databaseNameValidationMessage, "DB"), () -> {
+          glueCatalog.isValidIdentifier(TableIdentifier.of("DB", "TABLE"));
+        });
+    AssertHelpers.assertThrows("Database Name Invalid", ValidationException.class,
+        String.format(databaseNameValidationMessage, "test-db"), () -> {
+          glueCatalog.isValidIdentifier(TableIdentifier.of("test-db", "table"));
+        });
+    AssertHelpers.assertThrows("Database Name Invalid", ValidationException.class,
+        String.format(databaseNameValidationMessage, "test db"), () -> {
+          glueCatalog.isValidIdentifier(TableIdentifier.of("test db", "table"));
+        });
+    AssertHelpers.assertThrows("Database and Table Name Invalid", ValidationException.class,
+        String.format(databaseNameValidationMessage, "test db"), () -> {
+          glueCatalog.isValidIdentifier(TableIdentifier.of("test db", "ta ble"));
+        });
+    AssertHelpers.assertThrows("Table Name Invalid", ValidationException.class,
+        String.format(tableNameValidationMessage, "test-table"), () -> {
+          glueCatalog.isValidIdentifier(TableIdentifier.of("db", "test-table"));
+        });
+    AssertHelpers.assertThrows("Table Name Invalid", ValidationException.class,
+        String.format(tableNameValidationMessage, Strings.repeat("a", 256)), () -> {

Review comment:
       should there be a similar test with 253 chars for the namespace part? Also might be worth adding tests for empty namespace/table names




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org