HBASE-25533 The metadata of the table and family should not be an empty string (#2916) (#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/HColumnDescriptor.java

@@ -583,8 +583,12 @@ public class HColumnDescriptor implements WritableComparable<HColumnDescriptor>
             "' for REPLICATION_SCOPE.");
       }
     }
-    values.put(new ImmutableBytesWritable(key),
+    if (value == null || value.length == 0) {
-      new ImmutableBytesWritable(value));
+      remove(key);
+    } else {
+      values.put(new ImmutableBytesWritable(key),
+        new ImmutableBytesWritable(value));
+    }
     return this;
   }
 
@@ -601,7 +605,7 @@ public class HColumnDescriptor implements WritableComparable<HColumnDescriptor>
    * @return this (for chained invocation)
    */
   public HColumnDescriptor setValue(String key, String value) {
-    if (value == null) {
+    if (value == null || value.length() == 0) {
       remove(Bytes.toBytes(key));
     } else {
       setValue(Bytes.toBytes(key), Bytes.toBytes(value));
@@ -1526,7 +1530,7 @@ public class HColumnDescriptor implements WritableComparable<HColumnDescriptor>
    * @param value String value. If null, removes the configuration.
    */
   public HColumnDescriptor setConfiguration(String key, String value) {
-    if (value == null) {
+    if (value == null || value.length() == 0) {
       removeConfiguration(key);
     } else {
       configuration.put(key, value);

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

@@ -556,7 +556,7 @@ public class HTableDescriptor implements WritableComparable<HTableDescriptor> {
     return this;
   }
 
-  /*
+  /**
    * @param key The key.
    * @param value The value.
    */
@@ -566,7 +566,7 @@ public class HTableDescriptor implements WritableComparable<HTableDescriptor> {
     return this;
   }
 
-  /*
+  /**
    * Setter for storing metadata as a (key, value) pair in {@link #values} map
    *
    * @param key The key.
@@ -581,7 +581,11 @@ public class HTableDescriptor implements WritableComparable<HTableDescriptor> {
       setDurability(isDeferredFlush ? Durability.ASYNC_WAL : DEFAULT_DURABLITY);
       return this;
     }
-    values.put(key, value);
+    if (value == null || value.getLength() == 0) {
+      remove(key);
+    } else {
+      values.put(key, value);
+    }
     return this;
   }
 
@@ -593,7 +597,7 @@ public class HTableDescriptor implements WritableComparable<HTableDescriptor> {
    * @see #values
    */
   public HTableDescriptor setValue(String key, String value) {
-    if (value == null) {
+    if (value == null || value.length() == 0) {
       remove(key);
     } else {
       setValue(Bytes.toBytes(key), Bytes.toBytes(value));
@@ -1779,7 +1783,7 @@ public class HTableDescriptor implements WritableComparable<HTableDescriptor> {
    * @param value String value. If null, removes the setting.
    */
   public HTableDescriptor setConfiguration(String key, String value) {
-    if (value == null) {
+    if (value == null || value.length() == 0) {
       removeConfiguration(key);
     } else {
       configuration.put(key, value);

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

@@ -18,6 +18,7 @@
 package org.apache.hadoop.hbase;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -125,4 +126,23 @@ public class TestHColumnDescriptor {
     HColumnDescriptor column = new HColumnDescriptor("f1");
     column.setScope(5);
   }
+
+  @Test
+  public void testSetEmptyValue() {
+    HColumnDescriptor hcd = new HColumnDescriptor(
+      HTableDescriptor.META_TABLEDESC.getColumnFamilies()[0]);
+    String testConf = "TestConfiguration";
+    String testValue = "TestValue";
+    // test set value
+    hcd.setValue(testValue, "2");
+    assertEquals("2", Bytes.toString(hcd.getValue(Bytes.toBytes(testValue))));
+    hcd.setValue(testValue, "");
+    assertNull(hcd.getValue(Bytes.toBytes(testValue)));
+
+    // test set configuration
+    hcd.setConfiguration(testConf, "1");
+    assertEquals("1", hcd.getConfigurationValue(testConf));
+    hcd.setConfiguration(testConf, "");
+    assertNull(hcd.getConfigurationValue(testConf));
+  }
 }

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

@@ -19,6 +19,7 @@ package org.apache.hadoop.hbase;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -315,4 +316,21 @@ public class TestHTableDescriptor {
     htd.setPriority(42);
     assertEquals(42, htd.getPriority());
   }
+
+  @Test
+  public void testSetEmptyValue() {
+    HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("table"));
+    String testValue = "TestValue";
+    // test setValue
+    htd.setValue(testValue, "2");
+    assertEquals("2", htd.getValue(testValue));
+    htd.setValue(testValue, "");
+    assertNull(htd.getValue(Bytes.toBytes(testValue)));
+
+    // test setFlushPolicyClassName
+    htd.setFlushPolicyClassName("class");
+    assertEquals("class", htd.getFlushPolicyClassName());
+    htd.setFlushPolicyClassName("");
+    assertNull(htd.getFlushPolicyClassName());
+  }
 }

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

@@ -446,6 +446,54 @@ 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"
+      admin.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))
+
+      admin.alter(@test_name, true, 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 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"
+      admin.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))
+
+      admin.alter(@test_name, true, 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"
+      admin.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))
+
+      admin.alter(@test_name, true, { 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 "get_table should get a real table" do
       drop_test_table(@test_name)
       create_test_table(@test_name)