HBASE-25533 The metadata of the table and family should not be an empty string (#2906)

Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org>

hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java

@@ -677,7 +677,7 @@ public class ColumnFamilyDescriptorBuilder {
      * @return this (for chained invocation)
      */
     private ModifyableColumnFamilyDescriptor setValue(Bytes key, Bytes value) {
-      if (value == null) {
+      if (value == null || value.getLength() == 0) {
         values.remove(key);
       } else {
         values.put(key, value);
@@ -1228,7 +1228,7 @@ public class ColumnFamilyDescriptorBuilder {
      * @return this (for chained invocation)
      */
     public ModifyableColumnFamilyDescriptor setConfiguration(String key, String value) {
-      if (value == null) {
+      if (value == null || value.length() == 0) {
         configuration.remove(key);
       } else {
         configuration.put(key, value);

hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java

@@ -701,7 +701,7 @@ public class TableDescriptorBuilder {
               toBytesOrNull(value, Bytes::toBytes));
     }
 
-    /*
+    /**
      * @param key The key.
      * @param value The value. If null, removes the setting.
      */
@@ -710,14 +710,14 @@ public class TableDescriptorBuilder {
       return setValue(key, toBytesOrNull(value, Bytes::toBytes));
     }
 
-    /*
+    /**
      * Setter for storing metadata as a (key, value) pair in {@link #values} map
      *
      * @param key The key.
      * @param value The value. If null, removes the setting.
      */
     public ModifyableTableDescriptor setValue(final Bytes key, final Bytes value) {
-      if (value == null) {
+      if (value == null || value.getLength() == 0) {
         values.remove(key);
       } else {
         values.put(key, value);

hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestColumnFamilyDescriptorBuilder.java

@@ -18,6 +18,7 @@
 package org.apache.hadoop.hbase.client;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
@@ -210,6 +211,24 @@ public class TestColumnFamilyDescriptorBuilder {
       KeepDeletedCells.FALSE.toString());
     assertEquals(defaultValueMap.get(ColumnFamilyDescriptorBuilder.DATA_BLOCK_ENCODING),
       DataBlockEncoding.NONE.toString());
+  }
 
+  @Test
+  public void testSetEmptyValue() {
+    ColumnFamilyDescriptorBuilder builder =
+      ColumnFamilyDescriptorBuilder.newBuilder(HConstants.CATALOG_FAMILY);
+    String testConf = "TestConfiguration";
+    String testValue = "TestValue";
+    // test set value
+    builder.setValue(testValue, "2");
+    assertEquals("2", Bytes.toString(builder.build().getValue(Bytes.toBytes(testValue))));
+    builder.setValue(testValue, "");
+    assertNull(builder.build().getValue(Bytes.toBytes(testValue)));
+
+    // test set configuration
+    builder.setConfiguration(testConf, "1");
+    assertEquals("1", builder.build().getConfigurationValue(testConf));
+    builder.setConfiguration(testConf, "");
+    assertNull(builder.build().getConfigurationValue(testConf));
   }
 }

hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestTableDescriptorBuilder.java

@@ -369,4 +369,22 @@ public class TestTableDescriptorBuilder {
     htd = TableDescriptorBuilder.newBuilder(htd).setRegionServerGroup(null).build();
     assertNull(htd.getValue(RSGroupInfo.TABLE_DESC_PROP_GROUP));
   }
+
+  @Test
+  public void testSetEmptyValue() {
+    TableDescriptorBuilder builder =
+      TableDescriptorBuilder.newBuilder(TableName.valueOf(name.getMethodName()));
+    String testValue = "TestValue";
+    // test setValue
+    builder.setValue(testValue, "2");
+    assertEquals("2", builder.build().getValue(testValue));
+    builder.setValue(testValue, "");
+    assertNull(builder.build().getValue(Bytes.toBytes(testValue)));
+
+    // test setFlushPolicyClassName
+    builder.setFlushPolicyClassName("class");
+    assertEquals("class", builder.build().getFlushPolicyClassName());
+    builder.setFlushPolicyClassName("");
+    assertNull(builder.build().getFlushPolicyClassName());
+  }
 }

hbase-shell/src/test/ruby/hbase/admin_test.rb

@@ -1013,6 +1013,21 @@ module Hbase
       assert_no_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
     end
 
+    define_test "alter should be able to remove a list of table attributes when value is empty" do
+      drop_test_table(@test_name)
+
+      key_1 = "TestAttr1"
+      key_2 = "TestAttr2"
+      command(:create, @test_name, { NAME => 'i'}, METADATA => { key_1 => 1, key_2 => 2 })
+
+      # eval() is used to convert a string to regex
+      assert_match(eval("/" + key_1 + "/"), admin.describe(@test_name))
+      assert_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
+
+      command(:alter, @test_name, METADATA => { key_1 => '', key_2 => '' })
+      assert_no_match(eval("/" + key_1 + "/"), admin.describe(@test_name))
+      assert_no_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
+    end
 
     define_test "alter should raise error trying to remove nonexistent attributes" do
       drop_test_table(@test_name)
@@ -1064,6 +1079,38 @@ module Hbase
       assert_no_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
     end
 
+    define_test "alter should be able to remove a list of table configuration  when value is empty" do
+      drop_test_table(@test_name)
+
+      key_1 = "TestConf1"
+      key_2 = "TestConf2"
+      command(:create, @test_name, { NAME => 'i'}, CONFIGURATION => { key_1 => 1, key_2 => 2 })
+
+      # eval() is used to convert a string to regex
+      assert_match(eval("/" + key_1 + "/"), admin.describe(@test_name))
+      assert_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
+
+      command(:alter, @test_name, CONFIGURATION => { key_1 => '', key_2 => '' })
+      assert_no_match(eval("/" + key_1 + "/"), admin.describe(@test_name))
+      assert_no_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
+    end
+
+    define_test "alter should be able to remove a list of column family configuration when value is empty" do
+      drop_test_table(@test_name)
+
+      key_1 = "TestConf1"
+      key_2 = "TestConf2"
+      command(:create, @test_name, { NAME => 'i', CONFIGURATION => { key_1 => 1, key_2 => 2 }})
+
+      # eval() is used to convert a string to regex
+      assert_match(eval("/" + key_1 + "/"), admin.describe(@test_name))
+      assert_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
+
+      command(:alter, @test_name, { NAME => 'i', CONFIGURATION => { key_1 => '', key_2 => '' }})
+      assert_no_match(eval("/" + key_1 + "/"), admin.describe(@test_name))
+      assert_no_match(eval("/" + key_2 + "/"), admin.describe(@test_name))
+    end
+
     define_test "alter should raise error trying to remove nonexistent configurations" do
       drop_test_table(@test_name)
       create_test_table(@test_name)