control-service: fix graphql NPE when sorting null description or team

why: Users reported that when trying to sort by a property that has a null value in the database, control service returns a 400 bad Request error. After some investigation I determined that this was caused by an incorrect implementation of the custom comparators for Description and Team which would throw NPE when comparing a property with a null value. what: Changed implementation so that it is null tolerant. Fixed indentation and removed unused imports in the classes that needed changing. testing: Added two new unit tests. Tested on a locally running control-service with the same request that previously returned 400 and it seems to be working as expected after the changes. Signed-off-by: Momchil Zhivkov [email protected]
vmware · Oct 28, 2021 · 875ca3d · 875ca3d
1 parent 2c2bcbe
commit 875ca3d
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 4 deletions.
diff --git a/...ava/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescription.java b/...ava/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescription.java
@@ -6,10 +6,9 @@
 package com.vmware.taurus.service.graphql.strategy.datajob;
 
 import com.vmware.taurus.service.graphql.model.Criteria;
+import com.vmware.taurus.service.graphql.model.Filter;
 import com.vmware.taurus.service.graphql.model.V2DataJob;
-import com.vmware.taurus.service.graphql.model.V2DataJobConfig;
 import com.vmware.taurus.service.graphql.strategy.FieldStrategy;
-import com.vmware.taurus.service.graphql.model.Filter;
 import org.apache.commons.lang3.StringUtils;
 import org.springframework.lang.NonNull;
 import org.springframework.stereotype.Component;
@@ -21,7 +20,7 @@
 public class JobFieldStrategyByDescription extends FieldStrategy<V2DataJob> {
 
    private static final Comparator<V2DataJob> COMPARATOR_DEFAULT = Comparator.comparing(
-         V2DataJob::getConfig, Comparator.nullsLast(Comparator.comparing(V2DataJobConfig::getDescription)));
+           e -> e.getConfig().getDescription(), Comparator.nullsLast(Comparator.naturalOrder()));
 
    @Override
    public Criteria<V2DataJob> computeFilterCriteria(@NonNull Criteria<V2DataJob> criteria, @NonNull Filter filter) {

diff --git a/.../main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeam.java b/.../main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeam.java
@@ -21,7 +21,7 @@
 public class JobFieldStrategyByTeam extends FieldStrategy<V2DataJob> {
 
    private static final Comparator<V2DataJob> COMPARATOR_DEFAULT = Comparator.comparing(
-         V2DataJob::getConfig, Comparator.nullsLast(Comparator.comparing(V2DataJobConfig::getTeam)));
+           e -> e.getConfig().getTeam(), Comparator.nullsLast(Comparator.naturalOrder()));
 
    @Override
    public Criteria<V2DataJob> computeFilterCriteria(@NonNull Criteria<V2DataJob> criteria, @NonNull Filter filter) {

diff --git a/...com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescriptionTest.java b/...com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescriptionTest.java
@@ -9,6 +9,7 @@
 import com.vmware.taurus.service.graphql.model.V2DataJob;
 import com.vmware.taurus.service.graphql.model.V2DataJobConfig;
 import com.vmware.taurus.service.graphql.model.Filter;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.springframework.data.domain.Sort;
 
@@ -83,6 +84,31 @@ void testJobDescriptionStrategy_whenComputingInvalidSearchProvided_shouldReturnV
       assertThat(predicate.test(a)).isFalse();
    }
 
+   @Test
+   public void testJobDescriptionComparator() {
+
+      V2DataJob a = createDummyJob("A");
+      V2DataJob a1 = createDummyJob("A");
+      V2DataJob b = createDummyJob("B");
+      V2DataJob c = createDummyJob(null);
+      V2DataJob c1 = createDummyJob(null);
+
+      var baseCriteria = new Criteria<>(Objects::nonNull, Comparator.comparing(V2DataJob::getJobName));
+      var filter = new Filter("description", "A", Sort.Direction.ASC);
+
+      var descriptionCriteria = strategyByDescription.computeFilterCriteria(baseCriteria, filter);
+      var comparator = descriptionCriteria.getComparator();
+
+      Assertions.assertEquals(-1, comparator.compare(a, b));
+      Assertions.assertEquals(-1, comparator.compare(b, c));
+      Assertions.assertEquals(-1, comparator.compare(a, c));
+      Assertions.assertEquals(1, comparator.compare(b, a));
+      Assertions.assertEquals(1, comparator.compare(c, a));
+      Assertions.assertEquals(0, comparator.compare(a1, a));
+      Assertions.assertEquals(0, comparator.compare(c1, c));
+
+   }
+
    private V2DataJob createDummyJob(String desc) {
       V2DataJob job = new V2DataJob();
       V2DataJobConfig config = new V2DataJobConfig();

diff --git a/...t/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeamTest.java b/...t/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeamTest.java
@@ -9,6 +9,7 @@
 import com.vmware.taurus.service.graphql.model.V2DataJob;
 import com.vmware.taurus.service.graphql.model.V2DataJobConfig;
 import com.vmware.taurus.service.graphql.model.Filter;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.springframework.data.domain.Sort;
 
@@ -83,6 +84,31 @@ void testJobTeamStrategy_whenComputingInvalidSearchProvided_shouldReturnValidPre
       assertThat(predicate.test(a)).isFalse();
    }
 
+   @Test
+   public void testJobTeamComparator() {
+      var strategyByTeam = new JobFieldStrategyByTeam();
+      V2DataJob a = createDummyJob("A");
+      V2DataJob a1 = createDummyJob("A");
+      V2DataJob b = createDummyJob("B");
+      V2DataJob c = createDummyJob(null);
+      V2DataJob c1 = createDummyJob(null);
+
+      var baseCriteria = new Criteria<>(Objects::nonNull, Comparator.comparing(V2DataJob::getJobName));
+      var filter = new Filter("team", "A", Sort.Direction.ASC);
+
+      var descriptionCriteria = strategyByTeam.computeFilterCriteria(baseCriteria, filter);
+      var comparator = descriptionCriteria.getComparator();
+
+      Assertions.assertEquals(-1, comparator.compare(a, b));
+      Assertions.assertEquals(-1, comparator.compare(b, c));
+      Assertions.assertEquals(-1, comparator.compare(a, c));
+      Assertions.assertEquals(1, comparator.compare(b, a));
+      Assertions.assertEquals(1, comparator.compare(c, a));
+      Assertions.assertEquals(0, comparator.compare(a1, a));
+      Assertions.assertEquals(0, comparator.compare(c1, c));
+
+   }
+
    private V2DataJob createDummyJob(String team) {
       V2DataJob job = new V2DataJob();
       V2DataJobConfig dataJobConfig = new V2DataJobConfig();