Merge pull request #1462 from pjain1/validate_rules

tieredReplicants can be null

server/src/main/java/io/druid/server/coordinator/rules/ForeverLoadRule.java

@@ -19,6 +19,8 @@ package io.druid.server.coordinator.rules;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import io.druid.client.DruidServer;
 import io.druid.timeline.DataSegment;
 import org.joda.time.DateTime;
 import org.joda.time.Interval;
@@ -36,8 +38,8 @@ public class ForeverLoadRule extends LoadRule
       @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
   )
   {
-    validateTieredReplicants(tieredReplicants);
+    this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
-    this.tieredReplicants = tieredReplicants;
+    validateTieredReplicants(this.tieredReplicants);
   }
 
   @Override

server/src/main/java/io/druid/server/coordinator/rules/IntervalLoadRule.java

@@ -19,7 +19,9 @@ package io.druid.server.coordinator.rules;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
 import com.metamx.common.logger.Logger;
+import io.druid.client.DruidServer;
 import io.druid.timeline.DataSegment;
 import org.joda.time.DateTime;
 import org.joda.time.Interval;
@@ -41,9 +43,9 @@ public class IntervalLoadRule extends LoadRule
       @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
   )
   {
-    validateTieredReplicants(tieredReplicants);
+    this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
+    validateTieredReplicants(this.tieredReplicants);
     this.interval = interval;
-    this.tieredReplicants = tieredReplicants;
   }
 
   @Override

server/src/main/java/io/druid/server/coordinator/rules/LoadRule.java

@@ -242,7 +242,11 @@ public abstract class LoadRule implements Rule
   }
 
   protected void validateTieredReplicants(Map<String, Integer> tieredReplicants){
+    if(tieredReplicants.size() == 0)
+      throw new IAE("A rule with empty tiered replicants is invalid");
     for (Map.Entry<String, Integer> entry: tieredReplicants.entrySet()) {
+      if (entry.getValue() == null)
+        throw new IAE("Replicant value cannot be empty");
       if (entry.getValue() < 0)
         throw new IAE("Replicant value [%d] is less than 0, which is not allowed", entry.getValue());
     }

server/src/main/java/io/druid/server/coordinator/rules/PeriodLoadRule.java

@@ -19,7 +19,9 @@ package io.druid.server.coordinator.rules;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
 import com.metamx.common.logger.Logger;
+import io.druid.client.DruidServer;
 import io.druid.timeline.DataSegment;
 import org.joda.time.DateTime;
 import org.joda.time.Interval;
@@ -42,9 +44,9 @@ public class PeriodLoadRule extends LoadRule
       @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants
   )
   {
-    validateTieredReplicants(tieredReplicants);
+    this.tieredReplicants = tieredReplicants == null ? ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS) : tieredReplicants;
+    validateTieredReplicants(this.tieredReplicants);
     this.period = period;
-    this.tieredReplicants = tieredReplicants;
   }
 
   @Override

server/src/test/java/io/druid/server/coordinator/rules/ForeverLoadRuleTest.java

@@ -0,0 +1,90 @@
+/*
+* Licensed to Metamarkets Group Inc. (Metamarkets) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. Metamarkets licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+
+package io.druid.server.coordinator.rules;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import com.metamx.common.IAE;
+import io.druid.client.DruidServer;
+import io.druid.jackson.DefaultObjectMapper;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class ForeverLoadRuleTest
+{
+  @Test
+  public void testSerdeNullTieredReplicants() throws Exception
+  {
+    ForeverLoadRule rule = new ForeverLoadRule(
+        null
+    );
+
+    ObjectMapper jsonMapper = new DefaultObjectMapper();
+    Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
+
+    Assert.assertEquals(rule.getTieredReplicants(), ((ForeverLoadRule)reread).getTieredReplicants());
+    Assert.assertEquals(ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS), rule.getTieredReplicants());
+  }
+
+  @Test
+  public void testMappingNullTieredReplicants() throws Exception{
+    String inputJson = "{\n"
+                      + " \"type\": \"loadForever\"\n"
+                      + "}";
+    String expectedJson = "    {\n"
+                          + "      \"tieredReplicants\": {\n"
+                          + "        \""+ DruidServer.DEFAULT_TIER +"\": "+ DruidServer.DEFAULT_NUM_REPLICANTS +"\n"
+                          + "      },\n"
+                          + "      \"type\": \"loadForever\"\n"
+                          + "    }";
+    ObjectMapper jsonMapper = new DefaultObjectMapper();
+    ForeverLoadRule inputForeverLoadRule = jsonMapper.readValue(inputJson, ForeverLoadRule.class);
+    ForeverLoadRule expectedForeverLoadRule = jsonMapper.readValue(expectedJson, ForeverLoadRule.class);
+    Assert.assertEquals(expectedForeverLoadRule.getTieredReplicants(), inputForeverLoadRule.getTieredReplicants());
+  }
+
+  @Test(expected = IAE.class)
+  public void testEmptyTieredReplicants() throws Exception
+  {
+    ForeverLoadRule rule = new ForeverLoadRule(
+        ImmutableMap.<String, Integer>of()
+    );
+
+    ObjectMapper jsonMapper = new DefaultObjectMapper();
+    Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
+  }
+
+  @Test(expected = IAE.class)
+  public void testEmptyReplicantValue() throws Exception
+  {
+    // Immutable map does not allow null values
+    Map<String, Integer> tieredReplicants= new HashMap<>();
+    tieredReplicants.put("tier", null);
+    ForeverLoadRule rule = new ForeverLoadRule(
+        tieredReplicants
+    );
+
+    ObjectMapper jsonMapper = new DefaultObjectMapper();
+    Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
+  }
+}

server/src/test/java/io/druid/server/coordinator/rules/IntervalLoadRuleTest.java

@@ -21,8 +21,8 @@ import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableMap;
 import io.druid.client.DruidServer;
 import io.druid.jackson.DefaultObjectMapper;
-import junit.framework.Assert;
 import org.joda.time.Interval;
+import org.junit.Assert;
 import org.junit.Test;
  
  /**
@@ -42,4 +42,37 @@ import org.junit.Test;
  
      Assert.assertEquals(rule, reread);
    }
+
+   @Test
+   public void testSerdeNullTieredReplicants() throws Exception
+   {
+     IntervalLoadRule rule = new IntervalLoadRule(
+         new Interval("0/3000"), null
+     );
+
+     ObjectMapper jsonMapper = new DefaultObjectMapper();
+     Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
+
+     Assert.assertEquals(rule, reread);
+     Assert.assertEquals(ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS), rule.getTieredReplicants());
+   }
+
+   @Test
+   public void testMappingNullTieredReplicants() throws Exception{
+     String inputJson = "    {\n"
+                        + "      \"interval\": \"0000-01-01T00:00:00.000-05:50:36/3000-01-01T00:00:00.000-06:00\",\n"
+                        + "      \"type\": \"loadByInterval\"\n"
+                        + "    }";
+     String expectedJson = "{\n"
+                           + "      \"interval\": \"0000-01-01T00:00:00.000-05:50:36/3000-01-01T00:00:00.000-06:00\",\n"
+                           + "      \"tieredReplicants\": {\n"
+                           + "        \""+ DruidServer.DEFAULT_TIER +"\": "+ DruidServer.DEFAULT_NUM_REPLICANTS +"\n"
+                           + "      },\n"
+                           + "      \"type\": \"loadByInterval\"\n"
+                           + "    }";
+     ObjectMapper jsonMapper = new DefaultObjectMapper();
+     IntervalLoadRule inputIntervalLoadRule = jsonMapper.readValue(inputJson, IntervalLoadRule.class);
+     IntervalLoadRule expectedIntervalLoadRule = jsonMapper.readValue(expectedJson, IntervalLoadRule.class);
+     Assert.assertEquals(expectedIntervalLoadRule, inputIntervalLoadRule);
+   }
  }

server/src/test/java/io/druid/server/coordinator/rules/PeriodLoadRuleTest.java

@@ -17,7 +17,10 @@
 
 package io.druid.server.coordinator.rules;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableMap;
+import io.druid.client.DruidServer;
+import io.druid.jackson.DefaultObjectMapper;
 import io.druid.timeline.DataSegment;
 import io.druid.timeline.partition.NoneShardSpec;
 import org.joda.time.DateTime;
@@ -74,4 +77,39 @@ public class PeriodLoadRuleTest
         )
     );
   }
+
+  @Test
+  public void testSerdeNullTieredReplicants() throws Exception
+  {
+    PeriodLoadRule rule = new PeriodLoadRule(
+        new Period("P1D"), null
+    );
+
+    ObjectMapper jsonMapper = new DefaultObjectMapper();
+    Rule reread = jsonMapper.readValue(jsonMapper.writeValueAsString(rule), Rule.class);
+
+    Assert.assertEquals(rule.getPeriod(), ((PeriodLoadRule)reread).getPeriod());
+    Assert.assertEquals(rule.getTieredReplicants(), ((PeriodLoadRule)reread).getTieredReplicants());
+    Assert.assertEquals(ImmutableMap.of(DruidServer.DEFAULT_TIER, DruidServer.DEFAULT_NUM_REPLICANTS), rule.getTieredReplicants());
+  }
+
+  @Test
+  public void testMappingNullTieredReplicants() throws Exception{
+    String inputJson = "{\n"
+                       + "      \"period\": \"P1D\",\n"
+                       + "      \"type\": \"loadByPeriod\"\n"
+                       + "    }";
+    String expectedJson = "{\n"
+                          + "      \"period\": \"P1D\",\n"
+                          + "      \"tieredReplicants\": {\n"
+                          + "        \""+ DruidServer.DEFAULT_TIER +"\": "+ DruidServer.DEFAULT_NUM_REPLICANTS +"\n"
+                          + "      },\n"
+                          + "      \"type\": \"loadByPeriod\"\n"
+                          + "    }";
+    ObjectMapper jsonMapper = new DefaultObjectMapper();
+    PeriodLoadRule inputPeriodLoadRule = jsonMapper.readValue(inputJson, PeriodLoadRule.class);
+    PeriodLoadRule expectedPeriodLoadRule = jsonMapper.readValue(expectedJson, PeriodLoadRule.class);
+    Assert.assertEquals(expectedPeriodLoadRule.getTieredReplicants(), inputPeriodLoadRule.getTieredReplicants());
+    Assert.assertEquals(expectedPeriodLoadRule.getPeriod(), inputPeriodLoadRule.getPeriod());
+  }
 }