Merge pull request #324 from tgianos/develop

Tag substrings are matching when they shouldn't

genie-core/src/main/java/com/netflix/genie/core/jpa/entities/CommonFieldsEntity.java

@@ -24,9 +24,8 @@
 import javax.persistence.Basic;
 import javax.persistence.Column;
 import javax.persistence.MappedSuperclass;
+import javax.validation.constraints.NotNull;
 import javax.validation.constraints.Size;
-import java.util.Arrays;
-import java.util.HashSet;
 import java.util.Set;
 import java.util.UUID;
 import java.util.stream.Collectors;
@@ -40,11 +39,14 @@
  */
 @MappedSuperclass
 public class CommonFieldsEntity extends BaseEntity {
+    /**
+     * The delimiter used to separate tags in the database.
+     */
+    public static final String TAG_DELIMITER = "|";
     protected static final String GENIE_TAG_NAMESPACE = "genie.";
     protected static final String GENIE_ID_TAG_NAMESPACE = GENIE_TAG_NAMESPACE + "id:";
     protected static final String GENIE_NAME_TAG_NAMESPACE = GENIE_TAG_NAMESPACE + "name:";
-    protected static final String PIPE = "|";
-    protected static final String PIPE_REGEX = "\\" + PIPE;
+    protected static final String TAG_DELIMITER_REGEX = "\\" + TAG_DELIMITER + "\\" + TAG_DELIMITER;
 
     private static final long serialVersionUID = -5040659007494311180L;
 
@@ -162,13 +164,11 @@ public void setDescription(final String description) {
      * @return The tags attached to this entity
      */
     public Set<String> getTags() {
-        final Set<String> returnTags = new HashSet<>();
-
         if (this.tags != null) {
-            returnTags.addAll(Arrays.asList(this.tags.split(PIPE_REGEX)));
+            return Sets.newHashSet(this.splitTags(this.tags));
+        } else {
+            return Sets.newHashSet();
         }
-
-        return returnTags;
     }
 
     /**
@@ -179,11 +179,13 @@ public Set<String> getTags() {
     public void setTags(final Set<String> tags) {
         this.tags = null;
         if (tags != null && !tags.isEmpty()) {
-            this.tags = tags
+            this.tags = TAG_DELIMITER
+                + tags
                 .stream()
                 .sorted(String.CASE_INSENSITIVE_ORDER)
-                .reduce((one, two) -> one + PIPE + two)
-                .get();
+                .reduce((one, two) -> one + TAG_DELIMITER + TAG_DELIMITER + two)
+                .get()
+                + TAG_DELIMITER;
         }
     }
 
@@ -198,7 +200,7 @@ protected Set<String> getFinalTags() throws GenieException {
         if (this.tags == null) {
             finalTags = Sets.newHashSet();
         } else {
-            finalTags = Sets.newHashSet(this.tags.split(PIPE_REGEX))
+            finalTags = Sets.newHashSet(this.splitTags(this.tags))
                 .stream()
                 .filter(tag -> !tag.contains(GENIE_TAG_NAMESPACE))
                 .collect(Collectors.toSet());
@@ -210,4 +212,9 @@ protected Set<String> getFinalTags() throws GenieException {
         finalTags.add(GENIE_NAME_TAG_NAMESPACE + this.getName());
         return finalTags;
     }
+
+    @NotNull
+    private String[] splitTags(@NotNull final String tagsToSplit) {
+        return tagsToSplit.substring(1, tagsToSplit.length() - 1).split(TAG_DELIMITER_REGEX);
+    }
 }

genie-core/src/main/java/com/netflix/genie/core/jpa/specifications/JpaSpecificationUtils.java

@@ -17,6 +17,7 @@
  */
 package com.netflix.genie.core.jpa.specifications;
 
+import com.netflix.genie.core.jpa.entities.CommonFieldsEntity;
 import org.apache.commons.lang3.StringUtils;
 
 import javax.validation.constraints.NotNull;
@@ -30,7 +31,7 @@
  */
 public final class JpaSpecificationUtils {
 
-//    private static final String PIPE_REGEX = "\\|";
+    private static final char PERCENT = '%';
 
     protected JpaSpecificationUtils() {
     }
@@ -43,11 +44,16 @@ protected JpaSpecificationUtils() {
      */
     public static String getTagLikeString(@NotNull final Set<String> tags) {
         final StringBuilder builder = new StringBuilder();
-        builder.append("%");
         tags.stream()
                 .filter(StringUtils::isNotBlank)
                 .sorted(String.CASE_INSENSITIVE_ORDER)
-                .forEach(tag -> builder.append(tag).append("%"));
-        return builder.toString();
+                .forEach(
+                    tag -> builder
+                        .append(PERCENT)
+                        .append(CommonFieldsEntity.TAG_DELIMITER)
+                        .append(tag)
+                        .append(CommonFieldsEntity.TAG_DELIMITER)
+                );
+        return builder.append(PERCENT).toString();
     }
 }

genie-core/src/test/java/com/netflix/genie/core/jpa/services/JpaClusterServiceImplIntegrationTests.java

@@ -21,10 +21,14 @@
 import com.github.fge.jsonpatch.JsonPatch;
 import com.github.springtestdbunit.annotation.DatabaseSetup;
 import com.github.springtestdbunit.annotation.DatabaseTearDown;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
 import com.netflix.genie.common.dto.Cluster;
+import com.netflix.genie.common.dto.ClusterCriteria;
 import com.netflix.genie.common.dto.ClusterStatus;
 import com.netflix.genie.common.dto.Command;
 import com.netflix.genie.common.dto.CommandStatus;
+import com.netflix.genie.common.dto.JobRequest;
 import com.netflix.genie.common.exceptions.GenieException;
 import com.netflix.genie.core.services.ClusterService;
 import com.netflix.genie.core.services.CommandService;
@@ -262,22 +266,58 @@ public void testGetClustersOrderBysCollectionField() {
     }
 
     /**
-     * Test the choseClusterForJob function.
+     * Test the choseClusterForJobRequest function.
      *
      * @throws GenieException For any problem
      */
-    @Ignore
     @Test
     public void testChooseClusterForJob() throws GenieException {
-//        final List<Cluster> clusters = this.service.chooseClusterForJob(JOB_1_ID);
-//        Assert.assertEquals(1, clusters.size());
-//        Assert.assertEquals(CLUSTER_1_ID, clusters.get(0).getId());
-//        final JobEntity jobEntity = this.jpaJobRepository.findOne(JOB_1_ID);
-//        final String chosen = jobEntity.getChosenClusterCriteriaString();
-//        Assert.assertEquals(8, chosen.length());
-//        Assert.assertTrue(chosen.contains("prod"));
-//        Assert.assertTrue(chosen.contains("pig"));
-//        Assert.assertTrue(chosen.contains(","));
+        final JobRequest one = new JobRequest.Builder(
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            Lists.newArrayList(new ClusterCriteria(Sets.newHashSet("genie.id:cluster1"))),
+            Sets.newHashSet("pig")
+        ).build();
+        final JobRequest two = new JobRequest.Builder(
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            Lists.newArrayList(new ClusterCriteria(Sets.newHashSet("genie.id:cluster"))),
+            Sets.newHashSet("pig")
+        ).build();
+        final JobRequest three = new JobRequest.Builder(
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            Lists.newArrayList(new ClusterCriteria(Sets.newHashSet("genie.id:cluster1"))),
+            Sets.newHashSet("pi")
+        ).build();
+        final JobRequest four = new JobRequest.Builder(
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            Lists.newArrayList(new ClusterCriteria(Sets.newHashSet("pig"))),
+            Sets.newHashSet("pig")
+        ).build();
+        final JobRequest five = new JobRequest.Builder(
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            UUID.randomUUID().toString(),
+            Lists.newArrayList(new ClusterCriteria(Sets.newHashSet("pig", "hive"))),
+            Sets.newHashSet("pig")
+        ).build();
+
+        Assert.assertThat(this.service.chooseClusterForJobRequest(one).size(), Matchers.is(1));
+        Assert.assertThat(this.service.chooseClusterForJobRequest(two).size(), Matchers.is(0));
+        Assert.assertThat(this.service.chooseClusterForJobRequest(three).size(), Matchers.is(0));
+        Assert.assertThat(this.service.chooseClusterForJobRequest(four).size(), Matchers.is(2));
+        Assert.assertThat(this.service.chooseClusterForJobRequest(five).size(), Matchers.is(2));
     }
 
     // TODO Add tests where jobRequest object is
@@ -488,7 +528,7 @@ public void testUpdateClusterNullUpdateCluster() throws GenieException {
      * Test to patch a cluster.
      *
      * @throws GenieException For any problem
-     * @throws IOException For Json serialization problem
+     * @throws IOException    For Json serialization problem
      */
     @Test
     public void testPatchCluster() throws GenieException, IOException {

genie-core/src/test/java/com/netflix/genie/core/jpa/specifications/JpaSpecificationUtilsUnitTests.java

@@ -18,6 +18,7 @@
 package com.netflix.genie.core.jpa.specifications;
 
 import com.google.common.collect.Sets;
+import com.netflix.genie.core.jpa.entities.CommonFieldsEntity;
 import com.netflix.genie.test.categories.UnitTest;
 import org.hamcrest.Matchers;
 import org.junit.Assert;
@@ -48,10 +49,27 @@ public void canAccessProtectedConstructor() {
     @Test
     public void canGetTagLikeString() {
         Assert.assertThat(JpaSpecificationUtils.getTagLikeString(Sets.newHashSet()), Matchers.is("%"));
-        Assert.assertThat(JpaSpecificationUtils.getTagLikeString(Sets.newHashSet("tag")), Matchers.is("%tag%"));
+        Assert.assertThat(
+            JpaSpecificationUtils.getTagLikeString(Sets.newHashSet("tag")),
+            Matchers.is("%" + CommonFieldsEntity.TAG_DELIMITER + "tag" + CommonFieldsEntity.TAG_DELIMITER + "%")
+        );
         Assert.assertThat(
             JpaSpecificationUtils.getTagLikeString(Sets.newHashSet("tag", "Stag", "rag")),
-            Matchers.is("%rag%Stag%tag%")
+            Matchers.is(
+                "%"
+                    + CommonFieldsEntity.TAG_DELIMITER
+                    + "rag"
+                    + CommonFieldsEntity.TAG_DELIMITER
+                    + "%"
+                    + CommonFieldsEntity.TAG_DELIMITER
+                    + "Stag"
+                    + CommonFieldsEntity.TAG_DELIMITER
+                    + "%"
+                    + CommonFieldsEntity.TAG_DELIMITER
+                    + "tag"
+                    + CommonFieldsEntity.TAG_DELIMITER
+                    + "%"
+            )
         );
     }
 }

genie-core/src/test/resources/com/netflix/genie/core/jpa/services/JpaApplicationServiceImplIntegrationTests/init.xml

@@ -24,7 +24,7 @@
         version="1.2.3"
         status="INACTIVE"
         entity_version="0"
-        tags="genie.id:app1|genie.name:tez|prod"
+        tags="|genie.id:app1||genie.name:tez||prod|"
     />
     <application_configs
         application_id="app1"
@@ -48,7 +48,7 @@
         version="4.5.6"
         status="ACTIVE"
         entity_version="0"
-        tags="genie.id:app2|genie.name:spark|prod|yarn"
+        tags="|genie.id:app2||genie.name:spark||prod||yarn|"
         type="spark"
     />
     <application_configs
@@ -70,7 +70,7 @@
         version="7.8.9"
         status="DEPRECATED"
         entity_version="0"
-        tags="genie.id:app3|genie.name:storm|prod"
+        tags="|genie.id:app3||genie.name:storm||prod|"
         type="storm"
     />
     <application_configs
@@ -94,7 +94,7 @@
         check_delay="10000"
         status="INACTIVE"
         entity_version="0"
-        tags="genie.id:command1|genie.name:pig_13_prod"
+        tags="|genie.id:command1||genie.name:pig_13_prod|"
     />
 
     <commands_applications command_id="command1" application_id="app1" application_order="0"/>

genie-core/src/test/resources/com/netflix/genie/core/jpa/services/JpaClusterServiceImplIntegrationTests/init.xml

@@ -26,7 +26,7 @@
         check_delay="15000"
         status="ACTIVE"
         entity_version="0"
-        tags="genie.id:command1|genie:name:pig_13_prod|pig|prod|tez"
+        tags="|genie.id:command1||genie:name:pig_13_prod||pig||prod||tez|"
     />
     <command_configs
         command_id="command1"
@@ -46,7 +46,7 @@
         check_delay="16000"
         status="INACTIVE"
         entity_version="0"
-        tags="genie.id:command2|genie:name:hive_11_prod|hive|prod"
+        tags="|genie.id:command2||genie:name:hive_11_prod||hive||prod|"
     />
     <command_configs
         command_id="command2"
@@ -63,7 +63,7 @@
         check_delay="17000"
         status="DEPRECATED"
         entity_version="0"
-        tags="deprecated|genie.id:command3|genie:name:pig_11_prod|pig|prod"
+        tags="|deprecated||genie.id:command3||genie:name:pig_11_prod||pig||prod|"
     />
     <command_configs
         command_id="command3"
@@ -78,7 +78,7 @@
         version="2.4.0"
         status="UP"
         entity_version="0"
-        tags="genie.id:cluster1|genie.name:h2prod|hive|pig|prod"
+        tags="|genie.id:cluster1||genie.name:h2prod||hive||pig||prod|"
     />
     <cluster_configs
         cluster_id="cluster1"
@@ -106,7 +106,7 @@
         version="2.4.0"
         status="UP"
         entity_version="0"
-        tags="genie.id:cluster2|genie.name:h2query|hive|pig|query"
+        tags="|genie.id:cluster2||genie.name:h2query||hive||pig||query|"
     />
     <cluster_configs
         cluster_id="cluster2"

genie-core/src/test/resources/com/netflix/genie/core/jpa/services/JpaCommandServiceImplIntegrationTests/init.xml

@@ -24,7 +24,7 @@
             version="1.2.3"
             status="ACTIVE"
             entity_version="0"
-            tags="genie.id:app1|genie.name:tez|prod|yarn"
+            tags="|genie.id:app1||genie.name:tez||prod||yarn|"
     />
     <application_configs
             application_id="app1"
@@ -50,7 +50,7 @@
             check_delay="18000"
             status="ACTIVE"
             entity_version="0"
-            tags="genie.id:command1|genie.name:pig_13_prod|pig|prod|tez"
+            tags="|genie.id:command1||genie.name:pig_13_prod||pig||prod||tez|"
     />
     <command_configs
             command_id="command1"
@@ -70,7 +70,7 @@
             check_delay="19000"
             status="INACTIVE"
             entity_version="0"
-            tags="genie.id:command2|genie.name:hive_11_prod|hive|prod"
+            tags="|genie.id:command2||genie.name:hive_11_prod||hive||prod|"
     />
     <command_configs
             command_id="command2"
@@ -87,7 +87,7 @@
             check_delay="20000"
             status="DEPRECATED"
             entity_version="0"
-            tags="deprecated|genie.id:command3|genie.name:pig_11_prod|pig|prod"
+            tags="|deprecated||genie.id:command3||genie.name:pig_11_prod||pig||prod|"
     />
     <command_configs
             command_id="command3"
@@ -102,7 +102,7 @@
             version="2.4.0"
             status="UP"
             entity_version="0"
-            tags="genie.id:cluster1|genie.name:h2prod|hive|pig|prod"
+            tags="|genie.id:cluster1||genie.name:h2prod||hive||pig||prod|"
     />
     <cluster_configs
             cluster_id="cluster1"