From 875ca3d08f1bce3791fbec7ebf82a1c30c4ff897 Mon Sep 17 00:00:00 2001 From: Momchil Z Date: Thu, 28 Oct 2021 12:38:38 +0300 Subject: [PATCH] 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 mzhivkov@vmware.com --- .../JobFieldStrategyByDescription.java | 5 ++-- .../datajob/JobFieldStrategyByTeam.java | 2 +- .../JobFieldStrategyByDescriptionTest.java | 26 +++++++++++++++++++ .../datajob/JobFieldStrategyByTeamTest.java | 26 +++++++++++++++++++ 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/projects/control-service/projects/pipelines_control_service/src/main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescription.java b/projects/control-service/projects/pipelines_control_service/src/main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescription.java index e547c3f0ea..b7667a7a99 100644 --- a/projects/control-service/projects/pipelines_control_service/src/main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescription.java +++ b/projects/control-service/projects/pipelines_control_service/src/main/java/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 { private static final Comparator COMPARATOR_DEFAULT = Comparator.comparing( - V2DataJob::getConfig, Comparator.nullsLast(Comparator.comparing(V2DataJobConfig::getDescription))); + e -> e.getConfig().getDescription(), Comparator.nullsLast(Comparator.naturalOrder())); @Override public Criteria computeFilterCriteria(@NonNull Criteria criteria, @NonNull Filter filter) { diff --git a/projects/control-service/projects/pipelines_control_service/src/main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeam.java b/projects/control-service/projects/pipelines_control_service/src/main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeam.java index ef5f763a5a..ac8a753943 100644 --- a/projects/control-service/projects/pipelines_control_service/src/main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeam.java +++ b/projects/control-service/projects/pipelines_control_service/src/main/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeam.java @@ -21,7 +21,7 @@ public class JobFieldStrategyByTeam extends FieldStrategy { private static final Comparator COMPARATOR_DEFAULT = Comparator.comparing( - V2DataJob::getConfig, Comparator.nullsLast(Comparator.comparing(V2DataJobConfig::getTeam))); + e -> e.getConfig().getTeam(), Comparator.nullsLast(Comparator.naturalOrder())); @Override public Criteria computeFilterCriteria(@NonNull Criteria criteria, @NonNull Filter filter) { diff --git a/projects/control-service/projects/pipelines_control_service/src/test/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescriptionTest.java b/projects/control-service/projects/pipelines_control_service/src/test/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescriptionTest.java index 2d9eabad90..e88814a3da 100644 --- a/projects/control-service/projects/pipelines_control_service/src/test/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByDescriptionTest.java +++ b/projects/control-service/projects/pipelines_control_service/src/test/java/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/projects/control-service/projects/pipelines_control_service/src/test/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeamTest.java b/projects/control-service/projects/pipelines_control_service/src/test/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeamTest.java index 8f2833b8ea..1b9824cb5d 100644 --- a/projects/control-service/projects/pipelines_control_service/src/test/java/com/vmware/taurus/service/graphql/strategy/datajob/JobFieldStrategyByTeamTest.java +++ b/projects/control-service/projects/pipelines_control_service/src/test/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();