Skip to content

Commit 7238dd3

Browse files
at055612at055612
authored
Merge pull request #4833 from gchq/gh-4827-node-find-npe
PR for #4827 - 7.9 Monitoring->Nodes = 500 Error
2 parents 6b18879 + d703595 commit 7238dd3

File tree

4 files changed

+241
-21
lines changed

4 files changed

+241
-21
lines changed

‎stroom-node/stroom-node-impl/src/main/java/stroom/node/impl/NodeResourceImpl.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@
3131
import stroom.node.shared.Node;
3232
import stroom.node.shared.NodeResource;
3333
import stroom.node.shared.NodeStatusResult;
34+
import stroom.util.logging.LambdaLogger;
35+
import stroom.util.logging.LambdaLoggerFactory;
36+
import stroom.util.logging.LogUtil;
3437
import stroom.util.shared.CompareUtil;
3538
import stroom.util.shared.ResourcePaths;
3639
import stroom.util.shared.StringCriteria;
@@ -54,6 +57,8 @@
5457
@AutoLogged
5558
class NodeResourceImpl implements NodeResource {
5659

60+
private static final LambdaLogger LOGGER = LambdaLoggerFactory.getLogger(NodeResourceImpl.class);
61+
5762
private final Provider<NodeServiceImpl> nodeServiceProvider;
5863
private final Provider<NodeInfo> nodeInfoProvider;
5964
private final Provider<ClusterNodeManager> clusterNodeManagerProvider;
@@ -163,6 +168,7 @@ public FetchNodeStatusResponse find(final FindNodeStatusCriteria findNodeStatusC
163168
response.getPageResponse(),
164169
null);
165170
} catch (final RuntimeException e) {
171+
LOGGER.error("Error finding nodes for {}: {}", findNodeStatusCriteria, LogUtil.exceptionMessage(e), e);
166172
documentEventLogProvider.get().search(
167173
typeId,
168174
query,
@@ -185,7 +191,6 @@ public ClusterNodeInfo info(final String nodeName) {
185191
NodeResource.INFO_PATH_PART,
186192
nodeName);
187193

188-
189194
try {
190195
final long now = System.currentTimeMillis();
191196

@@ -239,7 +244,7 @@ public Long ping(final String nodeName) {
239244
SyncInvoker::get);
240245
} catch (WebApplicationException e) {
241246
throw new RuntimeException("Unable to connect to node '" + nodeName + "': "
242-
+ e.getMessage());
247+
+ e.getMessage());
243248
}
244249

245250
Objects.requireNonNull(ping, "Null ping");

‎stroom-util-shared/src/main/java/stroom/util/shared/CompareUtil.java‎

Lines changed: 61 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -119,40 +119,34 @@ public static <T> Comparator<T> buildCriteriaComparator(
119119

120120
Comparator<T> comparator = null;
121121

122-
if (criteria.getSortList() != null) {
122+
if (GwtNullSafe.hasItems(criteria.getSortList())) {
123123
for (final CriteriaFieldSort sort : criteria.getSortList()) {
124124
final String field = sort.getId();
125125

126+
// TODO we are ignoring sort.isIgnoreCase() so are comparing with whatever
127+
// comparators are in the map. Might be be better to have a Map<String, ComparatorPair<T>>
128+
// such that the pair contains both case sesnse and insense versions or for non-strings
129+
// just one comparator.
126130
Comparator<T> fieldComparator = fieldComparatorsMap.get(field);
127131

128132
Objects.requireNonNull(fieldComparator, () ->
129133
"Missing comparator for field " + field);
130134

131-
if (sort.isDesc()) {
132-
fieldComparator = fieldComparator.reversed();
133-
}
135+
fieldComparator = CompareUtil.reverseIf(fieldComparator, sort.isDesc());
134136

135-
comparator = comparator == null
136-
? fieldComparator
137-
: comparator.thenComparing(fieldComparator);
137+
comparator = CompareUtil.combine(comparator, fieldComparator);
138138
}
139-
} else if (defaultSortFields.length > 0) {
139+
} else {
140140
for (final String defaultField : defaultSortFields) {
141-
142-
Comparator<T> fieldComparator = fieldComparatorsMap.get(defaultField);
141+
final Comparator<T> fieldComparator = fieldComparatorsMap.get(defaultField);
143142

144143
Objects.requireNonNull(fieldComparator, () ->
145144
"Missing comparator for field " + defaultField);
146145

147-
comparator = comparator == null
148-
? fieldComparator
149-
: comparator.thenComparing(fieldComparator);
146+
comparator = CompareUtil.combine(comparator, fieldComparator);
150147
}
151-
} else {
152-
// No comparator
153-
comparator = Comparator.comparingInt(x -> 0);
154148
}
155-
return comparator;
149+
return CompareUtil.nonNull(comparator);
156150
}
157151

158152
/**
@@ -261,4 +255,54 @@ public static <T1, T2, T3, T4 extends Comparable<T4>> Comparator<T1> getNullSafe
261255
public static <T> Comparator<T> name(final String name, final Comparator<T> comparator) {
262256
return new NamedComparator<>(name, comparator);
263257
}
258+
259+
/**
260+
* Combine two comparators in a null safe way.
261+
* If one arg is null, the other arg is returned.
262+
* If both are null, null is returned.
263+
* If both args are non-null it is equivalent to:
264+
* <pre>{@code
265+
* comparator1.thenComparing(comparator2);
266+
* }</pre>
267+
*/
268+
public static <T> Comparator<T> combine(final Comparator<T> comparator1, final Comparator<T> comparator2) {
269+
if (comparator1 == null) {
270+
return comparator2;
271+
} else if (comparator2 == null) {
272+
return comparator1;
273+
} else {
274+
return comparator1.thenComparing(comparator2);
275+
}
276+
}
277+
278+
/**
279+
* Reverse the comparator if isReversed is true, else just return comparator unchanged.
280+
* Null-safe.
281+
*/
282+
public static <T> Comparator<T> reverseIf(final Comparator<T> comparator, final boolean isReversed) {
283+
if (comparator == null) {
284+
return comparator;
285+
} else {
286+
if (isReversed) {
287+
return comparator.reversed();
288+
} else {
289+
return comparator;
290+
}
291+
}
292+
}
293+
294+
/**
295+
* A comparator that always returns zero and thus does not change the order
296+
*/
297+
public static <T> Comparator<T> noOpComparator() {
298+
return (o1, o2) -> 0;
299+
}
300+
301+
/**
302+
* Returns a non-null comparator, either comparator if non-null, else a no-op
303+
* comparator ({@link CompareUtil#noOpComparator()}) that does nothing.
304+
*/
305+
public static <T> Comparator<T> nonNull(final Comparator<T> comparator) {
306+
return GwtNullSafe.requireNonNullElseGet(comparator, CompareUtil::noOpComparator);
307+
}
264308
}

‎stroom-util/src/test/java/stroom/util/testshared/TestCompareUtil.java‎

Lines changed: 149 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,18 +23,23 @@
2323
import com.google.inject.TypeLiteral;
2424
import io.vavr.Tuple;
2525
import io.vavr.Tuple2;
26-
import org.assertj.core.api.Assertions;
2726
import org.junit.jupiter.api.DynamicTest;
2827
import org.junit.jupiter.api.Test;
2928
import org.junit.jupiter.api.TestFactory;
3029

3130
import java.util.Comparator;
31+
import java.util.List;
3232
import java.util.stream.Stream;
3333

3434
import static org.assertj.core.api.Assertions.assertThat;
3535

3636
class TestCompareUtil {
3737

38+
private static final Person BOB = new Person("Bob", 42);
39+
private static final Person FRED = new Person("Fred", 37);
40+
private static final Person NADINE = new Person("Nadine", 37);
41+
private static final Person JANE = new Person("Jane", 57);
42+
3843
@Test
3944
void testStringCompare() {
4045
assertThat(CompareUtil.compareString(null, null)).isEqualTo(0);
@@ -74,7 +79,7 @@ Stream<DynamicTest> getNullSafeCaseInsensitiveComparator() {
7479
if (compareResult != 0) {
7580
// Test the reverse
7681
final int compareResult2 = comparator.compare(wrapper2, wrapper1);
77-
Assertions.assertThat(compareResult2)
82+
assertThat(compareResult2)
7883
.isEqualTo(compareResult * -1);
7984
}
8085
return CompareResult.of(compareResult);
@@ -90,6 +95,136 @@ Stream<DynamicTest> getNullSafeCaseInsensitiveComparator() {
9095
.build();
9196
}
9297

98+
@Test
99+
void testCombine1() {
100+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE);
101+
102+
final Comparator<Person> comparator = CompareUtil.combine(null, Comparator.comparing(Person::name));
103+
final List<Person> sorted = people.stream()
104+
.sorted(comparator)
105+
.toList();
106+
107+
assertThat(sorted)
108+
.containsExactly(BOB, FRED, JANE, NADINE);
109+
}
110+
111+
@Test
112+
void testCombine2() {
113+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE);
114+
115+
final Comparator<Person> comparator = CompareUtil.combine(Comparator.comparing(Person::name), null);
116+
final List<Person> sorted = people.stream()
117+
.sorted(comparator)
118+
.toList();
119+
120+
assertThat(sorted)
121+
.containsExactly(BOB, FRED, JANE, NADINE);
122+
}
123+
124+
@Test
125+
void testCombine3() {
126+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE);
127+
128+
final Comparator<Person> comparator = CompareUtil.combine(
129+
Comparator.comparingInt(Person::age),
130+
Comparator.comparing(Person::name));
131+
132+
final List<Person> sorted = people.stream()
133+
.sorted(comparator)
134+
.toList();
135+
136+
assertThat(sorted)
137+
.containsExactly(FRED, NADINE, BOB, JANE);
138+
}
139+
140+
@Test
141+
void testCombine4() {
142+
//noinspection ConstantValue
143+
final Comparator<Person> comparator = CompareUtil.combine(null, null);
144+
assertThat(comparator)
145+
.isNull();
146+
}
147+
148+
@Test
149+
void testReverse() {
150+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE);
151+
final Comparator<Person> comparator = CompareUtil.reverseIf(Comparator.comparing(Person::name), false);
152+
153+
final List<Person> sorted = people.stream()
154+
.sorted(comparator)
155+
.toList();
156+
157+
assertThat(sorted)
158+
.containsExactly(BOB, FRED, JANE, NADINE);
159+
}
160+
161+
@Test
162+
void testReverse2() {
163+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE);
164+
final Comparator<Person> comparator = CompareUtil.reverseIf(Comparator.comparing(Person::name), true);
165+
166+
final List<Person> sorted = people.stream()
167+
.sorted(comparator)
168+
.toList();
169+
170+
assertThat(sorted)
171+
.containsExactly(NADINE, JANE, FRED, BOB);
172+
}
173+
174+
@Test
175+
void testReverse3() {
176+
//noinspection ConstantValue
177+
final Comparator<Person> comparator = CompareUtil.reverseIf(null, true);
178+
assertThat(comparator)
179+
.isNull();
180+
}
181+
182+
@Test
183+
void testReverse4() {
184+
//noinspection ConstantValue
185+
final Comparator<Person> comparator = CompareUtil.reverseIf(null, false);
186+
assertThat(comparator)
187+
.isNull();
188+
}
189+
190+
@Test
191+
void testNoOpComparator() {
192+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE, BOB, NADINE);
193+
final Comparator<Person> comparator = CompareUtil.noOpComparator();
194+
195+
final List<Person> sorted = people.stream()
196+
.sorted(comparator)
197+
.toList();
198+
199+
assertThat(sorted)
200+
.containsExactly(BOB, FRED, NADINE, JANE, BOB, NADINE);
201+
}
202+
203+
@Test
204+
void testNonNull() {
205+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE, BOB, NADINE);
206+
final Comparator<Person> comparator = CompareUtil.nonNull(Comparator.comparing(Person::name));
207+
208+
final List<Person> sorted = people.stream()
209+
.sorted(comparator)
210+
.toList();
211+
212+
assertThat(sorted)
213+
.containsExactly(BOB, BOB, FRED, JANE, NADINE, NADINE);
214+
}
215+
216+
@Test
217+
void testNonNull2() {
218+
final List<Person> people = List.of(BOB, FRED, NADINE, JANE, BOB, NADINE);
219+
final Comparator<Person> comparator = CompareUtil.nonNull(null);
220+
221+
final List<Person> sorted = people.stream()
222+
.sorted(comparator)
223+
.toList();
224+
225+
assertThat(sorted)
226+
.containsExactly(BOB, FRED, NADINE, JANE, BOB, NADINE);
227+
}
93228

94229
// --------------------------------------------------------------------------------
95230

@@ -101,6 +236,10 @@ private static Wrapper of(final String val) {
101236
}
102237
}
103238

239+
240+
// --------------------------------------------------------------------------------
241+
242+
104243
private enum CompareResult {
105244
LESS_THAN,
106245
EQUAL,
@@ -117,4 +256,12 @@ private static CompareResult of(final int compareResult) {
117256
}
118257
}
119258
}
259+
260+
261+
// --------------------------------------------------------------------------------
262+
263+
264+
private record Person(String name, int age) {
265+
266+
}
120267
}

‎unreleased_changes/20250325_105137_269__4827.md‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
* Issue **#4827** : Fix NPE when opening the Nodes screen.
2+
3+
4+
```sh
5+
# ********************************************************************************
6+
# Issue title: 7.9 Monitoring->Nodes = 500 Error
7+
# Issue link: https://github.com/gchq/stroom/issues/4827
8+
# ********************************************************************************
9+
10+
# ONLY the top line will be included as a change entry in the CHANGELOG.
11+
# The entry should be in GitHub flavour markdown and should be written on a SINGLE
12+
# line with no hard breaks. You can have multiple change files for a single GitHub issue.
13+
# The entry should be written in the imperative mood, i.e. 'Fix nasty bug' rather than
14+
# 'Fixed nasty bug'.
15+
#
16+
# Examples of acceptable entries are:
17+
#
18+
#
19+
# * Issue **123** : Fix bug with an associated GitHub issue in this repository
20+
#
21+
# * Issue **namespace/other-repo#456** : Fix bug with an associated GitHub issue in another repository
22+
#
23+
# * Fix bug with no associated GitHub issue.
24+
```

0 commit comments

Comments
 (0)